11355 zfs create needs dry-run

Review Request #2208 — Created Aug. 2, 2019 and updated

mgerdts
illumos-gate
general

I first did this work in zfsonlinux and SmartOS. This should bring zfsonlinux, SmartOS, and illumos in sync.

https://github.com/zfsonlinux/zfs/commit/d45d7f08fa56f94fc9577a6578cb411071a42e8d

SmartOS, as a private option:

https://github.com/joyent/illumos-joyent/commit/b322ba367cdb75882f1cede5a8b22c7fd17ced94

SmartOS, now public:

https://github.com/joyent/illumos-joyent/commit/d0efab8443fcce33c336a009d253617419db9909

This has been exercised in SmartOS for a couple weeks, with it in the critical path for various bhyve instance operations (create, storage management) without any issues. Testing of the upstreamin work recorded in illumos#11355.

  • 8
  • 0
  • 0
  • 0
  • 8
Description From Last Updated
We had an issue -- 4712 Prefer 'parsable' over 'parseable' in the manual pages -- let's stick to using parsable ... yuri.pankov yuri.pankov
C-style: "Do not nest the ternary conditional operator (?:)." domag02 domag02
Missing gettext calls? yuri.pankov yuri.pankov
C-style: "Do not nest the ternary conditional operator (?:)." domag02 domag02
Missing gettext calls? yuri.pankov yuri.pankov
(everywhere) use fnvpair family of functions for better readability? yuri.pankov yuri.pankov
Missing gettext calls? yuri.pankov yuri.pankov
should be? yuri.pankov yuri.pankov
domag02
  1. 
      
  2. usr/src/cmd/zfs/zfs_main.c (Diff revision 1)
     
     
    C-style:
    "Do not nest the ternary conditional operator (?:)."
    1. I think that this shows intent more clearly and concisely than the alternative. I completely agree that examples like that shown in the c-style guide should be avoided. A look at new code committed in 2019 shows the following commits use of the ternary operator in a way quite similar to the way I've used it.

      eb633035c80613ec93d62f90482837adaaf21a0a
      93bc28dbaee6387120d48b12b3dc1ba5f7418e6e
      663207adb1669640c01c5ec6949ce78fd806efae
      eef4f27b270242808b43b4b23bd161df52839361

      There were 40 different people listed as authors, reviewers, or approvers on those commits. That seems to be pretty good evidence that reasonable use of nestered ternaries is widely accepted.

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

    C-style:
    "Do not nest the ternary conditional operator (?:)."

  4. 
      
yuri.pankov
  1. 
      
  2. usr/src/cmd/zfs/zfs_main.c (Diff revision 1)
     
     

    We had an issue -- 4712 Prefer 'parsable' over 'parseable' in the manual pages -- let's stick to using parsable everywhere?

    1. I noticed this discrepancy in man page updates and fixed it before review. Missed it here - good catch!

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

    (everywhere) use fnvpair family of functions for better readability?

    1. Huh, hadn't noticed those before.

  4. 
      
yuri.pankov
  1. 
      
  2. usr/src/cmd/zfs/zfs_main.c (Diff revision 1)
     
     

    Missing gettext calls?

    1. Yep, for all but parsable values.

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

    Don't need to initialize nvp as you overwrite this value anyway below?

    1. It needs to be initialized as it is passed to nvlist_next_nvpair() before it is assigned the value returned by that call.

    2. Ugh, sorry.

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

    Missing gettext calls?

    1. yep, for all but parsable values

  5. usr/src/cmd/zfs/zfs_main.c (Diff revision 1)
     
     

    Missing gettext calls?

  6. 
      
mgerdts
Review request changed
jjelinek
  1. You might want to close out the open issues, but this looks good to me with the latest changes.

  2. 
      
Loading...