Add dd sparse output mode

Review Request #662 — Created Sept. 15, 2017 and updated

seeemef@mac.com
illumos-gate
8542
general
It would be convenient to have a conv=sparse mode for /usr/bin/dd akin to FreeBSD's and compatible with illumos's ostride mode.

N.b. the patch has many git-pbchk, cstyle fixes; but the effective changes are the new flsh2() and dd_close() functions and the two, new calls to the latter when reading input either has finished or produced an error.
* added dd_copy_test runner (and supporting scripts, bincomp and mkrandzero):
** regular-file dd for an input with no NUL bytes
** regular-file conv=sparse dd for an input with no NUL bytes
** regular-file dd for an input with some NUL bytes (random, but with certain, tail NUL blocks)
** regular-file conv=sparse dd for an input with some NUL bytes (random, but with certain, tail NUL blocks)
** regular-file dd for an input with all NUL bytes
** regular-file conv=sparse dd for an input with all NUL bytes
** same tests as above but for writing to a ZVOL
** same tests as above but for writing to a ZVOL in multiple passes using ostride
** same tests as above but for writing to a ZVOL in multiple passes using ostride with alternate block size
  • 0
  • 0
  • 11
  • 4
  • 15
Description From Last Updated
tsoome
  1. In general, when there is larger amount of cstyle fixes, I suggest either separate patch for it, or with RB you can push separate diff to the issue.

  2. usr/src/README.license-files (Diff revision 1)
     
     

    can we burn this red one?:)

  3. usr/src/cmd/dd/dd.c (Diff revision 1)
     
     

    can we burn this red one?:)

  4. usr/src/cmd/dd/dd.c (Diff revision 1)
     
     

    align comments?

  5. 
      
rm
  1. Thanks for putting this together. In general it looks good. I have a few comments on the change.

  2. usr/src/cmd/dd/dd.c (Diff revision 1)
     
     
    Any reason to keep around the old flush function? Why not just update callers to pass B_FALSE?
    1. Yeah, when it is only a new, single user (dd_close()) that needs the new mode, then I do prefer to keep the ~10 old callers as-is calling the simple, old flsh() function.
  3. usr/src/cmd/dd/dd.c (Diff revision 1)
     
     
    Maybe just have sparse be a boolean_t since we're only ever assigning B_TRUE (1) and B_FALSE (0)?
  4. usr/src/cmd/dd/dd.c (Diff revision 1)
     
     

    Does this check represent a bug with the existing ostride support?

    1. I would not call it a bug. It strictly prevents misalignment of ostride -- that could only occur if the user had specified continuation-on-error with conv=noerror (without conv=sync) and an input error had occurred.
  5. usr/src/cmd/dd/dd.c (Diff revision 1)
     
     
    bc is a signed integer where as oc is an unsigned integer. There's a sign mismatch. Is there a reason that bc is signed and not unsigned?
    1. Yes, because bc does also store the result from a write() elsewhere in this function and because the variable participates in both equality- and inequality-comparisons.
      
      The mismatch is accommodated explicitly in this function, and I think it's preferable to forcing an alignment of signs but then doing e.g. a (bc == (unsigned)-1) as is done elsewhere in this file when a read() is stored to an unsigned int.
  6. usr/src/cmd/dd/dd.c (Diff revision 1)
     
     
    Because pending and stride_pending are off_t types which are signed, is it worth making the comparisons use '>' rather than '!=' in case some odd bug would cause them to become negative?
    1. No, I do not believe so. That would mean in the case of your hypothetical bug that zero was the safe assumption which I would not agree is correct. I think this comparison is right, and the new dd_copy_tests affirm the correct behavior.
  7. usr/src/test/util-tests/cmd/bincomp.pl (Diff revision 1)
     
     
    This should probably use /usr/perl5/bin/perl, as that's the only perl guaranteed to be part of the OS. There is no guarantee for a /usr/bin/perl.
  8. usr/src/test/util-tests/cmd/bincomp.pl (Diff revision 1)
     
     
    Can you please use the new license headers from prototype/?
  9. usr/src/test/util-tests/cmd/bincomp.pl (Diff revision 1)
     
     
     

    Perl style in illumos (see things like cmd/dtrace/test/cmd/scripts/dtest.pl) has this look like C. Can you bring the elsif statements up to the previous line?

  10. This should probably use /usr/perl5/bin/perl, as that's the only perl guaranteed to be part of the OS. There is no guarantee for a /usr/bin/perl.
  11. Can you please use the new license headers from prototype/?
  12. usr/src/test/util-tests/cmd/mkrandzero.pl (Diff revision 1)
     
     
     
     
     
     
     
    Perl style in illumos (see things like cmd/dtrace/test/cmd/scripts/dtest.pl) has this look like C. Can you bring the '||' up to the previous line to match, please?
  13. Can you please use the new license headers from prototype/?
  14. Let's use [[ for consistency with the rest of the file?
  15. 
      
seeemef@mac.com
Review request changed
tsoome
  1. Ship It!
  2. 
      
kmays
  1. Ship It!
  2. 
      
Loading...