Re: dd SIGUSR1 race

[Pádraig Brady]

On 09/25/2014 01:26 PM, Federico Simoncelli wrote:
> ----- Original Message -----
>> From: "Pádraig Brady" <address@hidden>
>> To: "Coreutils" <address@hidden>
>> Cc: "Federico Simoncelli" <address@hidden>
>> Sent: Monday, September 22, 2014 5:25:58 PM
>> Subject: dd SIGUSR1 race
>>
>> There is a race with dd setting up the signal handler for SIGUSR1,
>> and some async process sending it.  If dd loses the race it's killed.
>> This was pointed out to me by Federico (CC'd). The race was addressed,
>> but only narrowed with:
>>   http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=e4caea5
>>
>> I was wondering about making things more general by allowing
>> the parent process to set SIGUSR1 to SIGIGN, and dd would still
>> setup the signal handler in this case.  This isn't too much of
>> a stretch I think as it would only possibly result in unwanted
>> output to stderr (and dd is already quite noisy to stderr by default),
>> rather than unwanted process termination.
> 
> If you think in terms of a monitoring process, it would have to:
> 
>  ignore_sigur1() // sadly I don't think you can move this inside fork
> 
>  if !fork()  {
>      exec("dd", ...)
>  }
> 
>  // how do we detect that exec got executed and we can stop ignoring
>  // sigusr1?

You wouldn't have to wait for the exec.
The child and parent processes should have divergent signal dispositions
after the fork.

> ...also we'd have a short period of time where the driving
>  // app would be ignoring sigusr1. Any suggestion on how to fix this are
>  // welcome.

Right, so you'd have to block rather than ignore.
I.E. set the mask for the child process.

> 
>  while true {
>      send_sigusr1_to_dd_process()
> 
>      wait_for_dd_output_or_death()
>      // we could be blocked here forever if the sigusr1 was received by
>      // the forked process before exec or if dd didn't setup the sigusr1
>      // handler yet. We could use a timeout but still, it doesn't seems
>      // a clean solution.
> 
>      if dd_is_dead()
>          break;
> 
>      update_dd_progress() // consume dd output to update the progress
>  }

The above is awkward, and would also need iflag=fullblock
to avoid dd doing short reads when receiving the signal.
It does have the advantage of only producing output when required,
and also immediately as wouldn't be blocked on large reads/writes.

>> Though doing the above would still need existing apps to change
>> (to ignore SIGUSR1), so instead it might be better to just support
>> a simpler mechanism through something like a "status=progress" option,
>> which would avoid the general issues and awkwardness of signals as mentioned
>> here:
>>   http://lists.gnu.org/archive/html/bug-coreutils/2004-04/threads.html#00127
>>
>> I'm leaning towards the latter option here.
> 
> +1, it seems much simpler, cleaner and has less implications.
> 
> Question is:
> 
> - should dd report bytes read/written or percentage? (I prefer bytes)

bytes probably best, since dd may not know the total,
and the caller could have more knowledge of that.
In fact we could probably just using the existing transfer stats line
which is in this format:

  "512 bytes (512 B) copied, 1.5828e-05 s, 32.3 MB/s"

> - how often should it be reported/updated? (every x seconds or x byte
>   transferred? configurable?)

We could support status=none,xfer to only output the above transfer statistics 
line.
I suppose we could do this every second but that would be approximate due
to blocking reads() and writes().

Pádraig.