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"
> 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
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
