| Message ID | 1464676956-8460-3-git-send-email-robert.hu@intel.com |
|---|---|
| State | New |
| Headers | show |
Robert Ho <robert.hu@intel.com> 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 <robert.hu@intel.com> > --- > 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. > + 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.
On Tue, 2016-05-31 at 09:51 +0200, Markus Armbruster wrote: > Robert Ho <robert.hu@intel.com> 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 <robert.hu@intel.com> > > --- > > 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. > > > + 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''.
Robert Hu <robert.hu@vmm.sh.intel.com> writes: > On Tue, 2016-05-31 at 09:51 +0200, Markus Armbruster wrote: >> Robert Ho <robert.hu@intel.com> 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 <robert.hu@intel.com> >> > --- >> > 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 :) >> > + 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. If we decide users need more information than the current "VNC server running on" line, perhaps it should be included right in that line.
On Tue, 2016-05-31 at 13:17 +0200, Markus Armbruster wrote: > Robert Hu <robert.hu@vmm.sh.intel.com> writes: > > > On Tue, 2016-05-31 at 09:51 +0200, Markus Armbruster wrote: > >> Robert Ho <robert.hu@intel.com> 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 <robert.hu@intel.com> > >> > --- > >> > 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'? > > If we decide users need more information than the current "VNC server > running on" line, perhaps it should be included right in that line.
Robert Hu <robert.hu@vmm.sh.intel.com> writes: > On Tue, 2016-05-31 at 13:17 +0200, Markus Armbruster wrote: >> Robert Hu <robert.hu@vmm.sh.intel.com> writes: >> >> > On Tue, 2016-05-31 at 09:51 +0200, Markus Armbruster wrote: >> >> Robert Ho <robert.hu@intel.com> 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 <robert.hu@intel.com> >> >> > --- >> >> > 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.
On Mon, 2016-06-06 at 09:28 +0200, Markus Armbruster wrote: > Robert Hu <robert.hu@vmm.sh.intel.com> writes: > > > On Tue, 2016-05-31 at 13:17 +0200, Markus Armbruster wrote: > >> Robert Hu <robert.hu@vmm.sh.intel.com> writes: > >> > >> > On Tue, 2016-05-31 at 09:51 +0200, Markus Armbruster wrote: [trim...] > > 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'? [seems repeated contents, trim...] > > 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. OK, if not in a hurry and assuming this is not a huge amount of work. I also need to look into the build arch so that completely understand your 'the default this binary will pick', till now I don't. Another concern is that I'm not a native English speaker, so those description words may not be that apt and concise. Meanwhile, this is another work extended from the original patch. How about accept the patch 1 first? as you and Paolo both think it is OK. Ought I rework a version 2 of single patch 1? or not necessary?
On Tue, 2016-06-07 at 08:28 +0800, Robert Hu wrote: > On Mon, 2016-06-06 at 09:28 +0200, Markus Armbruster wrote: > > Robert Hu <robert.hu@vmm.sh.intel.com> writes: > > > > > On Tue, 2016-05-31 at 13:17 +0200, Markus Armbruster wrote: > > >> Robert Hu <robert.hu@vmm.sh.intel.com> writes: > > >> > > >> > On Tue, 2016-05-31 at 09:51 +0200, Markus Armbruster wrote: > [trim...] > > > 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'? > [seems repeated contents, trim...] > > > 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. > > OK, if not in a hurry and assuming this is not a huge amount of work. > I also need to look into the build arch so that completely understand > your 'the default this binary will pick', till now I don't. > > Another concern is that I'm not a native English speaker, so those > description words may not be that apt and concise. > > Meanwhile, this is another work extended from the original patch. How > about accept the patch 1 first? as you and Paolo both think it is OK. > Ought I rework a version 2 of single patch 1? or not necessary? Oh, I see Paolo already get it in. Thanks! > >
Robert Hu <robert.hu@vmm.sh.intel.com> writes: > On Mon, 2016-06-06 at 09:28 +0200, Markus Armbruster wrote: >> Robert Hu <robert.hu@vmm.sh.intel.com> writes: >> >> > On Tue, 2016-05-31 at 13:17 +0200, Markus Armbruster wrote: >> >> Robert Hu <robert.hu@vmm.sh.intel.com> writes: >> >> >> >> > On Tue, 2016-05-31 at 09:51 +0200, Markus Armbruster wrote: > [trim...] >> > 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'? > [seems repeated contents, trim...] >> > 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. > > OK, if not in a hurry and assuming this is not a huge amount of work. > I also need to look into the build arch so that completely understand > your 'the default this binary will pick', till now I don't. > > Another concern is that I'm not a native English speaker, so those > description words may not be that apt and concise. Imperfect English can be addressed in review. Can be inefficient when most of the work is English rather than code. But if you want to try anyway, go right ahead regardless. [...]
On Wed, 2016-06-08 at 16:22 +0200, Markus Armbruster wrote: > Robert Hu <robert.hu@vmm.sh.intel.com> writes: > > > On Mon, 2016-06-06 at 09:28 +0200, Markus Armbruster wrote: > >> Robert Hu <robert.hu@vmm.sh.intel.com> writes: > >> > >> > On Tue, 2016-05-31 at 13:17 +0200, Markus Armbruster wrote: > >> >> Robert Hu <robert.hu@vmm.sh.intel.com> writes: > >> >> > >> >> > On Tue, 2016-05-31 at 09:51 +0200, Markus Armbruster wrote: > > [trim...] > >> > 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'? > > [seems repeated contents, trim...] > >> > 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. > > > > OK, if not in a hurry and assuming this is not a huge amount of work. > > I also need to look into the build arch so that completely understand > > your 'the default this binary will pick', till now I don't. > > > > Another concern is that I'm not a native English speaker, so those > > description words may not be that apt and concise. > > Imperfect English can be addressed in review. Can be inefficient when > most of the work is English rather than code. But if you want to try > anyway, go right ahead regardless. Hi Markus, After did some part-time investigation, I'd like to confirm with you: this change you required seems also just qemu-options.hx involved, am I right? If so, I'd like to do that. > > [...]
Robert Hu <robert.hu@vmm.sh.intel.com> writes: > On Wed, 2016-06-08 at 16:22 +0200, Markus Armbruster wrote: >> Robert Hu <robert.hu@vmm.sh.intel.com> writes: >> >> > On Mon, 2016-06-06 at 09:28 +0200, Markus Armbruster wrote: >> >> Robert Hu <robert.hu@vmm.sh.intel.com> writes: >> >> >> >> > On Tue, 2016-05-31 at 13:17 +0200, Markus Armbruster wrote: >> >> >> Robert Hu <robert.hu@vmm.sh.intel.com> writes: >> >> >> >> >> >> > On Tue, 2016-05-31 at 09:51 +0200, Markus Armbruster wrote: >> > [trim...] >> >> > 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'? >> > [seems repeated contents, trim...] >> >> > 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. >> > >> > OK, if not in a hurry and assuming this is not a huge amount of work. >> > I also need to look into the build arch so that completely understand >> > your 'the default this binary will pick', till now I don't. >> > >> > Another concern is that I'm not a native English speaker, so those >> > description words may not be that apt and concise. >> >> Imperfect English can be addressed in review. Can be inefficient when >> most of the work is English rather than code. But if you want to try >> anyway, go right ahead regardless. > > Hi Markus, > > After did some part-time investigation, I'd like to confirm with you: > this change you required seems also just qemu-options.hx involved, am I > right? > If so, I'd like to do that. Yes, because commmand line option help is defined there.
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" + 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;
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 <robert.hu@intel.com> --- vl.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)