Re: [PATCH] vl: Print display options for -display help

[BALATON Zoltan]

On Fri, 15 Dec 2023, Akihiko Odaki wrote:
> On 2023/12/14 22:00, BALATON Zoltan wrote:
> > On Thu, 14 Dec 2023, Philippe Mathieu-Daudé wrote:
> > > Hi Akihiko,
> > >
> > > On 14/12/23 07:47, Akihiko Odaki wrote:
> > > > -display lists display backends, but does not tell their options.
> > > > Use the help messages from qemu-options.def, which include the list of
> > > > options.
> > > >
> > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > > ---
> > > >   include/ui/console.h |  1 -
> > > >   system/vl.c          | 11 ++++++-----
> > > >   ui/console.c         | 20 --------------------
> > > >   3 files changed, 6 insertions(+), 26 deletions(-)
> > >
> > >
> > > > diff --git a/ui/console.c b/ui/console.c
> > > > index 7db921e3b7d6..6aee5e9a7ffb 100644
> > > > --- a/ui/console.c
> > > > +++ b/ui/console.c
> > > > @@ -1691,23 +1691,3 @@ const char *qemu_display_get_vc(DisplayOptions 
> > > > *opts)
> > > >       }
> > > >       return vc;
> > > >   }
> > > > -
> > > > -void qemu_display_help(void)
> > > > -{
> > > > -    int idx;
> > > > -
> > > > -    printf("Available display backend types:\n");
> > > > -    printf("none\n");
> > > > -    for (idx = DISPLAY_TYPE_NONE; idx < DISPLAY_TYPE__MAX; idx++) {
> > > > -        if (!dpys[idx]) {
> > > > -            Error *local_err = NULL;
> > > > -            int rv = ui_module_load(DisplayType_str(idx), &local_err);
> > > > -            if (rv < 0) {
> > > > -                error_report_err(local_err);
> > > > -            }
> > > > -        }
> > > > -        if (dpys[idx]) {
> > > > -            printf("%s\n",  DisplayType_str(dpys[idx]->type));
> > >
> > > Is the "qapi/qapi-commands-ui.h" header still necessary?
> > >
> > > > -        }
> > > > -    }
> > > > -}
> > >
> > > So we go from:
> > >
> > > $ ./qemu-system-aarch64 -display help
> > > Available display backend types:
> > > none
> > > gtk
> > > sdl
> > > curses
> > > cocoa
> > > dbus
> > >
> > > to:
> > >
> > > $ ./qemu-system-aarch64 -display help
> > > -display sdl[,gl=on|core|es|off][,grab-mod=<mod>][,show-cursor=on|off]
> > >            [,window-close=on|off]
> > > -display gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]
> > >            [,show-tabs=on|off][,show-cursor=on|off][,window-close=on|off]
> > >            [,show-menubar=on|off]
> > > -display vnc=<display>[,<optargs>]
> > > -display curses[,charset=<encoding>]
> > > -display cocoa[,full-grab=on|off][,swap-opt-cmd=on|off]
> > > -display dbus[,addr=<dbusaddr>]
> > >             [,gl=on|core|es|off][,rendernode=<file>]
> > > -display cocoa[,show-cursor=on|off][,left-command-key=on|off]
> > > -display none
> > >                select display backend type
> > >                The default display is equivalent to
> > >                "-display gtk"
> > >
> > > The latter is indeed more helpful.
> >
> > It is more helpful but maybe a bit overwhelming. Would it be possible to 
> > only print the options with -display cocoa,help similar to how -device help 
> > lists devices and -device sm501,help lists options for one device? Adding 
> > info about default to -display help is really helpful though (that could 
> > also be marked with (default) like in -machine help.
>
> It's copied from what qemu-system-aarch64 -h outputs. At least it's less 
> overwhelming than qemu-system-aarch64 -h.

This changes what -display help does so if some script depends on that it 
may not be a good idea. Since the same info is already in -help maybe this 
change to add that to -display help as well is not the best solution so 
I'd say drop this patch and leave it as it is for now.

Adding (default) to show default as with -machine help would be useful but 
the default in help seems to be added by preprocessor magic so it's not 
easy to use that in qemu_display_help(). Maybe if a constant would be 
defined with the default value instead of adding it directly to help text 
then that could be used but we have '-vnc some-arguments' as opposed to 
'-display something' for all other casess so if that -vnc option is 
correct and it's not like '-display vnc' then that's not trivial either. 
So I'd say just fotget about this for now as it's not that important so 
may not worth the effort.

> > I'm not complaining, thanks for taking care of this so quickly but if it's 
> > not too difficult to add separate -display cocoa,help and not list options 
> > in -display help maybe that would be better and more consistent with other 
> > help options.
>
> Yes, that will require some major refactoring so I'm not going to do that for 
> now.

I've also looked at that and concluded the same that it would take some 
qapi expert to solve this. It seems the options are parsed into some qapi 
types which may have help to display but I could not find out how that 
works. For device it may be handled in qdev_device_help() but I don't know 
if that would be applicable for display backends and if so how. Maybe 
someone who knows about this could chime in and give some idea how e.g. 
-display gtk,help could be implemented similar to -device somedevice,help. 
(Other than passing through the help text as your patch does and rhen cut 
the relevant part from that with string functions but that's likely not 
the right way to do this.)

Regards,
BALATON Zoltan