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

2014-03-04 Thread Daniel P. Berrange 
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 cy...@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.


 @@ -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.


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

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

2014-03-04 Thread Chunyan Liu 
2014-03-04 20:17 GMT+08:00 Daniel P. Berrange berra...@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 cy...@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-

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

2014-02-28 Thread Chunyan Liu 
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 cy...@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_hostdev.c b/src/qemu/qemu_hostdev.c
index e4f6b1b..e9c33f8 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -847,7 +847,7 @@ cleanup:
 
 
 int
-qemuPrepareHostdevUSBDevices(virQEMUDriverPtr driver,
+qemuPrepareHostdevUSBDevices(virHostdevManagerPtr mgr,
  const char *name,
  virUSBDeviceListPtr list)
 {
@@ -991,7 +991,7 @@ out:
 }
 
 
-static int
+int
 qemuPrepareHostUSBDevices(virQEMUDriverPtr driver,
   virDomainDefPtr def,
   bool coldBoot)
@@ -1217,7 +1217,7 @@ qemuPrepareHostDevices(virQEMUDriverPtr driver,
  * are locked
  */
 void
-qemuReattachPciDevice(virPCIDevicePtr dev, virQEMUDriverPtr driver)
+qemuReattachPciDevice(virPCIDevicePtr dev, QEMUDriverPtr driver)
 {
 int retries = 100;
 
@@ -1333,7 +1333,7 @@ cleanup:
 }
 
 
-static void
+void
 qemuDomainReAttachHostUsbDevices(virQEMUDriverPtr driver,
  const char *name,
  virDomainHostdevDefPtr *hostdevs,
diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h
index ffb3167..1567c1d 100644
--- a/src/qemu/qemu_hostdev.h
+++ b/src/qemu/qemu_hostdev.h
@@ -47,6 +47,10 @@ int qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev,
 int qemuPrepareHostdevUSBDevices(virQEMUDriverPtr driver,
  const char *name,
  virUSBDeviceListPtr list);
+int
+qemuPrepareHostUSBDevices(virQEMUDriverPtr driver,
+  virDomainDefPtr def,
+  bool coldBoot);
 int qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
   const char *name,
   virDomainHostdevDefPtr *hostdevs,
@@ -55,11 +59,16 @@ int qemuPrepareHostDevices(virQEMUDriverPtr driver,
virDomainDefPtr def,
virQEMUCapsPtr qemuCaps,
bool coldBoot);
+void qemuReattachPciDevice(virPCIDevicePtr dev, virQEMUDriverPtr driver);
+void
+qemuDomainReAttachHostUsbDevices(virQEMUDriverPtr driver,
+ const char *name,
+ virDomainHostdevDefPtr *hostdevs,
+ int nhostdevs);
 void qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
const char *name,
virDomainHostdevDefPtr *hostdevs,
int nhostdevs);
-void qemuReattachPciDevice(virPCIDevicePtr dev, virQEMUDriverPtr driver);
 void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver,
   const char *name,
   virDomainHostdevDefPtr *hostdevs,
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)
+if (qemuPrepareHostUSBDevices(driver, vm-def, 0)  0)
 goto cleanup;
 
 added = true;
-virUSBDeviceListSteal(list, usb);
 
 if (qemuSetupHostdevCGroup(vm, hostdev)  0)
 goto cleanup;
@@ -1520,13 +1508,8 @@ cleanup:
   vm-def, hostdev, NULL)  0)
 VIR_WARN(Unable to restore host device labelling on hotplug 
fail);
 if (added)
-virUSBDeviceListSteal(driver-activeUsbHostdevs, usb);
+qemuDomainReAttachHostUsbDevices(driver, vm-def-name, hostdev, 
1);
 }
-if (list  usb 
-!virUSBDeviceListFind(list, usb) 
-!virUSBDeviceListFind(driver-activeUsbHostdevs, usb))
-virUSBDeviceFree(usb);
-virObjectUnref(list);
 VIR_FREE(devstr);
 return ret;
 }
@@