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
--------------------------------------------------
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 :)
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.
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));
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));
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));
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.
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));
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));
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));
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 issue, only with fs_names on lines:
142 be_print_err(gettext("be_init: failed to lookup fs "
143 "attributes\n"));
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"));
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));
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.
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?
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
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.
This same issue could also happen on line 1000.
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
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;
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.
e.g. (and in other places...)
1263 ret = errno;
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.
That's it.
Hope this helps.
Joe
