Ethan Quach wrote: > Evan, > > 179 - Does this need to get freed somewhere before the function > ends or is it referenced in cb somewhere?
Good catch, we use it in several places but we save it into the node structure using strdup() so yes this should be getting freed before we return from this function. > > 261-263 - This callback is used to iterate pools, and gets > called whether or not a specific BE name has been specified. > I think this description needs an overall update. Yes it does need a bit more work. I'll re-write the description a bit. > > 280 - Initialize to NULL done. > > 291 - Can you add a comment before this line noting it'll be > dealing with a BE root dataset if a specific be_name was passed > in, otherwise its a pool's BE container dataset. Definitely a good thing to do. I'll add the comment here and correct the comments shown below. > > 299, 304-306, 314 also need to updated accordingly. > > 323 - If err != 0, don't you need to check if cb->be_nodes_head > got allocated and then free it if so? If err != 0 cb->be_nodes_head should always be NULL since the calloc failed. I can probably just remove the check for err != 0 since it can only be set to non zero if cb->be_nodes_head is NULL. I've made the changes to this point and updated the webrev. > > 342 - Seems like you should be calling zfs_iter_filesystems() > here instead, that way it doesn't end up iterating through > snapshots of this pool's BE container dataset unnecessarily, > and then you can remove that code that specifically checks > for this at 373-379. > This and the two items below are things I had been talking about doing as part of the fix for 3168, 3169 and 3170. If you feel they should be done as part of this I can do that but I would prefer to keep them separate to make these changes a bit easier to work with. > > I think there can be some further reduction done here. > Instead of using be_add_children_callback() as the iteration > function that processes BE roots for the case where we're > listing all BEs (line 342), use be_get_node_data() as that > iteration. You'd have to modify be_get_node_data() to become > a callback, and inturn have it call be_add_children_callback() > to process children, but that shouldn't be too hard. This is definitely something I plan to do but I had purposely not done this here because I planned to redo these callback functions as part of the changes mentioned above. > > This would allow you to remove all the code in > be_add_children_callback() that checks if the dataset its > processing is a BE root. In addition, to determine if what > you're dealing with is a file system or a snapshot, you can > just use zfs_get_type() instead of those ugly string searches > for the characters '/' or '@' to determine that difference. Definitely! I do want to remove these very ugly string searches when I redo the node structure. Thanks! -evan > > > thanks, > -ethan > > > Evan Layton wrote: >> Hi, >> >> I need to get a review the following fix. This is one in a series >> of bugs related to code refactoring in libbe. >> >> 3608 libbe: Remove duplicate code from be_list >> http://defect.opensolaris.org/bz/show_bug.cgi?id=3608 >> >> Webrev: >> http://cr.opensolaris.org/~evanl/3608/ >> >> Thanks! >> -evan >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
