[RFC QEMU PATCH] ui: Make the DisplayType enum entries conditional
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
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
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 :|
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
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 :|
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...
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
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. [...]
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
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).
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.
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
participants (4)
-
Daniel P. Berrangé -
Gerd Hoffmann -
Markus Armbruster -
Thomas Huth