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]