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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list