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

[Chunyan Liu]

2014-03-04 20:17 GMT+08:00 Daniel P. Berrange <berrange at redhat.com>:

> 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 at suse.com>
> > ---
> >  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.
>
>
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.
Will update.

> @@ -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
qemuDomainReAttachHostdevDevices after using the latter in
qemuDomainRemovePCIHostDevice. So, it's not needed here.


> 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:|
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140305/b298b03c/attachment-0001.htm>