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

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@eu.citrix.com>
# Date 1332257809 0
# Node ID 46f8afe643dee8de2c592c65204567fbad657616
# Parent  4e1d091d10d83130842170cd61f1194e5459f2aa
Add vncviewer xm compatibility options the ''xl create'' command

Signed-off-by: Goncalo Gomes <goncalo.gomes@eu.citrix.com>

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''},
+
         {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)  {
+        dom = libxl_domid_to_name(&ctx, rc);
+        if (dom) {
+            rc = vncviewer(dom, vncautopass);
+            if (!rc) {
+                goto cleanup;
+            } else {
+               rc = 1;
+              goto out;
+            }
+        }
+    }
+
+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@lists.xen.org
http://lists.xen.org/xen-devel

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@eu.citrix.com>
> # 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@eu.citrix.com>
> 
> 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,
> 
> 
>

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@eu.citrix.com>
> > # 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@eu.citrix.com>
> > 
> > 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,
> > 
> > 
> > 
> 
>

On Tue, 2012-03-20 at 16:43 +0000, Goncalo Gomes wrote:
> 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@eu.citrix.com>
> > > # 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? 
I don''t see -A or -V in xend but looking again I do see the long forms,
I guess that''s fine.
> > 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.
I can''t see anything in xl which would do this, are you sure you
don''t
mean with xm?
> Do you have a pointer to the original request?
it was Jim Burns posting on xen-devel in February, bullet 3 in
http://lists.xen.org/archives/html/xen-devel/2012-02/msg01364.html

[...]
> > 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?
Yes. This should make it trivial to support for xl restore too.

Ian.

On Tue, 20 Mar 2012, Ian Campbell wrote:
> On Tue, 2012-03-20 at 16:43 +0000, Goncalo Gomes wrote:
> > 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@eu.citrix.com>
> > > > # 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? 
> 
> I don''t see -A or -V in xend but looking again I do see the long
forms,
> I guess that''s fine.
> 
> > > 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.
> 
> I can''t see anything in xl which would do this, are you sure you
don''t
> mean with xm?
The problem was that I initialized vnc and vncautopass with a value of 
1 when I originally wrote the code, which would always make the 
libxl_vncviewer_exec codepath to take place and forgot to change it 
back. I will add the support code to the config file to support this 
officially.
 
> > Do you have a pointer to the original request?
> 
> it was Jim Burns posting on xen-devel in February, bullet 3 in
> http://lists.xen.org/archives/html/xen-devel/2012-02/msg01364.html
Thanks!
> [...]
> > > 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?
> 
> Yes. This should make it trivial to support for xl restore too.
I''ll move it to that code path.

Goncalo
 
> Ian.
>

On Tue, 2012-03-20 at 20:00 +0000, Goncalo Gomes wrote:

Hi Goncalo,

are you still looking into this?

Ian.