Re: [Xen-devel] [PATCH] Add vncviewer xm compatibility options the 'xl create' command

[Goncalo Gomes <Goncalo.Gomes@xxxxxxxxxxxxx>]

On Tue, 20 Mar 2012, Ian Campbell wrote:

> On Tue, 2012-03-20 at 15:50 +0000, Goncalo Gomes wrote:
> > I've attached the preliminary patch to add vncviewer options to the 
> > 'xl create'. It applies cleanly against c/s 4e1d091d10d8. All feedback 
> > is welcome.
> > 
> > Goncalo
> > 
> > # HG changeset patch
> > # User Goncalo Gomes <goncalo.gomes@xxxxxxxxxxxxx>
> > # Date 1332257809 0
> > # Node ID 46f8afe643dee8de2c592c65204567fbad657616
> > # Parent  4e1d091d10d83130842170cd61f1194e5459f2aa
> > Add vncviewer xm compatibility options the 'xl create' command
> 
> Thanks. Are these options actually compatible with "xm create"? Are 

I suppose so. Anything specifically that makes you think otherwise? 

> you intending to also add the vncviewer option to the config file? 
> (I think that was a feature of xm mentioned by the original 
> requester of this functionality).

Do you mean having an option like vncviewer=1 in the config file which 
would execute the vnc client automatically upon domain creation?  If 
so, I think someone may already have implemented that, as I noticed it 
worked for me, even without adding the said code, but I will do some 
more testing around this just to be sure. Do you have a pointer to the 
original request?
 
> Please can you also patch docs/man/xl.pod.1 and/or xl.cfg.pod.5 as
> appropriate.

Yup, will send it along with my second attempt.


> Unfortunately your patch appears to have been whitespace wrapped so it
> won't apply. http://wiki.xen.org/wiki/SubmittingXenPatches contains some
> tips for using "hg email" which can avoid this, otherwise you could try
> Documentation/email-clients.txt from the Linux source for help (I'd give
> you a direct link, but git.kernel.org seems to be down), last resort you
> can attach the file (but also include a copy inline to aid review)
> 
> Also please can you add 
>         [diff]
>         showfunc = True
> to ~/.hgrc (it makes function names show up after the @@ lines which
> aids review)
> 
> Some review of the code follows.
> 
> Thanks,
> Ian.
> 
> > 
> > Signed-off-by: Goncalo Gomes <goncalo.gomes@xxxxxxxxxxxxx>
> > 
> > diff -r 4e1d091d10d8 -r 46f8afe643de tools/libxl/xl_cmdimpl.c
> > --- a/tools/libxl/xl_cmdimpl.c  Fri Mar 16 15:24:25 2012 +0000
> > +++ b/tools/libxl/xl_cmdimpl.c  Tue Mar 20 15:36:49 2012 +0000
> > @@ -1347,6 +1347,8 @@
> >      int dryrun;
> >      int quiet;
> >      int console_autoconnect;
> > +    int vncviewer;
> > +    int vncviewer_autopass;
> >      const char *config_file;
> >      const char *extra_config; /* extra config string */
> >      const char *restore_file;
> > @@ -3306,11 +3308,12 @@
> >  int main_create(int argc, char **argv)
> >  {
> >      const char *filename = NULL;
> > +    char *dom = NULL;
> >      char *p;
> >      char extra_config[1024];
> >      struct domain_create dom_info;
> >      int paused = 0, debug = 0, daemonize = 1, console_autoconnect = 
> > 0,
> > -        quiet = 0, monitor = 1;
> > +        quiet = 0, monitor = 1, vnc = 1, vncautopass = 1;
> >      int opt, rc;
> >      int option_index = 0;
> >      static struct option long_options[] = {
> > @@ -3318,6 +3321,9 @@
> >          {"quiet", 0, 0, 'q'},
> >          {"help", 0, 0, 'h'},
> >          {"defconfig", 1, 0, 'f'},
> > +        {"vncviewer", 0, 0, 'V'},
> > +        {"vncviewer-autopass", 0, 0, 'A'},
> 
> Here you add 'V' and 'A' but later (in the switch) on you look for 'v'
> and 'a'.

True, I was original using 'v' and 'a', but after re-reading your 
email, which suggests '-V', I changed those without much care for the 
switch. Good catch!
 
> It would be useful to have these options for restore too?
> 
> > +
> 
> Mind the extra whitespace here.
> 
> >          {0, 0, 0, 0}
> >      };
> >  
> > @@ -3327,7 +3333,7 @@
> >      }
> >  
> >      while (1) {
> > -        opt = getopt_long(argc, argv, "Fhnqf:pcde", long_options, 
> > &option_index);
> > +        opt = getopt_long(argc, argv, "Fhnqf:pcdeVA", long_options, 
> > &option_index);
> >          if (opt == -1)
> >              break;
> >  
> > @@ -3360,6 +3366,12 @@
> >          case 'q':
> >              quiet = 1;
> >              break;
> >              break;
> > +        case 'v':
> > +            vnc = 1;
> > +            break;
> > +        case 'a':
> > +            vnc = vncautopass = 1;
> > +            break;
> >          default:
> >              fprintf(stderr, "option `%c' not supported.\n", optopt);
> >              break;
> > @@ -3391,12 +3403,40 @@
> >      dom_info.migrate_fd = -1;
> >      dom_info.console_autoconnect = console_autoconnect;
> >      dom_info.incr_generationid = 0;
> > +    dom_info.vncviewer = vnc;
> > +    dom_info.vncviewer_autopass = vncautopass;
> > +
> >  
> >      rc = create_domain(&dom_info);
> >      if (rc < 0)
> >          return -rc;
> >  
> > -    return 0;
> > +    if (vnc && dryrun) {
> > +        printf("vncviewer not being executed for a dryrun\n");
> > +        goto out;
> > +    } 
> > +
> > +    if (vnc)  {
> 
> When you seeded dom_info with the necessary fields I expected that
> create_domain would do this based on those fields, I think that is
> better than doing it here.

So you are suggesting me to move this code inside the create_domain() 
path?

> > +        dom = libxl_domid_to_name(&ctx, rc);
> > +        if (dom) {
> > +            rc = vncviewer(dom, vncautopass);
> > +            if (!rc) {
> > +                goto cleanup;
> > +            } else {
> > +               rc = 1;
> > +              goto out;
> 
> Mind your indentation and/or hard tabs please.

Thanks! Will double-check with the next revision.

Goncalo
 
> > +            }
> > +        }
> > +    }
> > +
> > +cleanup:
> > +    if (dom) {
> > +        free(dom);
> > +        dom = NULL;
> > +    }
> > +
> > +out:
> > +    return rc;
> >  }
> >  
> >  static void button_press(const char *p, const char *b)
> > diff -r 4e1d091d10d8 -r 46f8afe643de tools/libxl/xl_cmdtable.c
> > --- a/tools/libxl/xl_cmdtable.c Fri Mar 16 15:24:25 2012 +0000
> > +++ b/tools/libxl/xl_cmdtable.c Tue Mar 20 15:36:49 2012 +0000
> > @@ -31,6 +31,11 @@
> >        "                         (deprecated in favour of global -N 
> > option).\n"
> >        "-d                      Enable debug messages.\n"
> >        "-e                      Do not wait in the background for the 
> > death of the domain."
> > +      "-V, --vncviewer         Connect to the VNC display after the 
> > domain is created.\n"
> > +      "-A, --vncviewer-autopass\n"
> > +      "                        Pass VNC password to viewer via stdin 
> > and -autopass.\n"
> > +      "--autopass              (xm compatibility)\n"
> > +
> >      },
> >      { "list",
> >        &main_list, 0,
> > 
> > 
> > 
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel