11355 zfs create needs dry-run

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

Information
mgerdts
illumos-gate
Reviewers
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
domag02 
  1. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/cmd/zfs/zfs_main.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    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.
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/cmd/zfs/zfs_main.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



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

 
    
  



 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 





 

  

   




yuri.pankov




   

   







  

 



 

  


   

   

    
 
yuri.pankov


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/cmd/zfs/zfs_main.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    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!
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/cmd/zfs/zfs_main.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



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

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Huh, hadn't noticed those before.
      
 
      

      2. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    should be?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 





 

  

   




yuri.pankov




   

   







  

 



 

  


   

   

    
 
yuri.pankov


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/cmd/zfs/zfs_main.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Missing gettext calls?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Yep, for all but parsable values.
      
 
      

      2. 


 
    


    

 
    

    3. 






  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. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Ugh, sorry.
      
 
      

      3. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/cmd/zfs/zfs_main.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Missing gettext calls?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      yep, for all but parsable values
      
 
      

      2. 


 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/cmd/zfs/zfs_main.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Missing gettext calls?
    
  
    
 
    

 
    
  



 
    

    6. 




  6. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  

 
















 



 

  

   




mgerdts




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  











 



  

   
Diff:

   
    
 Revision 2 (+530 -31)
Show changes
 
    usr/src/cmd/zfs/zfs_main.c 
    usr/src/man/man1m/zfs.1m 
    usr/src/pkg/manifests/system-test-zfstest.mf 
    usr/src/test/zfs-tests/runfiles/delphix.run 
    usr/src/test/zfs-tests/runfiles/omnios.run 
    usr/src/test/zfs-tests/runfiles/openindiana.run 
    usr/src/test/zfs-tests/runfiles/smartos.run 
    usr/src/test/zfs-tests/tests/functional/cli_root/zfs_create/zfs_create_dryrun.ksh 
  1 more
 

   		
  



 










  

 
















 





 

  

   




jjelinek




   

   







  

 



 

  


   

   

    
 
jjelinek


    
   


  

  

  



    




  1. 
 
    
  
    
   
    You might want to close out the open issues, but this looks good to me with the latest changes.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 









     

    

   

  

  

   
   Loading...