Re: dd SIGUSR1 race

[Pádraig Brady]

On 09/29/2014 10:15 PM, Bernhard Voelker wrote:
> On 09/29/2014 01:32 PM, Pádraig Brady wrote:
>> From 45d61c2dfb8fa01a4934f607fda5dfdf20e40266 Mon Sep 17 00:00:00 2001
>> From: Federico Simoncelli<address@hidden>
>> Date: Fri, 26 Sep 2014 17:12:32 +0000
>> Subject: [PATCH] dd: new status=progress operand to print stats periodically
>>
>> * src/dd.c: Report the transfer progress every second when the
>> new status=progress operand is used.
>> * doc/corutils.texi (dd invocation): Document the new progress
>> status operand.
>> * tests/dd/stats.sh: Add new test for status=progress.
>> * tests/dd/misc.sh: check status=none has precedence over 'progress'.
>> * NEWS: Mention the feature.
>> ---
>>   NEWS               |    3 +
>>   doc/coreutils.texi |    6 +++
>>   src/dd.c           |  114 
>> ++++++++++++++++++++++++++++++++++++++++------------
>>   tests/dd/misc.sh   |    3 +
>>   tests/dd/stats.sh  |    7 +++
>>   5 files changed, 107 insertions(+), 26 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index 3e10ac4..99cff31 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -19,6 +19,9 @@ GNU coreutils NEWS                                    -*- 
>> outline -*-
>>     king directory.  The new option is only permitted if the new root 
>> directory is
>>     the old "/", and therefore is useful with the --group and --userspec 
>> options.
>>
>> +  dd accepts the new status=progress option to update data transfer 
>> statistics
>> +  on stderr approximately every second.
>> +
> 
> I'd use "print" rather than "update" here, too (as you did in usage()).
> The word "update" is not very descriptive what's actually going on it.

I was trying to convey that a single line was updated.
I'll just use "print" so.


>> diff --git a/src/dd.c b/src/dd.c
>> index d22ec59..810379a 100644
>> --- a/src/dd.c
>> +++ b/src/dd.c
> ...
>> @@ -787,7 +798,45 @@ print_stats (void)
>>        but that was incorrect for languages like Polish.  To fix this
>>        bug we now use SI symbols even though they're a bit more
>>        confusing in English.  */
>> -  fprintf (stderr, _(", %g s, %s/s\n"), delta_s, bytes_per_second);
>> +  char const *time_fmt;
>> +  if (progress_time)
>> +    time_fmt = _(", %.6f s, %s/s%c");  /* OK with '\r' as increasing width. 
>>  */
>> +  else
>> +    time_fmt = _(", %g s, %s/s%c");
>> +  fprintf (stderr, time_fmt, delta_s, bytes_per_second,
>> +           progress_time ? '\r' : '\n');
>> +
>> +  newline_pending = !!progress_time;
>> +}
> 
> Why not moving the '\r' into time_fmt? There's no need for the
> ternary expression in the fprintf statement again.

Good point, though this is adjusted again. See below.

> BTW: I'm not sure about how the TRANSLATORS comment above will show
> up in their files, but it may have to be adapted, too.

I'll check

>> @@ -2029,6 +2078,19 @@ dd_copy (void)
>>
>>     while (1)
>>       {
>> +      if ((status_flags & STATUS_PROGRESS) && !(status_flags & STATUS_NONE))
> 
> Somehow, I'm not happy with how the status is handled. Currently it is
> treated as a flag with NONE, NOXFER and PROGRESS, always letting NONE
> taking precedence.  As with regular, orthogonal option values, shouldn't
> just the latest win?  E.g.
> 
>   alias dd='/usr/bin/dd status=none'
>   dd status=progress ...
> 
> expecting 'progress' information, overriding 'none'? Thus, we could
> have 4 levels of verbosity: none, noxfer, normal, progress.

Yes that's probably more consistent and less confusing.

> BTW:
> there's still something wrong with newline_pending, e.g. when
> dd(1) receives a signal to exit, then the program should print the
> final newline.  Otherwise, half of the last progress string is remaining
> on the command line after the following prompt.
> 
>   berny@susi: ~/cu > timeout 3 src/dd if=/dev/zero of=/dev/null 
> status=progress
>   berny@susi: ~/cu > 9 MB) copied, 2.000003 s, 134 MB/s
> 
> I guess that interrupt_handler() needs to take care about newline_pending?

dd doesn't handle SIGTERM currently, SIGINT is fine:

  $ timeout -sINT 3 src/dd if=/dev/zero of=/dev/null status=progress

I'm not sure it's worth handling other signals explicitly,
though we can improve things by outputting the \r first rather than last.
That means for that edge case the prompt will not overwrite anything
and appear at the end of the line.  Also a ^C output to the terminal
will not overwrite any of the status information.

> 
>> diff --git a/tests/dd/misc.sh b/tests/dd/misc.sh
>> index f877fdd..b141268 100755
>> --- a/tests/dd/misc.sh
>> +++ b/tests/dd/misc.sh
>> @@ -38,6 +38,9 @@ compare /dev/null err || fail=1
>>   # check status=none is cumulative with status=noxfer
>>   dd status=none status=noxfer if=$tmp_in of=/dev/null 2> err || fail=1
>>   compare /dev/null err || fail=1
>> +# check status=none has precedence over status=progress
>> +dd status=none status=progress if=$tmp_in of=/dev/null 2> err || fail=1
>> +compare /dev/null err || fail=1
> 
> ok
> 
>>   dd if=$tmp_in of=$tmp_out 2> /dev/null || fail=1
>>   compare $tmp_in $tmp_out || fail=1
>> diff --git a/tests/dd/stats.sh b/tests/dd/stats.sh
>> index 386752e..3a4bbad 100755
>> --- a/tests/dd/stats.sh
>> +++ b/tests/dd/stats.sh
>> @@ -54,4 +54,11 @@ for open in '' '1'; do
>>     grep '500000000 bytes .* copied' err || { cat err; fail=1; }
>>   done
>>
>> +progress_output()
>> +{
>> +  { sleep "$1"; echo 1; } | dd bs=1 status=noxfer,progress of=/dev/null 
>> 2>err
>> +  grep 'byte.*copied' err
>> +}
>> +retry_delay_ progress_output 1 4 || { cat err; fail=1; }
>> +
>>   Exit $fail
>> -- 1.7.7.
> 
> There's no proof yet that dd(1) really omits the final xfer stats
> when also progress stats are requested, so the above test would
> not work if that would fail since the pattern of the final stats is
> identical to the progress stats.

Notice the noxfer above, which implies only progress would be outputting that 
line.
Though with your suggested orthogonal levels above, this wouldn't work,
so I'll adjust accordingly.

> Instead of adding an additional test, it is maybe sufficient to check
> the content of 'err': ensure that
> a) there are 'byte.*copied' lines _before_ the 2 'records' lines, and
> b) there is no 'byte.*copied' line _after_ the 2 'records' lines.
> 
> Another minor nit: this test doesn't match the test's title:
> 
>   #!/bin/sh
>   # Check robust handling of SIG{INFO,USR1}
> 
> After all, this is a nice and helpful feature.
> The discussion started with avoiding the SIGUSR1 race which seems to
> be impossible. Shouldn't we therefore inform our users that using the
> new status=progress is better to be used instead?

The SIGUSR1 race should now be handled using the example
in the texinfo from the first patch.  So strictly this progress isn't needed,
though it is much easier to use interactively at least.

thanks for the careful review,
Pádraig.