Evan Layton wrote:
> I've made the changes to this point and updated the webrev. 350 - Can you also update this comment. For the case where you're getting data for just a specific BE, this iteration is just getting data for that BE's subordinate datasets. 397-401 and 412-416 are duplicate chunks. Seems like that chunk can just exist once, after line 417. 717 - Is this true? I don't see where this function is passed into a zfs_iter_* call. It can't be, its got too many arguments. 739,740 - Initialize to NULL 895,896 - Initialize to NULL 1002,1003,1004 - Initialize to 0/NULL > > >> >> 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. The changes I'm suggesting seem more appropriately related to 3608, but if you were planning to do these with the other bugs then I'm fine with that. thanks, -ethan > >> >> 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 >> > >
