| Message ID | 20171117103046.15943-20-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 17, 2017 at 11:30:41AM +0100, Gerd Hoffmann wrote: >Using the new registry instead of #ifdefs in vl.c. > >Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> >--- > include/ui/console.h | 1 + > ui/console.c | 19 +++++++++++++++++++ > vl.c | 15 +++++---------- > 3 files changed, 25 insertions(+), 10 deletions(-) > >diff --git a/include/ui/console.h b/include/ui/console.h >index 6c89599355..7c9e40cc9a 100644 >--- a/include/ui/console.h >+++ b/include/ui/console.h >@@ -442,6 +442,7 @@ struct QemuDisplay { > }; > > void qemu_display_register(QemuDisplay *ui); >+bool qemu_display_find_default(DisplayOptions *opts); > void qemu_display_early_init(DisplayOptions *opts); > void qemu_display_init(DisplayState *ds, DisplayOptions *opts); > >diff --git a/ui/console.c b/ui/console.c >index aa8afbfe26..4c9ac8b21a 100644 >--- a/ui/console.c >+++ b/ui/console.c >@@ -2176,6 +2176,25 @@ void qemu_display_register(QemuDisplay *ui) > dpys[ui->type] = ui; > } > >+bool qemu_display_find_default(DisplayOptions *opts) >+{ >+ static DisplayType prio[] = { >+ DISPLAY_TYPE_GTK, >+ DISPLAY_TYPE_SDL, >+ DISPLAY_TYPE_COCOA >+ }; >+ int i; >+ >+ for (i = 0; i < ARRAY_SIZE(prio); i++) { >+ if (dpys[prio[i]] == NULL) { >+ continue; >+ } >+ opts->type = prio[i]; >+ return true; >+ } >+ return false; >+} >+ > void qemu_display_early_init(DisplayOptions *opts) > { > assert(opts->type < DISPLAY_TYPE__MAX); >diff --git a/vl.c b/vl.c >index df0c10398d..f861562bb2 100644 >--- a/vl.c >+++ b/vl.c >@@ -4406,17 +4406,12 @@ int main(int argc, char **argv, char **envp) > } > #endif > if (dpy.type == DISPLAY_TYPE_DEFAULT && !display_remote) { >-#if defined(CONFIG_GTK) >- dpy.type = DISPLAY_TYPE_GTK; >-#elif defined(CONFIG_SDL) >- dpy.type = DISPLAY_TYPE_SDL; >-#elif defined(CONFIG_COCOA) >- dpy.type = DISPLAY_TYPE_COCOA; >-#elif defined(CONFIG_VNC) >- vnc_parse("localhost:0,to=99,id=default", &error_abort); >-#else >- dpy.type = DISPLAY_TYPE_NONE; >+ if (!qemu_display_find_default(&dpy)) { >+ dpy.type = DISPLAY_TYPE_NONE; >+#if defined(CONFIG_VNC) >+ vnc_parse("localhost:0,to=99,id=default", &error_abort); > #endif Mostly some questions on this: - I'm curious, why is VNC not just one of the ones searched for in qemu_display_find_default()? - What if there is another display type added? Would it be added to qemu_display_find_default() or in this if statement? - Is there a reason that VNC doesn't follow the same registration mechanism you're proposing here (as in you've no changes to that module in this patch set). - In the spirit of encapsulation, would it not make sense for the vnc_parse() to be in the VNC code itself as a fall back if no specific options are given to the -vnc option? Thanks, Darren.
Hi, > > - dpy.type = DISPLAY_TYPE_NONE; > > + if (!qemu_display_find_default(&dpy)) { > > + dpy.type = DISPLAY_TYPE_NONE; > > +#if defined(CONFIG_VNC) > > + vnc_parse("localhost:0,to=99,id=default", &error_abort); > > #endif > > Mostly some questions on this: > > - I'm curious, why is VNC not just one of the ones searched for in > qemu_display_find_default()? vnc is a bit different. It's a remote display protocol, not a (local) user interface. There is no DISPLAY_TYPE_VNC for that reason (without this series too). And IMO it was a bad idea to add -display vnc=$config (same as -vnc $config) in the first place. There can be only one user interface instance, i.e. you can't have gtk and sdl at the same time. With vnc (and spice) that works though, i.e. you can start qemu with some local user interface and enable vnc or spice (even both) remote access at the same time. > - What if there is another display type added? Would it be added to > qemu_display_find_default() or in this if statement? qemu_display_find_default() most likely, but depends on what exactly it is. > - Is there a reason that VNC doesn't follow the same registration > mechanism you're proposing here (as in you've no changes to that > module in this patch set). See above ;) > - In the spirit of encapsulation, would it not make sense for the > vnc_parse() to be in the VNC code itself as a fall back if no > specific options are given to the -vnc option? It's not that simple. The default vnc server is started only in case no (graphical) local user interface is available, so it's not something the vnc code can figure purely on its own. cheers, Gerd
Hi Gerd, Thanks for clarifying things for me. On Fri, Nov 17, 2017 at 02:24:54PM +0100, Gerd Hoffmann wrote: > Hi, > >> > - dpy.type = DISPLAY_TYPE_NONE; >> > + if (!qemu_display_find_default(&dpy)) { >> > + dpy.type = DISPLAY_TYPE_NONE; >> > +#if defined(CONFIG_VNC) >> > + vnc_parse("localhost:0,to=99,id=default", &error_abort); >> > #endif >> >> Mostly some questions on this: >> >> - I'm curious, why is VNC not just one of the ones searched for in >> qemu_display_find_default()? > >vnc is a bit different. It's a remote display protocol, not a (local) >user interface. There is no DISPLAY_TYPE_VNC for that reason (without >this series too). And IMO it was a bad idea to add -display vnc=$config >(same as -vnc $config) in the first place. > >There can be only one user interface instance, i.e. you can't have gtk >and sdl at the same time. With vnc (and spice) that works though, i.e. >you can start qemu with some local user interface and enable vnc or >spice (even both) remote access at the same time. Ah, wasn't sure about being able to do both a local UI and VNC - I've only ever used one or the other :) OK, so the odd thing then is the check for !remote_display earlier on in the function (missing from the quote above) which seems to end up initializing VNC (albeit with localhost) when CONFIG_VNC is defined, but no other local display type has been selected. But, I suppose it's not strictly incorrect with the 'localhost' :) > >> - What if there is another display type added? Would it be added to >> qemu_display_find_default() or in this if statement? > >qemu_display_find_default() most likely, >but depends on what exactly it is. OK, so neither SPICE or VNC really fit into this patchset because they are seen as additional to the display type, not the actual display type - SPICE is even more special too given the dependency on a guest VM driver to really function correctly. I wonder should there be a, similar, qemu_display_find_remote_default() which would be used when no local display is defined. And that would use a similar mechanism to permit selecting VNC or SPICE via registration mechanisms over rather than the #ifdef style for remote displays that will remain ever after this patchset (certainly could be another patchset beyond this one). Maybe it's not worth it, given that VNC is really he only remote UI module you can use. > >> - Is there a reason that VNC doesn't follow the same registration >> mechanism you're proposing here (as in you've no changes to that >> module in this patch set). > >See above ;) > >> - In the spirit of encapsulation, would it not make sense for the >> vnc_parse() to be in the VNC code itself as a fall back if no >> specific options are given to the -vnc option? > >It's not that simple. The default vnc server is started only in case >no (graphical) local user interface is available, so it's not something >the vnc code can figure purely on its own. Fair enough. Just a thought, but maybe the specific string could be defined in vnc.h instead of a string here at least? (e.g. VNC_DEFAULT_LOCALHOST_OPTIONS, or similar). Thanks, Darren.
Hi, > OK, so the odd thing then is the check for !remote_display earlier > on in the function (missing from the quote above) which seems to end > up initializing VNC (albeit with localhost) when CONFIG_VNC is > defined, but no other local display type has been selected. remote_display is set in case either -vnc or -spice was given on the command line, and qemu will not start a local user display in that case (unless you explicitly ask for it via -display). > OK, so neither SPICE or VNC really fit into this patchset because > they are seen as additional to the display type, not the actual > display type - SPICE is even more special too given the dependency > on a guest VM driver to really function correctly. Hmm? spice works just fine with any display. > I wonder should there be a, similar, qemu_display_find_remote_default() > which would be used when no local display is defined. And that would > use a similar mechanism to permit selecting VNC or SPICE via > registration mechanisms over rather than the #ifdef style for remote > displays that will remain ever after this patchset (certainly could > be another patchset beyond this one). Well, historically qemu tried to do alot of stuff (like picking a display to use) automatically for the user. And we did stick to that for backward compatibility reasons. But this automagic causes quite some headache now and then, when changing things and the autmagic logic gets into the way. So I don't feel like adding more of this. Also vnc is in pretty much any qemu build, because it is enabled by default and doesn't need any external dependencies, so falling back to spice would be quite rare ... cheers, Gerd
On Fri, Nov 17, 2017 at 04:03:28PM +0100, Gerd Hoffmann wrote: > Hi, > >> OK, so the odd thing then is the check for !remote_display earlier >> on in the function (missing from the quote above) which seems to end >> up initializing VNC (albeit with localhost) when CONFIG_VNC is >> defined, but no other local display type has been selected. > >remote_display is set in case either -vnc or -spice was given on the >command line, and qemu will not start a local user display in that case >(unless you explicitly ask for it via -display). Makes sense. > >> OK, so neither SPICE or VNC really fit into this patchset because >> they are seen as additional to the display type, not the actual >> display type - SPICE is even more special too given the dependency >> on a guest VM driver to really function correctly. > >Hmm? spice works just fine with any display. Sorry, mixed that up with EGL I think. > >> I wonder should there be a, similar, qemu_display_find_remote_default() >> which would be used when no local display is defined. And that would >> use a similar mechanism to permit selecting VNC or SPICE via >> registration mechanisms over rather than the #ifdef style for remote >> displays that will remain ever after this patchset (certainly could >> be another patchset beyond this one). > >Well, historically qemu tried to do alot of stuff (like picking a >display to use) automatically for the user. And we did stick to that >for backward compatibility reasons. But this automagic causes quite >some headache now and then, when changing things and the autmagic logic >gets into the way. So I don't feel like adding more of this. Also vnc Agree, there are times when automagic works - but unfortunately for more consistent usage, you are better being explicit. >is in pretty much any qemu build, because it is enabled by default and >doesn't need any external dependencies, so falling back to spice would >be quite rare ... Understood - definitely VNC is a pretty good fall-back for most people. Thanks for outlining your thoughts on this, very helpful in understanding the UI code and why somethings are how they are. Thanks, Darren.
diff --git a/include/ui/console.h b/include/ui/console.h index 6c89599355..7c9e40cc9a 100644 --- a/include/ui/console.h +++ b/include/ui/console.h @@ -442,6 +442,7 @@ struct QemuDisplay { }; void qemu_display_register(QemuDisplay *ui); +bool qemu_display_find_default(DisplayOptions *opts); void qemu_display_early_init(DisplayOptions *opts); void qemu_display_init(DisplayState *ds, DisplayOptions *opts); diff --git a/ui/console.c b/ui/console.c index aa8afbfe26..4c9ac8b21a 100644 --- a/ui/console.c +++ b/ui/console.c @@ -2176,6 +2176,25 @@ void qemu_display_register(QemuDisplay *ui) dpys[ui->type] = ui; } +bool qemu_display_find_default(DisplayOptions *opts) +{ + static DisplayType prio[] = { + DISPLAY_TYPE_GTK, + DISPLAY_TYPE_SDL, + DISPLAY_TYPE_COCOA + }; + int i; + + for (i = 0; i < ARRAY_SIZE(prio); i++) { + if (dpys[prio[i]] == NULL) { + continue; + } + opts->type = prio[i]; + return true; + } + return false; +} + void qemu_display_early_init(DisplayOptions *opts) { assert(opts->type < DISPLAY_TYPE__MAX); diff --git a/vl.c b/vl.c index df0c10398d..f861562bb2 100644 --- a/vl.c +++ b/vl.c @@ -4406,17 +4406,12 @@ int main(int argc, char **argv, char **envp) } #endif if (dpy.type == DISPLAY_TYPE_DEFAULT && !display_remote) { -#if defined(CONFIG_GTK) - dpy.type = DISPLAY_TYPE_GTK; -#elif defined(CONFIG_SDL) - dpy.type = DISPLAY_TYPE_SDL; -#elif defined(CONFIG_COCOA) - dpy.type = DISPLAY_TYPE_COCOA; -#elif defined(CONFIG_VNC) - vnc_parse("localhost:0,to=99,id=default", &error_abort); -#else - dpy.type = DISPLAY_TYPE_NONE; + if (!qemu_display_find_default(&dpy)) { + dpy.type = DISPLAY_TYPE_NONE; +#if defined(CONFIG_VNC) + vnc_parse("localhost:0,to=99,id=default", &error_abort); #endif + } } if (dpy.type == DISPLAY_TYPE_DEFAULT) { dpy.type = DISPLAY_TYPE_NONE;
Using the new registry instead of #ifdefs in vl.c. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/ui/console.h | 1 + ui/console.c | 19 +++++++++++++++++++ vl.c | 15 +++++---------- 3 files changed, 25 insertions(+), 10 deletions(-)