Hi Ethan, See comments inline. Ethan Quach wrote: > Keith, > > > General comment > ----------------------------- > The functions with a name ending in _callback are used as > callback functions passed to the recursive zfs iteration methods. > These zfs iteration methods explicitly look for a return code of > '0' as the success case from these callbacks (non-zero return > is considered failure and is passed through). So for these > functions, they should continue to return an explicit 0 for > success. Ok. I'll fix this and make note of the difference in the bug when I submit. > > > be_snapshot.c: > ---------------------- > 138 - tabbing is off. Thanks for pointing this out. My editor had tabs set to 4 spaces so it looked correct to me. Cstyle didn't catch it (though hg nits did, now that I've learned of that command). > > be_mount.c > ------------------ > 322,503,1220,2382,2456 - These are not part of your changes > but can you change these to 0. The zfs iteration methods > explicitly return 0 for success. Fixed. > > 512 - This has nothing to do with your changes, but since > you're here, need to call ZFS_CLOSE(zhp) here before > returning. Fixed. > > 735 - tabbing is off. Fixed. > > 1686 - err isn't ever used to hold a be_errno_t type in > this function, so initializing it to BE_SUCCESS isn't > necessary. Fixed. > > 2497,2501,2510 - ret isn't being used as a be_errno_t > here, so we shouldn't use BE_SUCCESS at these lines. Changed ret to zret. I also undid my change at line 2501, as returning "success" for this function doesn't make sense. > > be_list.c > ------------- > 748-749 - err doesn't need to be used here at all. These > two lines can just be: > > return(zfs_err_to_be_err(g_zfs)); Changed. > > > 707 - given the previous comment, 'err' isn't ever used > as a be_errno_t type in this function (in fact, its set to > errno at lines 719, 729, and 737) so it should stay > initialized to 0. Reverted. > > 777-782 - this is another usage of err that seems suspect. > These lines can be simplified to: > > if (zfs_prop_get(zhp, ZFS_PROP_MOUNTPOINT, prop_buf, > ZFS_MAXPROPLEN, NULL, NULL, 0, B_FALSE) != 0) > be_node->be_mntpt = NULL; > else > be_node->be_mntpt = strdup(prop_bug); > > > Otherwise, err seems like it would get set to some non-zero > number here and the function continues on, and at the end > of the function at line 820, we potentially return something > gathered from the zfs_prop_get() function call which is not > a be_errno_t type. This doesn't look right. What should the correct return be in this case? At present, if zfs_prop_get fails, be_node->be_mntpt is set to NULL, and the error code from zfs_prop_get is returned at the end of the function (which is not a be_errno_t, as you mention). Should the code actually stay as is, capturing err = zfs_prop_get? If so, is there a libzfs err to be err function somewhere, and should that conversion & return be done at the end of the function, or immediately after the failure? If not, with your suggested change, there is a change in functionality: a failure to find the mountpoint returns BE_SUCCESS (0) where previously it returned a non-zero error code.
> > > 852 - Same comment for the usage of 'err' in be_get_ds_data(). Reverted. > 879-885 can be simplified to: > > if (zfs_prop_get(zfshp, ZFS_PROP_MOUNTPOINT, > prop_buf, ZFS_MAXPROPLEN, NULL, NULL, 0, > B_FALSE) != 0) > dataset->be_ds_mntpt = NULL; > else > dataset->be_ds_mntpt = strdup(prop_buf); > > > Otherwise err gets set and return as a non be_errno_t > type. Same response as above. > > > 948 - Same comment for the usage or 'err' in be_get_ss_data(). Reverted. > > 987 - In this function, its pretty clear that 'err' could only be > equal to 0 when we get to this last line so I think returning > BE_SUCCESS explicitly here is what needs to be done. Fixed. > > > be_create.c > ------------------ > 521 - tabbing is off. Fixed. > > 620,624 - Why the change from zret to ret ? Not sure why I did that. Reverted. > > 999 - tabbing is off. Fixed. > > 1617 - tabbing is off. Fixed. > > 2768-2769 - Change these lines to > return(zfs_err_to_be_err(g_zfs); > > and then change 'ret' back to being initialized to 0 > at line 2731. This makes it more consistent since > 'ret' is used not as a be_errno_t at 2819. Done. Also renamed ret to err. > > > be_activate.c > -------------------- > 153-154 - This shouldn't be changed. zpool_iter() explicitly > returns 0 for success. Fixed. Also changed err to ret, and used zret to capture result from zpool_iter. > > > thanks, > -ethan > Thanks for the comments. By the way, if you have any tips for getting DIY to run, I'm having issues with that... I'll post an updated webrev once I know what to do in regards to the zfs_prop_get question above. - Keith > > > Keith Mitchell wrote: >> 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 >>> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
