Re: dd SIGUSR1 race

[Bernhard Voelker]

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.

>   ** Changes in behavior
>
>     chroot changes the current directory to "/" in again - unless the above new
> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> index 7d32af5..3092a08 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -8649,6 +8649,12 @@ that normally make up the last status line.
>   Do not print any informational or warning messages to stderr.
>   Error messages are output as normal.
>
> +@item progress
> +@opindex progress @r{dd status=}
> +Update the transfer rate and volume statistics on stderr,
> +when processing each input block.  Statistics are output
> +at most once every second, and can be delayed when waiting on I/O.
> +

here, too.

BTW: In the paragraph above this in line 8638, you could possibly squash
this in:
-Specifying @var{which} will identify which information to suppress.
+Specifying @var{which} will identify which information to print.


> 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.

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.

> @@ -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.

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?

> 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.
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?

Thank you & have a nice day,
Berny