Re: dd SIGUSR1 race

[Pádraig Brady]

On 09/26/2014 09:58 PM, Bernhard Voelker wrote:
> On 09/26/2014 05:05 PM, Pádraig Brady wrote:
>> Subject: [PATCH] dd: use more robust SIGUSR handling
> 
> s/USR/USR1/
> 
>> * src/dd.c (ifd_reopen): A new wrapper to ensure we
>> don't exit upon receiving a SIGUSR1 in a blocking open()
>> on a fifo for example.

I also added an EINTR wrapper for ftruncate().
Note I considered using SA_RESTART instead, but that
would mean that uncommitted read()/write() would block on SIGUSR1.
I could have undone the SA_RESTART before dd_copy(),
though that would be messier I think.

>> (iread): Process signals also after a short read.
>> (install_signal_handlers): Install SIGINFO/SIGUSR1 handler
>> even if set to SIG_IGN, as this is what the parent can easily
>> set from a shell script that can send SIGUSR1 without the
>> possiblity of inadvertently killing the dd process.
>> * doc/corutils.texi (dd invocation): Improve the example to
>> show robust usage wrt signal races and short reads.
>> * tests/dd/stats.sh: A new test for various signal races.
>> * tests/local.mk: Reference the new test.
>> * NEWS: Mention the fix.
> 
> Another minor nit:
> s/corutils/coreutils/
> 
> The rest LGTM.
> 
> What about adding "trap '' USR1;" to usage(), too?
> You know, many folks are only reading that instead of the
> texinfo manual.

Yes there should be no inconsistency here.

> OTOH, that stats-on-signal feature is such a detail that it may
> be worth removing from the manpage at all.

I didn't notice it in the man page.
I think it's best to leave that detail to the full info page,
so I've removed that.

Thanks for the review!

Latest version is attached.

Pádraig.

dd-siginfo.patch
Description: Text Data