On 4/28/25 1:37 AM, Daniel P. Berrangé wrote:
On Fri, Apr 25, 2025 at 01:39:49PM -0700, Pierrick Bouvier wrote:
On 4/25/25 00:35, Daniel P. Berrangé wrote:
On Thu, Apr 24, 2025 at 11:33:47AM -0700, Pierrick Bouvier wrote:
Feedback
========
The goal of this series is to be spark a conversation around following topics:
- Would you be open to such an approach? (expose all code, and restrict commands
registered at runtime only for specific targets)
QMP defines a public API between QEMU and external mgmt apps, and personally I
like the idea that the API exposed is identical across all binaries and thus
the API becomes independent of the impl choice of combined vs separate
binaries,.
- Are there unexpected consequences for libvirt or other consumers to expose
more definitions than what we have now?
QEMU used the selective hiding of commands in the QMP schema as a mechanism
to allow mgmt apps to probe for supported features. We need to check usage
of each QMP API feature that's behind a TARGET_* condition and identify
which libvirt uses as a feature probe, then come up with a strategy for how
best to handle each case in libvirt in future. We might need some additional
runtime mechanism to probe for certain things, but we won't know until we
look at things in more detail.
Could we consider to hide the concerned commands from introspection related
commands as well? The same way we prevent those commands to be registered,
we can probably prevent them from being visible for libvirt.
The code would still be there, and compiled once, but based on runtime
target_X() value, it would not appear in introspected schema.
I'm not sure how all this is implemented from QAPI code generator, maybe
it's hard to do something like this, if we build the schema at compile time
for instance.
- Would you recommend another approach instead? I experimented with having per
target generated files, but we still need to expose quite a lot in headers,
so
my opinion is that it's much more complicated for zero benefit. As well, the
code size impact is more than negligible, so the simpler, the better.
IMHO it is unfortunate that the API we currently expose has a dependency on
a specific impl choice that mgmt apps are expected to rely on for feature
probing. An ideal API design is not so closely coupled to impl choice
(separate vs combined binaries), and would expose enough functionality
such that mgmt apps continue to work regardless of the impl choices.
At this point, do we know which kind of "feature" gets probed? Is it only
the list of commands available, or is there probes based on enum/struct
definition?
In general if it is visible from QMP it is liable to get probed - any
and every aspect of it is in scope.
To figure this out you need to produce a list of each command/struct/field
that has a 'if $TARGET' conditional, and check whether libvirt uses it or
not.
At this point, it seems more natural to expose exactly the same
information, instead of trying to see "potential" impact of exposing
more. I just feel like we'll open a pandora box, and it will take a long
time to converge anywhere.
The initial motivation is just to remove per target code in QAPI
generated code, and not to trigger a whole refactoring in several projects.
We thought the conditionals were a good thing when we first designed QMP
this way. We ended up using two distinct classes of conditionals, one
reflecting build time features and one reflecting which target binary is
used. I don't think we fully contemplated the implications that the latter
set of conditionals would have on our ability to change our impl approach
in future. I think the proposal here is taking us in a good direction
given what we now know.
Thanks for considering an alternative way given the new needs, that's
appreciated.
Would that possible to get some help from people from libvirt or QAPI
developers for this?
There challenge here is how QEMU will change this without back compat
problems.
Our deprecation process won't work well here. There's no nice way to flag
up that we're about to change the way conditionals work.
If we expose exactly the same schema, there is no need to change
anything. The only impact is on modifying the .json, to add new *_if
clauses, targetting runtime, instead of compile time.
When posting my series, I was not aware of all the introspection aspect,
and simply was focused on qemu monitor commands which are registered, so
thanks for bringing this.
After looking at schema introspection code, I think it will not be hard
to expose parts based on a runtime check.
Most of the time libvirt adapts by changing the way we "probe the data",
but in this case its about adapting to the way we "probe data about the
data".
This kind of problem is why I liked the previous idea that Phillippe
was trying of introducing a "qemu-system-any" - it didn't affect the
behaviour of existing qemu-system-$TARGET commands, so apps had a
clean line in the sand between old & new behaviour.
It's something we might talk about later for sure.
On this thread, if possible, I would like to avoid starting talking
about the new binary, new command line, new monitor schema, as they are
a different topic, and I'm sure people will have a lot of different
opinions, which won't solve the current issue of compiling QAPI
generated code only once.
With regards,
Daniel
