4:59 a.m.
On Tue, Jun 04, 2024 at 11:45:11AM +0200, Philippe Mathieu-Daudé wrote:
Hi Daniel, Dave, Markus & Thomas.
On 4/6/24 06:58, Markus Armbruster wrote:
> "Dr. David Alan Gilbert" <dave(a)treblig.org> writes:
> > * Daniel P. Berrangé (berrange(a)redhat.com) wrote:
> > > On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote:
> > > > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote:
> > > > > We are trying to unify all qemu-system-FOO to a single binary.
> > > > > In order to do that we need to remove QAPI target specific
code.
> > > > >
> > > > > @dump-skeys is only available on qemu-system-s390x. This series
> > > > > rename it as @dump-s390-skey, making it available on other
> > > > > binaries. We take care of backward compatibility via
deprecation.
> > > > >
> > > > > Philippe Mathieu-Daudé (4):
> > > > > hw/s390x: Introduce the @dump-s390-skeys QMP command
> > > > > hw/s390x: Introduce the 'dump_s390_skeys' HMP
command
> > > > > hw/s390x: Deprecate the HMP 'dump_skeys' command
> > > > > hw/s390x: Deprecate the QMP @dump-skeys command
> > > >
> > > > Why do we have to rename the command? Just for the sake of it? I
think
> > > > renaming HMP commands is maybe ok, but breaking the API in QMP is
something
> > > > you should consider twice.
I'm looking at how to include this command in the new "single binary".
Markus explained in an earlier series, just expanding this command as
stub to targets that don't implement it is not backward compatible and
breaks QMP introspection. Currently on s390x we get a result, on other
targets the command doesn't exist. If we add a stubs, then other targets
return something (even if it is an empty list), confusing management
interface.
I'm not convinced about calling this "not backward compatible".
We're always free to add new commands, and there's never any
guarantee that you can execute a given command under every
possible QEMU configuration.
IOW, adding 'dump-skeys' and having it always return an error
on non-s390x targets is valid IMHO, and I wouldn't call that
a backwards compatibilit problem.
Apps shouldn't assume that just because a command is reported
in introspection, it can be used in any scenario. An app is
expected to itself understand the behaviour of any given command
sufficiently well, to know when they can execute it, or be prpared
to accept errors.
> PRO rename: the command's tie to S390 is them immediately
obvious, which
> may be useful when the command becomes available in qemu-systems capable
> of running other targets.
>
> CON rename: users need to adapt.
>
> What are the users? Not libvirt, as far as I can tell.
Years ago we said, "all HMP must be based on QMP". Now we realize HMP
became stable because QMP-exposed, although not consumed externally...
That's not the case though. We've added plenty of conversions of
HMP to QMP, while declaring them unstable. We might have missed
that in some cases, but that's just a bug, and we can fix that
any time by adding the 'unstable' feature tag to the schema.
Does the concept of "internal QMP commands" makes sense for
HMP debug
ones? (Looking at a way to not expose them). We could use the "x-"
prefix to not care about stable / backward compat, but what is the point
of exposing to QMP commands that will never be accessed there?
Debug only, unstable commands are totally valid for QMP. There's nothing
that requires them to be inherantly HMP only.
QAPI modelling can benefit many debug only, unstable commands, but we
can also still just return printf formatted strings as documented here:
https://www.qemu.org/docs/master/devel/writing-monitor-commands.html#writ...
I remain convinced that we should eventually delete the HMP impl as it
exists today, and just provide a QMP client whose interactive interface
matches what HMP provides today. There's no compelling need for HMP to
run inside QEMU, if QMP exposes all the required functionality.
> docs/devel/qapi-code-gen.rst:
>
> Names beginning with ``x-`` used to signify "experimental". This
> convention has been replaced by special feature "unstable".
>
> Feature "unstable" is what makes something unstable, and is what
> machines should check.
What I mentioned earlier could be 'Feature "internal" or
"debug"'.
IMHO we don't need to invent new terms for this.
"unstable" is sufficient as it expresses that the command's
inputs and outputs are liable to change their format, which
is the case for most of the historical debug only commands.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|