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.
