On 12/10/2015 12:36 PM, Andrea Bolognani wrote: > 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? > You mean of course before the pesky "virDomainHostdevDefFree(hostdev)"? You could perhaps do it after the qemuDomainRemovePCIHostDevice in the switch or within that qemuDomainRemovePCIHostDevice code. John >> It was clear to me when I wrote it ;-) > > Again, I know exactly what you mean ;) > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list