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

[PATCH] [v3] 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 v2:
 * using ERROR_INVAL instead of ERROR_FAIL in libxl_console_get_tty and
 libxl__primary_console_find if need.
 * remove _out from some value name in libxl_primary_console_exec
 * add error handler and log message in libxl_console_get_tty.
 BUT, NOT update strdup to libxl__strdup. bacause libxl__strdup(0, tty) will lead to null pointer access in CTX maco.
 * add empty line between my comment and other function.

diff -r d7318231cfe3 -r 3496f86000b8 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Thu May 31 10:18:52 2012 +0200
+++ b/tools/libxl/libxl.c	Fri Jun 01 15:10:45 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,25 +1215,82 @@ 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;
+
+    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_INVAL;
+        goto out;
+    }
+
+    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;
+    }
+
+    *path = strdup(tty);
+    if (!*path)
+        libxl__alloc_failed(CTX, __func__, strlen(*path), 1);
+
+    rc = 0;
+
+out:
+    GC_FREE;
+    return rc;
+}
+
+static int libxl__primary_console_find(libxl_ctx *ctx, uint32_t domid_vm, 
+                                       uint32_t *domid, int *cons_num, 
+                                       libxl_console_type *type)
 {
     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 = stubdomid;
+        *cons_num = STUBDOM_CONSOLE_SERIAL;
+        *type = 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 = domid_vm;
+            *cons_num = 0;
+            *type = LIBXL_CONSOLE_TYPE_SERIAL;
             break;
         case LIBXL_DOMAIN_TYPE_PV:
-            rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_PV);
+            *domid = domid_vm;
+            *cons_num = 0;
+            *type = LIBXL_CONSOLE_TYPE_PV;
             break;
         case -1:
-            LOG(ERROR,"unable to get domain type for domid=%"PRIu32,domid_vm);
-            rc = ERROR_FAIL;
+            LOG(ERROR,"unable to get domain type for domid=%"PRIu32, domid_vm);
+            rc = ERROR_INVAL;
             break;
         default:
             abort();
@@ -1242,6 +1300,31 @@ int libxl_primary_console_exec(libxl_ctx
     return rc;
 }
 
+int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
+{
+    uint32_t domid;
+    int cons_num;
+    libxl_console_type type;
+    int rc;
+
+    rc = libxl__primary_console_find(ctx, domid_vm, &domid, &cons_num, &type);
+    if ( rc ) return rc;
+    return libxl_console_exec(ctx, domid, cons_num, type);
+}
+
+int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm,
+                                  char **path)
+{
+    uint32_t domid;
+    int cons_num;
+    libxl_console_type type;
+    int rc;
+
+    rc = libxl__primary_console_find(ctx, domid_vm, &domid, &cons_num, &type);
+    if ( rc ) return rc;
+    return libxl_console_get_tty(ctx, domid, cons_num, type, path);
+}
+
 int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass)
 {
     GC_INIT(ctx);
diff -r d7318231cfe3 -r 3496f86000b8 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Thu May 31 10:18:52 2012 +0200
+++ b/tools/libxl/libxl.h	Fri Jun 01 15:10:45 2012 +0800
@@ -570,6 +570,18 @@ int libxl_console_exec(libxl_ctx *ctx, u
  * 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,
                       uint32_t domid);

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

[Ian Campbell]

On Fri, 2012-06-01 at 08:21 +0100, Bamvor Jian Zhang wrote:
>  BUT, NOT update strdup to libxl__strdup. bacause libxl__strdup(0,
> tty) will lead to null pointer access in CTX maco. 

I haven't looked at the rest of the patch you but thought I'd pick up on
this now since we have at least one existing libxl__strdup(0, ..) in
        b_info->blkdev_start = libxl__strdup(0, "xvda");
        
so we need to decide what to do in general. libxl__strdup wants the CTX
so it can complain via the logs on malloc failure.

This looks like a problem even with the functions which take a
"gc_opt" (i.e. where gc==NULL is explicitly supposed to be allowed).

libxl__alloc_failed() could take a gc_opt instead of a ctx and only log
if it is non-null. It already also logs via stderr, since this is a
catastrophic failure from which we won't return.

We could also have a process-global libxl_alloc_failure_hook(const char
*argh) to allow the calling application to have some chance to log via
its own routines...

IanJ -- you added all this, what do you think?

Ian.

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

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] [v3] libxl: Add API to retrieve domain console tty"):
> On Fri, 2012-06-01 at 08:21 +0100, Bamvor Jian Zhang wrote:
> >  BUT, NOT update strdup to libxl__strdup. bacause libxl__strdup(0,
> > tty) will lead to null pointer access in CTX maco. 
> 
> I haven't looked at the rest of the patch you but thought I'd pick up on
> this now since we have at least one existing libxl__strdup(0, ..) in
>         b_info->blkdev_start = libxl__strdup(0, "xvda");
>         
> so we need to decide what to do in general. libxl__strdup wants the CTX
> so it can complain via the logs on malloc failure.

Damn.  I shouldn't have let you talk me into adding the extra
ctx-based logging, evidently.  I had forgotten the reason for the lack
of that.  (Because it was in the next patch.)

> This looks like a problem even with the functions which take a
> "gc_opt" (i.e. where gc==NULL is explicitly supposed to be allowed).

Yes.

> libxl__alloc_failed() could take a gc_opt instead of a ctx and only log
> if it is non-null. It already also logs via stderr, since this is a
> catastrophic failure from which we won't return.

Yes.

> We could also have a process-global libxl_alloc_failure_hook(const char
> *argh) to allow the calling application to have some chance to log via
> its own routines...

Yes.

> IanJ -- you added all this, what do you think?

I think the above is a good proposal.

Ian.

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

[Ian Campbell]

On Fri, 2012-06-01 at 11:37 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH] [v3] libxl: Add API to retrieve domain console tty"):
> > On Fri, 2012-06-01 at 08:21 +0100, Bamvor Jian Zhang wrote:
> > >  BUT, NOT update strdup to libxl__strdup. bacause libxl__strdup(0,
> > > tty) will lead to null pointer access in CTX maco. 
> > 
> > I haven't looked at the rest of the patch you but thought I'd pick up on
> > this now since we have at least one existing libxl__strdup(0, ..) in
> >         b_info->blkdev_start = libxl__strdup(0, "xvda");
> >         
> > so we need to decide what to do in general. libxl__strdup wants the CTX
> > so it can complain via the logs on malloc failure.
> 
> Damn.  I shouldn't have let you talk me into adding the extra
> ctx-based logging, evidently.  I had forgotten the reason for the lack
> of that.  (Because it was in the next patch.)
> 
> > This looks like a problem even with the functions which take a
> > "gc_opt" (i.e. where gc==NULL is explicitly supposed to be allowed).
> 
> Yes.
> 
> > libxl__alloc_failed() could take a gc_opt instead of a ctx and only log
> > if it is non-null. It already also logs via stderr, since this is a
> > catastrophic failure from which we won't return.
> 
> Yes.
> 
> > We could also have a process-global libxl_alloc_failure_hook(const char
> > *argh) to allow the calling application to have some chance to log via
> > its own routines...
> 
> Yes.
> 
> > IanJ -- you added all this, what do you think?
> 
> I think the above is a good proposal.

Who's going to do it then ;-)

(FWIW I think libxl_alloc_failure_hook is an optional nice to have)

Ian.

Re: [PATCH] [v3] libxl: Add API to retrieve domain console tty [and 2 more messages]

[Ian Jackson]

Bamvor Jian Zhang writes ("[PATCH] [v3] libxl: Add API to retrieve
domain consol
>  BUT, NOT update strdup to libxl__strdup. bacause libxl__strdup(0,
>  tty) will lead to null pointer access in CTX maco.

So once again, well done for spotting this.

During some private discussion with Ian Campbell I came up with the
a proposal to fix it, which I have now implemented in the form of the
patch below.

Please comment.

NB this patch cannot be applied to xen-unstable.hg staging tip.  It
needs to go in the middle of my save/restore series.  I think this is
a sufficiently off-the-mainline bug (it will only show up with
allocation errors) that it's better to avoid introducing any more
merge conflicts than absolutely necessary.  And doing it this way
avoids my series ending up with a tip containing callers which wrongly
pass 0 for gc_opt.

Thanks,
Ian.

From: Ian Jackson <ian.jackson@eu.citrix.com>
Subject: [PATCH] libxl: Do not pass NULL as gc_opt; introduce NOGC

In 25182:6c3345d7e9d9 the practice of passing NULL to gc-using memory
allocation functions was introduced.  However, the arrangements there
were not correct as committed, because the error handling and logging
depends on getting a ctx from the gc - so an allocation error would in
fact result in libxl dereferencing NULL.

Instead, provide a special dummy gc in the ctx, called `nogc_gc'.  It
is marked out specially by having alloc_maxsize==-1, which is
otherwise invalid.

Functions which need to actually look into the gc use the new test
function gc_is_real (whose purpose is mainly clarity of the code) to
check whether the gc is the dummy one, and do nothing if it is.  And
we provide a helper macro NOGC which uses the in-scope real gc to find
the ctx and hence the dummy gc.

Change all callers which pass 0 or NULL to an allocation function to
use NOGC or &ctx->nogc_gc, as applicable in the context.

We add a comment near the definition of LIBXL_INIT_GC pointing out
that it isn't any more the only place a libxl__gc struct is
initialised, for the benefit of anyone changing the contents of gc's
in the future.

Also, actually document that libxl__ptr_add is legal with ptr==NULL,
and change a couple of calls not to check for NULL argument.

Reported-by: Bamvor Jian Zhang <bjzhang@suse.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Bamvor Jian Zhang <bjzhang@suse.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>

---
 tools/libxl/libxl.c          |    3 +++
 tools/libxl/libxl_aoutils.c  |    3 ++-
 tools/libxl/libxl_create.c   |    2 +-
 tools/libxl/libxl_event.c    |    5 +++--
 tools/libxl/libxl_exec.c     |    2 +-
 tools/libxl/libxl_fork.c     |    2 +-
 tools/libxl/libxl_internal.c |   11 +++++++++--
 tools/libxl/libxl_internal.h |   36 +++++++++++++++++++++++-------------
 tools/libxl/libxl_utils.c    |    6 ++----
 tools/libxl/libxl_xshelp.c   |    7 ++-----
 10 files changed, 47 insertions(+), 30 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 53626ae..6a0fa85 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -41,6 +41,9 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
 
     /* First initialise pointers etc. (cannot fail) */
 
+    ctx->nogc_gc.alloc_maxsize = -1;
+    ctx->nogc_gc.owner = ctx;
+
     LIBXL_TAILQ_INIT(&ctx->occurred);
 
     ctx->osevent_hooks = 0;
diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 7f8d6d3..99972a2 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -77,6 +77,7 @@ static void datacopier_check_state(libxl__egc *egc, libxl__datacopier_state *dc)
 void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
                                   const void *data, size_t len)
 {
+    EGC_GC;
     libxl__datacopier_buf *buf;
     /*
      * It is safe for this to be called immediately after _start, as
@@ -88,7 +89,7 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
 
     assert(len < dc->maxsz - dc->used);
 
-    buf = libxl__zalloc(0, sizeof(*buf) - sizeof(buf->buf) + len);
+    buf = libxl__zalloc(NOGC, sizeof(*buf) - sizeof(buf->buf) + len);
     buf->used = len;
     memcpy(buf->buf, data, len);
 
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index eaf378b..431791a 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -107,7 +107,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
     }
 
     if (b_info->blkdev_start == NULL)
-        b_info->blkdev_start = libxl__strdup(0, "xvda");
+        b_info->blkdev_start = libxl__strdup(NOGC, "xvda");
 
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
         if (!b_info->u.hvm.bios)
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 565d2c2..eb23a93 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -772,7 +772,7 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller,
         if (poller->fd_rindices_allocd < maxfd) {
             assert(ARRAY_SIZE_OK(poller->fd_rindices, maxfd));
             poller->fd_rindices =
-                libxl__realloc(0, poller->fd_rindices,
+                libxl__realloc(NOGC, poller->fd_rindices,
                                maxfd * sizeof(*poller->fd_rindices));
             memset(poller->fd_rindices + poller->fd_rindices_allocd,
                    0,
@@ -1099,9 +1099,10 @@ void libxl_event_free(libxl_ctx *ctx, libxl_event *event)
 libxl_event *libxl__event_new(libxl__egc *egc,
                               libxl_event_type type, uint32_t domid)
 {
+    EGC_GC;
     libxl_event *ev;
 
-    ev = libxl__zalloc(0,sizeof(*ev));
+    ev = libxl__zalloc(NOGC,sizeof(*ev));
     ev->type = type;
     ev->domid = domid;
 
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 082bf44..cfa379c 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -280,7 +280,7 @@ int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
     int status, rc;
 
     libxl__spawn_init(ss);
-    ss->ssd = libxl__zalloc(0, sizeof(*ss->ssd));
+    ss->ssd = libxl__zalloc(NOGC, sizeof(*ss->ssd));
     libxl__ev_child_init(&ss->ssd->mid);
 
     rc = libxl__ev_time_register_rel(gc, &ss->timeout,
diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index 9ff99e0..044ddad 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -92,7 +92,7 @@ libxl__carefd *libxl__carefd_record(libxl_ctx *ctx, int fd)
     libxl__carefd *cf = 0;
 
     libxl_fd_set_cloexec(ctx, fd, 1);
-    cf = libxl__zalloc(NULL, sizeof(*cf));
+    cf = libxl__zalloc(&ctx->nogc_gc, sizeof(*cf));
     cf->fd = fd;
     LIBXL_LIST_INSERT_HEAD(&carefds, cf, entry);
     return cf;
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 8139520..fbff7d0 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -30,11 +30,16 @@ void libxl__alloc_failed(libxl_ctx *ctx, const char *func,
 #undef L
 }
 
+static int gc_is_real(const libxl__gc *gc)
+{
+    return gc->alloc_maxsize >= 0;
+}
+
 void libxl__ptr_add(libxl__gc *gc, void *ptr)
 {
     int i;
 
-    if (!gc)
+    if (!gc_is_real(gc))
         return;
 
     if (!ptr)
@@ -66,6 +71,8 @@ void libxl__free_all(libxl__gc *gc)
     void *ptr;
     int i;
 
+    assert(gc_is_real(gc));
+
     for (i = 0; i < gc->alloc_maxsize; i++) {
         ptr = gc->alloc_ptrs[i];
         gc->alloc_ptrs[i] = NULL;
@@ -104,7 +111,7 @@ void *libxl__realloc(libxl__gc *gc, void *ptr, size_t new_size)
 
     if (ptr == NULL) {
         libxl__ptr_add(gc, new_ptr);
-    } else if (new_ptr != ptr && gc != NULL) {
+    } else if (new_ptr != ptr && gc_is_real(gc)) {
         for (i = 0; i < gc->alloc_maxsize; i++) {
             if (gc->alloc_ptrs[i] == ptr) {
                 gc->alloc_ptrs[i] = new_ptr;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 13d2fc1..875aaea 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -277,10 +277,18 @@ struct libxl__poller {
     int wakeup_pipe[2]; /* 0 means no fd allocated */
 };
 
+struct libxl__gc {
+    /* mini-GC */
+    int alloc_maxsize; /* -1 means this is the dummy non-gc gc */
+    void **alloc_ptrs;
+    libxl_ctx *owner;
+};
+
 struct libxl__ctx {
     xentoollog_logger *lg;
     xc_interface *xch;
     struct xs_handle *xsh;
+    libxl__gc nogc_gc;
 
     const libxl_event_hooks *event_hooks;
     void *event_hooks_user;
@@ -356,13 +364,6 @@ typedef struct {
 
 #define PRINTF_ATTRIBUTE(x, y) __attribute__((format(printf, x, y)))
 
-struct libxl__gc {
-    /* mini-GC */
-    int alloc_maxsize;
-    void **alloc_ptrs;
-    libxl_ctx *owner;
-};
-
 struct libxl__egc {
     /* For event-generating functions only.
      * The egc and its gc may be accessed only on the creating thread. */
@@ -420,6 +421,7 @@ struct libxl__ao {
         (gc).alloc_ptrs = 0;                    \
         (gc).owner = (ctx);                     \
     } while(0)
+    /* NB, also, a gc struct ctx->nogc_gc is initialised in libxl_ctx_alloc */
 
 static inline libxl_ctx *libxl__gc_owner(libxl__gc *gc)
 {
@@ -438,13 +440,20 @@ static inline libxl_ctx *libxl__gc_owner(libxl__gc *gc)
  * All pointers returned by these functions are registered for garbage
  * collection on exit from the outermost libxl callframe.
  *
- * However, where the argument is stated to be "gc_opt", NULL may be
- * passed instead, in which case no garbage collection will occur; the
- * pointer must later be freed with free().  This is for memory
- * allocations of types (b) and (c).
+ * However, where the argument is stated to be "gc_opt", &ctx->nogc_gc
+ * may be passed instead, in which case no garbage collection will
+ * occur; the pointer must later be freed with free().  (Passing NULL
+ * for gc_opt is not permitted.)  This is for memory allocations of
+ * types (b) and (c).  The convenience macro NOGC should be used where
+ * possible.
+ *
+ * NOGC (and ctx->nogc_gc) may ONLY be used with functions which
+ * explicitly declare that it's OK.  Use with nonconsenting functions
+ * may result in leaks of those functions' internal allocations on the
+ * psuedo-gc.
  */
-/* register @ptr in @gc for free on exit from outermost libxl callframe. */
-_hidden void libxl__ptr_add(libxl__gc *gc_opt, void *ptr);
+/* register ptr in gc for free on exit from outermost libxl callframe. */
+_hidden void libxl__ptr_add(libxl__gc *gc_opt, void *ptr /* may be NULL */);
 /* if this is the outermost libxl callframe then free all pointers in @gc */
 _hidden void libxl__free_all(libxl__gc *gc);
 /* allocate and zero @bytes. (similar to a gc'd malloc(3)+memzero()) */
@@ -2111,6 +2120,7 @@ _hidden const char *libxl__device_model_savefile(libxl__gc *gc, uint32_t domid);
 #define GC_INIT(ctx)  libxl__gc gc[1]; LIBXL_INIT_GC(gc[0],ctx)
 #define GC_FREE       libxl__free_all(gc)
 #define CTX           libxl__gc_owner(gc)
+#define NOGC          (&CTX->nogc_gc) /* pass only to consenting functions */
 
 /* Allocation macros all of which use the gc. */
 
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 2382c9d..3a5f292 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -58,8 +58,7 @@ char *libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid)
 char *libxl__domid_to_name(libxl__gc *gc, uint32_t domid)
 {
     char *s = libxl_domid_to_name(libxl__gc_owner(gc), domid);
-    if ( s )
-        libxl__ptr_add(gc, s);
+    libxl__ptr_add(gc, s);
     return s;
 }
 
@@ -107,8 +106,7 @@ char *libxl_cpupoolid_to_name(libxl_ctx *ctx, uint32_t poolid)
 char *libxl__cpupoolid_to_name(libxl__gc *gc, uint32_t poolid)
 {
     char *s = libxl_cpupoolid_to_name(libxl__gc_owner(gc), poolid);
-    if ( s )
-        libxl__ptr_add(gc, s);
+    libxl__ptr_add(gc, s);
     return s;
 }
 
diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
index 993f527..7fdf164 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -86,11 +86,8 @@ char * libxl__xs_read(libxl__gc *gc, xs_transaction_t t, const char *path)
     char *ptr;
 
     ptr = xs_read(ctx->xsh, t, path, NULL);
-    if (ptr != NULL) {
-        libxl__ptr_add(gc, ptr);
-        return ptr;
-    }
-    return 0;
+    libxl__ptr_add(gc, ptr);
+    return ptr;
 }
 
 char *libxl__xs_get_dompath(libxl__gc *gc, uint32_t domid)
-- 
tg: (e2dea8e..) t/xen/xl.nogc (depends on: t/xen/xl.savefile.async)

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

[Ian Campbell]

On Fri, 2012-06-01 at 08:21 +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 v2:
>  * using ERROR_INVAL instead of ERROR_FAIL in libxl_console_get_tty and
>  libxl__primary_console_find if need.
>  * remove _out from some value name in libxl_primary_console_exec
>  * add error handler and log message in libxl_console_get_tty.
>  BUT, NOT update strdup to libxl__strdup. bacause libxl__strdup(0, tty) will lead to null pointer access in CTX maco.
>  * add empty line between my comment and other function.
> 
> diff -r d7318231cfe3 -r 3496f86000b8 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Thu May 31 10:18:52 2012 +0200
> +++ b/tools/libxl/libxl.c	Fri Jun 01 15:10:45 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,25 +1215,82 @@ 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;

Is the compiler giving false positives about using these without
initialising them?

Otherwise these initialisations are simply hiding what would a useful
warning from the compiler.

Or is there some deliberate path by which these can be used without
first being set to something explicit?

> +    int rc;
> +
> +    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_INVAL;
> +        goto out;
> +    }
> +
> +    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;
> +    }
> +
> +    *path = strdup(tty);
> +    if (!*path)
> +        libxl__alloc_failed(CTX, __func__, strlen(*path), 1);
> +
> +    rc = 0;
> +
> +out:
> +    GC_FREE;
> +    return rc;
> +}
> +
> +static int libxl__primary_console_find(libxl_ctx *ctx, uint32_t domid_vm, 
> +                                       uint32_t *domid, int *cons_num, 
> +                                       libxl_console_type *type)
>  {
>      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;

This initialisation also potentially hides errors, I think. Better to
explicitly set in the cases where we succeed.

> +
> +    if (stubdomid) {
> +        *domid = stubdomid;
> +        *cons_num = STUBDOM_CONSOLE_SERIAL;
> +        *type = 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 = domid_vm;
> +            *cons_num = 0;
> +            *type = LIBXL_CONSOLE_TYPE_SERIAL;
>              break;
>          case LIBXL_DOMAIN_TYPE_PV:
> -            rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_PV);
> +            *domid = domid_vm;
> +            *cons_num = 0;
> +            *type = LIBXL_CONSOLE_TYPE_PV;
>              break;
>          case -1:
> -            LOG(ERROR,"unable to get domain type for domid=%"PRIu32,domid_vm);
> -            rc = ERROR_FAIL;
> +            LOG(ERROR,"unable to get domain type for domid=%"PRIu32, domid_vm);
> +            rc = ERROR_INVAL;
>              break;
>          default:
>              abort();
> @@ -1242,6 +1300,31 @@ int libxl_primary_console_exec(libxl_ctx
>      return rc;
>  }
>