[PATCH 0/1] qemu: support VIR_DOMAIN_VCPU_ASYNC for live vCPU unplug
This series adds asynchronous live vCPU unplug support for QEMU. It introduces a new flag `VIR_DOMAIN_VCPU_ASYNC` for the vCPU management APIs and adds a new libvirt event with the ID `VIR_DOMAIN_EVENT_ID_VCPU_REMOVED` that contains the vCPU ID. When async mode is requested, libvirt returns after successfully submitting the unplug request, instead of doing a short wait for the event to arrive (like its non-async counterpart); it reports the final outcome through domain events: successful completion emits a `vcpu-removed` event while guest-rejected unplug requests continue to emit `device-removal-failed`. This also addresses the current gap where a successful vCPU hot-unplug does not emit any domain event that `virsh event` (or any listener that uses the libvirt APIs) can observe. With the new event now in place, third-party API users can now watch for successful unplug completion events, instead of only seeing failure notification events. Async mode is limited to live vCPU unplug only. The series also threads the new event through the remote protocol and extends the user-facing tooling around it: both `virsh setvcpus` and `virsh setvcpu` gain a new `--async` flag, and `virsh event` gains `vcpu-removed` as an event that it can watch for and track successful unplug operations. All existing tests covering these paths have passed. A follow-up commit adding/enhancing explicit async test coverage is forthcoming. Related discussion: [0] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/2J53U... --- examples/c/misc/event-test.c | 12 ++++++ include/libvirt/libvirt-domain.h | 23 ++++++++++++ src/conf/domain_event.c | 66 +++++++++++++++++++++++++++++++++ src/conf/domain_event.h | 6 +++ src/libvirt-domain.c | 40 +++++++++++++++++++- src/libvirt_private.syms | 2 + src/qemu/qemu_driver.c | 33 ++++++++++++++--- src/qemu/qemu_hotplug.c | 74 ++++++++++++++++++++++++++++++++----- src/qemu/qemu_hotplug.h | 8 ++-- src/remote/remote_daemon_dispatch.c | 26 +++++++++++++ src/remote/remote_driver.c | 29 +++++++++++++++ src/remote/remote_protocol.x | 14 ++++++- src/remote_protocol-structs | 6 +++ tests/qemuhotplugtest.c | 6 ++- tools/virsh-domain-event.c | 16 ++++++++ tools/virsh-domain.c | 34 +++++++++++++++++ 16 files changed, 373 insertions(+), 22 deletions(-)
Add VIR_DOMAIN_VCPU_ASYNC to the vCPU management APIs and introduce VIR_DOMAIN_EVENT_ID_VCPU_REMOVED, carrying the libvirt XML vCPU id. For live vCPU unplug, async mode returns after successfully submitting the unplug request instead of doing the short wait used by the non-async path. The live XML is left unchanged until completion is confirmed, and the final outcome is reported through domain events: successful completion emits VCPU_REMOVED, while guest-rejected unplug requests continue to emit DEVICE_REMOVAL_FAILED. This closes the current gap where successful vCPU hot-unplug emits no domain event for virsh event or other libvirt event consumers to observe. Thread the new event through the remote protocol, add --async to virsh setvcpus and virsh setvcpu, and teach virsh event and event-test about vcpu-removed. Async mode is supported only for live vCPU unplug. Signed-off-by: Akash Kulhalli <akash.kulhalli@oracle.com> --- examples/c/misc/event-test.c | 12 +++++ include/libvirt/libvirt-domain.h | 23 +++++++++ src/conf/domain_event.c | 66 +++++++++++++++++++++++++ src/conf/domain_event.h | 6 +++ src/libvirt-domain.c | 40 +++++++++++++++- src/libvirt_private.syms | 2 + src/qemu/qemu_driver.c | 33 +++++++++++-- src/qemu/qemu_hotplug.c | 74 +++++++++++++++++++++++++---- src/qemu/qemu_hotplug.h | 8 ++-- src/remote/remote_daemon_dispatch.c | 26 ++++++++++ src/remote/remote_driver.c | 29 +++++++++++ src/remote/remote_protocol.x | 14 +++++- src/remote_protocol-structs | 6 +++ tests/qemuhotplugtest.c | 6 ++- tools/virsh-domain-event.c | 16 +++++++ tools/virsh-domain.c | 34 +++++++++++++ 16 files changed, 373 insertions(+), 22 deletions(-) diff --git a/examples/c/misc/event-test.c b/examples/c/misc/event-test.c index 2ce82ca9e088..f9e65c55f064 100644 --- a/examples/c/misc/event-test.c +++ b/examples/c/misc/event-test.c @@ -1039,6 +1039,17 @@ myDomainEventDeviceRemovalFailedCallback(virConnectPtr conn G_GNUC_UNUSED, return 0; } +static int +myDomainEventVcpuRemovedCallback(virConnectPtr conn G_GNUC_UNUSED, + virDomainPtr dom, + unsigned int vcpuid, + void *opaque G_GNUC_UNUSED) +{ + printf("%s EVENT: Domain %s(%d) vcpu removed: %u\n", + __func__, virDomainGetName(dom), virDomainGetID(dom), vcpuid); + return 0; +} + static const char * metadataTypeToStr(int status) @@ -1183,6 +1194,7 @@ struct domainEventData domainEvents[] = { DOMAIN_EVENT(VIR_DOMAIN_EVENT_ID_MEMORY_FAILURE, myDomainEventMemoryFailureCallback), DOMAIN_EVENT(VIR_DOMAIN_EVENT_ID_MEMORY_DEVICE_SIZE_CHANGE, myDomainEventMemoryDeviceSizeChangeCallback), DOMAIN_EVENT(VIR_DOMAIN_EVENT_ID_NIC_MAC_CHANGE, myDomainEventNICMACChangeCallback), + DOMAIN_EVENT(VIR_DOMAIN_EVENT_ID_VCPU_REMOVED, myDomainEventVcpuRemovedCallback), }; struct storagePoolEventData { diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 4a8e3114b35d..a326d133ee41 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2605,6 +2605,7 @@ typedef enum { VIR_DOMAIN_VCPU_MAXIMUM = (1 << 2), /* Max rather than current count (Since: 0.8.5) */ VIR_DOMAIN_VCPU_GUEST = (1 << 3), /* Modify state of the cpu in the guest (Since: 1.1.0) */ VIR_DOMAIN_VCPU_HOTPLUGGABLE = (1 << 4), /* Make vcpus added hot(un)pluggable (Since: 2.4.0) */ + VIR_DOMAIN_VCPU_ASYNC = (1 << 5), /* Return after firing live unplug request(s) (Since: 12.3.0) */ } virDomainVcpuFlags; int virDomainSetVcpus (virDomainPtr domain, @@ -6965,6 +6966,27 @@ typedef void (*virConnectDomainEventDeviceRemovalFailedCallback)(virConnectPtr c const char *devAlias, void *opaque); +/** + * virConnectDomainEventVcpuRemovedCallback: + * @conn: connection object + * @dom: domain on which the event occurred + * @vcpuid: libvirt XML vCPU id + * @opaque: application specified data + * + * This callback occurs when a vCPU is removed from the domain. + * + * The @vcpuid value matches the ``<vcpu id='...'>`` value from the domain XML. + * + * The callback signature to use when registering for an event of type + * VIR_DOMAIN_EVENT_ID_VCPU_REMOVED with virConnectDomainEventRegisterAny(). + * + * Since: 12.3.0 + */ +typedef void (*virConnectDomainEventVcpuRemovedCallback)(virConnectPtr conn, + virDomainPtr dom, + unsigned int vcpuid, + void *opaque); + /** * virConnectDomainEventMetadataChangeCallback: * @conn: connection object @@ -7617,6 +7639,7 @@ typedef enum { VIR_DOMAIN_EVENT_ID_MEMORY_FAILURE = 25, /* virConnectDomainEventMemoryFailureCallback (Since: 6.9.0) */ VIR_DOMAIN_EVENT_ID_MEMORY_DEVICE_SIZE_CHANGE = 26, /* virConnectDomainEventMemoryDeviceSizeChangeCallback (Since: 7.9.0) */ VIR_DOMAIN_EVENT_ID_NIC_MAC_CHANGE = 27, /* virConnectDomainEventNICMACChangeCallback (Since: 11.2.0) */ + VIR_DOMAIN_EVENT_ID_VCPU_REMOVED = 28, /* virConnectDomainEventVcpuRemovedCallback (Since: 12.3.0) */ # ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_EVENT_ID_LAST diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 88087bad4f21..17ad4a0d2c91 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -53,6 +53,7 @@ static virClass *virDomainEventDeviceAddedClass; static virClass *virDomainEventMigrationIterationClass; static virClass *virDomainEventJobCompletedClass; static virClass *virDomainEventDeviceRemovalFailedClass; +static virClass *virDomainEventVcpuRemovedClass; static virClass *virDomainEventMetadataChangeClass; static virClass *virDomainEventBlockThresholdClass; static virClass *virDomainEventMemoryFailureClass; @@ -78,6 +79,7 @@ static void virDomainEventDeviceAddedDispose(void *obj); static void virDomainEventMigrationIterationDispose(void *obj); static void virDomainEventJobCompletedDispose(void *obj); static void virDomainEventDeviceRemovalFailedDispose(void *obj); +static void virDomainEventVcpuRemovedDispose(void *obj); static void virDomainEventMetadataChangeDispose(void *obj); static void virDomainEventBlockThresholdDispose(void *obj); static void virDomainEventMemoryFailureDispose(void *obj); @@ -251,6 +253,13 @@ struct _virDomainEventDeviceRemovalFailed { }; typedef struct _virDomainEventDeviceRemovalFailed virDomainEventDeviceRemovalFailed; +struct _virDomainEventVcpuRemoved { + virDomainEvent parent; + + unsigned int vcpuid; +}; +typedef struct _virDomainEventVcpuRemoved virDomainEventVcpuRemoved; + struct _virDomainEventMetadataChange { virDomainEvent parent; @@ -337,6 +346,8 @@ virDomainEventsOnceInit(void) return -1; if (!VIR_CLASS_NEW(virDomainEventDeviceRemovalFailed, virDomainEventClass)) return -1; + if (!VIR_CLASS_NEW(virDomainEventVcpuRemoved, virDomainEventClass)) + return -1; if (!VIR_CLASS_NEW(virDomainEventMetadataChange, virDomainEventClass)) return -1; if (!VIR_CLASS_NEW(virDomainEventBlockThreshold, virDomainEventClass)) @@ -484,6 +495,13 @@ virDomainEventDeviceRemovalFailedDispose(void *obj) g_free(event->devAlias); } +static void +virDomainEventVcpuRemovedDispose(void *obj) +{ + virDomainEventVcpuRemoved *event = obj; + VIR_DEBUG("obj=%p", event); +} + static void virDomainEventPMDispose(void *obj) @@ -1382,6 +1400,43 @@ virDomainEventDeviceRemovalFailedNewFromDom(virDomainPtr dom, devAlias); } +static virObjectEvent * +virDomainEventVcpuRemovedNew(int id, + const char *name, + unsigned char *uuid, + unsigned int vcpuid) +{ + virDomainEventVcpuRemoved *ev; + + if (virDomainEventsInitialize() < 0) + return NULL; + + if (!(ev = virDomainEventNew(virDomainEventVcpuRemovedClass, + VIR_DOMAIN_EVENT_ID_VCPU_REMOVED, + id, name, uuid))) + return NULL; + + ev->vcpuid = vcpuid; + + return (virObjectEvent *)ev; +} + +virObjectEvent * +virDomainEventVcpuRemovedNewFromObj(virDomainObj *obj, + unsigned int vcpuid) +{ + return virDomainEventVcpuRemovedNew(obj->def->id, obj->def->name, + obj->def->uuid, vcpuid); +} + +virObjectEvent * +virDomainEventVcpuRemovedNewFromDom(virDomainPtr dom, + unsigned int vcpuid) +{ + return virDomainEventVcpuRemovedNew(dom->id, dom->name, dom->uuid, + vcpuid); +} + static virObjectEvent * virDomainEventAgentLifecycleNew(int id, @@ -2134,6 +2189,17 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn, goto cleanup; } + case VIR_DOMAIN_EVENT_ID_VCPU_REMOVED: + { + virDomainEventVcpuRemoved *vcpuRemovedEvent; + + vcpuRemovedEvent = (virDomainEventVcpuRemoved *)event; + ((virConnectDomainEventVcpuRemovedCallback)cb)(conn, dom, + vcpuRemovedEvent->vcpuid, + 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 f31cfb9e42ad..1b1b16095a77 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -192,6 +192,12 @@ virDomainEventDeviceRemovalFailedNewFromObj(virDomainObj *obj, virObjectEvent * virDomainEventDeviceRemovalFailedNewFromDom(virDomainPtr dom, const char *devAlias); +virObjectEvent * +virDomainEventVcpuRemovedNewFromObj(virDomainObj *obj, + unsigned int vcpuid); +virObjectEvent * +virDomainEventVcpuRemovedNewFromDom(virDomainPtr dom, + unsigned int vcpuid); virObjectEvent * virDomainEventTunableNewFromObj(virDomainObj *obj, diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index db9eea57745c..c290dc6efeca 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -7702,6 +7702,14 @@ virDomainSendProcessSignal(virDomainPtr domain, * whether it also affects persistent configuration; for more control, * use virDomainSetVcpusFlags(). * + * When this API decreases the live vCPU count by hot-unplugging vCPUs in + * the hypervisor, completion may be asynchronous. Successful unplug + * completion is reported by VIR_DOMAIN_EVENT_ID_VCPU_REMOVED, carrying the + * XML ``<vcpu id='...'>`` value. Rejected unplug requests continue to be + * reported by VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED. The success event + * may be delivered before this API call returns. A timeout is reported only + * through the returned error, not through a domain event. + * * Returns 0 in case of success, -1 in case of failure. * * Since: 0.1.4 @@ -7773,8 +7781,23 @@ virDomainSetVcpus(virDomainPtr domain, unsigned int nvcpus) * be used with live guests and is incompatible with VIR_DOMAIN_VCPU_MAXIMUM. * The usage of this flag may require a guest agent configured. * + * If @flags includes VIR_DOMAIN_VCPU_ASYNC, only vCPU hot-unplug is requested + * asynchronously. In this mode, success means that all required unplug + * request(s) were successfully fired; final completion is reported by + * the VIR_DOMAIN_EVENT_ID_VCPU_REMOVED event, and rejection is reported by a + * `VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED` event type. + * * Not all hypervisors can support all flag combinations. * + * When this API decreases the live vCPU count by hot-unplugging vCPUs, + * completion may be asynchronous. Successful unplug completion is reported by + * VIR_DOMAIN_EVENT_ID_VCPU_REMOVED, carrying the XML ``<vcpu id='...'>`` + * value within the event data. Rejected unplug requests continue to be + * reported by VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED. The success event + * may be delivered before this API call returns. In non-async mode, a + * timeout is reported only through the returned error, not through a domain + * event. + * * Returns 0 in case of success, -1 in case of failure. * * Since: 0.8.5 @@ -13125,7 +13148,8 @@ virDomainSetGuestVcpus(virDomainPtr domain, * @domain: pointer to domain object * @vcpumap: text representation of a bitmap of vcpus to set * @state: 0 to disable/1 to enable cpus described by @vcpumap - * @flags: bitwise-OR of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact with optional + * VIR_DOMAIN_VCPU_ASYNC * * Enables/disables individual vcpus described by @vcpumap in the hypervisor. * @@ -13134,6 +13158,20 @@ virDomainSetGuestVcpus(virDomainPtr domain, * * Note that OSes and hypervisors may require vCPU 0 to stay online. * + * If @flags includes VIR_DOMAIN_VCPU_ASYNC, only live vCPU disable + * (hot-unplug) is requested asynchronously. In this mode, success means the + * unplug request was successfully fired; final completion is reported by + * VIR_DOMAIN_EVENT_ID_VCPU_REMOVED, while rejection is reported by + * VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED. + * + * When this API disables live vCPUs by hot-unplugging them, the operation + * completion may be asynchronous. Successful unplug completion is reported by + * VIR_DOMAIN_EVENT_ID_VCPU_REMOVED, carrying the XML ``<vcpu id='...'>`` + * value. Rejected unplug requests continue to be reported by + * VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED. The success event may be + * delivered before this API call returns. In non-async mode, a timeout is + * reported only through the returned error, not through a domain event. + * * Returns 0 on success, -1 on error. * * Since: 3.1.0 diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cf0e71cc6af1..f22b5895db12 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -811,6 +811,8 @@ virDomainEventTrayChangeNewFromDom; virDomainEventTrayChangeNewFromObj; virDomainEventTunableNewFromDom; virDomainEventTunableNewFromObj; +virDomainEventVcpuRemovedNewFromDom; +virDomainEventVcpuRemovedNewFromObj; virDomainEventWatchdogNewFromDom; virDomainEventWatchdogNewFromObj; virDomainQemuMonitorEventNew; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d227ac58cdb4..ad4dc11c970f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3620,7 +3620,14 @@ processDeviceDeletedEvent(virQEMUDriver *driver, } if (STRPREFIX(devAlias, "vcpu")) { - qemuDomainRemoveVcpuAlias(vm, devAlias); + int vcpuid; + virObjectEvent *event; + if ((vcpuid = qemuDomainRemoveVcpuAlias(vm, devAlias)) == -1) + goto endjob; + + event = virDomainEventVcpuRemovedNewFromObj(vm, vcpuid); + virObjectEventStateQueue(driver->domainEventState, event); + } else { if (virDomainDefFindDevice(vm->def, devAlias, &dev, true) < 0) goto endjob; @@ -4269,6 +4276,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, virDomainObj *vm = NULL; virDomainDef *def; virDomainDef *persistentDef; + bool async = !!(flags & VIR_DOMAIN_VCPU_ASYNC); bool hotpluggable = !!(flags & VIR_DOMAIN_VCPU_HOTPLUGGABLE); bool useAgent = !!(flags & VIR_DOMAIN_VCPU_GUEST); int ret = -1; @@ -4277,7 +4285,8 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_VCPU_GUEST | - VIR_DOMAIN_VCPU_HOTPLUGGABLE, -1); + VIR_DOMAIN_VCPU_HOTPLUGGABLE | + VIR_DOMAIN_VCPU_ASYNC, -1); if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup; @@ -4297,13 +4306,19 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) goto endjob; + if (async && (useAgent || persistentDef || !def)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("asynchronous mode is supported only for live vcpu unplug")); + goto endjob; + } + if (useAgent) ret = qemuDomainSetVcpusAgent(vm, nvcpus); else if (flags & VIR_DOMAIN_VCPU_MAXIMUM) ret = qemuDomainSetVcpusMax(driver, vm, def, persistentDef, nvcpus); else ret = qemuDomainSetVcpusInternal(driver, vm, def, persistentDef, - nvcpus, hotpluggable); + nvcpus, hotpluggable, async); endjob: if (useAgent) @@ -19169,12 +19184,14 @@ qemuDomainSetVcpu(virDomainPtr dom, virDomainObj *vm = NULL; virDomainDef *def = NULL; virDomainDef *persistentDef = NULL; + bool async = !!(flags & VIR_DOMAIN_VCPU_ASYNC); g_autoptr(virBitmap) map = NULL; ssize_t lastvcpu; int ret = -1; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | - VIR_DOMAIN_AFFECT_CONFIG, -1); + VIR_DOMAIN_AFFECT_CONFIG | + VIR_DOMAIN_VCPU_ASYNC, -1); if (state != 0 && state != 1) { virReportInvalidArg(state, "%s", _("unsupported state value")); @@ -19220,7 +19237,13 @@ qemuDomainSetVcpu(virDomainPtr dom, } } - ret = qemuDomainSetVcpuInternal(driver, vm, def, persistentDef, map, !!state); + if (async && (persistentDef || !def)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("asynchronous mode is supported only for live vcpu unplug")); + goto endjob; + } + + ret = qemuDomainSetVcpuInternal(driver, vm, def, persistentDef, map, !!state, async); endjob: virDomainObjEndJob(vm); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b7a282b96e52..0b3a781cea19 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5723,6 +5723,12 @@ qemuDomainRemoveDevice(virQEMUDriver *driver, return 0; } +static bool +qemuDomainDeviceRemoved(virDomainObj *vm) +{ + qemuDomainObjPrivate *priv = vm->privateData; + return priv->unplug.status == QEMU_DOMAIN_UNPLUGGING_DEVICE_STATUS_OK; +} static void qemuDomainMarkDeviceAliasForRemoval(virDomainObj *vm, @@ -6761,7 +6767,7 @@ qemuDomainRemoveVcpu(virDomainObj *vm, } -void +int qemuDomainRemoveVcpuAlias(virDomainObj *vm, const char *alias) { @@ -6774,13 +6780,16 @@ qemuDomainRemoveVcpuAlias(virDomainObj *vm, vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu); if (STREQ_NULLABLE(alias, vcpupriv->alias)) { - qemuDomainRemoveVcpu(vm, i); - return; + if (qemuDomainRemoveVcpu(vm, i) < 0) + return -1; + + return i; } } VIR_DEBUG("vcpu '%s' not found in vcpulist of domain '%s'", alias, vm->def->name); + return -1; } @@ -6788,7 +6797,8 @@ static int qemuDomainHotplugDelVcpu(virQEMUDriver *driver, virQEMUDriverConfig *cfg, virDomainObj *vm, - unsigned int vcpu) + unsigned int vcpu, + bool async) { virDomainVcpuDef *vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu); qemuDomainVcpuPrivate *vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo); @@ -6796,6 +6806,7 @@ qemuDomainHotplugDelVcpu(virQEMUDriver *driver, unsigned int nvcpus = vcpupriv->vcpus; int rc; int ret = -1; + virObjectEvent *event = NULL; if (!vcpupriv->alias) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, @@ -6813,6 +6824,22 @@ qemuDomainHotplugDelVcpu(virQEMUDriver *driver, goto cleanup; } } else { + if (async) { + /* rc = 0 is implied in this branch */ + if (qemuDomainDeviceRemoved(vm)) { + /* event has already arrived, handle it now */ + goto success; + } + /* + * event has not arrived yet, but the monitor operation was + * successful; there will not be a waiter anymore when this thread + * exits. Reset removal state now so that the event handling path + * can be properly triggered if and when the event does arrive + */ + ret = 0; + goto cleanup; + } + if ((rc = qemuDomainWaitForDeviceRemoval(vm)) <= 0) { if (rc == 0) virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s", @@ -6821,6 +6848,7 @@ qemuDomainHotplugDelVcpu(virQEMUDriver *driver, } } + success: if (qemuDomainRemoveVcpu(vm, vcpu) < 0) goto cleanup; @@ -6831,6 +6859,10 @@ qemuDomainHotplugDelVcpu(virQEMUDriver *driver, ret = 0; + /* emit event now to close the async caller loop */ + event = virDomainEventVcpuRemovedNewFromObj(vm, vcpu); + virObjectEventStateQueue(driver->domainEventState, event); + cleanup: qemuDomainResetDeviceRemoval(vm); return ret; @@ -7003,7 +7035,8 @@ qemuDomainSetVcpusLive(virQEMUDriver *driver, virQEMUDriverConfig *cfg, virDomainObj *vm, virBitmap *vcpumap, - bool enable) + bool enable, + bool async) { qemuDomainObjPrivate *priv = vm->privateData; virCgroupEmulatorAllNodesData *emulatorCgroup = NULL; @@ -7023,7 +7056,7 @@ qemuDomainSetVcpusLive(virQEMUDriver *driver, if (!virBitmapIsBitSet(vcpumap, nextvcpu)) continue; - if (qemuDomainHotplugDelVcpu(driver, cfg, vm, nextvcpu) < 0) + if (qemuDomainHotplugDelVcpu(driver, cfg, vm, nextvcpu, async) < 0) goto cleanup; } } @@ -7114,12 +7147,20 @@ qemuDomainSetVcpusInternal(virQEMUDriver *driver, virDomainDef *def, virDomainDef *persistentDef, unsigned int nvcpus, - bool hotpluggable) + bool hotpluggable, + bool async) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(virBitmap) vcpumap = NULL; bool enable; + if (async && persistentDef) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("asynchronous mode is supported only for live vcpu unplug")); + + return -1; + } + if (def && nvcpus > virDomainDefGetVcpusMax(def)) { virReportError(VIR_ERR_INVALID_ARG, _("requested vcpus is greater than max allowable vcpus for the live domain: %1$u > %2$u"), @@ -7139,7 +7180,13 @@ qemuDomainSetVcpusInternal(virQEMUDriver *driver, &enable))) return -1; - if (qemuDomainSetVcpusLive(driver, cfg, vm, vcpumap, enable) < 0) + if (async && enable) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("asynchronous mode is supported only for vcpu unplug")); + return -1; + } + + if (qemuDomainSetVcpusLive(driver, cfg, vm, vcpumap, enable, async) < 0) return -1; } @@ -7289,7 +7336,8 @@ qemuDomainSetVcpuInternal(virQEMUDriver *driver, virDomainDef *def, virDomainDef *persistentDef, virBitmap *map, - bool state) + bool state, + bool async) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(virBitmap) livevcpus = NULL; @@ -7320,8 +7368,14 @@ qemuDomainSetVcpuInternal(virQEMUDriver *driver, return -1; } + if (async && state) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("asynchronous mode is supported only for vcpu unplug")); + return -1; + } + if (livevcpus && - qemuDomainSetVcpusLive(driver, cfg, vm, livevcpus, state) < 0) + qemuDomainSetVcpusLive(driver, cfg, vm, livevcpus, state, async) < 0) return -1; if (persistentDef) { diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index e6c90253e416..aa2a9ea6eb75 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -79,7 +79,7 @@ qemuDomainUpdateDeviceLive(virDomainObj *vm, virQEMUDriver *driver, bool force); -void +int qemuDomainRemoveVcpuAlias(virDomainObj *vm, const char *alias); @@ -106,7 +106,8 @@ qemuDomainSetVcpusInternal(virQEMUDriver *driver, virDomainDef *def, virDomainDef *persistentDef, unsigned int nvcpus, - bool hotpluggable); + bool hotpluggable, + bool async); int qemuDomainSetVcpuInternal(virQEMUDriver *driver, @@ -114,7 +115,8 @@ qemuDomainSetVcpuInternal(virQEMUDriver *driver, virDomainDef *def, virDomainDef *persistentDef, virBitmap *vcpus, - bool state); + bool state, + bool async); unsigned long long qemuDomainGetUnplugTimeout(virDomainObj *vm) ATTRIBUTE_MOCKABLE; diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 7e74ff063f5b..81b0ed00da1a 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -1322,6 +1322,31 @@ remoteRelayDomainEventMemoryDeviceSizeChange(virConnectPtr conn, return 0; } +static int +remoteRelayDomainEventVcpuRemoved(virConnectPtr conn, + virDomainPtr dom, + unsigned int vcpuid, + void *opaque) +{ + daemonClientEventCallback *callback = opaque; + remote_domain_event_vcpu_removed_msg data; + + if (callback->callbackID < 0 || + !remoteRelayDomainEventCheckACL(callback->client, conn, dom)) + return -1; + + memset(&data, 0, sizeof(data)); + data.callbackID = callback->callbackID; + data.vcpuid = vcpuid; + make_nonnull_domain(&data.dom, dom); + + remoteDispatchObjectEventSend(callback->client, remoteProgram, + REMOTE_PROC_DOMAIN_EVENT_VCPU_REMOVED, + (xdrproc_t)xdr_remote_domain_event_vcpu_removed_msg, + &data); + return 0; +} + static int remoteRelayDomainEventNICMACChange(virConnectPtr conn, @@ -1383,6 +1408,7 @@ static virConnectDomainEventGenericCallback domainEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventMemoryFailure), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventMemoryDeviceSizeChange), VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventNICMACChange), + VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventVcpuRemoved), }; G_STATIC_ASSERT(G_N_ELEMENTS(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST); diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index ec71eaed8762..c8a4e3f6da98 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -428,6 +428,10 @@ remoteDomainBuildEventMemoryDeviceSizeChange(virNetClientProgram *prog, virNetClient *client, void *evdata, void *opaque); static void +remoteDomainBuildEventVcpuRemoved(virNetClientProgram *prog, + virNetClient *client, + void *evdata, void *opaque); +static void remoteConnectNotifyEventConnectionClosed(virNetClientProgram *prog G_GNUC_UNUSED, virNetClient *client G_GNUC_UNUSED, void *evdata, void *opaque); @@ -659,6 +663,10 @@ static virNetClientProgramEvent remoteEvents[] = { remoteDomainBuildEventNICMACChange, sizeof(remote_domain_event_nic_mac_change_msg), (xdrproc_t)xdr_remote_domain_event_nic_mac_change_msg }, + { REMOTE_PROC_DOMAIN_EVENT_VCPU_REMOVED, + remoteDomainBuildEventVcpuRemoved, + sizeof(remote_domain_event_vcpu_removed_msg), + (xdrproc_t)xdr_remote_domain_event_vcpu_removed_msg }, }; static void @@ -5138,6 +5146,27 @@ remoteDomainBuildEventMemoryDeviceSizeChange(virNetClientProgram *prog G_GNUC_UN virObjectEventStateQueueRemote(priv->eventState, event, msg->callbackID); } +static void +remoteDomainBuildEventVcpuRemoved(virNetClientProgram *prog G_GNUC_UNUSED, + virNetClient *client G_GNUC_UNUSED, + void *evdata, void *opaque) +{ + virConnectPtr conn = opaque; + remote_domain_event_vcpu_removed_msg *msg = evdata; + struct private_data *priv = conn->privateData; + virDomainPtr dom; + virObjectEvent *event = NULL; + + if (!(dom = get_nonnull_domain(conn, msg->dom))) + return; + + event = virDomainEventVcpuRemovedNewFromDom(dom, msg->vcpuid); + + virObjectUnref(dom); + + virObjectEventStateQueueRemote(priv->eventState, event, msg->callbackID); +} + static void remoteDomainBuildEventNICMACChange(virNetClientProgram *prog G_GNUC_UNUSED, diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 38a83c64eadb..23699e99a60a 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3981,6 +3981,12 @@ struct remote_domain_event_memory_device_size_change_msg { unsigned hyper size; }; +struct remote_domain_event_vcpu_removed_msg { + int callbackID; + remote_nonnull_domain dom; + unsigned int vcpuid; +}; + struct remote_domain_fd_associate_args { remote_nonnull_domain dom; @@ -7120,5 +7126,11 @@ enum remote_procedure { * @generate: both * @acl: none */ - REMOTE_PROC_DOMAIN_EVENT_NIC_MAC_CHANGE = 453 + REMOTE_PROC_DOMAIN_EVENT_NIC_MAC_CHANGE = 453, + + /** + * @generate: both + * @acl: none + */ + REMOTE_PROC_DOMAIN_EVENT_VCPU_REMOVED = 454 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 0f87d13a5ae1..75c3d0cb2e06 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -3315,6 +3315,11 @@ struct remote_domain_event_memory_device_size_change_msg { remote_nonnull_string alias; uint64_t size; }; +struct remote_domain_event_vcpu_removed_msg { + int callbackID; + remote_nonnull_domain dom; + u_int vcpuid; +}; struct remote_domain_fd_associate_args { remote_nonnull_domain dom; remote_nonnull_string name; @@ -3791,4 +3796,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SET_THROTTLE_GROUP = 451, REMOTE_PROC_DOMAIN_DEL_THROTTLE_GROUP = 452, REMOTE_PROC_DOMAIN_EVENT_NIC_MAC_CHANGE = 453, + REMOTE_PROC_DOMAIN_EVENT_VCPU_REMOVED = 454, }; diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index ea9d3243f8b1..36bc4d913826 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -322,6 +322,7 @@ struct testQemuHotplugCpuParams { GHashTable *capsLatestFiles; GHashTable *capsCache; GHashTable *schemaCache; + bool async; }; @@ -420,7 +421,7 @@ testQemuHotplugCpuGroup(const void *opaque) rc = qemuDomainSetVcpusInternal(&driver, data->vm, data->vm->def, data->vm->newDef, params->newcpus, - true); + true, params->async); if (params->fail) { if (rc == 0) @@ -458,7 +459,8 @@ testQemuHotplugCpuIndividual(const void *opaque) goto cleanup; rc = qemuDomainSetVcpuInternal(&driver, data->vm, data->vm->def, - data->vm->newDef, map, params->state); + data->vm->newDef, map, params->state, + params->async); if (params->fail) { if (rc == 0) diff --git a/tools/virsh-domain-event.c b/tools/virsh-domain-event.c index b9d1cdf019ca..fdd7bf611605 100644 --- a/tools/virsh-domain-event.c +++ b/tools/virsh-domain-event.c @@ -705,6 +705,20 @@ virshEventDeviceRemovalFailedPrint(virConnectPtr conn G_GNUC_UNUSED, virshEventPrint(opaque, &buf); } +static void +virshEventVcpuRemovedPrint(virConnectPtr conn G_GNUC_UNUSED, + virDomainPtr dom, + unsigned int vcpuid, + void *opaque) +{ + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&buf, + _("event 'vcpu-removed' for domain '%1$s': vcpu: %2$u\n"), + virDomainGetName(dom), vcpuid); + virshEventPrint(opaque, &buf); +} + VIR_ENUM_DECL(virshEventMetadataChangeType); VIR_ENUM_IMPL(virshEventMetadataChangeType, VIR_DOMAIN_METADATA_LAST, @@ -873,6 +887,8 @@ virshDomainEventCallback virshDomainEventCallbacks[] = { VIR_DOMAIN_EVENT_CALLBACK(virshEventMemoryDeviceSizeChangePrint), }, { "nic-mac-change", VIR_DOMAIN_EVENT_CALLBACK(virshEventNICMACChangePrint), }, + { "vcpu-removed", + VIR_DOMAIN_EVENT_CALLBACK(virshEventVcpuRemovedPrint), }, }; G_STATIC_ASSERT(VIR_DOMAIN_EVENT_ID_LAST == G_N_ELEMENTS(virshDomainEventCallbacks)); diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 08a1ce395378..350a3b6cd8b2 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7667,6 +7667,10 @@ static const vshCmdOptDef opts_setvcpus[] = { .type = VSH_OT_BOOL, .help = N_("make added vcpus hot(un)pluggable") }, + {.name = "async", + .type = VSH_OT_BOOL, + .help = N_("return after firing live vcpu unplug request(s)") + }, {.name = NULL} }; @@ -7681,6 +7685,7 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) bool current = vshCommandOptBool(cmd, "current"); bool guest = vshCommandOptBool(cmd, "guest"); bool hotpluggable = vshCommandOptBool(cmd, "hotpluggable"); + bool async = vshCommandOptBool(cmd, "async"); unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; VSH_EXCLUSIVE_OPTIONS_VAR(current, live); @@ -7699,6 +7704,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_VCPU_MAXIMUM; if (hotpluggable) flags |= VIR_DOMAIN_VCPU_HOTPLUGGABLE; + if (async) + flags |= VIR_DOMAIN_VCPU_ASYNC; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -7711,6 +7718,12 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) return false; } + if (async && (config || maximum || guest || hotpluggable)) { + vshError(ctl, "%s", + _("--async can be used only for live hypervisor vcpu unplug")); + return false; + } + /* none of the options were specified */ if (!current && flags == 0) { if (virDomainSetVcpus(dom, count) != 0) @@ -7720,6 +7733,10 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) return false; } + if (async) + vshPrintExtra(ctl, "%s", + _("vCPU unplug requests sent successfully\n")); + return true; } @@ -7829,6 +7846,10 @@ static const vshCmdOptDef opts_setvcpu[] = { .type = VSH_OT_BOOL, .help = N_("disable cpus specified by cpumap") }, + {.name = "async", + .type = VSH_OT_BOOL, + .help = N_("return after firing live vcpu unplug request") + }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, VIRSH_COMMON_OPT_DOMAIN_CURRENT, @@ -7843,6 +7864,7 @@ cmdSetvcpu(vshControl *ctl, const vshCmd *cmd) bool disable = vshCommandOptBool(cmd, "disable"); bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); + bool async = vshCommandOptBool(cmd, "async"); const char *vcpulist = NULL; int state = 0; unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; @@ -7856,12 +7878,20 @@ cmdSetvcpu(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_AFFECT_CONFIG; if (live) flags |= VIR_DOMAIN_AFFECT_LIVE; + if (async) + flags |= VIR_DOMAIN_VCPU_ASYNC; if (!(enable || disable)) { vshError(ctl, "%s", _("one of --enable, --disable is required")); return false; } + if (async && (config || enable)) { + vshError(ctl, "%s", + _("--async can be used only for live vcpu disable")); + return false; + } + if (vshCommandOptString(ctl, cmd, "vcpulist", &vcpulist)) return false; @@ -7874,6 +7904,10 @@ cmdSetvcpu(vshControl *ctl, const vshCmd *cmd) if (virDomainSetVcpu(dom, vcpulist, state, flags) < 0) return false; + if (async) + vshPrintExtra(ctl, "%s", + _("vCPU unplug request sent successfully\n")); + return true; } -- 2.47.3
On Thu, Apr 16, 2026 at 21:51:23 +0530, Akash Kulhalli via Devel wrote:
Add VIR_DOMAIN_VCPU_ASYNC to the vCPU management APIs and introduce VIR_DOMAIN_EVENT_ID_VCPU_REMOVED, carrying the libvirt XML vCPU id.
For live vCPU unplug, async mode returns after successfully submitting the unplug request instead of doing the short wait used by the non-async path. The live XML is left unchanged until completion is confirmed, and the final outcome is reported through domain events: successful completion emits VCPU_REMOVED, while guest-rejected unplug requests continue to emit DEVICE_REMOVAL_FAILED.
This closes the current gap where successful vCPU hot-unplug emits no domain event for virsh event or other libvirt event consumers to observe. Thread the new event through the remote protocol, add --async to virsh setvcpus and virsh setvcpu, and teach virsh event and event-test about vcpu-removed.
Async mode is supported only for live vCPU unplug.
Signed-off-by: Akash Kulhalli <akash.kulhalli@oracle.com> --- examples/c/misc/event-test.c | 12 +++++ include/libvirt/libvirt-domain.h | 23 +++++++++ src/conf/domain_event.c | 66 +++++++++++++++++++++++++ src/conf/domain_event.h | 6 +++ src/libvirt-domain.c | 40 +++++++++++++++- src/libvirt_private.syms | 2 + src/qemu/qemu_driver.c | 33 +++++++++++-- src/qemu/qemu_hotplug.c | 74 +++++++++++++++++++++++++---- src/qemu/qemu_hotplug.h | 8 ++-- src/remote/remote_daemon_dispatch.c | 26 ++++++++++ src/remote/remote_driver.c | 29 +++++++++++ src/remote/remote_protocol.x | 14 +++++- src/remote_protocol-structs | 6 +++ tests/qemuhotplugtest.c | 6 ++- tools/virsh-domain-event.c | 16 +++++++ tools/virsh-domain.c | 34 +++++++++++++
Before I even start looking at this note that we require patches to be separated to reasonable logical blocks (as individual patches, yet the code must still build cleanly). For this patch you will have to definitely separate at least the addition of the event and corresponding RPC changes. They are definitely a separate block. next patch then adds new flag and the implementation (if it's not too complex) and/or the virsh implementation, depending how you choose. Proper review will follow later.
16 files changed, 373 insertions(+), 22 deletions(-)
[...]
@@ -19220,7 +19237,13 @@ qemuDomainSetVcpu(virDomainPtr dom, } }
- ret = qemuDomainSetVcpuInternal(driver, vm, def, persistentDef, map, !!state); + if (async && (persistentDef || !def)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("asynchronous mode is supported only for live vcpu unplug")); + goto endjob;
When you're doing a combined live+config unplug it imo should also allow to use the async mode.
On Thu, Apr 16, 2026 at 21:51:23 +0530, Akash Kulhalli via Devel wrote:
Add VIR_DOMAIN_VCPU_ASYNC to the vCPU management APIs and introduce VIR_DOMAIN_EVENT_ID_VCPU_REMOVED, carrying the libvirt XML vCPU id.
For live vCPU unplug, async mode returns after successfully submitting the unplug request instead of doing the short wait used by the non-async path. The live XML is left unchanged until completion is confirmed, and the final outcome is reported through domain events: successful completion emits VCPU_REMOVED, while guest-rejected unplug requests continue to emit DEVICE_REMOVAL_FAILED.
This closes the current gap where successful vCPU hot-unplug emits no domain event for virsh event or other libvirt event consumers to observe. Thread the new event through the remote protocol, add --async to virsh setvcpus and virsh setvcpu, and teach virsh event and event-test about vcpu-removed.
Async mode is supported only for live vCPU unplug.
Signed-off-by: Akash Kulhalli <akash.kulhalli@oracle.com> --- examples/c/misc/event-test.c | 12 +++++ include/libvirt/libvirt-domain.h | 23 +++++++++ src/conf/domain_event.c | 66 +++++++++++++++++++++++++ src/conf/domain_event.h | 6 +++ src/libvirt-domain.c | 40 +++++++++++++++- src/libvirt_private.syms | 2 + src/qemu/qemu_driver.c | 33 +++++++++++-- src/qemu/qemu_hotplug.c | 74 +++++++++++++++++++++++++---- src/qemu/qemu_hotplug.h | 8 ++-- src/remote/remote_daemon_dispatch.c | 26 ++++++++++ src/remote/remote_driver.c | 29 +++++++++++ src/remote/remote_protocol.x | 14 +++++- src/remote_protocol-structs | 6 +++ tests/qemuhotplugtest.c | 6 ++- tools/virsh-domain-event.c | 16 +++++++ tools/virsh-domain.c | 34 +++++++++++++ 16 files changed, 373 insertions(+), 22 deletions(-)
As noted previously you'll need to split this patch. Split it to the following commits: 1) new event callback and corresponding boilerplate (including required virsh changes since they are enfocred at compile time) 2) wire up firing of the event on existing unplug scenarios (including corresponding change to (qemuDomainRemoveVcpuAlias) 3) changes to qemu driver internals to plumb in the flags to skip waiting for the unplug, they will be dormant at this point 4) addition of the flag for 'virDomainSetVcpusFlags', including docs and impl in qemu driver, corresponding virsh change can be included here 5) addition of the flag for 'virDomainSetVcpu' etc... ^^^
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 4a8e3114b35d..a326d133ee41 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2605,6 +2605,7 @@ typedef enum { VIR_DOMAIN_VCPU_MAXIMUM = (1 << 2), /* Max rather than current count (Since: 0.8.5) */ VIR_DOMAIN_VCPU_GUEST = (1 << 3), /* Modify state of the cpu in the guest (Since: 1.1.0) */ VIR_DOMAIN_VCPU_HOTPLUGGABLE = (1 << 4), /* Make vcpus added hot(un)pluggable (Since: 2.4.0) */ + VIR_DOMAIN_VCPU_ASYNC = (1 << 5), /* Return after firing live unplug request(s) (Since: 12.3.0) */
The description is a bit vague. I'd say "Don't wait for the guest to comply with unplug request(s)" and then explain it later in the function. If you decide to explain it here you'll have to first (in a separate patch) have to refactor the docs here to swith to the prefix-mode (move the existing comments for *all* fields before the enum so that you can use proper multiline comments. [...]
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index db9eea57745c..c290dc6efeca 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -7702,6 +7702,14 @@ virDomainSendProcessSignal(virDomainPtr domain, * whether it also affects persistent configuration; for more control, * use virDomainSetVcpusFlags(). * + * When this API decreases the live vCPU count by hot-unplugging vCPUs in + * the hypervisor, completion may be asynchronous. Successful unplug + * completion is reported by VIR_DOMAIN_EVENT_ID_VCPU_REMOVED, carrying the + * XML ``<vcpu id='...'>`` value. Rejected unplug requests continue to be + * reported by VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED. The success event + * may be delivered before this API call returns. A timeout is reported only + * through the returned error, not through a domain event.
This needs to be added when adding the event.
+ * * Returns 0 in case of success, -1 in case of failure. * * Since: 0.1.4 @@ -7773,8 +7781,23 @@ virDomainSetVcpus(virDomainPtr domain, unsigned int nvcpus) * be used with live guests and is incompatible with VIR_DOMAIN_VCPU_MAXIMUM. * The usage of this flag may require a guest agent configured. * + * If @flags includes VIR_DOMAIN_VCPU_ASYNC, only vCPU hot-unplug is requested + * asynchronously. In this mode, success means that all required unplug + * request(s) were successfully fired; final completion is reported by + * the VIR_DOMAIN_EVENT_ID_VCPU_REMOVED event, and rejection is reported by a + * `VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED` event type.
This needs to be added when adding the impl of the flag
+ * * Not all hypervisors can support all flag combinations. * + * When this API decreases the live vCPU count by hot-unplugging vCPUs, + * completion may be asynchronous. Successful unplug completion is reported by + * VIR_DOMAIN_EVENT_ID_VCPU_REMOVED, carrying the XML ``<vcpu id='...'>`` + * value within the event data. Rejected unplug requests continue to be + * reported by VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED. The success event + * may be delivered before this API call returns. In non-async mode, a + * timeout is reported only through the returned error, not through a domain + * event.
This needs to be added when adding the event.
+ * * Returns 0 in case of success, -1 in case of failure. * * Since: 0.8.5 @@ -13125,7 +13148,8 @@ virDomainSetGuestVcpus(virDomainPtr domain, * @domain: pointer to domain object * @vcpumap: text representation of a bitmap of vcpus to set * @state: 0 to disable/1 to enable cpus described by @vcpumap - * @flags: bitwise-OR of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact with optional + * VIR_DOMAIN_VCPU_ASYNC
You can't do this. You'll need to add a new enum of flags specifically for this API. Mirror virDomainModificationImpact in the enum and add new flags separately.
@@ -13134,6 +13158,20 @@ virDomainSetGuestVcpus(virDomainPtr domain, * * Note that OSes and hypervisors may require vCPU 0 to stay online. * + * If @flags includes VIR_DOMAIN_VCPU_ASYNC, only live vCPU disable + * (hot-unplug) is requested asynchronously. In this mode, success means the + * unplug request was successfully fired; final completion is reported by + * VIR_DOMAIN_EVENT_ID_VCPU_REMOVED, while rejection is reported by + * VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED.
This will need to be reworded once the above is done and added when adding the impl.
+ * + * When this API disables live vCPUs by hot-unplugging them, the operation + * completion may be asynchronous. Successful unplug completion is reported by + * VIR_DOMAIN_EVENT_ID_VCPU_REMOVED, carrying the XML ``<vcpu id='...'>`` + * value. Rejected unplug requests continue to be reported by + * VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED. The success event may be + * delivered before this API call returns. In non-async mode, a timeout is + * reported only through the returned error, not through a domain event.
This needs to be added when adding the event/
+ * * Returns 0 on success, -1 on error. * * Since: 3.1.0
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d227ac58cdb4..ad4dc11c970f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3620,7 +3620,14 @@ processDeviceDeletedEvent(virQEMUDriver *driver, }
if (STRPREFIX(devAlias, "vcpu")) { - qemuDomainRemoveVcpuAlias(vm, devAlias); + int vcpuid; + virObjectEvent *event; + if ((vcpuid = qemuDomainRemoveVcpuAlias(vm, devAlias)) == -1) + goto endjob; + + event = virDomainEventVcpuRemovedNewFromObj(vm, vcpuid); + virObjectEventStateQueue(driver->domainEventState, event); +
Extra line.
} else { if (virDomainDefFindDevice(vm->def, devAlias, &dev, true) < 0) goto endjob; @@ -4269,6 +4276,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, virDomainObj *vm = NULL; virDomainDef *def; virDomainDef *persistentDef; + bool async = !!(flags & VIR_DOMAIN_VCPU_ASYNC); bool hotpluggable = !!(flags & VIR_DOMAIN_VCPU_HOTPLUGGABLE); bool useAgent = !!(flags & VIR_DOMAIN_VCPU_GUEST); int ret = -1; @@ -4277,7 +4285,8 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_VCPU_GUEST | - VIR_DOMAIN_VCPU_HOTPLUGGABLE, -1); + VIR_DOMAIN_VCPU_HOTPLUGGABLE | + VIR_DOMAIN_VCPU_ASYNC, -1);
if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup; @@ -4297,13 +4306,19 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) goto endjob;
+ if (async && (useAgent || persistentDef || !def)) {
Do not reject this on persistentDef update. It will break the compound operation and require users do 2 calls needlessly. It does need to be rejected with useAgend, but the rest doesn't make sense, even on a persistent-only update IMO we can accept the flag, since the hypervisor will not need to be informed and thus there's nothiong to do .
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("asynchronous mode is supported only for live vcpu unplug")); + goto endjob; + } + if (useAgent) ret = qemuDomainSetVcpusAgent(vm, nvcpus); else if (flags & VIR_DOMAIN_VCPU_MAXIMUM) ret = qemuDomainSetVcpusMax(driver, vm, def, persistentDef, nvcpus); else ret = qemuDomainSetVcpusInternal(driver, vm, def, persistentDef, - nvcpus, hotpluggable); + nvcpus, hotpluggable, async);
endjob: if (useAgent) @@ -19169,12 +19184,14 @@ qemuDomainSetVcpu(virDomainPtr dom, virDomainObj *vm = NULL; virDomainDef *def = NULL; virDomainDef *persistentDef = NULL; + bool async = !!(flags & VIR_DOMAIN_VCPU_ASYNC);
As noted you'll have to use a separate flag here.
g_autoptr(virBitmap) map = NULL; ssize_t lastvcpu; int ret = -1;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | - VIR_DOMAIN_AFFECT_CONFIG, -1); + VIR_DOMAIN_AFFECT_CONFIG | + VIR_DOMAIN_VCPU_ASYNC, -1);
if (state != 0 && state != 1) { virReportInvalidArg(state, "%s", _("unsupported state value")); @@ -19220,7 +19237,13 @@ qemuDomainSetVcpu(virDomainPtr dom, } }
- ret = qemuDomainSetVcpuInternal(driver, vm, def, persistentDef, map, !!state); + if (async && (persistentDef || !def)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("asynchronous mode is supported only for live vcpu unplug")); + goto endjob; + }
Here this doesn't make sense at all per the above explanation.
+ + ret = qemuDomainSetVcpuInternal(driver, vm, def, persistentDef, map, !!state, async);
endjob: virDomainObjEndJob(vm); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b7a282b96e52..0b3a781cea19 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5723,6 +5723,12 @@ qemuDomainRemoveDevice(virQEMUDriver *driver, return 0; }
+static bool +qemuDomainDeviceRemoved(virDomainObj *vm) +{ + qemuDomainObjPrivate *priv = vm->privateData; + return priv->unplug.status == QEMU_DOMAIN_UNPLUGGING_DEVICE_STATUS_OK; +}
static void qemuDomainMarkDeviceAliasForRemoval(virDomainObj *vm, @@ -6761,7 +6767,7 @@ qemuDomainRemoveVcpu(virDomainObj *vm, }
-void +int qemuDomainRemoveVcpuAlias(virDomainObj *vm, const char *alias) { @@ -6774,13 +6780,16 @@ qemuDomainRemoveVcpuAlias(virDomainObj *vm, vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu);
if (STREQ_NULLABLE(alias, vcpupriv->alias)) { - qemuDomainRemoveVcpu(vm, i); - return; + if (qemuDomainRemoveVcpu(vm, i) < 0) + return -1; + + return i; } }
VIR_DEBUG("vcpu '%s' not found in vcpulist of domain '%s'", alias, vm->def->name); + return -1; }
@@ -6788,7 +6797,8 @@ static int qemuDomainHotplugDelVcpu(virQEMUDriver *driver, virQEMUDriverConfig *cfg, virDomainObj *vm, - unsigned int vcpu) + unsigned int vcpu, + bool async) { virDomainVcpuDef *vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu); qemuDomainVcpuPrivate *vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo); @@ -6796,6 +6806,7 @@ qemuDomainHotplugDelVcpu(virQEMUDriver *driver, unsigned int nvcpus = vcpupriv->vcpus; int rc; int ret = -1; + virObjectEvent *event = NULL;
if (!vcpupriv->alias) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, @@ -6813,6 +6824,22 @@ qemuDomainHotplugDelVcpu(virQEMUDriver *driver, goto cleanup; } } else { + if (async) { + /* rc = 0 is implied in this branch */ + if (qemuDomainDeviceRemoved(vm)) { + /* event has already arrived, handle it now */ + goto success; + } + /* + * event has not arrived yet, but the monitor operation was + * successful; there will not be a waiter anymore when this thread + * exits. Reset removal state now so that the event handling path + * can be properly triggered if and when the event does arrive + */ + ret = 0; + goto cleanup;
Why didn't you do this as with async device removal? Specifically skip the call to 'qemuDomainMarkDeviceAliasForRemoval' in async mode and just return 0 in async mode here. IMO there's no need for qemuDomainDeviceRemoved or the 'success label, just skip qemuDomainMarkDeviceAliasForRemoval and qemuDomainWaitForDeviceRemoval and assume success without removing the device from the definition. That way the removal will be always handled asynchronously without the need for any different/questionable code/approach.
+ } + if ((rc = qemuDomainWaitForDeviceRemoval(vm)) <= 0) { if (rc == 0) virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s", @@ -6821,6 +6848,7 @@ qemuDomainHotplugDelVcpu(virQEMUDriver *driver, } }
+ success: if (qemuDomainRemoveVcpu(vm, vcpu) < 0) goto cleanup;
@@ -6831,6 +6859,10 @@ qemuDomainHotplugDelVcpu(virQEMUDriver *driver,
ret = 0;
+ /* emit event now to close the async caller loop */ + event = virDomainEventVcpuRemovedNewFromObj(vm, vcpu); + virObjectEventStateQueue(driver->domainEventState, event);
This ought to happen from qemuDomainRemoveVcpu rather than scattering it randomly through the multiple paths to that function.
+ cleanup: qemuDomainResetDeviceRemoval(vm); return ret;
[...]
@@ -7114,12 +7147,20 @@ qemuDomainSetVcpusInternal(virQEMUDriver *driver, virDomainDef *def, virDomainDef *persistentDef, unsigned int nvcpus, - bool hotpluggable) + bool hotpluggable, + bool async) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(virBitmap) vcpumap = NULL; bool enable;
+ if (async && persistentDef) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("asynchronous mode is supported only for live vcpu unplug")); + + return -1; + }
Remove this per previous explanation. (It has also broken alignment).
+ if (def && nvcpus > virDomainDefGetVcpusMax(def)) { virReportError(VIR_ERR_INVALID_ARG, _("requested vcpus is greater than max allowable vcpus for the live domain: %1$u > %2$u"), @@ -7139,7 +7180,13 @@ qemuDomainSetVcpusInternal(virQEMUDriver *driver, &enable))) return -1;
- if (qemuDomainSetVcpusLive(driver, cfg, vm, vcpumap, enable) < 0) + if (async && enable) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("asynchronous mode is supported only for vcpu unplug")); + return -1; + }
This breaks the semantics of the API where it sets a specific cpu amount. the caller doesn't say if removing or adding cpus. IMO this should accept async mode also when enabling cpus and it will just do nothing because no cpus will be removed,
+ + if (qemuDomainSetVcpusLive(driver, cfg, vm, vcpumap, enable, async) < 0) return -1; }
@@ -7289,7 +7336,8 @@ qemuDomainSetVcpuInternal(virQEMUDriver *driver, virDomainDef *def, virDomainDef *persistentDef, virBitmap *map, - bool state) + bool state, + bool async) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(virBitmap) livevcpus = NULL; @@ -7320,8 +7368,14 @@ qemuDomainSetVcpuInternal(virQEMUDriver *driver, return -1; }
+ if (async && state) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("asynchronous mode is supported only for vcpu unplug")); + return -1; + }
Here, since the caller explicitly sends the state this could make sense but IMO is not needed same as in the previous case. Just document that async mode is for unplug only. Consider also renaming the variable to async_unplug everywhere.
+ if (livevcpus && - qemuDomainSetVcpusLive(driver, cfg, vm, livevcpus, state) < 0) + qemuDomainSetVcpusLive(driver, cfg, vm, livevcpus, state, async) < 0) return -1;
if (persistentDef) {
[...]
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index ea9d3243f8b1..36bc4d913826 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -322,6 +322,7 @@ struct testQemuHotplugCpuParams { GHashTable *capsLatestFiles; GHashTable *capsCache; GHashTable *schemaCache; + bool async;
What is the point of this ...
};
@@ -420,7 +421,7 @@ testQemuHotplugCpuGroup(const void *opaque)
rc = qemuDomainSetVcpusInternal(&driver, data->vm, data->vm->def, data->vm->newDef, params->newcpus, - true); + true, params->async);
if (params->fail) { if (rc == 0) @@ -458,7 +459,8 @@ testQemuHotplugCpuIndividual(const void *opaque) goto cleanup;
rc = qemuDomainSetVcpuInternal(&driver, data->vm, data->vm->def, - data->vm->newDef, map, params->state); + data->vm->newDef, map, params->state, + params->async);
... if the test never changes to async mode?
if (params->fail) { if (rc == 0)
[...]
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 08a1ce395378..350a3b6cd8b2 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7667,6 +7667,10 @@ static const vshCmdOptDef opts_setvcpus[] = { .type = VSH_OT_BOOL, .help = N_("make added vcpus hot(un)pluggable") }, + {.name = "async", + .type = VSH_OT_BOOL, + .help = N_("return after firing live vcpu unplug request(s)") + },
The new flag needs to be documented in the virsh man page. See docs/manpages/virsh.rst
{.name = NULL} };
@@ -7681,6 +7685,7 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) bool current = vshCommandOptBool(cmd, "current"); bool guest = vshCommandOptBool(cmd, "guest"); bool hotpluggable = vshCommandOptBool(cmd, "hotpluggable"); + bool async = vshCommandOptBool(cmd, "async"); unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
VSH_EXCLUSIVE_OPTIONS_VAR(current, live); @@ -7699,6 +7704,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_VCPU_MAXIMUM; if (hotpluggable) flags |= VIR_DOMAIN_VCPU_HOTPLUGGABLE; + if (async) + flags |= VIR_DOMAIN_VCPU_ASYNC;
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -7711,6 +7718,12 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) return false; }
+ if (async && (config || maximum || guest || hotpluggable)) { + vshError(ctl, "%s", + _("--async can be used only for live hypervisor vcpu unplug"));
'hypervisor vpcu unplug' doesn't make sense. You are unplugging the cpu from the guest. In addition this is setting cpus potentially both ways so shows that the checks rejecting To fix the above
+ return false; + } + /* none of the options were specified */ if (!current && flags == 0) { if (virDomainSetVcpus(dom, count) != 0) @@ -7720,6 +7733,10 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) return false; }
+ if (async) + vshPrintExtra(ctl, "%s", + _("vCPU unplug requests sent successfully\n")); +
No need for the extra blurb.
return true; }
[...]
@@ -7829,6 +7846,10 @@ static const vshCmdOptDef opts_setvcpu[] = { .type = VSH_OT_BOOL, .help = N_("disable cpus specified by cpumap") }, + {.name = "async", + .type = VSH_OT_BOOL, + .help = N_("return after firing live vcpu unplug request") + },
The new flag needs to be documented in the virsh man page. See docs/manpages/virsh.rst
VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, VIRSH_COMMON_OPT_DOMAIN_CURRENT, @@ -7843,6 +7864,7 @@ cmdSetvcpu(vshControl *ctl, const vshCmd *cmd) bool disable = vshCommandOptBool(cmd, "disable"); bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); + bool async = vshCommandOptBool(cmd, "async"); const char *vcpulist = NULL; int state = 0; unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; @@ -7856,12 +7878,20 @@ cmdSetvcpu(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_AFFECT_CONFIG; if (live) flags |= VIR_DOMAIN_AFFECT_LIVE; + if (async) + flags |= VIR_DOMAIN_VCPU_ASYNC;
if (!(enable || disable)) { vshError(ctl, "%s", _("one of --enable, --disable is required")); return false; }
+ if (async && (config || enable)) { + vshError(ctl, "%s", + _("--async can be used only for live vcpu disable")); + return false; + }
Use VSH_EXCLUSIVE_OPTIONS("async", "enable"); as noted elsewhere do not reject aync with --config.
+ if (vshCommandOptString(ctl, cmd, "vcpulist", &vcpulist)) return false;
@@ -7874,6 +7904,10 @@ cmdSetvcpu(vshControl *ctl, const vshCmd *cmd) if (virDomainSetVcpu(dom, vcpulist, state, flags) < 0) return false;
+ if (async) + vshPrintExtra(ctl, "%s", + _("vCPU unplug request sent successfully\n"));
IMO there's no need for the extra message.
+ return true; }
-- 2.47.3
I think I have most of the other review comments incorporated (other that the patch separation comment itself), but had a few questions for the ones below- On 2026-04-17 3:06 PM, Peter Krempa wrote: [snip]
+ * * Returns 0 in case of success, -1 in case of failure. * * Since: 0.8.5 @@ -13125,7 +13148,8 @@ virDomainSetGuestVcpus(virDomainPtr domain, * @domain: pointer to domain object * @vcpumap: text representation of a bitmap of vcpus to set * @state: 0 to disable/1 to enable cpus described by @vcpumap - * @flags: bitwise-OR of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact with optional + * VIR_DOMAIN_VCPU_ASYNC
You can't do this. You'll need to add a new enum of flags specifically for this API. Mirror virDomainModificationImpact in the enum and add new flags separately.
Doesn't it make more sense to add this flag to `enum virDomainVcpuFlags`, since that enum already encompasses all the other flags that are used in the two functions (qemuDomainSetVcpusFlags and qemuDomainSetVcpu)? If I mirror `virDomainModificationImpact` and create a new enum then there's a false positive in qemuDomainSetVcpusFlags, since the enum values of `VIR_DOMAIN_VCPU_GUEST` will collide with this new flag (added immediately after the last mirrored value of virDomainModificationImpact)? Both would get the enum value of (1 << 3, i.e. 8). I assume this is the exact situation why you suggested not to mix the enums in the first place. Since qemuDomainSetVcpuFlags is interested in the following flags- ``` virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_VCPU_GUEST | VIR_DOMAIN_VCPU_HOTPLUGGABLE | VIR_DOMAIN_SETVCPU_ASYNC_UNPLUG, -1); ``` (Note: I've renamed the new flag in the code snippet above) Which technically works, but it can mask/fake a presence of the actual async unplug flag if checked later on. Adding it to enum virDomainVcpuFlags ensures this is handled correctly, since virDomainModificationImpact is already mirrored in this enum. Plus doing it this way ensures that both the setvcpu* functions are covered by the same enum (as they should be, since they are so closely related). [snip]
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index ea9d3243f8b1..36bc4d913826 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -322,6 +322,7 @@ struct testQemuHotplugCpuParams { GHashTable *capsLatestFiles; GHashTable *capsCache; GHashTable *schemaCache; + bool async;
What is the point of this ...
};
@@ -420,7 +421,7 @@ testQemuHotplugCpuGroup(const void *opaque)
rc = qemuDomainSetVcpusInternal(&driver, data->vm, data->vm->def, data->vm->newDef, params->newcpus, - true); + true, params->async);
if (params->fail) { if (rc == 0) @@ -458,7 +459,8 @@ testQemuHotplugCpuIndividual(const void *opaque) goto cleanup;
rc = qemuDomainSetVcpuInternal(&driver, data->vm, data->vm->def, - data->vm->newDef, map, params->state); + data->vm->newDef, map, params->state, + params->async);
... if the test never changes to async mode?
That was intended to align with the api changes so that the commit with the test cases would fit in cleanly with it. I'll remove this for now, hardcoding it to `false`.
@@ -7720,6 +7733,10 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) return false; }
+ if (async) + vshPrintExtra(ctl, "%s", + _("vCPU unplug requests sent successfully\n")); +
No need for the extra blurb.
I'd only added it because other than checking the virsh exit code, there was no other output that would be printed on the terminal about what happened - it just looked like it exited without doing anything. So some sort of a marker to ensure things went as planned felt appropriate here, hence the addition. Same with the change below for the `setvcpu` subcommand. [snip]
On Tue, Apr 21, 2026 at 15:06:03 +0530, Akash Kulhalli wrote:
I think I have most of the other review comments incorporated (other that the patch separation comment itself), but had a few questions for the ones below-
On 2026-04-17 3:06 PM, Peter Krempa wrote: [snip]
+ * * Returns 0 in case of success, -1 in case of failure. * * Since: 0.8.5 @@ -13125,7 +13148,8 @@ virDomainSetGuestVcpus(virDomainPtr domain, * @domain: pointer to domain object * @vcpumap: text representation of a bitmap of vcpus to set * @state: 0 to disable/1 to enable cpus described by @vcpumap - * @flags: bitwise-OR of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact with optional + * VIR_DOMAIN_VCPU_ASYNC
You can't do this. You'll need to add a new enum of flags specifically for this API. Mirror virDomainModificationImpact in the enum and add new flags separately.
Doesn't it make more sense to add this flag to `enum virDomainVcpuFlags`, since that enum already encompasses all the other flags that are used in the two functions (qemuDomainSetVcpusFlags and qemuDomainSetVcpu)? If I mirror
No, because since 'virDomainSetVcpu' accepts only a subset of the flags you can't then state in the documentation that 'flags' is a bitwise-or of 'virDomainVcpuFlag'. You'd then have to express in text that some flags are not in fact supported. In fact many of the flags such as VIR_DOMAIN_VCPU_MAXIMUM make no sense with 'virDomainSetVcpu' so it makes no sense to even have them there.
`virDomainModificationImpact` and create a new enum then there's a false positive in qemuDomainSetVcpusFlags, since the enum values of `VIR_DOMAIN_VCPU_GUEST` will collide with this new flag (added immediately
That isn't a problem, because either of those functions will extract it before passing into the internals.
after the last mirrored value of virDomainModificationImpact)? Both would get the enum value of (1 << 3, i.e. 8). I assume this is the exact situation why you suggested not to mix the enums in the first place.
It doesn't matter what actual numeric value the flags have.
Since qemuDomainSetVcpuFlags is interested in the following flags- ``` virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_VCPU_GUEST | VIR_DOMAIN_VCPU_HOTPLUGGABLE | VIR_DOMAIN_SETVCPU_ASYNC_UNPLUG, -1); ``` (Note: I've renamed the new flag in the code snippet above)
Which technically works, but it can mask/fake a presence of the actual async unplug flag if checked later on. Adding it to enum virDomainVcpuFlags ensures this is handled correctly, since virDomainModificationImpact is already mirrored in this enum.
This seems to hinge on the fact that they'd have the same name which is what I'm saying should not be the case.
Plus doing it this way ensures that both the setvcpu* functions are covered by the same enum (as they should be, since they are so closely related).
That's a non-goal. As noted above they are *not* in fact related so closely because of some flags which make no sense in virDomainSetVcpu.
[snip]
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index ea9d3243f8b1..36bc4d913826 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -322,6 +322,7 @@ struct testQemuHotplugCpuParams { GHashTable *capsLatestFiles; GHashTable *capsCache; GHashTable *schemaCache; + bool async;
What is the point of this ...
}; @@ -420,7 +421,7 @@ testQemuHotplugCpuGroup(const void *opaque) rc = qemuDomainSetVcpusInternal(&driver, data->vm, data->vm->def, data->vm->newDef, params->newcpus, - true); + true, params->async); if (params->fail) { if (rc == 0) @@ -458,7 +459,8 @@ testQemuHotplugCpuIndividual(const void *opaque) goto cleanup; rc = qemuDomainSetVcpuInternal(&driver, data->vm, data->vm->def, - data->vm->newDef, map, params->state); + data->vm->newDef, map, params->state, + params->async);
... if the test never changes to async mode?
That was intended to align with the api changes so that the commit with the test cases would fit in cleanly with it. I'll remove this for now, hardcoding it to `false`.
yup
@@ -7720,6 +7733,10 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) return false; } + if (async) + vshPrintExtra(ctl, "%s", + _("vCPU unplug requests sent successfully\n")); +
No need for the extra blurb.
I'd only added it because other than checking the virsh exit code, there was no other output that would be printed on the terminal about what happened - it just looked like it exited without doing anything. So some sort of a marker to ensure things went as planned felt appropriate here, hence the addition. Same with the change below for the `setvcpu` subcommand.
Well since the command doesn't print anything when doing normal unplug I don't think we should be adding it for the async case. If you want to print anything I suggest you follow up with patches which add print on any successful exit. IMO that is not needed.
On 2026-04-21 5:26 PM, Peter Krempa wrote: [snip]
@@ -13125,7 +13148,8 @@ virDomainSetGuestVcpus(virDomainPtr domain, * @domain: pointer to domain object * @vcpumap: text representation of a bitmap of vcpus to set * @state: 0 to disable/1 to enable cpus described by @vcpumap - * @flags: bitwise-OR of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact with optional + * VIR_DOMAIN_VCPU_ASYNC
You can't do this. You'll need to add a new enum of flags specifically for this API. Mirror virDomainModificationImpact in the enum and add new flags separately.
Doesn't it make more sense to add this flag to `enum virDomainVcpuFlags`, since that enum already encompasses all the other flags that are used in the two functions (qemuDomainSetVcpusFlags and qemuDomainSetVcpu)? If I mirror
No, because since 'virDomainSetVcpu' accepts only a subset of the flags you can't then state in the documentation that 'flags' is a bitwise-or of 'virDomainVcpuFlag'. You'd then have to express in text that some flags are not in fact supported.
In fact many of the flags such as VIR_DOMAIN_VCPU_MAXIMUM make no sense with 'virDomainSetVcpu' so it makes no sense to even have them there.
`virDomainModificationImpact` and create a new enum then there's a false positive in qemuDomainSetVcpusFlags, since the enum values of `VIR_DOMAIN_VCPU_GUEST` will collide with this new flag (added immediately
That isn't a problem, because either of those functions will extract it before passing into the internals.
Understood. My earlier confusion stemmed from the fact that the flags being passed to either of the setvcpu/setvcpuflags functions were still using the original VIR_DOMAIN_AFFECT_* enums, even though they had equivalents defined in each new enum. That led me to make a false association with the original `virDomainModificationImpact` enum continuity. I'd like to fix this in a separate patch that uses the correctly-named aliases from the appropriate enum defined for the functions. Since this is a cosmetic-only change and has a potential to conflict with some hunks from v2 of this changeset, I will send that patch out separately once this changeset comes to a head. Will be sending out a v2 shortly.
participants (2)
-
Akash Kulhalli -
Peter Krempa