Joseph J VLcek wrote:
> Evan Layton wrote:
>> I've updated the webrev with the comments I've received so far.
>>
>> I added two new functions, used within the library only, that will map a zfs
>> error to a BE error (be_errno_t) and an errno to a BE error. I've also
>> increased
>> the number of errors defined in libbe.h to accommodate this mapping and
>> provide
>> the errors asked for in 2058 (I closed 2058 as a dup of 1817).
>>
>> The link to the webrev is the same as before.
>>
>> Thanks,
>> -evan
>>
>> Evan Layton wrote:
>>> Based on conversations with Dave and others I'm rethinking some of this so
>>> there
>>> will be some changes. I'll send out an updated webrev when I have this done.
>>>
>>> Thanks,
>>> -evan
>>>
>>> Evan Layton wrote:
>>>> This fix adds the use of error codes throughout libbe. All functions
>>>> should now return the errors codes defined in libbe.h.
>>>>
>>>> I've also added the use of an environment variable (BE_PRINT_ERR) that
>>>> enables printing the error output in the library without having to
>>>> compile in changes to enable this printing.
>>>>
>>>> Defect:
>>>> ------
>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=1817
>>>>
>>>> Webrev:
>>>> ------
>>>> http://cr.opensolaris.org/~evanl/1817/
>>>>
>>>> Thanks,
>>>> -evan
>>>> _______________________________________________
>>>> 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
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
>
> Evan,
>
> I think cleaning up the messages is great! I hope my input helps.
>
> Joe
Hi Joe,
Thanks for the nicely constructed comments! I'm going to do most of the
clean-up
you've sugggested even though when we fix 2053 all these be_print_err lines
will
go away in favor of dtrace scripts which will give us basically the same thing
but without all these print commands inside a library.
More comments in-line.
Thanks again for the great comments.
-evan
>
>
> --------------------------------------------------
>
> Issue 1:
> --------
>
>
> There is no issue 1... I had an issue 1 but I realized it wasn't an
> issue after all and I did not want to renumber the other issues :)
>
hehe :-D
>
>
> Issue 2: file be_activate.c
> --------
>
> 218 be_print_err(gettext("set_bootfs: failed to open pool: %s\n"),
> 219 libzfs_error_description(g_zfs));
>
>
> I think this should be:
>
> 218 be_print_err(gettext("set_bootfs: failed to open pool: %s %s\n"),
> 219 boot_rpool, libzfs_error_description(g_zfs));
>
> I think lines 227->228 has this correct.
sounds fine, fixed...
>
> Issue 3: file be_activate.c
> --------
>
> 268 be_print_err(gettext("set_canmount: failed to open "
> 269 "dataset: %s\n"), libzfs_error_description(g_zfs));
>
> Same issue as issue #2
>
> I think you want the dataset name followed by what is returned from
> libzfs_error_description()
>
> I think this should be:
>
> 268 be_print_err(gettext("set_canmount: failed to open "
> 269 "dataset: %s %s\n"), ds_path, libzfs_error_description(g_zfs));
fixed
>
> Issue 4: file be_activate.c
> --------
>
> Same issue as issue #3
>
> 277 be_print_err(gettext("set_canmount: failed to "
> 278 "open dataset: %s\n"),
> 279 libzfs_error_description(g_zfs));
>
> I think this should be:
>
> 277 be_print_err(gettext("set_canmount: failed to "
> 278 "open dataset: %s %s\n"),
> 279 ds_path, libzfs_error_description(g_zfs));
>
fixed
>
> Issue 5: file be_activate.c
> --------
>
> I also think having "canmount" in the error test is redundant since
> set_canmount is already there.
>
> I think you might also want the dataset name followed by what is
> returned from libzfs_error_description()
>
>
> 305 be_print_err(gettext("set_canmount: failed to "
> 306 "set canmount dataset property: %s\n"),
> 307 libzfs_error_description(g_zfs));
>
> Assuming ds_path contains the correct dataset path at this point in the
> code. I think/propose this should be:
>
> 305 be_print_err(gettext("set_canmount: failed to "
> 306 "set dataset property: %s %s\n"),
> 307 ds_path, libzfs_error_description(g_zfs));
fixed
>
> Issue 6: file be_activate.c
> --------
>
> Same issue as issue #3
>
> 331 be_print_err(gettext("set_canmount: "
> 332 dataset: %s\n"),
> 333 libzfs_error_description(g_zfs));
>
> I think this should be:
>
> 331 be_print_err(gettext("set_canmount: "
> 332 dataset: %s %s\n"),
> 333 ds_path, libzfs_error_description(g_zfs));
>
> I think lines 321->323 has this correct.
fixed
>
> Issue 7: file be_activate.c
> --------
>
> 344 be_print_err(gettext("set_canmount: "
> 345 "Failed to promote the dataset\n"));
>
> It might be valuable to have the dataset name displayed in the error
> message.
>
> I think this should be:
>
> 344 be_print_err(gettext("set_canmount: "
> 345 "Failed to promote the dataset: %s\n", ds_path));
>
>
fixed
> Issue 8: file be_activate.c
> -------
>
> 361 be_print_err(gettext("set_canmount: "
> 362 "Failed to set canmount dataset "
> 363 "property: %s\n"),
> 364 libzfs_error_description(g_zfs));
>
> I think the message could be improved by including the dataset name and
> property value.
>
> Perhaps this could be changed to:
>
> 361 be_print_err(gettext("set_canmount: "
> 362 "Failed to set property value: %s for dataset %s %s \n"),
> 364 value, ds_path, libzfs_error_description(g_zfs));
fixed
>
> Issue 9: file be_create.c
> -------
>
> Some message might benefit by including values.
>
> 116 be_print_err(gettext("be_init: failed to lookup "
> 117 "BE_ATTR_NEW_BE_NAME attribute\n"));
>
> e.g. including the be name might make this message more useful.
>
> 116 be_print_err(gettext("be_init: failed to lookup "
> 117 "BE_ATTR_NEW_BE_NAME attribute for %s\n", bt.nbe_name));
bt.nbe_name won't have anything in it if the nvlist call fails so putting this
here wouldn't work.
>
> Where you feel it makes sense, please consider adding values to other
> messages throughout the code.
>
> Some other locations (but not all) in the code were I think values might
> make the message clearer are:
>
> Same issue, only with be_zpool name on lines:
>
> 131 be_print_err(gettext("be_init: failed to lookup "
> 132 "BE_ATTR_NEW_BE_POOL attribute\n"));
Same isue with all these if the lookup fails these won't have anything in them
and so we'd be attempting to use them before they set (or with a NULL value).
>
> Same issue, only with fs_names on lines:
>
> 142 be_print_err(gettext("be_init: failed to lookup fs "
> 143 "attributes\n"));
>
Same issue here
> Same issue, only with fs_num and nelen on lines:
>
> 147 be_print_err(gettext("be_init: size of FS_NAMES array does not "
> 148 "match FS_NUM\n"));
Should be able to add values here.... (done)
>
> Issue 10: file be_list.c
> ---------
>
> 922 be_print_err(gettext("be_add_children_callback: %s\n"),
> 923 libzfs_error_description(g_zfs));
>
> Should this message include "failed" or maybe "encountered error" ?
>
> Please consider this suggestion:
>
> 922 be_print_err(gettext("be_add_children_callback: encountered error:
> %s\n"),
> 923 libzfs_error_description(g_zfs));
done
>
> Issue 11: file be_list.c
> ---------
>
>
> 973 temp_node = list;
> 974 list = list->be_next_node;
> 975 if (temp_node->be_node_name != NULL)
> 976 free(temp_node->be_node_name);
>
> Looking at this code I think there might be a possible bug.
>
> What if temp_node is NULL? Line 975 would be invalid.
Can't be since it's set to list and if list was NULL we wouldn't be in the
while
loop still.
>
>
>
> Issue 12: file be_list.c
> ---------
>
> A suggestion.
>
> In the past I have suggested not checking if something is NULL before
> passing it to free so I understand that you like to do that but wouldn't
> A macro to check if what is about to be freed is null might make the
> code more readable?
Yep we've talked about this before. It's not a big issue I just like to do
the check because the if statement is less expensive than the context switch
to free(). I don't think the if statements have a readability issue. I'm not
all that found of macros as they can often tend to make the code harder to
read and often much harder to debug especially when having to dig through a
crash dump using mdb. ( at least for me... ;-) )
>
> e.g.:
>
> #define freeIfNotNULL( variable ) \
> if( (variable) != NULL ) \
> free( (variable) )
>
> Then, for example, the block of code at lines 974 -> 985 could be:
>
> if (temp_node != NULL ) {
> freeIfNotNULL(temp_node->be_node_name);
> freeIfNotNULL(temp_node->be_root_ds);
> freeIfNotNULL(temp_node->be_rpool);
> freeIfNotNULL(temp_node->be_mntpt);
> freeIfNotNULL(temp_node->be_policy_type);
> freeIfNotNULL(temp_node);
> }
>
>
> Issue 13: file be_mount.c
> ---------
>
> My comment regarding including values in error message above can also
> apply to some of the error messages in file be_mount.c
Same problem as before where if the lookup fails we don't have a valid
value to print out.
>
> Issue 14: file be_mount.c
> ---------
>
> 691 if ((ret = _be_unmount(be_name, 0)) != 0) {
>
> Line 691 will overwrite ret, which could have been set earlier. Which
> could result in losing track of the prior error.
>
> This may or may not be OK because the unmount error may reflect the
> earlier error. However the mount may succeed, setting ret to SUCCESS,
> overwriting the error, for example that happened on line 679.
Right I should be using err to grab the error from _be_mount() and only setting
ret equal to it if there is a failre returned from _be_mount().
fixed.
>
> This same issue could also happen on line 1000.
Here we only change ret if there is an error from zfs_prop_set. We need to
return this error and don't have a good way to return both so we end up
returning the last error.
>
> Issue 15: file be_rename.c
> ---------
>
> My comment regarding including values in error message above can also
> apply to some of the error messages in file be_rename.c
I've changed what I can here.
>
>
> Issue 16: file be_snapshot.c
> ---------
>
> My comment regarding including values in error message above can also
> apply to some of the error messages in file be_snapshot.c
>
>
> Issue 17: file be_snapshot.c
> ---------
>
>
> ret needs to be initialized
>
> 384 int i, ret;
>
> should be:
>
> 384 int i, ret = 0;
>
fixed
>
> Issue 18: file be_utils.c
> ---------
>
> Nit only:
>
> ret is set to errno where in other code err is set to errno. I think it
> would be nicer to consistently user err = errno and not user ret =
> errno, but not necessary.
I cleaned these up.
>
> e.g. (and in other places...)
>
> 1263 ret = errno;
fixed
>
> Issue 18: file be_utils.c
> ---------
>
> 1455 if ((ret = _be_unmount(be_name, 0)) == 0) {
>
> Line 1455 will overwrite ret, which could have been set earlier. Which
> could result in losing track of the prior error.
>
line 1455 changed to if ((err = _be_unmount(be_name, 0)) == 0) {
and ret = err only if err is not 0.
>
> That's it.
>
> Hope this helps.
It does, Thanks!!!
>
> Joe
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
