On Wed, 2016-03-16 at 19:50 +0100, Andrea Bolognani wrote:
Thinking in terms of current code - step4 will take the everything that was formerly on the active list and: 1. Steal the pcidevs entry 2. Call virHostdevReattachPCIDevice with the stolen lookaside pcidevs entry... While it appears to be an actual, it isn't. Of course, I think you address that in patch 22, but trying not to get too far ahead. 2a. If the device is not managed, then add to the inactiveList and return (call virPCIDeviceFree if unsuccessfully add). What really confuses me here is why we place onto the inactiveList if not managed; however, you address that in patch22. Still it could be addressed earlier if that is a separate bug... I would have to go back to patch 22 to make sure this is really handled correctly there: I'm pretty confident it is, but I've been wrong before! Most recently, while writing this series :( The incorrect behaviour you've noticed before has actually been introduced by patch 15. Before that, 'pcidevs' in virHostdevReAttachPCIDevices() was obtained by copying instances off the active list, hence virHostdevReattachPCIDevice() would really be passed (a copy of) an actual; now that's no longer the case, and the code has become incorrect. Will fix as part of the respin.
Turns out that fixing this in a separate patch would basically mean reverting the patch that introduced the behaviour in the first place. I don't think it would be worth the effort. I've confirmed that the fix is actually in patch 22, as suspected, so we should be good anyway. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team