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.