Re: [libvirt] [PATCH 5/6] qemu: Reduce memlock limit after detaching hostdev

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 ;)