A new webrev has been posted at: http://cr.opensolaris.org/~kemitche/3837a/
Evan Layton wrote: > Keith Mitchell wrote: >> >>> >>> line 591 zret should be initialized to 0 (or BE_SUCESS since it's >>> used for both zfs_iter function return codes and be_clone_fs_callback). >> Done. >>> It might be best as part of this to clean up these return variables >>> so we make the use of things like err, zret and ret consistent. This >>> needs to be looked at throughout... >>> Maybe something like: >>> err for capturing errno's >>> zret for capturing zfs related errors >>> ret for returning BE errors and errors that have been translated >>> into BE errors. >> Ok. I'll go through again and clean up these instances. > > It's probably not worth having you look through all of these right now > but would be helpful to have you look at the areas that you've touched > only. As for the rest of these could you file a bug to state that we > should be more consistent with these error variables and attach that > to 9941 as well? Agreed. 10481 filed. http://defect.opensolaris.org/bz/show_bug.cgi?id=10481 > >>> >>> 1796 ret is defined here and again at line 1886. The second one >>> isn't needed and should be removed. >> Fixed. >>> >>> 2148 - 2150 returns err but doesn't set it so we could be sending an >>> error from somewhere else or just what was initialized. This needs >>> an actual error message. >> err is set at line 2139. However, The inner 'if' statement is kind of >> backwards. I'll change it to: >> >> 2139 if ((err = zfs_iter_filesystems(zhp, >> be_clone_fs_callback, bt)) != 0) { >> 2140 /* >> 2141 * Error occurred while processing a child dataset. >> 2142 * Destroy this dataset and return error. >> 2143 */ >> 2144 zfs_handle_t *d_zhp = NULL; >> 2145 2146 ZFS_CLOSE(zhp); >> 2147 2148 if ((d_zhp = zfs_open(g_zfs, clone_ds, >> ZFS_TYPE_FILESYSTEM)) >> 2149 != NULL) { >> (void) zfs_destroy(d_zhp); >> ZFS_CLOSE(d_zhp); >> 2150 2151 } >> 2155 return (err); >> 2156 } >> >> This is logically equivalent to the previous but doesn't have the >> same 'backwardness' to it. > > I think this is fine for now. What I was originally worried about was > the fact that if the zfs_destroy fails we would not catch that error > and we could silently leave the clone dataset hanging around. However > I think fixing that so we provide some notice that the clean-up after > the original failure was also not successful is really more something > that should be addressed in bug 9941 and the BE management > observability project. > > All this being said, I'm fine with the changes now. Excellent. As mentioned, a new webrev was posted. I had an issue setting up the DIY test so I'm still awaiting those results. - Keith > > -evan > >>> >>> be_mount.c: >>> =========== >>> lines 238 and 395 ret should also be set to BE_SUCCESS >> Fixed. >>> >>> 1441 set err = BE_SUCCESS? >> Yes, done. >>> >>> >>> Other than these I don't see anything that hasn't already been >>> mentioned. >>> >>> -evan >> New webrev will be posted after I've: >> * changed relevant "== 0" statements to "== BE_SUCCESS" >> * reviewed instances of err, errno, ret and zret for consistent usage >> * run the DIY libbe test against the changes >> >> - Keith >
