From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51280) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b9Xu5-0007de-9Y for qemu-devel@nongnu.org; Sun, 05 Jun 2016 09:15:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b9Xu0-0003bl-Iu for qemu-devel@nongnu.org; Sun, 05 Jun 2016 09:15:45 -0400 Received: from mga02.intel.com ([134.134.136.20]:8917) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b9Xu0-0003bh-3m for qemu-devel@nongnu.org; Sun, 05 Jun 2016 09:15:40 -0400 Message-ID: <1465132537.14690.20.camel@vmm.sh.intel.com> From: Robert Hu Reply-To: robert.hu@intel.com Date: Sun, 05 Jun 2016 21:15:37 +0800 In-Reply-To: <87vb1ufpdm.fsf@dusky.pond.sub.org> 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> Content-Type: text/plain; charset="ISO-8859-15" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit 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: Markus Armbruster Cc: robert.hu@intel.com, qemu-devel@nongnu.org, pbonzini@redhat.com 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'? > > If we decide users need more information than the current "VNC server > running on" line, perhaps it should be included right in that line.