Re: [Qemu-devel] [PATCH 2/2] Explicitly print out default vnc option in use

[Markus Armbruster]

Robert Hu <address@hidden> writes:

> On Tue, 2016-05-31 at 13:17 +0200, Markus Armbruster wrote:
>> Robert Hu <address@hidden> writes:
>> 
>> > On Tue, 2016-05-31 at 09:51 +0200, Markus Armbruster wrote:
>> >> Robert Ho <address@hidden> writes:
>> >> 
>> >> > If no display option defined in QEMU command line, and SDL is not
>> >> > available, then it by default uses '-vnc localhost:0,to=99,id=default'.
>> >> > This patch simply print out the default option parameters out, so that
>> >> > user is aware of that.
>> >> >
>> >> > Signed-off-by: Robert Ho <address@hidden>
>> >> > ---
>> >> >  vl.c | 5 ++++-
>> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/vl.c b/vl.c
>> >> > index 18d1423..8617a68 100644
>> >> > --- a/vl.c
>> >> > +++ b/vl.c
>> >> > @@ -4213,7 +4213,10 @@ int main(int argc, char **argv, char **envp)
>> >> >  #elif defined(CONFIG_COCOA)
>> >> >          display_type = DT_COCOA;
>> >> >  #elif defined(CONFIG_VNC)
>> >> > -        vnc_parse("localhost:0,to=99,id=default", &error_abort);
>> >> > +               #define DEFAULT_VNC_DISPLAY_OPTION      
>> >> > "localhost:0,to=99,id=default"
>> >> 
>> >> Preprocessor directives shouldn't be indented.
>> >> 
>> >> Also tab damage.  Please use scripts/checkpatch.pl to check your patches.
>> >
>> > Thanks Markus for your review.
>> > Firstly apologize if you received multiple copies of this patch. I'm
>> > still struggling with my egress SMTP setting. I've no idea how many
>> > copies you received:( but glad now see your reply.
>> >
>> > Yes, sorry about haven't checked the patch with the auxiliary scripts. I
>> > didn't know that. Thanks for pointing out.
>> > I'm new here, will learn these upstream convention ASAP.
>> 
>> No problem.  All we expect from new contributors is making an effort to
>> get their patches right.  Actually getting them 100% right from the
>> start isn't really in the cards :)
>
> Thank You!
> Sorry for late following up; for I just part-time do this. I'm too busy
> on work these days.
>
>> 
>> >> > +        vnc_parse(DEFAULT_VNC_DISPLAY_OPTION, &error_abort);
>> >> > +               printf("No display option defined, using '-vnc %s' by 
>> >> > default   \
>> >> > +\n", DEFAULT_VNC_DISPLAY_OPTION);
>> >> >          show_vnc_port = 1;
>> >> >  #else
>> >> >          display_type = DT_NONE;
>> >> 
>> >> I don't like this.  Programs should be quiet unless they got something
>> >> important to say.  Can't see why this particular default is more
>> >> important than all the other defaults we don't print.
>> >> 
>> >> The default could be documented in output of --help.
>> >
>> > Actually my thought was this is not using the default value and
>> > implicitly. The default of 'to' is 0, while in this case (when no
>> > display option defined and SDL not configured in), it implicitly uses
>> > non-default value '99'. Therefore I thought it shall be explicitly print
>> > out so that user would be aware of what was chosen on behalf of him;
>> > like the final print of 'VNC server running on '::1;5900''. 
>> 
>> The default depends on configuration options.  Ideally, --help output
>> would show the defaults for this build's configuration.
>
> I don't see a './configure' option related to this '-vnc to' param. Is
> there any?
> '--help', you mean 'qemu-system_x86-64 --help'? or './configure --help'?

The former.

The modern way to select a display is -display.  The older -nographic,
-curses, -sdl are retained for backward compatibility.

Relevant parts of -help:

    Display options:
    -display sdl[,frame=on|off][,alt_grab=on|off][,ctrl_grab=on|off]
                [,window_close=on|off]|curses|none|
                gtk[,grab_on_hover=on|off]|
                vnc=<display>[,<optargs>]
                    select display type
    -nographic      disable graphical output and redirect serial I/Os to console
    -curses         use a curses/ncurses interface instead of SDL
    [...]
    -sdl            enable SDL
    [...]
    -vnc display    start a VNC server on display

Issues:

* Help for -display is broken: the mutually exclusive option arguments
  are concatenated.  -display curses and -display none are undocumented.
  It should look more like this:

    -display sdl[,frame=on|off][,alt_grab=on|off][,ctrl_grab=on|off]
                [,window_close=on|off]|curses|none|
    -display gtk[,grab_on_hover=on|off]|
    -display vnc=<display>[,<optargs>]
    -display curses
    -display none
                    select display type

* There is no help on the <display> in -display vnc=<display>.

* There is no help on the default.  main() picks the default depending
  on configure options:

    #if defined(CONFIG_GTK)
            display_type = DT_GTK;
    #elif defined(CONFIG_SDL)
            display_type = DT_SDL;
    #elif defined(CONFIG_COCOA)
            display_type = DT_COCOA;
    #elif defined(CONFIG_VNC)
            vnc_parse("localhost:0,to=99,id=default", &error_abort);
            show_vnc_port = 1;
    #else
            display_type = DT_NONE;
    #endif

  Help should show the default this binary will pick.  This is what I
  meant by "Ideally, --help output
>> would show the defaults for this build's configuration.
>
> I don't see a './configure' option related to this '-vnc to' param. Is
> there any?
> '--help', you mean 'qemu-system_x86-64 --help'? or './configure --help'?

The former.

The modern way to select a display is -display.  The older -nographic,
-curses, -sdl are retained for backward compatibility.

Relevant parts of -help:

    Display options:
    -display sdl[,frame=on|off][,alt_grab=on|off][,ctrl_grab=on|off]
                [,window_close=on|off]|curses|none|
                gtk[,grab_on_hover=on|off]|
                vnc=<display>[,<optargs>]
                    select display type
    -nographic      disable graphical output and redirect serial I/Os to console
    -curses         use a curses/ncurses interface instead of SDL
    [...]
    -sdl            enable SDL
    [...]
    -vnc display    start a VNC server on display

Issues:

* Help for -display is broken: the mutually exclusive option arguments
  are concatenated.  -display curses and -display none are undocumented.
  It should look more like this:

    -display sdl[,frame=on|off][,alt_grab=on|off][,ctrl_grab=on|off]
                [,window_close=on|off]|curses|none|
    -display gtk[,grab_on_hover=on|off]|
    -display vnc=<display>[,<optargs>]
    -display curses
    -display none
                    select display type

* -display sdl,gl=on|off and -display gtk,gl=on|off are undocumented
   (missed in commit 0b71a5d5c and 97edf3b).

* There is no help on the <display> in -display vnc=<display>.

* There is no help on the default.  main() picks the default depending
  on configure options:

    #if defined(CONFIG_GTK)
            display_type = DT_GTK;
    #elif defined(CONFIG_SDL)
            display_type = DT_SDL;
    #elif defined(CONFIG_COCOA)
            display_type = DT_COCOA;
    #elif defined(CONFIG_VNC)
            vnc_parse("localhost:0,to=99,id=default", &error_abort);
            show_vnc_port = 1;
    #else
            display_type = DT_NONE;
    #endif

  Help should show the default this binary will pick.  This is what I
  meant by "Ideally, --help output would show the defaults for this
  build's configuration."

* Help should explain syntacic sugar:
  -curses is sugar for -display curses
  -sdl is sugar for -display sdl
  -vnc display is sugar for -display vnc=display

  -nographic is also sugar, but too complicated to explain; I'd leave it
  as is.

Non-issue

* Help shows options even when they're not compiled in.  That's okay,
  because trying to use them fails with an "FOO support is disabled"
  error message.

>> If we decide users need more information than the current "VNC server
>> running on" line, perhaps it should be included right in that line.

This would complement, but not replace better -help ouput.

If you would like to work on these issues, let us know.