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]
