[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2] Explicitly print out default vnc option in
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 2/2] Explicitly print out default vnc option in use |
Date: |
Mon, 06 Jun 2016 09:28:26 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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.