standards/177742: conflict of dd's bs= option with use of conv=sparse

[Matthew Rezny]

The following reply was made to PR standards/177742; it has been noted by GNATS.

From: Matthew Rezny <mrezny at hexaneinc.com>
To: Konstantin Belousov <kostikbel at gmail.com>, bug-followup at 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 at 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 at hexaneinc.com>
 > > To: bug-followup at 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).