[PATCH] [v2] libxl: Add API to retrieve domain console tty

[PATCH] [v2] libxl: Add API to retrieve domain console tty

[Bamvor Jian Zhang]

This api retrieve domain console from xenstore. With this new api, it is easy to implement "virsh console" command in libvirt libxl driver.

Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com>

Changes since v1:
 * Replace function libxl__sprintf with macro GSPRINTF
 * check return value and handle error for libxl__xs_read and libxl__domain_type
 * merge common code for libxl_primary_console_get_tty and
   libxl_primary_console_exec as libxl__primary_console_find
 * Reformat code to be less than 80 characters.

diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Tue May 22 11:55:02 2012 +0100
+++ b/tools/libxl/libxl.c	Wed May 23 14:27:57 2012 +0800
@@ -1188,7 +1188,8 @@ out:
     return rc;
 }
 
-int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num, libxl_console_type type)
+int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
+                       libxl_console_type type)
 {
     GC_INIT(ctx);
     char *p = libxl__sprintf(gc, "%s/xenconsole", libxl__private_bindir_path());
@@ -1214,24 +1215,74 @@ out:
     return ERROR_FAIL;
 }
 
-int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
+int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num, 
+                          libxl_console_type type, char **path)
+{
+    GC_INIT(ctx);
+    char *dom_path = 0;
+    char *tty_path = 0;
+    char *tty = 0;
+    int rc = 0;
+
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    if (!dom_path) {
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    switch (type) {
+    case LIBXL_CONSOLE_TYPE_SERIAL:
+        tty_path = GCSPRINTF("%s/serial/0/tty", dom_path);
+        break;
+    case LIBXL_CONSOLE_TYPE_PV:
+        if (cons_num == 0)
+            tty_path = GCSPRINTF("%s/console/tty", dom_path);
+        else
+            tty_path = GCSPRINTF("%s/device/console/%d/tty", dom_path, 
+                                cons_num);
+        break;
+    default:
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    tty = libxl__xs_read(gc, XBT_NULL, tty_path);
+    if (tty) 
+        *path = strdup(tty);
+    else
+        rc = ERROR_FAIL;
+
+out:
+    GC_FREE;
+    return rc;
+}
+
+static int libxl__primary_console_find(libxl_ctx *ctx, uint32_t domid_vm, 
+                                       uint32_t *domid_out, int *cons_num_out, 
+                                       libxl_console_type *type_out)
 {
     GC_INIT(ctx);
     uint32_t stubdomid = libxl_get_stubdom_id(ctx, domid_vm);
-    int rc;
-    if (stubdomid)
-        rc = libxl_console_exec(ctx, stubdomid,
-                                STUBDOM_CONSOLE_SERIAL, LIBXL_CONSOLE_TYPE_PV);
-    else {
+    int rc = 0;
+
+    if (stubdomid) {
+        *domid_out = stubdomid;
+        *cons_num_out = STUBDOM_CONSOLE_SERIAL;
+        *type_out = LIBXL_CONSOLE_TYPE_PV;
+    } else {
         switch (libxl__domain_type(gc, domid_vm)) {
         case LIBXL_DOMAIN_TYPE_HVM:
-            rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_SERIAL);
+            *domid_out = domid_vm;
+            *cons_num_out = 0;
+            *type_out = LIBXL_CONSOLE_TYPE_SERIAL;
             break;
         case LIBXL_DOMAIN_TYPE_PV:
-            rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_PV);
+            *domid_out = domid_vm;
+            *cons_num_out = 0;
+            *type_out = LIBXL_CONSOLE_TYPE_PV;
             break;
         case -1:
-            LOG(ERROR,"unable to get domain type for domid=%"PRIu32,domid_vm);
+            LOG(ERROR,"unable to get domain type for domid=%"PRIu32, domid_vm);
             rc = ERROR_FAIL;
             break;
         default:
@@ -1242,6 +1293,33 @@ int libxl_primary_console_exec(libxl_ctx
     return rc;
 }
 
+int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
+{
+    uint32_t domid_out;
+    int cons_num_out;
+    libxl_console_type type_out;
+    int rc;
+
+    rc = libxl__primary_console_find(ctx, domid_vm, &domid_out, &cons_num_out, 
+                                     &type_out);
+    if ( rc ) return rc;
+    return libxl_console_exec(ctx, domid_out, cons_num_out, type_out);
+}
+
+int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm,
+                                  char **path)
+{
+    uint32_t domid_out;
+    int cons_num_out;
+    libxl_console_type type_out;
+    int rc;
+
+    rc = libxl__primary_console_find(ctx, domid_vm, &domid_out, &cons_num_out, 
+                                     &type_out);
+    if ( rc ) return rc;
+    return libxl_console_get_tty(ctx, domid_out, cons_num_out, type_out, path);
+}
+
 int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
 {
     GC_INIT(ctx);
diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Tue May 22 11:55:02 2012 +0100
+++ b/tools/libxl/libxl.h	Wed May 23 14:27:57 2012 +0800
@@ -601,6 +601,16 @@ int libxl_console_exec(libxl_ctx *ctx, u
  * case of HVM guests, and before libxl_run_bootloader in case of PV
  * guests using pygrub. */
 int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm);
+/* libxl_console_get_tty retrieves the specified domain's console tty path
+ * and stores it in path. Caller is responsible for freeing the memory.
+ */
+int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num, 
+                          libxl_console_type type, char **path);
+/* libxl_primary_console_get_tty retrieves the specified domain's primary 
+ * console tty path and stores it in path. Caller is responsible for freeing
+ * the memory.
+ */
+int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm, char **path);
 
 /* May be called with info_r == NULL to check for domain's existance */
 int libxl_domain_info(libxl_ctx*, libxl_dominfo *info_r,

Re: [PATCH] [v2] libxl: Add API to retrieve domain console tty

[Ian Campbell]

On Wed, 2012-05-23 at 07:35 +0100, Bamvor Jian Zhang wrote:
> This api retrieve domain console from xenstore. With this new api, it is easy to implement "virsh console" command in libvirt libxl driver.
> 
> Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com>
> 
> Changes since v1:
>  * Replace function libxl__sprintf with macro GSPRINTF
>  * check return value and handle error for libxl__xs_read and libxl__domain_type
>  * merge common code for libxl_primary_console_get_tty and
>    libxl_primary_console_exec as libxl__primary_console_find
>  * Reformat code to be less than 80 characters.
> 
> diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Tue May 22 11:55:02 2012 +0100
> +++ b/tools/libxl/libxl.c	Wed May 23 14:27:57 2012 +0800
> @@ -1188,7 +1188,8 @@ out:
>      return rc;
>  }
>  
> -int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num, libxl_console_type type)
> +int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
> +                       libxl_console_type type)
>  {
>      GC_INIT(ctx);
>      char *p = libxl__sprintf(gc, "%s/xenconsole", libxl__private_bindir_path());
> @@ -1214,24 +1215,74 @@ out:
>      return ERROR_FAIL;
>  }
>  
> -int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
> +int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num, 
> +                          libxl_console_type type, char **path)
> +{
> +    GC_INIT(ctx);
> +    char *dom_path = 0;
> +    char *tty_path = 0;
> +    char *tty = 0;
> +    int rc = 0;
> +
> +    dom_path = libxl__xs_get_dompath(gc, domid);
> +    if (!dom_path) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    switch (type) {
> +    case LIBXL_CONSOLE_TYPE_SERIAL:
> +        tty_path = GCSPRINTF("%s/serial/0/tty", dom_path);
> +        break;
> +    case LIBXL_CONSOLE_TYPE_PV:
> +        if (cons_num == 0)
> +            tty_path = GCSPRINTF("%s/console/tty", dom_path);
> +        else
> +            tty_path = GCSPRINTF("%s/device/console/%d/tty", dom_path, 
> +                                cons_num);
> +        break;
> +    default:
> +        rc = ERROR_FAIL;

Strictly this ought to be ERROR_INVAL.

> +        goto out;
> +    }
> +
> +    tty = libxl__xs_read(gc, XBT_NULL, tty_path);
> +    if (tty) 
> +        *path = strdup(tty);
> +    else
> +        rc = ERROR_FAIL;
> +
> +out:
> +    GC_FREE;
> +    return rc;
> +}
> +
> +static int libxl__primary_console_find(libxl_ctx *ctx, uint32_t domid_vm, 

Generally since this is an internal function it should take a libxl__gc
*gc not a ctx and drop the GC_INIT and GC_FREE. You can use the CTX
macro to get a ctx where you need one.

However since the two callers are thinish wrappers around this and you'd
then need the GC_INIT, GC_FREE stuff there I'm inclined to suggest we
make an exception here and leave it as is, Ian J what do you think?

> +                                       uint32_t *domid_out, int *cons_num_out, 
> +                                       libxl_console_type *type_out)
>  {
>      GC_INIT(ctx);
>      uint32_t stubdomid = libxl_get_stubdom_id(ctx, domid_vm);
> -    int rc;
> -    if (stubdomid)
> -        rc = libxl_console_exec(ctx, stubdomid,
> -                                STUBDOM_CONSOLE_SERIAL, LIBXL_CONSOLE_TYPE_PV);
> -    else {
> +    int rc = 0;
> +
> +    if (stubdomid) {
> +        *domid_out = stubdomid;
> +        *cons_num_out = STUBDOM_CONSOLE_SERIAL;
> +        *type_out = LIBXL_CONSOLE_TYPE_PV;
> +    } else {
>          switch (libxl__domain_type(gc, domid_vm)) {
>          case LIBXL_DOMAIN_TYPE_HVM:
> -            rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_SERIAL);
> +            *domid_out = domid_vm;
> +            *cons_num_out = 0;
> +            *type_out = LIBXL_CONSOLE_TYPE_SERIAL;
>              break;
>          case LIBXL_DOMAIN_TYPE_PV:
> -            rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_PV);
> +            *domid_out = domid_vm;
> +            *cons_num_out = 0;
> +            *type_out = LIBXL_CONSOLE_TYPE_PV;
>              break;
>          case -1:
> -            LOG(ERROR,"unable to get domain type for domid=%"PRIu32,domid_vm);
> +            LOG(ERROR,"unable to get domain type for domid=%"PRIu32, domid_vm);
>              rc = ERROR_FAIL;
>              break;
>          default:
> @@ -1242,6 +1293,33 @@ int libxl_primary_console_exec(libxl_ctx
>      return rc;
>  }
>  
> +int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
> +{
> +    uint32_t domid_out;
> +    int cons_num_out;
> +    libxl_console_type type_out;

These aren't really _out vars here...

There isn't much here which warrants a resend IMHO, if Ian J is happy
with the libxl__primary_console_find interface (as discussed above) I'd
be inclined to take it as is unless you really want to do a respin.

> +    int rc;
> +
> +    rc = libxl__primary_console_find(ctx, domid_vm, &domid_out, &cons_num_out, 
> +                                     &type_out);
> +    if ( rc ) return rc;
> +    return libxl_console_exec(ctx, domid_out, cons_num_out, type_out);
> +}
> +
> +int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm,
> +                                  char **path)
> +{
> +    uint32_t domid_out;
> +    int cons_num_out;
> +    libxl_console_type type_out;
> +    int rc;
> +
> +    rc = libxl__primary_console_find(ctx, domid_vm, &domid_out, &cons_num_out, 
> +                                     &type_out);
> +    if ( rc ) return rc;
> +    return libxl_console_get_tty(ctx, domid_out, cons_num_out, type_out, path);
> +}
> +
>  int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
>  {
>      GC_INIT(ctx);
> diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h	Tue May 22 11:55:02 2012 +0100
> +++ b/tools/libxl/libxl.h	Wed May 23 14:27:57 2012 +0800
> @@ -601,6 +601,16 @@ int libxl_console_exec(libxl_ctx *ctx, u
>   * case of HVM guests, and before libxl_run_bootloader in case of PV
>   * guests using pygrub. */
>  int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm);
> +/* libxl_console_get_tty retrieves the specified domain's console tty path
> + * and stores it in path. Caller is responsible for freeing the memory.
> + */
> +int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num, 
> +                          libxl_console_type type, char **path);
> +/* libxl_primary_console_get_tty retrieves the specified domain's primary 
> + * console tty path and stores it in path. Caller is responsible for freeing
> + * the memory.
> + */
> +int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm, char **path);
>  
>  /* May be called with info_r == NULL to check for domain's existance */
>  int libxl_domain_info(libxl_ctx*, libxl_dominfo *info_r,

Re: [PATCH] [v2] libxl: Add API to retrieve domain console tty

[Ian Jackson]

Bamvor Jian Zhang writes ("[PATCH] [v2] libxl: Add API to retrieve domain console tty"):
> This api retrieve domain console from xenstore. With this new api, it is easy to implement "virsh console" command in libvirt libxl driver.

It's looking pretty good.

> diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Tue May 22 11:55:02 2012 +0100
> +++ b/tools/libxl/libxl.c	Wed May 23 14:27:57 2012 +0800
> @@ -1188,7 +1188,8 @@ out:
...
> -int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
> +int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num, 
> +                          libxl_console_type type, char **path)
> +{
...
> +    tty = libxl__xs_read(gc, XBT_NULL, tty_path);
> +    if (tty) 
> +        *path = strdup(tty);
> +    else
> +        rc = ERROR_FAIL;

Firstly, you need to log something on error here or this function will
fail without logging anything if the console is broken.

Secondly, you should use
    libxl__strdup(0, tty)
to get the right error behaviour (if strdup's malloc fails).  Ie,

Thirdly, this is a rather odd error handling pattern.  I would write
       tty = libxl__xs_read(gc, XBT_NULL, tty_path);
       if (!tty) {
           LOGE(ERROR,"unable to read console tty path `%s'",tty_path);
           rc = ERROR_FAIL;
           goto out;
       }
leaving the main flow to carry on:
       *path = libxl__strdup(0, tty);
       rc = 0;

Fourthly, you can then leave rc uninitialised at the top of the
function.  Any path which as a result gets to the exit with it
uninitialised will produce a compiler warning which would previously
have been erroneously suppressed.

> +static int libxl__primary_console_find(libxl_ctx *ctx, uint32_t domid_vm, 
> +                                       uint32_t *domid_out, int *cons_num_out, 
> +                                       libxl_console_type *type_out)
>  {
...
>              break;
>          case -1:
> -            LOG(ERROR,"unable to get domain type for domid=%"PRIu32,domid_vm);
> +            LOG(ERROR,"unable to get domain type for domid=%"PRIu32, domid_vm);

Unrelated whitespace change.

> +int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
> +{
> +    uint32_t domid_out;
> +    int cons_num_out;
> +    libxl_console_type type_out;

As Ian C says, these shouldn't be called "*_out".

> diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h	Tue May 22 11:55:02 2012 +0100
> +++ b/tools/libxl/libxl.h	Wed May 23 14:27:57 2012 +0800
> @@ -601,6 +601,16 @@ int libxl_console_exec(libxl_ctx *ctx, u
>   * case of HVM guests, and before libxl_run_bootloader in case of PV
>   * guests using pygrub. */
>  int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm);
> +/* libxl_console_get_tty retrieves the specified domain's console tty path
> + * and stores it in path. Caller is responsible for freeing the memory.
> + */
> +int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num, 
> +                          libxl_console_type type, char **path);
> +/* libxl_primary_console_get_tty retrieves the specified domain's primary 
> + * console tty path and stores it in path. Caller is responsible for freeing
> + * the memory.
> + */
> +int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm, char **path);
>  
>  /* May be called with info_r == NULL to check for domain's existance */
>  int libxl_domain_info(libxl_ctx*, libxl_dominfo *info_r,

A formatting nit: I think if we're going to have multi-line comments
associated with functions we should have a blank line either side of
the information about the function to visually associate the comment
with the right function.

Thanks,
Ian.

Re: [PATCH] [v2] libxl: Add API to retrieve domain console tty

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] [v2] libxl: Add API to retrieve domain console tty"):
> On Wed, 2012-05-23 at 07:35 +0100, Bamvor Jian Zhang wrote:
> > This api retrieve domain console from xenstore. With this new api, it is easy to implement "virsh console" command in libvirt libxl driver.
> > 
> > Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com>
> > 
> > Changes since v1:
> >  * Replace function libxl__sprintf with macro GSPRINTF
> >  * check return value and handle error for libxl__xs_read and libxl__domain_type
> >  * merge common code for libxl_primary_console_get_tty and
> >    libxl_primary_console_exec as libxl__primary_console_find
> >  * Reformat code to be less than 80 characters.
> > 
> > diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c	Tue May 22 11:55:02 2012 +0100
> > +++ b/tools/libxl/libxl.c	Wed May 23 14:27:57 2012 +0800
> > @@ -1188,7 +1188,8 @@ out:
...
> > -int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
> > +int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num, 
> > +                          libxl_console_type type, char **path)
> > +{
...
> > +    switch (type) {
> > +    case LIBXL_CONSOLE_TYPE_SERIAL:
...
> > +    default:
> > +        rc = ERROR_FAIL;
> 
> Strictly this ought to be ERROR_INVAL.

Yes.

> > +static int libxl__primary_console_find(libxl_ctx *ctx, uint32_t domid_vm, 
> 
> Generally since this is an internal function it should take a libxl__gc
> *gc not a ctx and drop the GC_INIT and GC_FREE. You can use the CTX
> macro to get a ctx where you need one.

I think this is fine.  In fact in the future we might want to make
this a public function (but not now).

> However since the two callers are thinish wrappers around this and you'd
> then need the GC_INIT, GC_FREE stuff there I'm inclined to suggest we
> make an exception here and leave it as is, Ian J what do you think?

I think there is no rule against internal functions taking ctx.  gc is
more conventional but here I think the balance of convenience is in
favour of what Bamvor has done.

Particularly since the outer two functions are so simple.

> There isn't much here which warrants a resend IMHO, if Ian J is happy
> with the libxl__primary_console_find interface (as discussed above) I'd
> be inclined to take it as is unless you really want to do a respin.

I think my comment about the log message ought to be addressed with a
resend.

The nits could have been fixed in-tree at our leisure but if we're
going to have a resend it would be best to address them all.

Thanks,
Ian.