Re: [libvirt] [PATCH v2 10/12] qemu_hotplug: Allow asynchronous detach

[Ján Tomko]

On Thu, May 24, 2018 at 01:13:37PM +0200, Michal Privoznik wrote:

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index e84dc909b9..5b5c27b011 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5414,7 +5462,8 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,

if (virDomainNetGetActualType(detach) == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
ret = qemuDomainDetachThisHostDevice(driver, vm,
- 
virDomainNetGetActualHostdev(detach));
+ 
virDomainNetGetActualHostdev(detach),\


Spurious backslash.

Jano


+ async);
goto cleanup;
}



signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 10/12] qemu_hotplug: Allow asynchronous detach

[Ján Tomko]

On Thu, May 24, 2018 at 01:13:37PM +0200, Michal Privoznik wrote:

The virDomainDetachDeviceAlias API is designed so that it only
sends detach request to qemu. It's user's responsibility to wait


s/user/the user/


for DEVICE_DELETED event, not libvirt's. Add @async flag to
qemuDomainDetach*Device() functions so that caller can chose if
detach is semi-synchronous (old virDomainDetachDeviceFlags()) or
fully asynchronous (new virDomainDetachDeviceFlags()).

Signed-off-by: Michal Privoznik 
---
src/qemu/qemu_driver.c  |  32 +++
src/qemu/qemu_hotplug.c | 231 
src/qemu/qemu_hotplug.h |  33 ---
tests/qemuhotplugtest.c |  13 +--
4 files changed, 203 insertions(+), 106 deletions(-)

@@ -4769,18 +4771,24 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr 
driver,
if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto cleanup;

-if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
-ret = qemuDomainRemoveDiskDevice(driver, vm, detach);
+if (async) {
+ret = 0;
+} else {
+if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
+ret = qemuDomainRemoveDiskDevice(driver, vm, detach);
+}


I'm sure the ret assignments could be reduced here, but that's out of
scope of this series...

Also, most of the code is executed if (!async). It might be nicer to
come up with a variable that can be used in a positive condition,
but it would be less precise, because only the new async API is
reasonably defined - even if (wait) does not seem to outweigh
the loss of clarity.



 cleanup:
-qemuDomainResetDeviceRemoval(vm);
+if (!async)
+qemuDomainResetDeviceRemoval(vm);
return ret;
}

static int
qemuDomainDetachDiskDevice(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
-   virDomainDiskDefPtr detach)
+   virDomainDiskDefPtr detach,
+   bool async)
{
int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -5336,7 +5375,8 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
return -1;
}

-qemuDomainMarkDeviceForRemoval(vm, >info);
+if (!async)
+qemuDomainMarkDeviceForRemoval(vm, >info);

qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorDelDevice(priv->mon, watchdog->info.alias) < 0) {


Missing condition around qemuDomainWaitForDeviceRemoval here in
qemuDomainDetachWatchdog. It should only be called if (!async).

With that fixed:

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 10/12] qemu_hotplug: Allow asynchronous detach

[Michal Privoznik]

The virDomainDetachDeviceAlias API is designed so that it only
sends detach request to qemu. It's user's responsibility to wait
for DEVICE_DELETED event, not libvirt's. Add @async flag to
qemuDomainDetach*Device() functions so that caller can chose if
detach is semi-synchronous (old virDomainDetachDeviceFlags()) or
fully asynchronous (new virDomainDetachDeviceFlags()).

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_driver.c  |  32 +++
 src/qemu/qemu_hotplug.c | 231 
 src/qemu/qemu_hotplug.h |  33 ---
 tests/qemuhotplugtest.c |  13 +--
 4 files changed, 203 insertions(+), 106 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2347e71cfb..81a9833b39 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7726,14 +7726,15 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
 static int
 qemuDomainDetachDeviceControllerLive(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
- virDomainDeviceDefPtr dev)
+ virDomainDeviceDefPtr dev,
+ bool async)
 {
 virDomainControllerDefPtr cont = dev->data.controller;
 int ret = -1;
 
 switch (cont->type) {
 case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
-ret = qemuDomainDetachControllerDevice(driver, vm, dev);
+ret = qemuDomainDetachControllerDevice(driver, vm, dev, async);
 break;
 default :
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
@@ -7746,46 +7747,47 @@ qemuDomainDetachDeviceControllerLive(virQEMUDriverPtr 
driver,
 static int
 qemuDomainDetachDeviceLive(virDomainObjPtr vm,
virDomainDeviceDefPtr dev,
-   virQEMUDriverPtr driver)
+   virQEMUDriverPtr driver,
+   bool async)
 {
 int ret = -1;
 
 switch ((virDomainDeviceType)dev->type) {
 case VIR_DOMAIN_DEVICE_DISK:
-ret = qemuDomainDetachDeviceDiskLive(driver, vm, dev);
+ret = qemuDomainDetachDeviceDiskLive(driver, vm, dev, async);
 break;
 case VIR_DOMAIN_DEVICE_CONTROLLER:
-ret = qemuDomainDetachDeviceControllerLive(driver, vm, dev);
+ret = qemuDomainDetachDeviceControllerLive(driver, vm, dev, async);
 break;
 case VIR_DOMAIN_DEVICE_LEASE:
 ret = qemuDomainDetachLease(driver, vm, dev->data.lease);
 break;
 case VIR_DOMAIN_DEVICE_NET:
-ret = qemuDomainDetachNetDevice(driver, vm, dev);
+ret = qemuDomainDetachNetDevice(driver, vm, dev, async);
 break;
 case VIR_DOMAIN_DEVICE_HOSTDEV:
-ret = qemuDomainDetachHostDevice(driver, vm, dev);
+ret = qemuDomainDetachHostDevice(driver, vm, dev, async);
 break;
 case VIR_DOMAIN_DEVICE_CHR:
-ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr);
+ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr, async);
 break;
 case VIR_DOMAIN_DEVICE_RNG:
-ret = qemuDomainDetachRNGDevice(driver, vm, dev->data.rng);
+ret = qemuDomainDetachRNGDevice(driver, vm, dev->data.rng, async);
 break;
 case VIR_DOMAIN_DEVICE_MEMORY:
-ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory);
+ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory, 
async);
 break;
 case VIR_DOMAIN_DEVICE_SHMEM:
-ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem);
+ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem, async);
 break;
 case VIR_DOMAIN_DEVICE_WATCHDOG:
-ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog);
+ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog, async);
 break;
 case VIR_DOMAIN_DEVICE_INPUT:
-ret = qemuDomainDetachInputDevice(vm, dev->data.input);
+ret = qemuDomainDetachInputDevice(vm, dev->data.input, async);
 break;
 case VIR_DOMAIN_DEVICE_REDIRDEV:
-ret = qemuDomainDetachRedirdevDevice(driver, vm, dev->data.redirdev);
+ret = qemuDomainDetachRedirdevDevice(driver, vm, dev->data.redirdev, 
async);
 break;
 
 case VIR_DOMAIN_DEVICE_FS:
@@ -8716,7 +8718,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr 
driver,
 }
 
 if (flags & VIR_DOMAIN_AFFECT_LIVE) {
-if (qemuDomainDetachDeviceLive(vm, dev_copy, driver) < 0)
+if (qemuDomainDetachDeviceLive(vm, dev_copy, driver, false) < 0)
 goto cleanup;
 /*
  * update domain status forcibly because the domain status may be
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index e84dc909b9..5b5c27b011 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4740,7 +4740,8 @@ qemuDomainSignalDeviceRemoval(virDomainObjPtr vm,
 static int