Re: [libvirt] [PATCH 22/24] hostdev: Streamline device ownership tracking

On 03/07/2016 12:24 PM, Andrea Bolognani wrote: FWIW: from your cover - splitting this up... I think it's possible that the changes for calling virHostdevReattachPCIDevice with an actual device could be in a separate patch... But I certainly see how this and that are "tied" together especially if you're considering the failure path through reattachdevs where you've definitely put things on the inactiveList. There may also be a 4 patches overall in here since I see the Reattach code could place an unmanaged device on the inactiveList (which you modify in this patch) and I think the ReAttach code adjustments could be separate too. I think you're on the right track, with all these changes - it's some minor details. I've been looking at the changes, walking away, looking at them, so hopefully the following is followable! BTW: I chose not to look at the last 2 patches as I think there are too many questions left over This is so much more logical to me than the previous code. One question though - why not use virPCIDeviceListStealIndex? Theoretically the inactiveList and the pcidevs list is the same right? So for every devices on pcidevs - steal index 0 of inactiveList and place it on activeList. It's just a slight optimization. Help me out... Why were these lines removed? The inactiveDevs is initially a copy of whatever is on 'pcidevs'... Thus activeDevs takes from there and not pcidevs, right? Another option is to VIR_WARN (INFO, DEBUG, whatever) the failure. You may also want to clear the error as well (virResetLastError). There are two possible errors - it already exists on the list and memory allocation errors. The continue is superfluous since this is the end of the loop - it only matters if someone adds more checks afterwards "expects" As I read it virPCIDeviceReattach look for the device on the activeList, but will "blindly" remove from the inactiveList *if* provided. That's only true for xen_driver and virpcitest. I believe this process is the more correct one. Is it just me getting really confused - this code used to add devices to the inactiveList even though (at some point in time) the Prepare code would "drop" them? E.G. step 2 before any of these changes. Was this perhaps what caused you to add that PCIDeviceListFind code in step 2 of patch 21 that I questioned? Perhaps this alone deserves it's own patch and I think is it's own problem since the lookaside lists are for devices that are managed, right? Hmm... mental note - what changes here... to cause us not to need to free.... Answered my question below [1] during cleanup. So step1 now removes all pcidevs that are on the inactive list and that are unmanaged since "by rule" only a managed device could be on the activeList. I think you need to update the comment for step1 in any case since there's no removal from the activeList. Coverity notes that there's no error checking here like there is in other calls to virPCIDeviceListAdd. Perhaps similar to step 5 in prepare, we could use VIR_WARN. I also think it might be worthwhile to make a common routine to perform this step - the only difference being perhaps a 'return/fail on error' flag that would be set for Prepare and clear for ReAttach So yes, step 3 makes perfect sense now. Why use pcidevs? Why not just walk inactivePCIHostdevs? If I've read things right - there shouldn't be any unmanaged device in pcidevs at this point as they should have been removed during step1. In any case, if you walk inactiveList, then all this is moot. Also now that we're no longer managing the device it should be removed from inactive and free'd. IOW: THis loop becomes a while inactivePCIHostdevs has elements, call Reattach, then free the device. [1] There could be a memory leak here - I think we need to walk the remaining pcidevs entry and free each entry before unreffing the containing object. I think that's what that the previous step4 would do with the removed virPCIDeviceFree after the ListStealIndex.