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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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 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).

Please can you also patch docs/man/xl.pod.1 and/or xl.cfg.pod.5 as
appropriate.

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'.

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.

> +        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.

> +            }
> +        }
> +    }
> +
> +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