Ethan Quach wrote:
>
>
> Ethan Quach wrote:
>>
>>
>> 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) {
>
> Ooops, this should have been a " == 0"
I saw that! Thanks for the info. I saw that zfs_prop_get returned -1 on
error, but wasn't sure what to do with it, so that clears that up. I've
made this change (here and at 879), and a webrev has been posted at
http://cr.opensolaris.org/~kemitche/3837b/
>
>
>> 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?
It turns out they updated some of their scripts or something, and I've
managed to find some of the bugs in them ;) Hopefully it will be running
ok now.
>>
>>> 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
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
