Re: [libvirt] [PATCH v2 1/9] hostdev: Add virHostdevOnlyReattachPCIDevice()

w/r/t: your [0/7] from initial series... As much as you don't want to keep living Groundhog Day - resolution of bugs like this are job security :-)... The subtleties of hostdev device Attach, Reattach, ReAttach, and Detach are such a tangle morass of shinola (ref: Steve Martin in "The Jerk")... w/r/t Suggestions for deamon restart issues... Seems like we need a way to save/restore the hostdev_mgr active/inactive lists using XML/JSON similar to how domains, storage, etc. handle it. Guess I just assumed that was already there ;-) since /var/run/libvirt/hostdevmgr exists. It seems that network stuff can be restored - virHostdevNetConfigRestore. Do you really think this series should be "held up" waiting to create some sort of status tracking? On 01/25/2016 11:20 AM, Andrea Bolognani wrote: Since I reviewed them all... I think the comment changes from 7/9 should just be inlined here and patch 4 instead of a separate patch Why not just reuse the function name you deleted? IOW: Is there a reason for "Only"? (not that I'm one that can complain about naming functions, but this just seems strange). Interesting ... In 2 instances, this will be a pointer to the "copy" element, while in the third instance this will be the "actual" on inactive list element. For a copy element, we'd *have* to search inactive; however, for an 'actual' we don't "need" to. This is tricky - the only time we care about the return status (now) is when skipUnmanaged == false, a/k/a the path where we pass the onlist element.. In my first pass through the changes I thought - oh no if we return -1, then this would be a path that could get that generic libvirt failed for some reason message; however, closer inspection noted that we guaranteed it was on the inactive list before calling here. <sigh>, unrelated and existing - virPCIDeviceGetManaged probably should return bool instead of unsigned int Existing, but if retries == 0, then cleanup never succeeded; however, perhaps one can assume that the subsequent call would fall over and play dead? Not that it really gets checked... Curious why the decision for a second loop - if activeDev matches, then it almost seems a shame to loop again. Why not (within if (activeDev): actual = virPCIDeviceListSteal(hostdev_mgr->activePCIHostdevs, activeDev); /* !actual should never happen, but better safe than sorry */ if (actual && virPCIDeviceListAdd(hostdev_mgr->inactivePCIHostdevs, actual) < 0) virPCIDeviceFree(actual); /* You could also... */ virPCIDeviceListDel(pcidevs, dev); } NOTE: Whether there is one or two loops, the second loop would need to call virPCIDeviceFree(actual) since we'd leak it otherwise. You'll also note I didn't keep the goto cleanup. Previously the code would completely go through the pcidevs Loop 4 regardless of whether virHostdevReattachPCIDevice code had failures. The new code has the subtle difference of jumping to cleanup if a failure is found. That could leave things in an awkward state especially since virHostdevReAttachPCIDevices is a void. Since failure for DeviceListAdd is because 1. device is already there (which I would *hope* isn't the case) or 2. memory allocation failure (in which case not much else successful will probably happen anyway), then perhaps continuing on rather than jumping to cleanup isn't a bad idea... We could be returning some memory that someone may find useful. My concerns about jumping to cleanup are that this API is called in error recovery paths as well as part of the ominous comment "For upgrade purpose:..." (comment before function header). So it seems the "existing logic" is try to clean up as many as possible. By potentially erroring out too soon could lead to more problems. So the question becomes what havoc is introduced if we cannot add to the inactive list but decide to continue as before... It seems we'll end up "failing" in virHostdevOnlyReattachPCIDevice since it's not in the inactiveList, but our Loop 4 logic doesn't care. Of course we could delete 'dev' from 'pcidevs' too before then... Hopefully this makes sense... It's been an 'edit in process'... This was a curious placement for *ListDel... If !activeDev, then calling *ListDel also won't find 'dev' on activelist... If the choice is to use two loops (and perhaps keep the cleanups)... 1. If this Steal fails, then something is seriously wrong, but we don't even give it a VIR_DEBUG message. 2. If this Steal fails, then going to cleanup is again a subtle difference with the prior logic that said, well I couldn't do anything with this, but I'm going to keep processing. 3. If we keep processing, then something on 'pcidevs' won't be in 'inactivePCIHostdevs', causing virHostdevOnlyReattachPCIDevice to fail. But that does not matter since we ignore the return value in Loop 4. 4. If we do Steal and if the subsequent Add fails, then we leak 'actual', so prior to the goto cleanup call virPCIDeviceFree(actual); (or instead if the goto cleanup;'s are removed). John