Evan, 179 - Does this need to get freed somewhere before the function ends or is it referenced in cb somewhere?
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. 280 - Initialize to NULL 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. 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? 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. 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 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. 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
