I've made the following changes. A new webrev is available at http://cr.opensolaris.org/~evanl/9594v2
Ethan Quach wrote: > Evan, > > > General comment - Update all files with copyright date > of 2008 to 2009. fixed > > > be_activate.c > --------------------- > 879 - This can be "%s%s" now. fixed > > 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? Yes zfs_prop_get also doesn't appear to set the libzfs_errno on any of these failures so we end up with no way to tell which (if any) error happened or if this simply means that we're at the parent. > > 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. Right so all we know is there is no origin returned but have no information on why. Since the libzfs_errno is not reset to an error in these cases we can't use it to determine the difference between hitting the parent and some other error where no origin is returned. > > 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. I've simplified the code here. > > > be_create.c > ------------------ > 1563 - this is a place that should use the new private > 'don't mount everything' flag. good point I've changed it... > > > be_list.c > ------------- > 309 - wrong function name "be_remove_menu" fixed > Am I missing something? What in be_get_list_callback() > requires access to the pool dataset? Getting the default entry and it's bootfs line from the grub menu is what is needed for however since the call to be_default_grub_bootfs has this code in it already this is really overkill to have it here as well. I had done this so that we didn't have to mount the root pool for every BE we process but a smaller window with mounting and unmounting this is probably a better idea. I've removed this from be_list. > > > be_mount.c > ------------------ > 332-343 - If we're really mounting root only, this chunk > needs to be at line 320 instead of here. That would cause us to skip mounting any of the BE's subordinate datasets. Isn't it possible that we could need those for getting things like zones information? I can easily move this but I'm not sure this is the correct thing here. Maybe what we need is a flag for ROOT_ONLY as well as one for DS_ONLY (or NO_SHARED) that just mounts the BE and it's datasets? > > 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. fixed > > 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. fixed > > 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() > Good point I've removed this. > 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? That's not what I'm seeing. In this case since we're doing this through the library directly it does not appear to be mounting the dataset when the mountpoint is changed. In any case it's not causing an error when we attempt the mount after setting the mountpoint to a temporary mountpoint. > > 1029 - it -> if fixed > > 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 ..." fixed > > 1054-1055 - This error should say something more along > the lines of: "failed to set mountpoint for dataset (%s) to > %s: %s\n" fixed > > 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. fixed for this instance of the use of be_make_tmp_mountpoint()... > > > be_utils.c > --------------- > 366,635,1074,1219,1434,1695 - Set variable just free'ed to > NULL after these lines, just in case. fixed > > 1017,1216 - This can be "%s%s" now. fixed > > 1301 - What is the reason why this is changed from > strcmp to strstr? changed back to strcmp > Also, this space " " should be > BE_WHITE_SPACE fixed > > 1421 - I don't think you want to ZFS_CLOSE() this here. > In fact, this function needs a cleanup: jump like the others. good point, I was closing it since it wasn't needed for the rest of the function but it doesn't really make sense to have to open it again just to unmount it... fixed > > > > libbe.h: > ----------- > 177 - nit - Can this be named to BE_MOUNT_FLAG_NULL yes > > > 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 yes and yes > > 64 - nit - any reason why you chose that digit? If you're > looking to use the upper order bits, it should be > 0x00010000 0x00010000 is what was intended, thanks for catching that! Thanks for the comments! -evan > > > 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
