[libvirt] [PATCH 0/7] Add notification for memory hot-unplug failure
Add a new libvirt event VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED that will handle failure to unplug a device if we are certain that it will not be unplugged and wire up the ACPI_DEVICE_OST qemu event that is emitted on memory hotunplug failure so that the event is propagated and the API fails in such case. Along with that a few refactors were necessary. Peter Krempa (7): qemu: hotplug: Properly handle errors in qemuDomainWaitForDeviceRemoval qemu: hotplug: Refactor semantics of qemuDomainWaitForDeviceRemoval qemu: Use domain condition for device removal singalling qemu: hotplug: Add support for signalling device unplug failure Add VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED event qemu: monitor: Add support for ACPI_DEVICE_OST event handling qemu: process: Wire up ACPI OST events to notify users of failed memory unplug daemon/remote.c | 36 +++++++++++++ include/libvirt/libvirt-domain.h | 21 ++++++++ src/conf/domain_event.c | 84 ++++++++++++++++++++++++++++++ src/conf/domain_event.h | 7 +++ src/libvirt_private.syms | 2 + src/qemu/qemu_domain.c | 4 -- src/qemu/qemu_domain.h | 18 ++++++- src/qemu/qemu_hotplug.c | 110 +++++++++++++++++---------------------- src/qemu/qemu_hotplug.h | 3 +- src/qemu/qemu_monitor.c | 19 +++++++ src/qemu/qemu_monitor.h | 18 +++++++ src/qemu/qemu_monitor_json.c | 39 ++++++++++++++ src/qemu/qemu_process.c | 71 ++++++++++++++++++++++++- src/remote/remote_driver.c | 30 +++++++++++ src/remote/remote_protocol.x | 14 ++++- src/remote_protocol-structs | 6 +++ tools/virsh-domain.c | 18 +++++++ 17 files changed, 428 insertions(+), 72 deletions(-) -- 2.8.0
Callers ignore if this function returns -1 and continue as though the DEVICE_DELETED event was not received. Since we can't be sure that the event was not received we should behave as if the event was not supported and remove the device definition right away. The error fortunately won't really happen here. --- src/qemu/qemu_hotplug.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 48bea6a..6c619e9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3351,8 +3351,8 @@ qemuDomainResetDeviceRemoval(virDomainObjPtr vm) } /* Returns: - * -1 on error - * 0 when DEVICE_DELETED event is unsupported + * 0 when DEVICE_DELETED event is unsupported, or we failed to reliably wait + * for the event * 1 when DEVICE_DELETED arrived before the timeout and the caller is * responsible for finishing the removal * 2 device removal did not finish in qemuDomainRemoveDeviceWaitTime @@ -3367,7 +3367,7 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm) return 0; if (virTimeMillisNow(&until) < 0) - return -1; + return 0; until += qemuDomainRemoveDeviceWaitTime; while (priv->unpluggingDevice) { @@ -3376,9 +3376,9 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm) if (errno == ETIMEDOUT) { return 2; } else { - virReportSystemError(errno, "%s", - _("Unable to wait on unplug condition")); - return -1; + VIR_WARN("Failed to wait on unplug condition for domain '%s' " + "device '%s'", vm->def->name, priv->unpluggingDevice); + return 0; } } } -- 2.8.0
On 04/05/2016 11:09 AM, Peter Krempa wrote:
Callers ignore if this function returns -1 and continue as though the DEVICE_DELETED event was not received. Since we can't be sure that the event was not received we should behave as if the event was not supported and remove the device definition right away. The error fortunately won't really happen here. --- src/qemu/qemu_hotplug.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 48bea6a..6c619e9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3351,8 +3351,8 @@ qemuDomainResetDeviceRemoval(virDomainObjPtr vm) }
/* Returns: - * -1 on error - * 0 when DEVICE_DELETED event is unsupported + * 0 when DEVICE_DELETED event is unsupported, or we failed to reliably wait + * for the event * 1 when DEVICE_DELETED arrived before the timeout and the caller is * responsible for finishing the removal * 2 device removal did not finish in qemuDomainRemoveDeviceWaitTime @@ -3367,7 +3367,7 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm) return 0;
if (virTimeMillisNow(&until) < 0) - return -1; + return 0; until += qemuDomainRemoveDeviceWaitTime;
while (priv->unpluggingDevice) { @@ -3376,9 +3376,9 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm) if (errno == ETIMEDOUT) { return 2; } else { - virReportSystemError(errno, "%s", - _("Unable to wait on unplug condition")); - return -1; + VIR_WARN("Failed to wait on unplug condition for domain '%s' " + "device '%s'", vm->def->name, priv->unpluggingDevice); + return 0; } } }
Makes sense, and I verified every caller is just checking the return value == 1. ACK - Cole
Neither of the callers cares whether the DEVICE_DELETED event isn't supported or the event was received. Simplify the code and callers by unifying the two values and changing the return value constants so that a temporary variable can be omitted. --- src/qemu/qemu_hotplug.c | 67 +++++++++++++++---------------------------------- 1 file changed, 20 insertions(+), 47 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6c619e9..7317089 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3351,11 +3351,13 @@ qemuDomainResetDeviceRemoval(virDomainObjPtr vm) } /* Returns: - * 0 when DEVICE_DELETED event is unsupported, or we failed to reliably wait - * for the event - * 1 when DEVICE_DELETED arrived before the timeout and the caller is - * responsible for finishing the removal - * 2 device removal did not finish in qemuDomainRemoveDeviceWaitTime + * 0 DEVICE_DELETED event is supported and removal of the device did not + * finish in qemuDomainRemoveDeviceWaitTime + * + * 1 when the caller is responsible for finishing the device removal: + * - DEVICE_DELETED event is unsupported + * - DEVICE_DELETED event arrived before the timeout time + * - we failed to reliably wait for the event and thus use fallback behavior */ static int qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm) @@ -3364,21 +3366,21 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm) unsigned long long until; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) - return 0; + return 1; if (virTimeMillisNow(&until) < 0) - return 0; + return 1; until += qemuDomainRemoveDeviceWaitTime; while (priv->unpluggingDevice) { if (virCondWaitUntil(&priv->unplugFinished, &vm->parent.lock, until) < 0) { if (errno == ETIMEDOUT) { - return 2; + return 0; } else { VIR_WARN("Failed to wait on unplug condition for domain '%s' " "device '%s'", vm->def->name, priv->unpluggingDevice); - return 0; + return 1; } } } @@ -3414,7 +3416,6 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - int rc; if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { virReportError(VIR_ERR_OPERATION_FAILED, @@ -3468,11 +3469,8 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - rc = qemuDomainWaitForDeviceRemoval(vm); - if (rc == 0 || rc == 1) + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) ret = qemuDomainRemoveDiskDevice(driver, vm, detach); - else - ret = 0; cleanup: qemuDomainResetDeviceRemoval(vm); @@ -3486,7 +3484,6 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - int rc; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { virReportError(VIR_ERR_OPERATION_FAILED, @@ -3514,11 +3511,8 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - rc = qemuDomainWaitForDeviceRemoval(vm); - if (rc == 0 || rc == 1) + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) ret = qemuDomainRemoveDiskDevice(driver, vm, detach); - else - ret = 0; cleanup: qemuDomainResetDeviceRemoval(vm); @@ -3634,7 +3628,6 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, int idx, ret = -1; virDomainControllerDefPtr detach = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; - int rc; if ((idx = virDomainControllerFind(vm->def, dev->data.controller->type, @@ -3702,11 +3695,8 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - rc = qemuDomainWaitForDeviceRemoval(vm); - if (rc == 0 || rc == 1) + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) ret = qemuDomainRemoveControllerDevice(driver, vm, detach); - else - ret = 0; cleanup: qemuDomainResetDeviceRemoval(vm); @@ -3846,10 +3836,8 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, if (ret < 0) { if (virDomainObjIsActive(vm)) virDomainAuditHostdev(vm, detach, "detach", false); - } else { - int rc = qemuDomainWaitForDeviceRemoval(vm); - if (rc == 0 || rc == 1) - ret = qemuDomainRemoveHostDevice(driver, vm, detach); + } else if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) { + ret = qemuDomainRemoveHostDevice(driver, vm, detach); } qemuDomainResetDeviceRemoval(vm); @@ -3940,7 +3928,6 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, int detachidx, ret = -1; virDomainNetDefPtr detach = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; - int rc; if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0) goto cleanup; @@ -4017,11 +4004,8 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - rc = qemuDomainWaitForDeviceRemoval(vm); - if (rc == 0 || rc == 1) + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) ret = qemuDomainRemoveNetDevice(driver, vm, detach); - else - ret = 0; cleanup: qemuDomainResetDeviceRemoval(vm); @@ -4157,7 +4141,6 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainDefPtr vmdef = vm->def; virDomainChrDefPtr tmpChr; char *devstr = NULL; - int rc; if (!(tmpChr = virDomainChrFind(vmdef, chr))) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -4189,15 +4172,11 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - rc = qemuDomainWaitForDeviceRemoval(vm); - if (rc == 0 || rc == 1) { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) { qemuDomainReleaseDeviceAddress(vm, &tmpChr->info, NULL); ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr); - } else { - ret = 0; } - cleanup: qemuDomainResetDeviceRemoval(vm); VIR_FREE(devstr); @@ -4243,11 +4222,8 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) || rc < 0) goto cleanup; - rc = qemuDomainWaitForDeviceRemoval(vm); - if (rc == 0 || rc == 1) + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) ret = qemuDomainRemoveRNGDevice(driver, vm, tmpRNG); - else - ret = 0; cleanup: qemuDomainResetDeviceRemoval(vm); @@ -4295,11 +4271,8 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) goto cleanup; - rc = qemuDomainWaitForDeviceRemoval(vm); - if (rc == 0 || rc == 1) + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) ret = qemuDomainRemoveMemoryDevice(driver, vm, mem); - else - ret = 0; cleanup: qemuDomainResetDeviceRemoval(vm); -- 2.8.0
On 04/05/2016 11:09 AM, Peter Krempa wrote:
Neither of the callers cares whether the DEVICE_DELETED event isn't supported or the event was received. Simplify the code and callers by unifying the two values and changing the return value constants so that a temporary variable can be omitted. --- src/qemu/qemu_hotplug.c | 67 +++++++++++++++---------------------------------- 1 file changed, 20 insertions(+), 47 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6c619e9..7317089 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3351,11 +3351,13 @@ qemuDomainResetDeviceRemoval(virDomainObjPtr vm) }
/* Returns: - * 0 when DEVICE_DELETED event is unsupported, or we failed to reliably wait - * for the event - * 1 when DEVICE_DELETED arrived before the timeout and the caller is - * responsible for finishing the removal - * 2 device removal did not finish in qemuDomainRemoveDeviceWaitTime + * 0 DEVICE_DELETED event is supported and removal of the device did not + * finish in qemuDomainRemoveDeviceWaitTime + * + * 1 when the caller is responsible for finishing the device removal: + * - DEVICE_DELETED event is unsupported + * - DEVICE_DELETED event arrived before the timeout time + * - we failed to reliably wait for the event and thus use fallback behavior */
Makes sense... return 1 basically means 'removal succeeded OR there's no way we can tell if it succeeded or not, so just update the XML to assume it did' ACK - Cole
No need to keep two separate conditions. A slight juggling of return values is needed to accomodate virDomainObjWaitUntil. --- src/qemu/qemu_domain.c | 4 ---- src/qemu/qemu_domain.h | 1 - src/qemu/qemu_hotplug.c | 19 +++++++++---------- 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f38b0f3..8a673f8 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -586,9 +586,6 @@ qemuDomainObjPrivateAlloc(void) goto error; } - if (virCondInit(&priv->unplugFinished) < 0) - goto error; - if (!(priv->devs = virChrdevAlloc())) goto error; @@ -618,7 +615,6 @@ qemuDomainObjPrivateFree(void *data) VIR_FREE(priv->lockState); VIR_FREE(priv->origname); - virCondDestroy(&priv->unplugFinished); virStringFreeList(priv->qemuDevices); virChrdevFree(priv->devs); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 54d7bd7..2b92a90 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -198,7 +198,6 @@ struct _qemuDomainObjPrivate { virPerfPtr perf; - virCond unplugFinished; /* signals that unpluggingDevice was unplugged */ const char *unpluggingDevice; /* alias of the device that is being unplugged */ char **qemuDevices; /* NULL-terminated list of devices aliases known to QEMU */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7317089..134f458 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3364,6 +3364,7 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; unsigned long long until; + int rc; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) return 1; @@ -3373,15 +3374,13 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm) until += qemuDomainRemoveDeviceWaitTime; while (priv->unpluggingDevice) { - if (virCondWaitUntil(&priv->unplugFinished, - &vm->parent.lock, until) < 0) { - if (errno == ETIMEDOUT) { - return 0; - } else { - VIR_WARN("Failed to wait on unplug condition for domain '%s' " - "device '%s'", vm->def->name, priv->unpluggingDevice); - return 1; - } + if ((rc = virDomainObjWaitUntil(vm, until)) == 1) + return 0; + + if (rc < 0) { + VIR_WARN("Failed to wait on unplug condition for domain '%s' " + "device '%s'", vm->def->name, priv->unpluggingDevice); + return 1; } } @@ -3402,7 +3401,7 @@ qemuDomainSignalDeviceRemoval(virDomainObjPtr vm, if (STREQ_NULLABLE(priv->unpluggingDevice, devAlias)) { qemuDomainResetDeviceRemoval(vm); - virCondSignal(&priv->unplugFinished); + virDomainObjBroadcast(vm); return true; } return false; -- 2.8.0
s/singalling/signaling/ On 04/05/2016 11:09 AM, Peter Krempa wrote:
No need to keep two separate conditions. A slight juggling of return values is needed to accomodate virDomainObjWaitUntil. --- src/qemu/qemu_domain.c | 4 ---- src/qemu/qemu_domain.h | 1 - src/qemu/qemu_hotplug.c | 19 +++++++++---------- 3 files changed, 9 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f38b0f3..8a673f8 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -586,9 +586,6 @@ qemuDomainObjPrivateAlloc(void) goto error; }
- if (virCondInit(&priv->unplugFinished) < 0) - goto error; - if (!(priv->devs = virChrdevAlloc())) goto error;
@@ -618,7 +615,6 @@ qemuDomainObjPrivateFree(void *data) VIR_FREE(priv->lockState); VIR_FREE(priv->origname);
- virCondDestroy(&priv->unplugFinished); virStringFreeList(priv->qemuDevices); virChrdevFree(priv->devs);
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 54d7bd7..2b92a90 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -198,7 +198,6 @@ struct _qemuDomainObjPrivate {
virPerfPtr perf;
- virCond unplugFinished; /* signals that unpluggingDevice was unplugged */ const char *unpluggingDevice; /* alias of the device that is being unplugged */ char **qemuDevices; /* NULL-terminated list of devices aliases known to QEMU */
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7317089..134f458 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3364,6 +3364,7 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; unsigned long long until; + int rc;
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) return 1; @@ -3373,15 +3374,13 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm) until += qemuDomainRemoveDeviceWaitTime;
while (priv->unpluggingDevice) { - if (virCondWaitUntil(&priv->unplugFinished, - &vm->parent.lock, until) < 0) { - if (errno == ETIMEDOUT) { - return 0; - } else { - VIR_WARN("Failed to wait on unplug condition for domain '%s' " - "device '%s'", vm->def->name, priv->unpluggingDevice); - return 1; - } + if ((rc = virDomainObjWaitUntil(vm, until)) == 1) + return 0; + + if (rc < 0) { + VIR_WARN("Failed to wait on unplug condition for domain '%s' " + "device '%s'", vm->def->name, priv->unpluggingDevice); + return 1; } }
@@ -3402,7 +3401,7 @@ qemuDomainSignalDeviceRemoval(virDomainObjPtr vm,
if (STREQ_NULLABLE(priv->unpluggingDevice, devAlias)) { qemuDomainResetDeviceRemoval(vm); - virCondSignal(&priv->unplugFinished); + virDomainObjBroadcast(vm); return true; } return false;
Neat, I didn't know about virDomainObjBroadcast etc. LGTM, ACK - Cole
Similarly to the DEVICE_DELETED event we will be able to tell when unplug of certain device types will be rejected by the guest OS. Wire up the device deletion signalling code to allow handling this. --- src/qemu/qemu_domain.h | 17 ++++++++++++++++- src/qemu/qemu_hotplug.c | 30 +++++++++++++++++++++--------- src/qemu/qemu_hotplug.h | 3 ++- src/qemu/qemu_process.c | 2 +- 4 files changed, 40 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 2b92a90..84b4b16 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -148,6 +148,20 @@ struct qemuDomainJobObj { typedef void (*qemuDomainCleanupCallback)(virQEMUDriverPtr driver, virDomainObjPtr vm); +/* helper data types for async device unplug */ +typedef enum { + QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_NONE = 0, + QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_OK, + QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_GUEST_REJECTED, +} qemuDomainUnplugDeviceStatus; + +typedef struct _qemuDomainUnplugDevice qemuDomainUnplugDevice; +typedef qemuDomainUnplugDevice *qemuDomainUnplugDevicePtr; +struct _qemuDomainUnplugDevice { + const char *alias; + qemuDomainUnplugDeviceStatus status; +}; + typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate; typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr; struct _qemuDomainObjPrivate { @@ -198,7 +212,8 @@ struct _qemuDomainObjPrivate { virPerfPtr perf; - const char *unpluggingDevice; /* alias of the device that is being unplugged */ + qemuDomainUnplugDevice unplug; + char **qemuDevices; /* NULL-terminated list of devices aliases known to QEMU */ bool hookRun; /* true if there was a hook run over this domain */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 134f458..bd0599f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3337,20 +3337,24 @@ qemuDomainMarkDeviceForRemoval(virDomainObjPtr vm, { qemuDomainObjPrivatePtr priv = vm->privateData; - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) - priv->unpluggingDevice = info->alias; - else - priv->unpluggingDevice = NULL; + memset(&priv->unplug, 0, sizeof(priv->unplug)); + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) + return; + + priv->unplug.alias = info->alias; } static void qemuDomainResetDeviceRemoval(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; - priv->unpluggingDevice = NULL; + priv->unplug.alias = NULL; } /* Returns: + * -1 Unplug of the device failed + * * 0 DEVICE_DELETED event is supported and removal of the device did not * finish in qemuDomainRemoveDeviceWaitTime * @@ -3373,17 +3377,23 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm) return 1; until += qemuDomainRemoveDeviceWaitTime; - while (priv->unpluggingDevice) { + while (priv->unplug.alias) { if ((rc = virDomainObjWaitUntil(vm, until)) == 1) return 0; if (rc < 0) { VIR_WARN("Failed to wait on unplug condition for domain '%s' " - "device '%s'", vm->def->name, priv->unpluggingDevice); + "device '%s'", vm->def->name, priv->unplug.alias); return 1; } } + if (priv->unplug.status == QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_GUEST_REJECTED) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("unplug of device was rejected by the guest")); + return -1; + } + return 1; } @@ -3395,12 +3405,14 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm) */ bool qemuDomainSignalDeviceRemoval(virDomainObjPtr vm, - const char *devAlias) + const char *devAlias, + qemuDomainUnplugDeviceStatus status) { qemuDomainObjPrivatePtr priv = vm->privateData; - if (STREQ_NULLABLE(priv->unpluggingDevice, devAlias)) { + if (STREQ_NULLABLE(priv->unplug.alias, devAlias)) { qemuDomainResetDeviceRemoval(vm); + priv->unplug.status = status; virDomainObjBroadcast(vm); return true; } diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 4140da3..e6833f0 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -122,6 +122,7 @@ int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainDeviceDefPtr dev); bool qemuDomainSignalDeviceRemoval(virDomainObjPtr vm, - const char *devAlias); + const char *devAlias, + qemuDomainUnplugDeviceStatus status); #endif /* __QEMU_HOTPLUG_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d9dca74..bbde32c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1359,7 +1359,7 @@ qemuProcessHandleDeviceDeleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED, VIR_DEBUG("Device %s removed from domain %p %s", devAlias, vm, vm->def->name); - if (qemuDomainSignalDeviceRemoval(vm, devAlias)) + if (qemuDomainSignalDeviceRemoval(vm, devAlias, QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_OK)) goto cleanup; if (VIR_ALLOC(processEvent) < 0) -- 2.8.0
On 04/05/2016 11:09 AM, Peter Krempa wrote:
Similarly to the DEVICE_DELETED event we will be able to tell when unplug of certain device types will be rejected by the guest OS. Wire up the device deletion signalling code to allow handling this. --- src/qemu/qemu_domain.h | 17 ++++++++++++++++- src/qemu/qemu_hotplug.c | 30 +++++++++++++++++++++--------- src/qemu/qemu_hotplug.h | 3 ++- src/qemu/qemu_process.c | 2 +- 4 files changed, 40 insertions(+), 12 deletions(-)
There's a slight conflict here with git master but it should rebase cleanly, git am --3way couldn't handle it for me for whatever reason
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 2b92a90..84b4b16 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -148,6 +148,20 @@ struct qemuDomainJobObj { typedef void (*qemuDomainCleanupCallback)(virQEMUDriverPtr driver, virDomainObjPtr vm);
+/* helper data types for async device unplug */ +typedef enum { + QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_NONE = 0, + QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_OK, + QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_GUEST_REJECTED, +} qemuDomainUnplugDeviceStatus; + +typedef struct _qemuDomainUnplugDevice qemuDomainUnplugDevice; +typedef qemuDomainUnplugDevice *qemuDomainUnplugDevicePtr; +struct _qemuDomainUnplugDevice { + const char *alias; + qemuDomainUnplugDeviceStatus status; +}; + typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate; typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr; struct _qemuDomainObjPrivate { @@ -198,7 +212,8 @@ struct _qemuDomainObjPrivate {
virPerfPtr perf;
- const char *unpluggingDevice; /* alias of the device that is being unplugged */ + qemuDomainUnplugDevice unplug; +
FWIW in isolation I'd think 'qemuDomainUnplugDevice' was a function name... maybe stick with the UnpluggingDevice name. Not a NACK, just an idea if you're motivated. Also lack Ptr use just looks weird, I'm so used to everything being a Ptr in libvirt code :)
char **qemuDevices; /* NULL-terminated list of devices aliases known to QEMU */
bool hookRun; /* true if there was a hook run over this domain */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 134f458..bd0599f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3337,20 +3337,24 @@ qemuDomainMarkDeviceForRemoval(virDomainObjPtr vm, { qemuDomainObjPrivatePtr priv = vm->privateData;
- if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) - priv->unpluggingDevice = info->alias; - else - priv->unpluggingDevice = NULL; + memset(&priv->unplug, 0, sizeof(priv->unplug)); + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT)) + return; + + priv->unplug.alias = info->alias; }
static void qemuDomainResetDeviceRemoval(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; - priv->unpluggingDevice = NULL; + priv->unplug.alias = NULL; }
/* Returns: + * -1 Unplug of the device failed + * * 0 DEVICE_DELETED event is supported and removal of the device did not * finish in qemuDomainRemoveDeviceWaitTime * @@ -3373,17 +3377,23 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm) return 1; until += qemuDomainRemoveDeviceWaitTime;
- while (priv->unpluggingDevice) { + while (priv->unplug.alias) { if ((rc = virDomainObjWaitUntil(vm, until)) == 1) return 0;
if (rc < 0) { VIR_WARN("Failed to wait on unplug condition for domain '%s' " - "device '%s'", vm->def->name, priv->unpluggingDevice); + "device '%s'", vm->def->name, priv->unplug.alias); return 1; } }
+ if (priv->unplug.status == QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_GUEST_REJECTED) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("unplug of device was rejected by the guest")); + return -1; + } + return 1; }
Okay, so 0 == didn't succeed in time, 1 == definitely succeeded OR we have no way of knowing so assume it worked, -1 == definitely failed. That's a lot of semantics :) But this makes sense, now when the -1 trickles up the stack, it's when we know for certain that the hotplug failed. ACK - Cole
@@ -3395,12 +3405,14 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm) */ bool qemuDomainSignalDeviceRemoval(virDomainObjPtr vm, - const char *devAlias) + const char *devAlias, + qemuDomainUnplugDeviceStatus status) { qemuDomainObjPrivatePtr priv = vm->privateData;
- if (STREQ_NULLABLE(priv->unpluggingDevice, devAlias)) { + if (STREQ_NULLABLE(priv->unplug.alias, devAlias)) { qemuDomainResetDeviceRemoval(vm); + priv->unplug.status = status; virDomainObjBroadcast(vm); return true; } diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 4140da3..e6833f0 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -122,6 +122,7 @@ int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainDeviceDefPtr dev);
bool qemuDomainSignalDeviceRemoval(virDomainObjPtr vm, - const char *devAlias); + const char *devAlias, + qemuDomainUnplugDeviceStatus status);
#endif /* __QEMU_HOTPLUG_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d9dca74..bbde32c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1359,7 +1359,7 @@ qemuProcessHandleDeviceDeleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED, VIR_DEBUG("Device %s removed from domain %p %s", devAlias, vm, vm->def->name);
- if (qemuDomainSignalDeviceRemoval(vm, devAlias)) + if (qemuDomainSignalDeviceRemoval(vm, devAlias, QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_OK)) goto cleanup;
if (VIR_ALLOC(processEvent) < 0)
Since we didn't opt to use one single event for device lifecycle for a VM we are missing one last event if the device removal failed. This event will be emitted once we asked to eject the device but for some reason it is not possible. --- daemon/remote.c | 36 +++++++++++++++++ include/libvirt/libvirt-domain.h | 21 ++++++++++ src/conf/domain_event.c | 84 ++++++++++++++++++++++++++++++++++++++++ src/conf/domain_event.h | 7 ++++ src/libvirt_private.syms | 2 + src/remote/remote_driver.c | 30 ++++++++++++++ src/remote/remote_protocol.x | 14 ++++++- src/remote_protocol-structs | 6 +++ tools/virsh-domain.c | 18 +++++++++ 9 files changed, 217 insertions(+), 1 deletion(-) diff --git a/daemon/remote.c b/daemon/remote.c index 9db93ff..fde029d 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1136,6 +1136,41 @@ remoteRelayDomainEventJobCompleted(virConnectPtr conn, } +static int +remoteRelayDomainEventDeviceRemovalFailed(virConnectPtr conn, + virDomainPtr dom, + const char *devAlias, + void *opaque) +{ + daemonClientEventCallbackPtr callback = opaque; + remote_domain_event_callback_device_removal_failed_msg data; + + if (callback->callbackID < 0 || + !remoteRelayDomainEventCheckACL(callback->client, conn, dom)) + return -1; + + VIR_DEBUG("Relaying domain device removal failed event %s %d %s, callback %d", + dom->name, dom->id, devAlias, callback->callbackID); + + /* build return data */ + memset(&data, 0, sizeof(data)); + + if (VIR_STRDUP(data.devAlias, devAlias) < 0) + return -1; + + make_nonnull_domain(&data.dom, dom); + data.callbackID = callback->callbackID; + + remoteDispatchObjectEventSend(callback->client, remoteProgram, + REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_REMOVAL_FAILED, + (xdrproc_t)xdr_remote_domain_event_callback_device_removal_failed_msg, + &data); + + return 0; +} + + + static virConnectDomainEventGenericCallback domainEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventLifecycle), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventReboot), @@ -1159,6 +1194,7 @@ static virConnectDomainEventGenericCallback domainEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventDeviceAdded), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventMigrationIteration), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventJobCompleted), + VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventDeviceRemovalFailed), }; verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST); diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 728b6eb..8220ab0 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -3342,6 +3342,26 @@ typedef void (*virConnectDomainEventDeviceAddedCallback)(virConnectPtr conn, const char *devAlias, void *opaque); + +/** + * virConnectDomainEventDeviceRemovalFailedCallback: + * @conn: connection object + * @dom: domain on which the event occurred + * @devAlias: device alias + * @opaque: application specified data + * + * This callback occurs when it's certain that removal of a device failed. + * + * The callback signature to use when registering for an event of type + * VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED with + * virConnectDomainEventRegisterAny(). + */ +typedef void (*virConnectDomainEventDeviceRemovalFailedCallback)(virConnectPtr conn, + virDomainPtr dom, + const char *devAlias, + void *opaque); + + /** * virConnectDomainEventMigrationIterationCallback: * @conn: connection object @@ -3687,6 +3707,7 @@ typedef enum { VIR_DOMAIN_EVENT_ID_DEVICE_ADDED = 19, /* virConnectDomainEventDeviceAddedCallback */ VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION = 20, /* virConnectDomainEventMigrationIterationCallback */ VIR_DOMAIN_EVENT_ID_JOB_COMPLETED = 21, /* virConnectDomainEventJobCompletedCallback */ + VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED = 22, /* virConnectDomainEventDeviceRemovalFailedCallback */ # ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_EVENT_ID_LAST diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index a9107e5..58823e8 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -58,6 +58,7 @@ static virClassPtr virDomainEventAgentLifecycleClass; static virClassPtr virDomainEventDeviceAddedClass; static virClassPtr virDomainEventMigrationIterationClass; static virClassPtr virDomainEventJobCompletedClass; +static virClassPtr virDomainEventDeviceRemovalFailedClass; static void virDomainEventDispose(void *obj); static void virDomainEventLifecycleDispose(void *obj); @@ -77,6 +78,7 @@ static void virDomainEventAgentLifecycleDispose(void *obj); static void virDomainEventDeviceAddedDispose(void *obj); static void virDomainEventMigrationIterationDispose(void *obj); static void virDomainEventJobCompletedDispose(void *obj); +static void virDomainEventDeviceRemovalFailedDispose(void *obj); static void virDomainEventDispatchDefaultFunc(virConnectPtr conn, @@ -256,6 +258,16 @@ struct _virDomainEventJobCompleted { typedef struct _virDomainEventJobCompleted virDomainEventJobCompleted; typedef virDomainEventJobCompleted *virDomainEventJobCompletedPtr; +struct _virDomainEventDeviceRemovalFailed { + virDomainEvent parent; + + char *devAlias; +}; +typedef struct _virDomainEventDeviceRemovalFailed virDomainEventDeviceRemovalFailed; +typedef virDomainEventDeviceRemovalFailed *virDomainEventDeviceRemovalFailedPtr; + + + static int virDomainEventsOnceInit(void) { @@ -367,6 +379,12 @@ virDomainEventsOnceInit(void) sizeof(virDomainEventJobCompleted), virDomainEventJobCompletedDispose))) return -1; + if (!(virDomainEventDeviceRemovalFailedClass = + virClassNew(virDomainEventClass, + "virDomainEventDeviceRemovalFailed", + sizeof(virDomainEventDeviceRemovalFailed), + virDomainEventDeviceRemovalFailedDispose))) + return -1; return 0; } @@ -494,6 +512,17 @@ virDomainEventDeviceAddedDispose(void *obj) VIR_FREE(event->devAlias); } + +static void +virDomainEventDeviceRemovalFailedDispose(void *obj) +{ + virDomainEventDeviceRemovalFailedPtr event = obj; + VIR_DEBUG("obj=%p", event); + + VIR_FREE(event->devAlias); +} + + static void virDomainEventPMDispose(void *obj) { @@ -1340,6 +1369,50 @@ virDomainEventDeviceAddedNewFromDom(virDomainPtr dom, devAlias); } + +static virObjectEventPtr +virDomainEventDeviceRemovalFailedNew(int id, + const char *name, + unsigned char *uuid, + const char *devAlias) +{ + virDomainEventDeviceRemovalFailedPtr ev; + + if (virDomainEventsInitialize() < 0) + return NULL; + + if (!(ev = virDomainEventNew(virDomainEventDeviceAddedClass, + VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED, + id, name, uuid))) + return NULL; + + if (VIR_STRDUP(ev->devAlias, devAlias) < 0) + goto error; + + return (virObjectEventPtr)ev; + + error: + virObjectUnref(ev); + return NULL; +} + +virObjectEventPtr +virDomainEventDeviceRemovalFailedNewFromObj(virDomainObjPtr obj, + const char *devAlias) +{ + return virDomainEventDeviceRemovalFailedNew(obj->def->id, obj->def->name, + obj->def->uuid, devAlias); +} + +virObjectEventPtr +virDomainEventDeviceRemovalFailedNewFromDom(virDomainPtr dom, + const char *devAlias) +{ + return virDomainEventDeviceRemovalFailedNew(dom->id, dom->name, dom->uuid, + devAlias); +} + + static virObjectEventPtr virDomainEventAgentLifecycleNew(int id, const char *name, @@ -1768,6 +1841,17 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn, goto cleanup; } + case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED: + { + virDomainEventDeviceRemovalFailedPtr deviceRemovalFailedEvent; + + deviceRemovalFailedEvent = (virDomainEventDeviceRemovalFailedPtr)event; + ((virConnectDomainEventDeviceRemovalFailedCallback)cb)(conn, dom, + deviceRemovalFailedEvent->devAlias, + cbopaque); + goto cleanup; + } + case VIR_DOMAIN_EVENT_ID_LAST: break; } diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index 3eb13c8..54fa879 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -191,6 +191,13 @@ virObjectEventPtr virDomainEventDeviceAddedNewFromDom(virDomainPtr dom, const char *devAlias); virObjectEventPtr +virDomainEventDeviceRemovalFailedNewFromObj(virDomainObjPtr obj, + const char *devAlias); +virObjectEventPtr +virDomainEventDeviceRemovalFailedNewFromDom(virDomainPtr dom, + const char *devAlias); + +virObjectEventPtr virDomainEventTunableNewFromObj(virDomainObjPtr obj, virTypedParameterPtr params, int nparams); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 684f06c..aff8622 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -496,6 +496,8 @@ virDomainEventControlErrorNewFromDom; virDomainEventControlErrorNewFromObj; virDomainEventDeviceAddedNewFromDom; virDomainEventDeviceAddedNewFromObj; +virDomainEventDeviceRemovalFailedNewFromDom; +virDomainEventDeviceRemovalFailedNewFromObj; virDomainEventDeviceRemovedNewFromDom; virDomainEventDeviceRemovedNewFromObj; virDomainEventDiskChangeNewFromDom; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b03c9ca..da94411 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -322,6 +322,10 @@ static void remoteDomainBuildEventCallbackDeviceAdded(virNetClientProgramPtr prog, virNetClientPtr client, void *evdata, void *opaque); +static void +remoteDomainBuildEventCallbackDeviceRemovalFailed(virNetClientProgramPtr prog, + virNetClientPtr client, + void *evdata, void *opaque); static void remoteDomainBuildEventBlockJob2(virNetClientProgramPtr prog, @@ -528,6 +532,10 @@ static virNetClientProgramEvent remoteEvents[] = { remoteDomainBuildEventCallbackJobCompleted, sizeof(remote_domain_event_callback_job_completed_msg), (xdrproc_t)xdr_remote_domain_event_callback_job_completed_msg }, + { REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_REMOVAL_FAILED, + remoteDomainBuildEventCallbackDeviceRemovalFailed, + sizeof(remote_domain_event_callback_device_removal_failed_msg), + (xdrproc_t)xdr_remote_domain_event_callback_device_removal_failed_msg }, }; static void @@ -4829,6 +4837,28 @@ remoteDomainBuildEventCallbackDeviceAdded(virNetClientProgramPtr prog ATTRIBUTE_ remoteEventQueue(priv, event, msg->callbackID); } + +static void +remoteDomainBuildEventCallbackDeviceRemovalFailed(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, + virNetClientPtr client ATTRIBUTE_UNUSED, + void *evdata, void *opaque) +{ + virConnectPtr conn = opaque; + remote_domain_event_callback_device_added_msg *msg = evdata; + struct private_data *priv = conn->privateData; + virDomainPtr dom; + virObjectEventPtr event = NULL; + + if (!(dom = get_nonnull_domain(conn, msg->dom))) + return; + + event = virDomainEventDeviceRemovalFailedNewFromDom(dom, msg->devAlias); + + virObjectUnref(dom); + + remoteEventQueue(priv, event, msg->callbackID); +} + static void remoteDomainBuildEventCallbackTunable(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, virNetClientPtr client ATTRIBUTE_UNUSED, diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 8bda792..bab8ef2 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3261,6 +3261,12 @@ struct remote_domain_migrate_start_post_copy_args { unsigned int flags; }; +struct remote_domain_event_callback_device_removal_failed_msg { + int callbackID; + remote_nonnull_domain dom; + remote_nonnull_string devAlias; +}; + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -5781,5 +5787,11 @@ enum remote_procedure { * @generate: both * @acl: domain:write */ - REMOTE_PROC_DOMAIN_SET_PERF_EVENTS = 366 + REMOTE_PROC_DOMAIN_SET_PERF_EVENTS = 366, + + /** + * @generate: both + * @acl: none + */ + REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_REMOVAL_FAILED = 367 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 6dddd52..fe1b8a8 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2730,6 +2730,11 @@ struct remote_domain_migrate_start_post_copy_args { remote_nonnull_domain dom; u_int flags; }; +struct remote_domain_event_callback_device_removal_failed_msg { + int callbackID; + remote_nonnull_domain dom; + remote_nonnull_string devAlias; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -3097,4 +3102,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_MIGRATE_START_POST_COPY = 364, REMOTE_PROC_DOMAIN_GET_PERF_EVENTS = 365, REMOTE_PROC_DOMAIN_SET_PERF_EVENTS = 366, + REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_REMOVAL_FAILED = 367, }; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 36d0353..6d4265c 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12301,6 +12301,22 @@ virshEventJobCompletedPrint(virConnectPtr conn ATTRIBUTE_UNUSED, virshEventPrint(opaque, &buf); } + +static void +virshEventDeviceRemovalFailedPrint(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainPtr dom, + const char *alias, + void *opaque) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&buf, _("event 'device-removal-failed' for domain %s: %s\n"), + virDomainGetName(dom), + alias); + virshEventPrint(opaque, &buf); +} + + static vshEventCallback vshEventCallbacks[] = { { "lifecycle", VIR_DOMAIN_EVENT_CALLBACK(virshEventLifecyclePrint), }, @@ -12344,6 +12360,8 @@ static vshEventCallback vshEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(virshEventMigrationIterationPrint), }, { "job-completed", VIR_DOMAIN_EVENT_CALLBACK(virshEventJobCompletedPrint), }, + { "device-removal-failed", + VIR_DOMAIN_EVENT_CALLBACK(virshEventDeviceRemovalFailedPrint), }, }; verify(VIR_DOMAIN_EVENT_ID_LAST == ARRAY_CARDINALITY(vshEventCallbacks)); -- 2.8.0
On 04/05/2016 11:09 AM, Peter Krempa wrote:
Since we didn't opt to use one single event for device lifecycle for a VM we are missing one last event if the device removal failed. This event will be emitted once we asked to eject the device but for some reason it is not possible. --- daemon/remote.c | 36 +++++++++++++++++ include/libvirt/libvirt-domain.h | 21 ++++++++++ src/conf/domain_event.c | 84 ++++++++++++++++++++++++++++++++++++++++ src/conf/domain_event.h | 7 ++++ src/libvirt_private.syms | 2 + src/remote/remote_driver.c | 30 ++++++++++++++ src/remote/remote_protocol.x | 14 ++++++- src/remote_protocol-structs | 6 +++ tools/virsh-domain.c | 18 +++++++++ 9 files changed, 217 insertions(+), 1 deletion(-)
ACK, I verified it largely mirrors DeviceAdded signal. event-test.c follow up patch? - Cole
On Tue, Apr 12, 2016 at 20:03:41 -0400, Cole Robinson wrote:
On 04/05/2016 11:09 AM, Peter Krempa wrote:
Since we didn't opt to use one single event for device lifecycle for a VM we are missing one last event if the device removal failed. This event will be emitted once we asked to eject the device but for some reason it is not possible. --- daemon/remote.c | 36 +++++++++++++++++ include/libvirt/libvirt-domain.h | 21 ++++++++++ src/conf/domain_event.c | 84 ++++++++++++++++++++++++++++++++++++++++ src/conf/domain_event.h | 7 ++++ src/libvirt_private.syms | 2 + src/remote/remote_driver.c | 30 ++++++++++++++ src/remote/remote_protocol.x | 14 ++++++- src/remote_protocol-structs | 6 +++ tools/virsh-domain.c | 18 +++++++++ 9 files changed, 217 insertions(+), 1 deletion(-)
ACK, I verified it largely mirrors DeviceAdded signal.
I largely inspired myself by DeviceAdded :)
event-test.c follow up patch?
gah I forgot. I'll follow up, looks like I'll refactor it prior to adding the new stuff. Thanks.
The event is emitted on ACPI OSPM Status Indication events. ACPI standard documentation describes the method as: This object is an optional control method that is invoked by OSPM to indicate processing status to the platform. During device ejection, device hot add, or other event processing, OSPM may need to perform specific handshaking with the platform. OSPM may also need to indicate to the platform its inability to complete a requested operation; for example, when a user presses an ejection button for a device that is currently in use or is otherwise currently incapable of being ejected. In this case, the processing of the ACPI Eject Request notification by OSPM fails. OSPM may indicate this failure to the platform through the invocation of the _OST control method. As a result of the status notification indicating ejection failure, the platform may take certain action including reissuing the notification or perhaps turning on an appropriate indicator light to signal the failure to the user. --- src/qemu/qemu_monitor.c | 19 +++++++++++++++++++ src/qemu/qemu_monitor.h | 18 ++++++++++++++++++ src/qemu/qemu_monitor_json.c | 39 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 10a6713..b7e4fa9 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1549,6 +1549,25 @@ qemuMonitorEmitMigrationPass(qemuMonitorPtr mon, int +qemuMonitorEmitAcpiOstInfo(qemuMonitorPtr mon, + const char *alias, + const char *slotType, + const char *slot, + unsigned int source, + unsigned int status) +{ + int ret = -1; + VIR_DEBUG("mon=%p, alias='%s', slotType='%s', slot='%s', source='%u' status=%u", + mon, NULLSTR(alias), slotType, slot, source, status); + + QEMU_MONITOR_CALLBACK(mon, ret, domainAcpiOstInfo, mon->vm, + alias, slotType, slot, source, status); + + return ret; +} + + +int qemuMonitorSetCapabilities(qemuMonitorPtr mon) { QEMU_CHECK_MONITOR(mon); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 7906361..bb74917 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -196,6 +196,16 @@ typedef int (*qemuMonitorDomainMigrationPassCallback)(qemuMonitorPtr mon, int pass, void *opaque); +typedef int (*qemuMonitorDomainAcpiOstInfoCallback)(qemuMonitorPtr mon, + virDomainObjPtr vm, + const char *alias, + const char *slotType, + const char *slot, + unsigned int source, + unsigned int status, + void *opaque); + + typedef struct _qemuMonitorCallbacks qemuMonitorCallbacks; typedef qemuMonitorCallbacks *qemuMonitorCallbacksPtr; struct _qemuMonitorCallbacks { @@ -226,6 +236,7 @@ struct _qemuMonitorCallbacks { qemuMonitorDomainSpiceMigratedCallback domainSpiceMigrated; qemuMonitorDomainMigrationStatusCallback domainMigrationStatus; qemuMonitorDomainMigrationPassCallback domainMigrationPass; + qemuMonitorDomainAcpiOstInfoCallback domainAcpiOstInfo; }; char *qemuMonitorEscapeArg(const char *in); @@ -338,6 +349,13 @@ int qemuMonitorEmitMigrationStatus(qemuMonitorPtr mon, int qemuMonitorEmitMigrationPass(qemuMonitorPtr mon, int pass); +int qemuMonitorEmitAcpiOstInfo(qemuMonitorPtr mon, + const char *alias, + const char *slotType, + const char *slot, + unsigned int source, + unsigned int status); + int qemuMonitorStartCPUs(qemuMonitorPtr mon, virConnectPtr conn); int qemuMonitorStopCPUs(qemuMonitorPtr mon); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e140d0e..78af83e 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -88,6 +88,7 @@ static void qemuMonitorJSONHandleSerialChange(qemuMonitorPtr mon, virJSONValuePt static void qemuMonitorJSONHandleSpiceMigrated(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleMigrationStatus(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleMigrationPass(qemuMonitorPtr mon, virJSONValuePtr data); +static void qemuMonitorJSONHandleAcpiOstInfo(qemuMonitorPtr mon, virJSONValuePtr data); typedef struct { const char *type; @@ -95,6 +96,7 @@ typedef struct { } qemuEventHandler; static qemuEventHandler eventHandlers[] = { + { "ACPI_DEVICE_OST", qemuMonitorJSONHandleAcpiOstInfo, }, { "BALLOON_CHANGE", qemuMonitorJSONHandleBalloonChange, }, { "BLOCK_IO_ERROR", qemuMonitorJSONHandleIOError, }, { "BLOCK_JOB_CANCELLED", qemuMonitorJSONHandleBlockJobCanceled, }, @@ -1026,6 +1028,43 @@ qemuMonitorJSONHandleMigrationPass(qemuMonitorPtr mon, } +static void +qemuMonitorJSONHandleAcpiOstInfo(qemuMonitorPtr mon, virJSONValuePtr data) +{ + virJSONValuePtr info; + const char *alias; + const char *slotType; + const char *slot; + unsigned int source; + unsigned int status; + + if (!(info = virJSONValueObjectGetObject(data, "info"))) + goto error; + + /* optional */ + alias = virJSONValueObjectGetString(info, "device"); + + if (!(slotType = virJSONValueObjectGetString(info, "slot-type"))) + goto error; + + if (!(slot = virJSONValueObjectGetString(info, "slot"))) + goto error; + + if (virJSONValueObjectGetNumberUint(info, "source", &source) < 0) + goto error; + + if (virJSONValueObjectGetNumberUint(info, "status", &status) < 0) + goto error; + + qemuMonitorEmitAcpiOstInfo(mon, alias, slotType, slot, source, status); + return; + + error: + VIR_WARN("malformed ACPI_DEVICE_OST event"); + return; +} + + int qemuMonitorJSONHumanCommandWithFd(qemuMonitorPtr mon, const char *cmd_str, -- 2.8.0
On 04/05/2016 11:09 AM, Peter Krempa wrote:
The event is emitted on ACPI OSPM Status Indication events.
ACPI standard documentation describes the method as:
This object is an optional control method that is invoked by OSPM to indicate processing status to the platform. During device ejection, device hot add, or other event processing, OSPM may need to perform specific handshaking with the platform. OSPM may also need to indicate to the platform its inability to complete a requested operation; for example, when a user presses an ejection button for a device that is currently in use or is otherwise currently incapable of being ejected. In this case, the processing of the ACPI Eject Request notification by OSPM fails. OSPM may indicate this failure to the platform through the invocation of the _OST control method. As a result of the status notification indicating ejection failure, the platform may take certain action including reissuing the notification or perhaps turning on an appropriate indicator light to signal the failure to the user.
ACK - Cole
Since qemu is now able to notify us that the guest rejected the memory unplug operation we can relay this to the user and make the API fail right away. Additionally document the possible values from the ACPI docs for future reference. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1320447 --- src/qemu/qemu_process.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bbde32c..effc9b1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1388,6 +1388,74 @@ qemuProcessHandleDeviceDeleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED, } +/** + * + * Meaning of fields reported by the event according to the ACPI standard: + * @source: + * 0x00 - 0xff: Notification values, as passed at the request time + * 0x100: Operating System Shutdown Processing + * 0x103: Ejection processing + * 0x200: Insertion processing + * other values are reserved + * + * @status: + * general values + * 0x00: success + * 0x01: non-specific failure + * 0x02: unrecognized notify code + * 0x03 - 0x7f: reserved + * other values are specific to the notification type + * + * for the 0x100 source the following additional codes are standardized + * 0x80: OS Shutdown request denied + * 0x81: OS Shutdown in progress + * 0x82: OS Shutdown completed + * 0x83: OS Graceful shutdown not supported + * other values are reserved + * + * Other fields and semantics are specific to the qemu handling of the event. + * - @alias may be NULL for successful unplug operations + * - @slotType describes the device type a bit more closely, currently the + * only known value is 'DIMM' + * - @slot describes the specific device + * + * Note that qemu does not emit the event for all the documented sources or + * devices. + */ +static int +qemuProcessHandleAcpiOstInfo(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm, + const char *alias, + const char *slotType, + const char *slot, + unsigned int source, + unsigned int status, + void *opaque) +{ + virQEMUDriverPtr driver = opaque; + virObjectEventPtr event = NULL; + + virObjectLock(vm); + + VIR_DEBUG("ACPI OST info for device %s domain %p %s. " + "slotType='%s' slot='%s' source=%u status=%u", + NULLSTR(alias), vm, vm->def->name, slotType, slot, source, status); + + /* handle memory unplug failure */ + if (STREQ(slotType, "DIMM") && alias && status == 1) { + qemuDomainSignalDeviceRemoval(vm, alias, + QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_GUEST_REJECTED); + + event = virDomainEventDeviceRemovalFailedNewFromObj(vm, alias); + } + + virObjectUnlock(vm); + qemuDomainEventQueue(driver, event); + + return 0; +} + + static int qemuProcessHandleNicRxFilterChanged(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virDomainObjPtr vm, @@ -1583,6 +1651,7 @@ static qemuMonitorCallbacks monitorCallbacks = { .domainSpiceMigrated = qemuProcessHandleSpiceMigrated, .domainMigrationStatus = qemuProcessHandleMigrationStatus, .domainMigrationPass = qemuProcessHandleMigrationPass, + .domainAcpiOstInfo = qemuProcessHandleAcpiOstInfo, }; static void -- 2.8.0
On 04/05/2016 11:09 AM, Peter Krempa wrote:
Since qemu is now able to notify us that the guest rejected the memory unplug operation we can relay this to the user and make the API fail right away.
Additionally document the possible values from the ACPI docs for future reference.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1320447
ACK, nice series. So do the ACPI bits give us info about any other device hotplug failures that can be added later? Or is this only for memory hotplug? - Cole
On Tue, Apr 12, 2016 at 20:13:47 -0400, Cole Robinson wrote:
On 04/05/2016 11:09 AM, Peter Krempa wrote:
Since qemu is now able to notify us that the guest rejected the memory unplug operation we can relay this to the user and make the API fail right away.
Additionally document the possible values from the ACPI docs for future reference.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1320447
ACK, nice series. So do the ACPI bits give us info about any other device hotplug failures that can be added later? Or is this only for memory hotplug?
AFAIK qemu does not currently emit that event for anything besides memory devices. It would be really cool and we will be prepared for the moment if they start emit IT. According to the ACPI specs it should be possible. Peter
On Tue, Apr 05, 2016 at 17:09:16 +0200, Peter Krempa wrote:
Add a new libvirt event VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED that will handle failure to unplug a device if we are certain that it will not be unplugged and wire up the ACPI_DEVICE_OST qemu event that is emitted on memory hotunplug failure so that the event is propagated and the API fails in such case.
Along with that a few refactors were necessary.
Ping? Anybody brave enough to look at this? Thanks. Peter
On 04/12/2016 09:53 AM, Peter Krempa wrote:
On Tue, Apr 05, 2016 at 17:09:16 +0200, Peter Krempa wrote:
Add a new libvirt event VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED that will handle failure to unplug a device if we are certain that it will not be unplugged and wire up the ACPI_DEVICE_OST qemu event that is emitted on memory hotunplug failure so that the event is propagated and the API fails in such case.
Along with that a few refactors were necessary.
Ping? Anybody brave enough to look at this? Thanks.
I'll review it today if no one else gets to it, I just started looking at your disk XML refactoring bits too - Cole
participants (2)
-
Cole Robinson -
Peter Krempa