<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">2014-03-04 20:17 GMT+08:00 Daniel P. Berrange <<a href="mailto:berrange@redhat.com" target="_blank">berrange@redhat.com</a>>: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="">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 <<a href="mailto:cyliu@suse.com">cyliu@suse.com</a>> > --- > 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(-) </div><div class="">> 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) </div>This code you're deleting took the 'hostdev' parameter that was passed into this method, and runs the 'qemuPrepareHostdevUSBDevices' method on it. <div class=""> > + if (qemuPrepareHostUSBDevices(driver, vm->def, 0) < 0) > goto cleanup; </div>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. <div><div> </div></div></blockquote><div> My mistake. It should be: qemuPrepareHostUSBDevices(driver, vm->def->name,driver, &hostdev, 1, 0); qemuPrepareHostUSBDevices interface shoule be updated a bit, just to be consistency with *PreparePCIDevices and *PrepareSCSIDevices. </div><div>Will update. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div><div class="h5"> > @@ -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); > - </div></div>This doesn't look valid to remove. </blockquote><div>qemuDomainHostdevNetConfigRestore will be done in qemuDomainReAttachHostdevDevices after using the latter in qemuDomainRemovePCIHostDevice. So, it's not needed here. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Regards, Daniel -- |: <a href="http://berrange.com" target="_blank">http://berrange.com</a> -o- <a href="http://www.flickr.com/photos/dberrange/" target="_blank">http://www.flickr.com/photos/dberrange/</a> :| |: <a href="http://libvirt.org" target="_blank">http://libvirt.org</a> -o- <a href="http://virt-manager.org" target="_blank">http://virt-manager.org</a> :| |: <a href="http://autobuild.org" target="_blank">http://autobuild.org</a> -o- <a href="http://search.cpan.org/~danberr/" target="_blank">http://search.cpan.org/~danberr/</a> :| |: <a href="http://entangle-photo.org" target="_blank">http://entangle-photo.org</a> -o- <a href="http://live.gnome.org/gtk-vnc" target="_blank">http://live.gnome.org/gtk-vnc</a> :| </blockquote></div> </div></div>