Date: Thu, 11 Apr 2013 04:00:01 GMT From: Matthew Rezny <mrezny@hexaneinc.com> To: freebsd-standards@FreeBSD.org Subject: Re: standards/177742: conflict of dd's bs= option with use of conv=sparse Message-ID: <201304110400.r3B401UV007850@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR standards/177742; it has been noted by GNATS.
From: Matthew Rezny <mrezny@hexaneinc.com>
To: Konstantin Belousov <kostikbel@gmail.com>, bug-followup@FreeBSD.org
Cc:
Subject: Re: standards/177742: conflict of dd's bs= option with use of
conv=sparse
Date: Thu, 11 Apr 2013 05:44:59 +0200
On Wed, 10 Apr 2013 22:01:10 +0300
Konstantin Belousov <kostikbel@gmail.com> wrote:
> On Wed, Apr 10, 2013 at 02:10:01PM +0000, Matthew Rezny wrote:
> > The following reply was made to PR standards/177742; it has been
> > noted by GNATS.
> >
> > From: Matthew Rezny <mrezny@hexaneinc.com>
> > To: bug-followup@FreeBSD.org
> > Cc:
> > Subject: Re: standards/177742: conflict of dd's bs= option with use
> > of conv=sparse
> > Date: Wed, 10 Apr 2013 15:31:08 +0200
> >
> > The patch I suggested got a little messed up by the web form, and
> > it also contained a typo. Further, I had neglected to consider the
> > C_BS flag itself should be present after masking off the few
> > allowed flags, so the patch should be amended as such follows.
> >
> > if (ddflags & C_BS) {
> > out.dbcnt = in.dbcnt;
> > - dd_out(1);
> > + dd_out((ddflags & !(C_NOERROR | C_NOTRUNC | C_SYNC)) == C_BS);
> > in.dbcnt = 0;
> > continue;
> > }
> >
> > This patch has been tested to confirm conv=sparse now works as
> > expected with bs= set. No other conversions have been checked with
> > the bs= option and from reading the code I don't think they will.
>
> I somehow followed what you wrote.
>
> Still, I think your patch is not correct. Its result is always using
> the block coalescing mode there. The issue is that the '!' operator
> result is defined to be either 0 or 1, which mask and-ed with ddflags
> never could be equal to the C_BS == 4. You probably mean '~' ?
>
> In fact, the test in the containing if () means that C_BS is
> guaranteed to be set there.
>
> So my question is, do you want to explicitely check that no other
> flags are set, except noerror, notrunc, sync and bs ? Or is it yet
> another bug ?
>
Sorry about that, your guess is right. I meant dd_out((ddflags
& ~(C_NOERROR | C_NOTRUNC | C_SYNC)) == C_BS);
I don't know what is more embarrassing, the number of errors copying
from one screen to another, or the fact I didn't catch the obviously bad
syntax. I am e-mailing from a separate PC than the one on which I am
doing this work, glancing at text on one a then going to the other a
meter away and trying to type it again from memory. Error prone indeed.
And yes, the test in the if means C_BS has to be set, which is what I
remembered after the initial report. I found the odd behavior, looked at
the code via svnweb, wrote up the PR, then went to test the patch. In
that process I realize the omission, go to amend the PR and notice I
introduced the spurious _ in the middle of ddflags when
typing earlier and correct that but completely miss the typo where I
hit ! instead of ~. Adjacent keys, both a form of negation, but
critically different in use.
To get to your question about do I really want to verify that no flags
are set other than noerror, notrunc, sync and bs, the answer to that is
yes. Yes because that is the test condition the comment alludes to but
was not present. Does that make sense overall? Not really, but then
again, the flow of dd_in() stops making any sense past this point.
I started from the point of figuring out why the force flag would be
set and altered the call to dd_out() to not set force when we are doing
some conversions. But really, any conversion other than sparse is
normally done on the in side, only sparse conversion happens on the out
side (which is is what the force flag would bypass). I can't actually
figure out why the force flag is needed in any situation except when
dd_close() calls to dd_out(). All other cases that should always be set
false.
Looking not just at my problem but the overall situation, it seems the
logic in dd_in() does not hold up. The last line, (*cfunc)();, actually
invokes the appropriate conversion(s) and then calls dd_out(0). With
the simple test on line 361, we will never get down to that call if bs=
option is used. Really, to make the code fit the comment, it should
probably be something like
if ((ddflags & ~(C_NOERROR | C_NOTRUNC | C_SYNC)) == C_BS) {
out.dbcnt = in.dbcnt;
dd_out(0);
in.dbcnt = 0;
continue;
}
That would actually perform the test early enough to only apply this
small optimization when really no conversions are done. The simple if
C_BS then dd_out() and continue looks like a small optimization attempt
gone bad. Some effort was made to avoid calling the conversion function
and not leave anything in the buffer between output rounds, but without
sufficient checks it ends up meaning that using the bs= option disables
all "conversions" except those that are applied during input read stage
(noerro, notrunc, sync). All things considered, completely removing
lines 361-366 of dd.c would be the most straightforward path to correct
output results (judged by the result on disk, ignoring the comment
about POSIX rules about buffering which really shouldn't make a
difference, the correctness of the result matters more than the path
taken to arrive at it).
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201304110400.r3B401UV007850>