bug-coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] status=noinfo option for dd


From: Pozsar Balazs
Subject: Re: [PATCH] status=noinfo option for dd
Date: Thu, 18 Feb 2010 03:23:45 +0100
User-agent: Mutt/1.5.18 (2008-05-17)

On Wed, Feb 17, 2010 at 07:10:50PM -0700, Eric Blake wrote:
> 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.

Hi Eric,

Thanks for your reply, no problem.


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

That's exactly why I want this feature: if everything goes ok, I do not 
want anything printed on stderr (standard _error_ :), but if something 
goes bad (like disk full), I want it to be reported.
(Bear in mind this is the good old unix behaviour.)


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


Sorry I made the patch against the latest tarball...


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

You are right, I overlooked it.


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


If they are flags, maybe they should be made to work orthogonally.


> 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


I will make a new patch soon.


Thanks again

Balazs Pozsar





reply via email to

[Prev in Thread] Current Thread [Next in Thread]