Ethan Quach wrote: > > > 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.
Done > > > 397-401 and 412-416 are duplicate chunks. Seems like that > chunk can just exist once, after line 417. Definitely better, I remove the duplicate chunk. > > 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. Nope it's not true... I fixed the comment that I had missed. > > 739,740 - Initialize to NULL > > 895,896 - Initialize to NULL > > 1002,1003,1004 - Initialize to 0/NULL All of these are fixed. > > >> >> >>> >>> 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. > As far as the changes you mentioned for lines 342 and 373-379 I had a change of heart and I agree that this fix is probably the proper place for this. I've changed the call to zfs_iter_filesystem so we're no longer calling this for snapshots we don't need to. I added a call to zfs_get_type to check for the snapshots as well and moved the snapshot processing up so that we only have to check for snapshots once. > 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. For the rest of the comments below I'd still prefer to do them as part of the bugs mentioned before. One of the reasons for this is that I'll be changing the structure so that the snapshots get attached to the dataset they are a snapshot of, not just attached to the BE node. When this change is made all the functions will also need to change. For these changes I wanted to iterate into a dataset and from the dataset into it's snapshots. Based on this I'd like to handle the rest of your comments with these later changes. I've updated the webrev with the changes mentioned above. Thanks! -evan > > > 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 >>> >> >>
