[RFC QEMU PATCH] ui: Make the DisplayType enum entries conditional

[RFC QEMU PATCH] ui: Make the DisplayType enum entries conditional

[Thomas Huth]

Libvirt's "domcapabilities" command has a way to state whether
certain graphic frontends are available in QEMU or not. Originally,
libvirt looked at the "--help" output of the QEMU binary to determine
whether SDL was available or not (by looking for the "-sdl" parameter
in the help text), but since libvirt stopped doing this analysis of
the help text, the detection of SDL is currently broken, see:

 https://bugzilla.redhat.com/show_bug.cgi?id=1790902

QEMU should provide a way via the QMP interface instead. The simplest
way, without introducing additional commands, is to make the DisplayType
enum entries conditional, so that the enum only contains the entries if
the corresponding CONFIG_xxx switches have been set. Unfortunately, this
only works for sdl, cocoa and spice, since gtk, egl-headless and curses
are hard-wired in the "data" section of the DisplayOptions, and thus
unfortunately always have to be defined.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 qapi/ui.json | 12 +++++++++---
 ui/console.c |  4 ++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 1052ca9c38..c4f44cfe50 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1126,9 +1126,15 @@
 #
 ##
 { 'enum'    : 'DisplayType',
-  'data'    : [ 'default', 'none', 'gtk', 'sdl',
-                'egl-headless', 'curses', 'cocoa',
-                'spice-app'] }
+  'data'    : [
+    { 'name': 'default' },
+    { 'name': 'none' },
+    { 'name': 'gtk' },
+    { 'name': 'sdl', 'if': 'defined(CONFIG_SDL)' },
+    { 'name': 'egl-headless' },
+    { 'name': 'curses' },
+    { 'name': 'cocoa', 'if': 'defined(CONFIG_COCOA)' },
+    { 'name': 'spice-app', 'if': 'defined(CONFIG_SPICE)'} ] }
 
 ##
 # @DisplayOptions:
diff --git a/ui/console.c b/ui/console.c
index 2de5f4105b..954f7162c3 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2371,8 +2371,12 @@ bool qemu_display_find_default(DisplayOptions *opts)
 {
     static DisplayType prio[] = {
         DISPLAY_TYPE_GTK,
+#if defined(CONFIG_SDL)
         DISPLAY_TYPE_SDL,
+#endif
+#if defined(CONFIG_COCOA)
         DISPLAY_TYPE_COCOA
+#endif
     };
     int i;
 
-- 
2.27.0

Re: [RFC QEMU PATCH] ui: Make the DisplayType enum entries conditional

[Gerd Hoffmann]

On Wed, Jun 09, 2021 at 12:02:40PM +0200, Thomas Huth wrote:
> Libvirt's "domcapabilities" command has a way to state whether
> certain graphic frontends are available in QEMU or not. Originally,
> libvirt looked at the "--help" output of the QEMU binary to determine
> whether SDL was available or not (by looking for the "-sdl" parameter
> in the help text), but since libvirt stopped doing this analysis of
> the help text, the detection of SDL is currently broken, see:
> 
>  https://bugzilla.redhat.com/show_bug.cgi?id=1790902
> 
> QEMU should provide a way via the QMP interface instead. The simplest
> way, without introducing additional commands, is to make the DisplayType
> enum entries conditional, so that the enum only contains the entries if
> the corresponding CONFIG_xxx switches have been set.

Hmm, that'll break for the "dnf remove qemu-ui-sdl" case ...

take care,
  Gerd

Re: [RFC QEMU PATCH] ui: Make the DisplayType enum entries conditional

[Daniel P. Berrangé]

On Wed, Jun 09, 2021 at 01:24:05PM +0200, Gerd Hoffmann wrote:
> On Wed, Jun 09, 2021 at 12:02:40PM +0200, Thomas Huth wrote:
> > Libvirt's "domcapabilities" command has a way to state whether
> > certain graphic frontends are available in QEMU or not. Originally,
> > libvirt looked at the "--help" output of the QEMU binary to determine
> > whether SDL was available or not (by looking for the "-sdl" parameter
> > in the help text), but since libvirt stopped doing this analysis of
> > the help text, the detection of SDL is currently broken, see:
> > 
> >  https://bugzilla.redhat.com/show_bug.cgi?id=1790902
> > 
> > QEMU should provide a way via the QMP interface instead. The simplest
> > way, without introducing additional commands, is to make the DisplayType
> > enum entries conditional, so that the enum only contains the entries if
> > the corresponding CONFIG_xxx switches have been set.
> 
> Hmm, that'll break for the "dnf remove qemu-ui-sdl" case ...

Note tht libvirt invalidates its cache of QEMU capabilities when it
sees the /usr/lib64/qemu directory timestamp change. So it ought to
pick up changes caused by installing/removing QEMU modules, and apply
this to future queries for domcapabilities, or when starting future
QEMU guests.

Note, however, that capabilities are preserved at the time each
the QEMU process is started.

IOW, if you boot QEMU, then remove a QEMU module, then attempt to
use a feature that implies loading of the removed QEMU module,
libvirt won't know the module has been removed for the running
QEMU.


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 :|

Re: [RFC QEMU PATCH] ui: Make the DisplayType enum entries conditional

[Gerd Hoffmann]

On Wed, Jun 09, 2021 at 12:29:58PM +0100, Daniel P. Berrangé wrote:
> On Wed, Jun 09, 2021 at 01:24:05PM +0200, Gerd Hoffmann wrote:
> > On Wed, Jun 09, 2021 at 12:02:40PM +0200, Thomas Huth wrote:
> > > Libvirt's "domcapabilities" command has a way to state whether
> > > certain graphic frontends are available in QEMU or not. Originally,
> > > libvirt looked at the "--help" output of the QEMU binary to determine
> > > whether SDL was available or not (by looking for the "-sdl" parameter
> > > in the help text), but since libvirt stopped doing this analysis of
> > > the help text, the detection of SDL is currently broken, see:
> > > 
> > >  https://bugzilla.redhat.com/show_bug.cgi?id=1790902
> > > 
> > > QEMU should provide a way via the QMP interface instead. The simplest
> > > way, without introducing additional commands, is to make the DisplayType
> > > enum entries conditional, so that the enum only contains the entries if
> > > the corresponding CONFIG_xxx switches have been set.
> > 
> > Hmm, that'll break for the "dnf remove qemu-ui-sdl" case ...
> 
> Note tht libvirt invalidates its cache of QEMU capabilities when it
> sees the /usr/lib64/qemu directory timestamp change. So it ought to
> pick up changes caused by installing/removing QEMU modules, and apply
> this to future queries for domcapabilities, or when starting future
> QEMU guests.

That'll work fine for modules implementing qom objects / devices,
because the list of available objects changes accordingly and libvirt
can see that.

The #if CONFIG_SDL approach will not work because qemu will continue to
report sdl as supported even when the sdl module is not installed any
more.

take care,
  Gerd

Re: [RFC QEMU PATCH] ui: Make the DisplayType enum entries conditional

[Daniel P. Berrangé]

On Wed, Jun 09, 2021 at 01:49:21PM +0200, Gerd Hoffmann wrote:
> On Wed, Jun 09, 2021 at 12:29:58PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Jun 09, 2021 at 01:24:05PM +0200, Gerd Hoffmann wrote:
> > > On Wed, Jun 09, 2021 at 12:02:40PM +0200, Thomas Huth wrote:
> > > > Libvirt's "domcapabilities" command has a way to state whether
> > > > certain graphic frontends are available in QEMU or not. Originally,
> > > > libvirt looked at the "--help" output of the QEMU binary to determine
> > > > whether SDL was available or not (by looking for the "-sdl" parameter
> > > > in the help text), but since libvirt stopped doing this analysis of
> > > > the help text, the detection of SDL is currently broken, see:
> > > > 
> > > >  https://bugzilla.redhat.com/show_bug.cgi?id=1790902
> > > > 
> > > > QEMU should provide a way via the QMP interface instead. The simplest
> > > > way, without introducing additional commands, is to make the DisplayType
> > > > enum entries conditional, so that the enum only contains the entries if
> > > > the corresponding CONFIG_xxx switches have been set.
> > > 
> > > Hmm, that'll break for the "dnf remove qemu-ui-sdl" case ...
> > 
> > Note tht libvirt invalidates its cache of QEMU capabilities when it
> > sees the /usr/lib64/qemu directory timestamp change. So it ought to
> > pick up changes caused by installing/removing QEMU modules, and apply
> > this to future queries for domcapabilities, or when starting future
> > QEMU guests.
> 
> That'll work fine for modules implementing qom objects / devices,
> because the list of available objects changes accordingly and libvirt
> can see that.
> 
> The #if CONFIG_SDL approach will not work because qemu will continue to
> report sdl as supported even when the sdl module is not installed any
> more.

Ah see what you mean now.  So libvirt can't merely query members of
the DisplayType enum. We need an actual 'query-display-types' command
that returns an array of DisplayType values corresponding to what is
actually built-in or available as a module at that instant.

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 :|

Re: [RFC QEMU PATCH] ui: Make the DisplayType enum entries conditional

[Thomas Huth]

On 09/06/2021 13.49, Gerd Hoffmann wrote:
> On Wed, Jun 09, 2021 at 12:29:58PM +0100, Daniel P. Berrangé wrote:
>> On Wed, Jun 09, 2021 at 01:24:05PM +0200, Gerd Hoffmann wrote:
>>> On Wed, Jun 09, 2021 at 12:02:40PM +0200, Thomas Huth wrote:
>>>> Libvirt's "domcapabilities" command has a way to state whether
>>>> certain graphic frontends are available in QEMU or not. Originally,
>>>> libvirt looked at the "--help" output of the QEMU binary to determine
>>>> whether SDL was available or not (by looking for the "-sdl" parameter
>>>> in the help text), but since libvirt stopped doing this analysis of
>>>> the help text, the detection of SDL is currently broken, see:
>>>>
>>>>   https://bugzilla.redhat.com/show_bug.cgi?id=1790902
>>>>
>>>> QEMU should provide a way via the QMP interface instead. The simplest
>>>> way, without introducing additional commands, is to make the DisplayType
>>>> enum entries conditional, so that the enum only contains the entries if
>>>> the corresponding CONFIG_xxx switches have been set.
>>>
>>> Hmm, that'll break for the "dnf remove qemu-ui-sdl" case ...
>>
>> Note tht libvirt invalidates its cache of QEMU capabilities when it
>> sees the /usr/lib64/qemu directory timestamp change. So it ought to
>> pick up changes caused by installing/removing QEMU modules, and apply
>> this to future queries for domcapabilities, or when starting future
>> QEMU guests.
> 
> That'll work fine for modules implementing qom objects / devices,
> because the list of available objects changes accordingly and libvirt
> can see that.
> 
> The #if CONFIG_SDL approach will not work because qemu will continue to
> report sdl as supported even when the sdl module is not installed any
> more.

I guess we'd need a separate QMP command to fix that, which tries to load 
the modules first when being called? Something similar to what is being done 
in qemu_display_help() ?
That's certainly doable, too, just a little bit more complex... do we want 
that? Or is the quick-n-easy way via the schema good enough for most use 
cases? (I'm not that familiar with "virsh domcapabilities" ... is there any 
real usage for the <graphics> section or is this rather cosmetical?)

  Thomas


PS: My CI runs with the patch just finished, and apparently I missed some 
#ifdefs in other parts of the code ... so I need to respin this patch 
anyway, no matter which direction we decide to go...

Re: [RFC QEMU PATCH] ui: Make the DisplayType enum entries conditional

[Gerd Hoffmann]

  Hi,

> > The #if CONFIG_SDL approach will not work because qemu will continue to
> > report sdl as supported even when the sdl module is not installed any
> > more.
> 
> I guess we'd need a separate QMP command to fix that, which tries to load
> the modules first when being called? Something similar to what is being done
> in qemu_display_help() ?

That would work, yes.

> That's certainly doable, too, just a little bit more complex...

Alternative idea: turn QemuDisplay into an ObjectClass, then it'll be
visible in qom introspection.  Likewise a bit more complex ...

> do we want that?  Or is the quick-n-easy way via the schema good
> enough for most use cases?

Would be better than nothing, but I'd prefer something which works
properly with modular qemu ...

> (I'm not that familiar with "virsh domcapabilities" ... is there any
> real usage for the <graphics> section or is this rather cosmetical?)

Management apps can use that info to avoid building unsupported
configurations.  Also you'll get a nice "$foo is not supported" error
from libvirt instead of libvirt dumping qemu stderr ;)

take care,
  Gerd

Re: [RFC QEMU PATCH] ui: Make the DisplayType enum entries conditional

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> > The #if CONFIG_SDL approach will not work because qemu will continue to
>> > report sdl as supported even when the sdl module is not installed any
>> > more.
>> 
>> I guess we'd need a separate QMP command to fix that, which tries to load
>> the modules first when being called? Something similar to what is being done
>> in qemu_display_help() ?
>
> That would work, yes.
>
>> That's certainly doable, too, just a little bit more complex...
>
> Alternative idea: turn QemuDisplay into an ObjectClass, then it'll be
> visible in qom introspection.  Likewise a bit more complex ...
>
>> do we want that?  Or is the quick-n-easy way via the schema good
>> enough for most use cases?
>
> Would be better than nothing, but I'd prefer something which works
> properly with modular qemu ...

Define "properly" :)

Without modules, qom-list-types has no side-effects, as introspection
should be.  With modules, it loads *all* modules known to define QOM
types, running their initialization code.

It loads them all even when asked to list only some, with argument
"implements".

In theory, management applications not having to know anything about
modules is nice.  Whether it'll work out in practice remains to be seen.
I'm not exactly confident.

[...]

Re: [RFC QEMU PATCH] ui: Make the DisplayType enum entries conditional

[Thomas Huth]

On 09/06/2021 14.50, Gerd Hoffmann wrote:
>    Hi,
> 
>>> The #if CONFIG_SDL approach will not work because qemu will continue to
>>> report sdl as supported even when the sdl module is not installed any
>>> more.
>>
>> I guess we'd need a separate QMP command to fix that, which tries to load
>> the modules first when being called? Something similar to what is being done
>> in qemu_display_help() ?
> 
> That would work, yes.
> 
>> That's certainly doable, too, just a little bit more complex...
> 
> Alternative idea: turn QemuDisplay into an ObjectClass, then it'll be
> visible in qom introspection.  Likewise a bit more complex ...
> 
>> do we want that?  Or is the quick-n-easy way via the schema good
>> enough for most use cases?
> 
> Would be better than nothing, but I'd prefer something which works
> properly with modular qemu ...

I'm not sure whether we can even make it 100% rock solid if we introduce a 
dedicated QMP command here. For example imagine that libvirt did its probing 
while a X11 server was running, so it discovered that QEMU could be used 
with SDL. Now the user stops the X11 server, thus the cached information 
that QEMU could be used with SDL becomes useless. I think that's somehow 
similar to the situation whether the module is available or not. The 
information that is shown by "virsh domcapabilities" can only be an 
indication of what can be used in the best case (module available, X11 
server running etc), but it can never be a 100% guarantee that the UI 
interface can really really be used.
Thus I tend to continue with the simple way via the QAPI schema right now, 
unless someone really has an urgent need for a separate QMP command (at 
least for the BZ that I listed in my original mail, the simple way via the 
QAPI schema is enough).

  Thomas

Re: [RFC QEMU PATCH] ui: Make the DisplayType enum entries conditional

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On Wed, Jun 09, 2021 at 12:02:40PM +0200, Thomas Huth wrote:
>> Libvirt's "domcapabilities" command has a way to state whether
>> certain graphic frontends are available in QEMU or not. Originally,
>> libvirt looked at the "--help" output of the QEMU binary to determine
>> whether SDL was available or not (by looking for the "-sdl" parameter
>> in the help text), but since libvirt stopped doing this analysis of
>> the help text, the detection of SDL is currently broken, see:
>> 
>>  https://bugzilla.redhat.com/show_bug.cgi?id=1790902
>> 
>> QEMU should provide a way via the QMP interface instead. The simplest
>> way, without introducing additional commands, is to make the DisplayType
>> enum entries conditional, so that the enum only contains the entries if
>> the corresponding CONFIG_xxx switches have been set.
>
> Hmm, that'll break for the "dnf remove qemu-ui-sdl" case ...

By design, query-qmp-schema reflects compile-time configuration.  It
predates modules, and has not been updated for modules.

For stuff that cannot be built as module, yes means yes in
query-qmp-schema.

For stuff that can be built as module, yes means maybe: yes if built-in
or the module is loadable, else no.

Modules are not quite transparent there.  Transparency is a design goal,
but I'm afraid it's an unattainable one.

Parsing -help output would have the same issue.

The only way to upgrade a maybe to a yes is to load the module.  As long
as module loading is implicit (because transparency!), the only way to
load the module is to try using the feature, and recognize "can't load
module" failures.  "Just try and recognize failures" is workable in
simple cases.  In not so simple cases, it can be complex, slow and
fragile (if it wasn't, introspection likely would not exist; see my
"QEMU interface introspection: From hacks to solutions" talk at KVM
Forum 2015).

Re: [RFC QEMU PATCH] ui: Make the DisplayType enum entries conditional

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> Libvirt's "domcapabilities" command has a way to state whether
> certain graphic frontends are available in QEMU or not. Originally,
> libvirt looked at the "--help" output of the QEMU binary to determine
> whether SDL was available or not (by looking for the "-sdl" parameter
> in the help text), but since libvirt stopped doing this analysis of
> the help text, the detection of SDL is currently broken, see:
>
>  https://bugzilla.redhat.com/show_bug.cgi?id=1790902
>
> QEMU should provide a way via the QMP interface instead. The simplest
> way, without introducing additional commands, is to make the DisplayType
> enum entries conditional, so that the enum only contains the entries if
> the corresponding CONFIG_xxx switches have been set. Unfortunately, this
> only works for sdl, cocoa and spice, since gtk, egl-headless and curses
> are hard-wired in the "data" section of the DisplayOptions, and thus
> unfortunately always have to be defined.

Here:

    { 'union'   : 'DisplayOptions',
      'base'    : { 'type'           : 'DisplayType',
                    '*full-screen'   : 'bool',
                    '*window-close'  : 'bool',
                    '*show-cursor'   : 'bool',
                    '*gl'            : 'DisplayGLMode' },
      'discriminator' : 'type',
      'data'    : { 'gtk'            : 'DisplayGTK',
                    'curses'         : 'DisplayCurses',
                    'egl-headless'   : 'DisplayEGLHeadless'} }

Flat union branches can be made conditional like so:

      'data'    : { 'gtk'            : { 'type': 'DisplayGTK',
                                         'if': 'defined(CONFIG_GTK)' },

Then you should be able to make the corresponding enum value
conditional, too.

Re: [RFC QEMU PATCH] ui: Make the DisplayType enum entries conditional

[Thomas Huth]

On 09/06/2021 15.16, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> Libvirt's "domcapabilities" command has a way to state whether
>> certain graphic frontends are available in QEMU or not. Originally,
>> libvirt looked at the "--help" output of the QEMU binary to determine
>> whether SDL was available or not (by looking for the "-sdl" parameter
>> in the help text), but since libvirt stopped doing this analysis of
>> the help text, the detection of SDL is currently broken, see:
>>
>>   https://bugzilla.redhat.com/show_bug.cgi?id=1790902
>>
>> QEMU should provide a way via the QMP interface instead. The simplest
>> way, without introducing additional commands, is to make the DisplayType
>> enum entries conditional, so that the enum only contains the entries if
>> the corresponding CONFIG_xxx switches have been set. Unfortunately, this
>> only works for sdl, cocoa and spice, since gtk, egl-headless and curses
>> are hard-wired in the "data" section of the DisplayOptions, and thus
>> unfortunately always have to be defined.
> 
> Here:
> 
>      { 'union'   : 'DisplayOptions',
>        'base'    : { 'type'           : 'DisplayType',
>                      '*full-screen'   : 'bool',
>                      '*window-close'  : 'bool',
>                      '*show-cursor'   : 'bool',
>                      '*gl'            : 'DisplayGLMode' },
>        'discriminator' : 'type',
>        'data'    : { 'gtk'            : 'DisplayGTK',
>                      'curses'         : 'DisplayCurses',
>                      'egl-headless'   : 'DisplayEGLHeadless'} }
> 
> Flat union branches can be made conditional like so:
> 
>        'data'    : { 'gtk'            : { 'type': 'DisplayGTK',
>                                           'if': 'defined(CONFIG_GTK)' },
> 
> Then you should be able to make the corresponding enum value
> conditional, too.

Thanks for the hint, I'll give it a try!

  Thomas