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.
be_snapshot.c:
----------------------
138 - tabbing is off.
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.
512 - This has nothing to do with your changes, but since
you're here, need to call ZFS_CLOSE(zhp) here before
returning.
735 - tabbing is off.
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.
2497,2501,2510 - ret isn't being used as a be_errno_t
here, so we shouldn't use BE_SUCCESS at these lines.
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));
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.
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.
852 - Same comment for the usage of 'err' in be_get_ds_data().
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.
948 - Same comment for the usage or 'err' in be_get_ss_data().
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.
be_create.c
------------------
521 - tabbing is off.
620,624 - Why the change from zret to ret ?
999 - tabbing is off.
1617 - tabbing is off.
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.
be_activate.c
--------------------
153-154 - This shouldn't be changed. zpool_iter() explicitly
returns 0 for success.
thanks,
-ethan
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
