zfs code - Aug 2006 - Code review request: new features and misc fixes

I''m going to start posting my codereviews externally, as we''re
beginning
to get more people outside of Sun (and the ZFS team) involved in the
code.  So all are welcome to comment, or simply peruse the changes to
see what''s in store[1].

This wad addresses the following bugs and RFEs:

PSARC 2006/486 ZFS canmount property
PSARC 2006/497 ZFS create time properties
PSARC 2006/502 ZFS get all datasets
PSARC 2006/504 ZFS user properties
6269805 properties should be set via an nvlist.
6281585 user defined properties
6349494 ''zfs list'' output annoying for even moderately long
dataset names
6366244 ''canmount'' option for container-like functionality
6367103 create-time properties
6416639 RFE: provide zfs get -a
6437808 ZFS module version should match on-disk version
6454551 ''zfs create -b blocksize filesystem'' should fail.
6457478 unrecognized character in error message with ''zpool create
-R'' command
6457865 missing device name in the error message of ''zpool
clear'' command
6458571 zfs_ioc_set_prop() doesn''t validate input
6458614 zfs ACL #defines should use prefix
6458638 get_configs() accesses bogus memory
6458678 zvol functions should be moved out of zfs_ioctl.h
6458683 zfs_cmd_t could use more cleanup
6458691 common routines to manage zfs_cmd_t nvlists
6460398 zpool import cores on zfs_prop_get
6461029 zpool status -x noexisting-pool has incorrect error message.
6461223 index translations should live with property definitions
6461424 zpool_unmount_datasets() has some busted logic
6461427 zfs_realloc() would be useful
6461757 ''zpool status'' can report the wrong number of
persistent errors
6461784 recursive zfs_snapshot() leaks memory

Webrev at:

http://cr.grommit.com/~eschrock/zfs-prop/

Note for zfs_prop_set() and zfs_prop_inherit(), I took two different
approaches for dealing with user properties.  With zfs_prop_set(), I
made the argument take a string, while with zfs_prop_inherit() I added a
new function user for user properties, zfs_prop_inherit_user().  I''m
still not sure which is better, but one will have to win out, so let me
know which makes more sense.

Other than that, it should be self-explanatory.  Adding user properties,
and setting properties required some massive changes, so let me know if
anything isn''t clear.

I''d like to get these into build 48, so if you can get review comments
to me by Monday next week it would be greatly appreciated.

- Eric

[1] I know the bug links are internal to Sun only, but as long as the
    majority of reviewers are Sun, this makes more sense.

--
Eric Schrock, Solaris Kernel Development       http://blogs.sun.com/eschrock

On 8/23/06, Eric Schrock <eric.schrock at sun.com>
wrote:
> I''m going to start posting my codereviews externally, as
we''re beginning
> to get more people outside of Sun (and the ZFS team) involved in the
> code.  So all are welcome to comment, or simply peruse the changes to
> see what''s in store[1].
>
excellent, its nice to see what is getting fixed and new features as
they emerge.

James Dickens
uadmin.blogspot.com
> This wad addresses the following bugs and RFEs:
>
> PSARC 2006/486 ZFS canmount property
> PSARC 2006/497 ZFS create time properties
> PSARC 2006/502 ZFS get all datasets
> PSARC 2006/504 ZFS user properties
> 6269805 properties should be set via an nvlist.
> 6281585 user defined properties
> 6349494 ''zfs list'' output annoying for even moderately
long dataset names
> 6366244 ''canmount'' option for container-like
functionality
> 6367103 create-time properties
> 6416639 RFE: provide zfs get -a
> 6437808 ZFS module version should match on-disk version
> 6454551 ''zfs create -b blocksize filesystem'' should fail.
> 6457478 unrecognized character in error message with ''zpool create
-R'' command
> 6457865 missing device name in the error message of ''zpool
clear'' command
> 6458571 zfs_ioc_set_prop() doesn''t validate input
> 6458614 zfs ACL #defines should use prefix
> 6458638 get_configs() accesses bogus memory
> 6458678 zvol functions should be moved out of zfs_ioctl.h
> 6458683 zfs_cmd_t could use more cleanup
> 6458691 common routines to manage zfs_cmd_t nvlists
> 6460398 zpool import cores on zfs_prop_get
> 6461029 zpool status -x noexisting-pool has incorrect error message.
> 6461223 index translations should live with property definitions
> 6461424 zpool_unmount_datasets() has some busted logic
> 6461427 zfs_realloc() would be useful
> 6461757 ''zpool status'' can report the wrong number of
persistent errors
> 6461784 recursive zfs_snapshot() leaks memory
>
> Webrev at:
>
> http://cr.grommit.com/~eschrock/zfs-prop/
>
> Note for zfs_prop_set() and zfs_prop_inherit(), I took two different
> approaches for dealing with user properties.  With zfs_prop_set(), I
> made the argument take a string, while with zfs_prop_inherit() I added a
> new function user for user properties, zfs_prop_inherit_user(). 
I''m
> still not sure which is better, but one will have to win out, so let me
> know which makes more sense.
>
> Other than that, it should be self-explanatory.  Adding user properties,
> and setting properties required some massive changes, so let me know if
> anything isn''t clear.
>
> I''d like to get these into build 48, so if you can get review
comments
> to me by Monday next week it would be greatly appreciated.
>
> - Eric
>
> [1] I know the bug links are internal to Sun only, but as long as the
>     majority of reviewers are Sun, this makes more sense.
>
> --
> Eric Schrock, Solaris Kernel Development      
http://blogs.sun.com/eschrock
> _______________________________________________
> zfs-code mailing list
> zfs-code at opensolaris.org
> http://opensolaris.org/mailman/listinfo/zfs-code
>

Eric Schrock wrote:
> http://cr.grommit.com/~eschrock/zfs-prop/
Shouldn''t dsl_prop_get* all take into account the inheritability of 
properties?  It looks like non-inheritable properties will be inherited 
unless you are using dsl_prop_get_all().  It seems like the check for 
inheritability needs to be in dsl_prop_get_impl() too.

--matt

Good point.  The current ''canmount'' property is never accessed
in the
kernel via dsl_prop_get_impl(), but future DSL-stored non-inheritable
properties might be.  I''ll add the logic to dsl_prop_get_impl() as
well.

- Eric

On Wed, Aug 30, 2006 at 05:41:32PM -0700, Matthew Ahrens
wrote:
> Eric Schrock wrote:
> >http://cr.grommit.com/~eschrock/zfs-prop/
> 
> Shouldn''t dsl_prop_get* all take into account the inheritability
of
> properties?  It looks like non-inheritable properties will be inherited 
> unless you are using dsl_prop_get_all().  It seems like the check for 
> inheritability needs to be in dsl_prop_get_impl() too.
> 
> --matt
--
Eric Schrock, Solaris Kernel Development       http://blogs.sun.com/eschrock

> 
> Webrev at:
> 
> http://cr.grommit.com/~eschrock/zfs-prop/
> 
I haven''t looked at all of it, but here is what I''ve noticed.

zfs_main.c

    Should use strlcpy() instead of strcpy() in get_callback().

    What are your plans for ZFS_ABORT env variable?  You should
    probably remove it before putback.

libzfs_dataset.c

    prop_parse_xxx functions should all check that nvpair_xxx_xxx()
    functions succeed before comparing the value of "value".
    I would suggest just wrapping a verify() around the nvpair_xxx_xxx()
    function, since they should never fail.

    Lines 283-2838 Should check that cp isn''t null before setting it to
    ''\0''.  The strchr() call could return null if
''@'' wasn''t in the
    string.

    strcpy() should use strlcpy() and strcat() should use strlcat().


zfs_ioctl.c

   put_nvlist() probably shouldn''t set zc_nvlist_dst_size when
   nvlist_pack fails.

Thanks.  Eric Kustarz has also done a thorough review, so don''t sweat
it
if you can''t get to everything.

On Thu, Aug 31, 2006 at 03:41:30PM -0600, Mark Shellenbaum
wrote:
> 
> zfs_main.c
> 
>    Should use strlcpy() instead of strcpy() in get_callback().
Sure thing.
>    What are your plans for ZFS_ABORT env variable?  You should
>    probably remove it before putback.
No, this is intentional.  It has been a part of the ZFS CLI utilities
since the intitial ZFS putback, as it allows for post-mortem
''::findleaks'' checks.  All I did was add it to the usage()
case as well.
> 
> libzfs_dataset.c
> 
>    prop_parse_xxx functions should all check that nvpair_xxx_xxx()
>    functions succeed before comparing the value of "value".
>    I would suggest just wrapping a verify() around the nvpair_xxx_xxx()
>    function, since they should never fail.
Sure.  It''s pretty pointless, since the only way they can fail is if
there is a bug in libnvpair.  Usually I use VERIFY() when it
"shouldn''t
fail unless we''ve screwed up", versus "shouldn''t
fail unless libnvpair
is broken".  But for userland it doesn''t hurt.
>    Lines 283-2838 Should check that cp isn''t null before setting
it to
>    ''\0''.  The strchr() call could return null if
''@'' wasn''t in the
>    string.
Sure, though this is unrelated to my changes.  I don''t really
understand
the receive code so I''d rather not touch it if it can be avoided.
>    strcpy() should use strlcpy() and strcat() should use strlcat().
Sure thing.
> zfs_ioctl.c
> 
>   put_nvlist() probably shouldn''t set zc_nvlist_dst_size when
>   nvlist_pack fails.
Yep.

Thanks,

- Eric

--
Eric Schrock, Solaris Kernel Development       http://blogs.sun.com/eschrock