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
