[Top][All Lists]
[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