Re: [libvirt] [PATCH v13 02/49] qemu: reuse hostdev interfaces to avoid duplicate

On Sat, Mar 01, 2014 at 02:28:57PM +0800, Chunyan Liu wrote: > Same logic of preparing/reattaching hostdevs could be used in attach/detach > hotplug places, so reuse hostdev interfaces to avoid duplicate, also for later > extracting general code to common library. > > Signed-off-by: Chunyan Liu <cyliu(a)suse.com&gt; > --- > src/qemu/qemu_hostdev.c | 8 +++--- > src/qemu/qemu_hostdev.h | 11 +++++++- > src/qemu/qemu_hotplug.c | 61 +++------------------------------------------- > 3 files changed, 18 insertions(+), 62 deletions(-) > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 6703c92..5546693 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -1454,28 +1454,16 @@ qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, > virDomainHostdevDefPtr hostdev) > { > qemuDomainObjPrivatePtr priv = vm->privateData; > - virUSBDeviceList *list = NULL; > - virUSBDevicePtr usb = NULL; > char *devstr = NULL; > bool added = false; > bool teardowncgroup = false; > bool teardownlabel = false; > int ret = -1; > > - if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0) > - return -1; > - > - if (!(list = virUSBDeviceListNew())) > - goto cleanup; > - > - if (virUSBDeviceListAdd(list, usb) < 0) > - goto cleanup; > - > - if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list) < 0) This code you're deleting took the 'hostdev' parameter that was passed into this method, and runs the 'qemuPrepareHostdevUSBDevices' method on it. > + if (qemuPrepareHostUSBDevices(driver, vm->def, 0) < 0) > goto cleanup; This code you're adding takes the list of existing hostdevs in the virDomainDefPtr and runs the 'qemuPrepareHostdevUSBDevices' method on it. I don't see how this can possibly be correct, since the before and after code processes totally different hostdev device instances.

@@ -2531,29 +2514,7 @@ qemuDomainRemovePCIHostDevice(virQEMUDriverPtr driver, > virDomainObjPtr vm, > virDomainHostdevDefPtr hostdev) > { > - virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; > - virPCIDevicePtr pci; > - virPCIDevicePtr activePci; > - > - virObjectLock(driver->activePciHostdevs); > - virObjectLock(driver->inactivePciHostdevs); > - pci = virPCIDeviceNew(subsys->u.pci.addr.domain, subsys->u.pci.addr.bus, > - subsys->u.pci.addr.slot, subsys->u.pci.addr.function); > - if (pci) { > - activePci = virPCIDeviceListSteal(driver->activePciHostdevs, pci); > - if (activePci && > - virPCIDeviceReset(activePci, driver->activePciHostdevs, > - driver->inactivePciHostdevs) == 0) { > - qemuReattachPciDevice(activePci, driver); > - } else { > - /* reset of the device failed, treat it as if it was returned */ > - virPCIDeviceFree(activePci); > - } > - virPCIDeviceFree(pci); > - } > - virObjectUnlock(driver->activePciHostdevs); > - virObjectUnlock(driver->inactivePciHostdevs); > - > + qemuDomainReAttachHostdevDevices(driver, vm->def->name, &hostdev, 1); > qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); > } > > @@ -2562,19 +2523,7 @@ qemuDomainRemoveUSBHostDevice(virQEMUDriverPtr driver, > virDomainObjPtr vm ATTRIBUTE_UNUSED, > virDomainHostdevDefPtr hostdev) > { > - virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; > - virUSBDevicePtr usb; > - > - usb = virUSBDeviceNew(subsys->u.usb.bus, subsys->u.usb.device, NULL); > - if (usb) { > - virObjectLock(driver->activeUsbHostdevs); > - virUSBDeviceListDel(driver->activeUsbHostdevs, usb); > - virObjectUnlock(driver->activeUsbHostdevs); > - virUSBDeviceFree(usb); > - } else { > - VIR_WARN("Unable to find device %03d.%03d in list of used USB devices", > - subsys->u.usb.bus, subsys->u.usb.device); > - } > + qemuDomainReAttachHostUsbDevices(driver, vm->def->name, &hostdev, 1); > } > > static void > @@ -2622,8 +2571,6 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, > > virDomainAuditHostdev(vm, hostdev, "detach", true); > > - qemuDomainHostdevNetConfigRestore(hostdev, cfg->stateDir); > - This doesn't look valid to remove. qemuDomainHostdevNetConfigRestore will be done in

Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/:| |: http://libvirt.org -o- http://virt-manager.org:| |: http://autobuild.org -o- http://search.cpan.org/~danberr/:| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc:|