I've read through your replies. All sounds good to me. Glad I could be of help.
Joe Evan Layton wrote: > 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 >
