From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36943) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b9oxf-0007GH-Dn for qemu-devel@nongnu.org; Mon, 06 Jun 2016 03:28:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b9oxb-0002pC-66 for qemu-devel@nongnu.org; Mon, 06 Jun 2016 03:28:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60235) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b9oxa-0002oZ-Tt for qemu-devel@nongnu.org; Mon, 06 Jun 2016 03:28:31 -0400 From: Markus Armbruster References: <1464676956-8460-1-git-send-email-robert.hu@intel.com> <1464676956-8460-3-git-send-email-robert.hu@intel.com> <878tyq7jin.fsf@dusky.pond.sub.org> <1464688183.11816.11.camel@vmm.sh.intel.com> <87vb1ufpdm.fsf@dusky.pond.sub.org> <1465132537.14690.20.camel@vmm.sh.intel.com> Date: Mon, 06 Jun 2016 09:28:26 +0200 In-Reply-To: <1465132537.14690.20.camel@vmm.sh.intel.com> (Robert Hu's message of "Sun, 05 Jun 2016 21:15:37 +0800") Message-ID: <87k2i2pyhx.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 2/2] Explicitly print out default vnc option in use List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Robert Hu Cc: robert.hu@intel.com, qemu-devel@nongnu.org, pbonzini@redhat.com, Gerd Hoffmann Robert Hu writes: > On Tue, 2016-05-31 at 13:17 +0200, Markus Armbruster wrote: >> Robert Hu writes: >> >> > On Tue, 2016-05-31 at 09:51 +0200, Markus Armbruster wrote: >> >> Robert Ho 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 >> >> > --- >> >> > 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=[,] 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 curses -display none select display type * There is no help on the in -display vnc=. * 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=[,] 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 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 in -display vnc=. * 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.