On Thu, 2015-12-10 at 12:11 -0500, John Ferlan wrote:
Since the add was in qemuDomainAttachHostPCIDevice, why is the remove not in qemuDomainDetachHostPCIDevice? Doing it here would mean it's call for any host device removal - whether or not it was ever added.
Because qemuDomainDetachHostPCIDevice() is where we ask for the device to be removed from the guest, but we have to wait for it to actually be removed (and for the guest definition to be updated) before changing the memory locking limit.
I guess I could move this code to qemuDomainDetachThisHostDevice(), but keeping it close to where the guest definition is updated looks cleaner to me.
Yeah - this all got confusing as I was paging back and forth... "DetachDevice" and "RemoveDevice"... Then I reviewed another change for shmem and got even more confused.
I know what you mean: the code could sure use some moving around to make it a bit more symmetric. And what about the names? qemuDomainDetachDiskDevice() qemuDomainDetachDeviceDiskLive() Ehm... qemuDomainDetachHostPCIDevice() qemuDomainDetachHostDevice() qemuDomainDetachThisHostDevice() Oh boy :)
I also fail to see how a device that was never added could be removed, could you please elaborate?
Since qemuDomainRemoveHostDevice could be called for any host device removal and not specifically the HostPCIDevice, I was pointing out by calling qemuDomainAdjustMaxMemLock there you could be calling it even when a HostPCIDevice wasn't being removed.
Oh, I get it now. Calling qemuDomainAdjustMaxMemLock() more than necessary shouldn't really cause any harm, but adding a check for hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI around the call is even better. Would that address your concerns?
It was clear to me when I wrote it ;-)
Again, I know exactly what you mean ;) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team