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

[Chunyan Liu <cyliu@xxxxxxxx>]

2014-03-04 20:17 GMT+08:00 Daniel P. Berrange <berrange@xxxxxxxxxx>:

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@xxxxxxxx>
> ---
>  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 :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list