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

[Robert Hu]

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'?
> 
> If we decide users need more information than the current "VNC server
> running on" line, perhaps it should be included right in that line.