Keith Mitchell wrote:
>>
>> be_list.c
>> -------------
>> 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.
I just looked at the code[1], zfs_prop_get() returns 0 on success
and -1 on error. We should definitely not be returning -1 from
our function here in any cases. I looked around at our other
uses of zfs_prop_get() and a non-zero return is (except for one
special usage of it in be_activate.c) always interpreted as an
unrecoverable error and we return at that point right away.
An non-zero return from zfs_prop_get() isn't a normal
occurrence so I think that the existing err path here probably
wasn't ever exercised at all anyway.
So I think this code should be:
if (zfs_prop_get(zhp, ZFS_PROP_MOUNTPOINT, prop_buf,
ZFS_MAXPROPLEN, NULL, NULL, 0, B_FALSE) != 0) {
be_node->be_mntpt = strdup(prop_buf);
} else {
return (zfs_err_to_be_err(g_zfs));
}
And the last line of the function should return BE_SUCCESS
explicitly.
[1] fyi, the zfs code lives in the ON gate. On the swan, you can
get to it here /ws/onnv-gate
>
>> 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.
See above. It applies here as well.
>>
> Thanks for the comments. By the way, if you have any tips for getting
> DIY to run, I'm having issues with that...
I've actually yet to do a DIY request for libbe changes so can't
really help you there. What are you having issues with?
> I'll post an updated webrev once I know what to do in regards to the
> zfs_prop_get question above.
Okay, I'll have another go around when you've updated
everything.
thanks,
-ethan
>
> - 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
