Evan,
General comment - Update all files with copyright date of 2008 to 2009. be_activate.c --------------------- 879 - This can be "%s%s" now. 1053-1058, 1086-1091 (old file) - Can you really remove these chunks? zfs_prop_get() doesn't suffer from the same lack of error code return as zfs_prop_get_int(), so a return of != 0 here would mean an error did indeed happen doesn't it? I just looked at the zfs_prop_get() code, and I think its an interface issue really. For cases where a dataset has no origin, it returns a -1 to indicate this unfortunately which is the same thing it returns on other errors, so there isn't a way for us to determine if a real error happened, or if the dataset just doesn't have an origin. I also looked at the zfs cli code. When it uses zfs_prop_get() to query the ORIGIN property, it simply interprets a != 0 return as 'dataset does not have a parent', so I suppose its okay if we interpret it that way here too. But if we do this, lines 1112-1147 (new file) can be simplified much more. be_create.c ------------------ 1563 - this is a place that should use the new private 'don't mount everything' flag. be_list.c ------------- 309 - wrong function name "be_remove_menu" Am I missing something? What in be_get_list_callback() requires access to the pool dataset? be_mount.c ------------------ 332-343 - If we're really mounting root only, this chunk needs to be at line 320 instead of here. 696 - This is another one of those places that would benefit from not having everything else mounted. But it should only do this when not processing a zone's legacy fs. i.e. zoneroot_ds ? BE_MOUNT_PRIV_NULL_FLAGS : BE_MOUNT_PRIV_ROOT_ONLY 926 - Is this comment still accurate? This function doesn't appear to determine if the pool's dataset is mounted. It looks like all of the callers of this function check that themselves before calling this function. 939 - All callers of be_mount_pool() check ahead of time whether or not they need to mount the pool, so this argument is not needed in this function. The callers can set the flag locally when they've called be_mount_pool() 980, 984 - It may have been a bug, but before, I recall that changing the mountpoint for a dataset automatically mounts the dataset if its canmount property is set to "on". If this is still the case, the zfs_mount() call at 984 would fail since the dataset is already mounted from the change in the mountpoint setting. Are you seeing this? 1029 - it -> if 1031 - "If this is NULL on return ..." This portion of the comment doesn't make sense. Don't you want to say, "if this is NULL ..." 1054-1055 - This error should say something more along the lines of: "failed to set mountpoint for dataset (%s) to %s: %s\n" 1060 - For cases where orig_mntpnt is non NULL, we also need to cleanup and remove the temp directory that the pool dataset was mounted on. be_utils.c --------------- 366,635,1074,1219,1434,1695 - Set variable just free'ed to NULL after these lines, just in case. 1017,1216 - This can be "%s%s" now. 1301 - What is the reason why this is changed from strcmp to strstr? Also, this space " " should be BE_WHITE_SPACE 1421 - I don't think you want to ZFS_CLOSE() this here. In fact, this function needs a cleanup: jump like the others. libbe.h: ----------- 177 - nit - Can this be named to BE_MOUNT_FLAG_NULL libbe_priv.h: ------------------- 63 - nit - Can this be named BE_MOUNT_PRIV_FLAG_NULL 64 - nit - Can this be named BE_MOUNT_PRIV_FLAG_ROOT_ONLY 64 - nit - any reason why you chose that digit? If you're looking to use the upper order bits, it should be 0x00010000 thanks, -ethan Evan Layton wrote: > The webrev at http://cr.opensolaris.org/~evanl/9594 has been updated > with these changes. > > -evan > > Evan Layton wrote: >> sanjay nadkarni (Laptop) wrote: >>> >>> be_activate.c: >>> 601: it should be if (err == 0) >>> That said I don't quite follow the logic 599 - 603. >>> if err is zero then err is assigned to ret which has been set to >>> zero 2 lines above. >> >> This is to make sure we don't miss any errors from above. However it >> should have been: >> >> 596 >> 597 cleanup: >> 598 if (pool_mounted) { >> 599 int ret = 0; >> 600 ret = be_unmount_pool(pool_zhp, orig_mntpt); >> 601 if (err == 0) >> 602 err = ret; >> 603 } >> 604 free(orig_mntpt); >> 605 ZFS_CLOSE(pool_zhp); >> 606 >> >> >>> >>> be_mount.c >>> nit. for better readability you might consider defining null macros >>> for flags or priv_flags >>> such as: >>> #define NULL_FLAGS 0 >>> #define NULL_PRIV_FLAGS 0 >>> since _be_mount is called from a number of places. >> >> I agree that will definitely make things more readable. I'll change >> this throughout the code and I'll send out an updated webrev as soon >> as I have this changed. >> >>> >>> be_mount.c: >>> typos: >>> 925 - fucntion->function. >>> 933: I think you mean "if" >>> The original mountpoint for the pool if a temporary mount point was >>> needed. >> >> Fixed. >> >> Thanks for looking at this for me! >> >> -evan >> >>> >>> >>> -Sanjay >>> >>> Evan Layton wrote: >>>> I need to get a code review for the following fixes. >>>> >>>> webrev: >>>> ------------ >>>> http://cr.opensolaris.org/~evanl/9594 >>>> >>>> >>>> Bugids: >>>> ----------- >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=8638 >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=9051 >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=9594 >>>> >>>> Some back ground: >>>> The main bug is 9594 which was an oversight in the original design >>>> where we did not take into account the possibility that the root >>>> pool dataset (/rpool) was not mounted. To fix this we attempt to >>>> mount it at it's normal mount point, if that mount fails we attempt >>>> to mount it on a temporary mount point. >>>> >>>> We also discovered we were erroneously checking for error conditions >>>> from libzfs property checking functions such as zfs_prop_get_int(). >>>> These functions are expected to allows succeed and do not set the >>>> libzfs error codes. The incorrect error checking was also fixed as >>>> part of this fix. >>>> >>>> >>>> Thanks, >>>> -evan >>>> _______________________________________________ >>>> caiman-discuss mailing list >>>> caiman-discuss at opensolaris.org >>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>> >> > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
