Re: [PATCH] status=noinfo option for dd

[Eric Blake]

According to Pozsar Balazs on 2/17/2010 3:52 PM:
> 
> Hi all,
> 
> Suppressing all dd status info can be useful sometimes, so please 
> consider merging this patch. Thanks.

First, a big thanks for submitting a patch - too many people make
suggestions with nothing to back them up.  The tone in the rest of my
response may sound harsher, but that's just because I'm being thorough in
the patch review, and not because I'm opposed to the idea.

What's wrong with 'dd 2>/dev/null'?  Seriously, the bar for adding new
functionality is high when it can already be accomplished in another
manner with existing shell functionality, since it takes several years for
the new function to propagate, and since, as an extension, portable code
can't use it anyways.

Hint - this feature may very well be worth adding, if you take the time to
add more justification describing how there are some things that are still
output to stderr (such as disk full errors) even when status is omitted,
and why exit status alone is not enough when stderr is redirected.  But
that means that you should probably include such reasoning in the
coreutils.texi documentation where you add an example of your new feature
and why it is useful.

> diff -Naurd a/man/dd.1 b/man/dd.1
> --- a/man/dd.1        2010-01-12 11:56:07.000000000 +0100
> +++ b/man/dd.1        2010-02-17 23:40:32.555209552 +0100

Man pages are generated.  Patching them is pointless.  Rather, you should
be patching src/dd.c from the latest git tree, then make will run help2man
to regenerate the man page from the updated --help output.

> +++ b/src/dd.c        2010-02-17 23:40:08.774188153 +0100
> @@ -132,7 +132,8 @@
>  /* Status bit masks.  */
>  enum
>    {
> -    STATUS_NOXFER = 01
> +    STATUS_NOXFER = 01,
> +    STATUS_NOINFO

Given that this is titled bit masks, shouldn't you explicitly request the
next bit (02), leaving the door open for future bits (04, 010)?  It's true
that STATUS_NOINFO will have the value 2 whether or not you take this
advice, but not future extensions.

> @@ -611,6 +614,9 @@
>    double delta_s;
>    char const *bytes_per_second;
>  
> +  if (status_flags & STATUS_NOINFO)
> +    return;
> +
>    fprintf (stderr,
>             _("%"PRIuMAX"+%"PRIuMAX" records in\n"
>               "%"PRIuMAX"+%"PRIuMAX" records out\n"),

Hmm, by this usage pattern, STATUS_NOINFO implies STATUS_NOXFER.  Is it
intended that they are always leveled like that, or might there be a
reason to split them into orthogonal names?

The patch is incomplete without a NEWS entry, and preferably a test of the
new option.  This may be helpful:
http://git.savannah.gnu.org/cgit/coreutils.git/tree/HACKING

-- 
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden

signature.asc
Description: OpenPGP digital signature