[PATCH 0/6] libxl, xl: fix domain name uniqueness check

[PATCH 0/6] libxl, xl: fix domain name uniqueness check

[Ian Jackson]

We knew that checking for the domain name uniqueness in
parse_config_data in xl was wrong.  Sadly not only is it wrong, but it
doesn't work properly because it prevents the parsing of domain config
files in various other contextx.

Instead, in this series, we put this check in libxl_domain_rename,
which is the right place for it to be.  This function is indeed called
during domain creation to set the domain's name at the start.  As a
sop to those who think domain names are not important, we allow any
number of domains with the name "".  This is not recommended.

However, after moving this check down into the bowels of libxl domain
creation, it became obvious during testing that the error paths with
are taken when the check fails didn't work properly and generated a
lot of noisy output.

So in this series:
  patch 1:     revert the original broken check
  patches 2-5: error path fixes
  patch 6:     add the check to libxl_domain_rename

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH 1/6] xl: Revert "xl: avoid creating domains with duplicate names"

[Ian Jackson]

This reverts commit 22820:310cc33bfc81.  This functionality should not
be in the domain parsing logic.  It needs to be in libxl_domain_make.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 76c6408..5826755 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -583,7 +583,6 @@ static void parse_config_data(const char *configfile_filename_report,
     XLU_ConfigList *vbds, *nics, *pcis, *cvfbs, *net2s, *cpuids;
     int pci_power_mgmt = 0;
     int pci_msitranslate = 1;
-    uint32_t domid_e;
     int e;
 
     libxl_domain_create_info *c_info = &d_config->c_info;
@@ -613,15 +612,6 @@ static void parse_config_data(const char *configfile_filename_report,
 
     if (xlu_cfg_replace_string (config, "name", &c_info->name))
         c_info->name = strdup("test");
-    e = libxl_name_to_domid(&ctx, c_info->name, &domid_e);
-    if (!e) {
-        fprintf(stderr, "A domain with name \"%s\" already exists.\n", c_info->name);
-        exit(1);
-    }
-    if (e != ERROR_INVAL) {
-        fprintf(stderr, "Unexpected error checking for existing domain"
-                " (error=%d)", e);
-    }
 
     if (!xlu_cfg_get_string (config, "uuid", &buf) ) {
         if ( libxl_uuid_from_string(&c_info->uuid, buf) ) {
-- 
1.5.6.5

[PATCH 2/6] libxl: fix error handling (xenstore transaction leak) in libxl__domain_make

[Ian Jackson]

libxl__domain_make could under some circumstances leak the xenstore
transaction (stored in the variable t).  Also, failures to commit the
xenstore transaction for reasons other than EAGAIN would be ignored (!)

Fix this as follows:

  * Initialise t to 0 (not a valid transaction id), and when the
    transaction is successfully committed or rolled back, reset it.

  * Change all the instances of:       libxl__free_all(&gc); return error;
    to instead do:                     rc=error;  goto out;

  * Use the out stanza for exiting, setting rc=0 on success first.

  * Explicitly abort the transaction in the out stanza.

Also add a note by the calls manipulating the gc, to note that as this
is an internal function, the gc should really be set up and destroyed
by its caller.  But let's not do that at this stage of the 4.1 release
cycle.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl_create.c |   49 ++++++++++++++++++++++++++------------------
 1 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index e2bd8d0..0d0c84f 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -284,7 +284,7 @@ out:
 int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
                        uint32_t *domid)
 {
-    libxl__gc gc = LIBXL_INIT_GC(ctx);
+    libxl__gc gc = LIBXL_INIT_GC(ctx); /* fixme: should be done by caller */
     int flags, ret, i, rc;
     char *uuid_string;
     char *rw_paths[] = { "device", "device/suspend/event-channel" , "data"};
@@ -293,13 +293,13 @@ int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
     char *dom_path, *vm_path;
     struct xs_permissions roperm[2];
     struct xs_permissions rwperm[1];
-    xs_transaction_t t;
+    xs_transaction_t t = 0;
     xen_domain_handle_t handle;
 
     uuid_string = libxl__uuid2string(&gc, info->uuid);
     if (!uuid_string) {
-        libxl__free_all(&gc);
-        return ERROR_NOMEM;
+        rc = ERROR_NOMEM;
+        goto out;
     }
 
     flags = info->hvm ? XEN_DOMCTL_CDF_hvm_guest : 0;
@@ -313,28 +313,28 @@ int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
     ret = xc_domain_create(ctx->xch, info->ssidref, handle, flags, domid);
     if (ret < 0) {
         LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret, "domain creation fail");
-        libxl__free_all(&gc);
-        return ERROR_FAIL;
+        rc = ERROR_FAIL;
+        goto out;
     }
 
     ret = xc_cpupool_movedomain(ctx->xch, info->poolid, *domid);
     if (ret < 0) {
         LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, ret, "domain move fail");
-        libxl__free_all(&gc);
-        return ERROR_FAIL;
+        rc = ERROR_FAIL;
+        goto out;
     }
 
     dom_path = libxl__xs_get_dompath(&gc, *domid);
     if (!dom_path) {
-        libxl__free_all(&gc);
-        return ERROR_FAIL;
+        rc = ERROR_FAIL;
+        goto out;
     }
 
     vm_path = libxl__sprintf(&gc, "/vm/%s", uuid_string);
     if (!vm_path) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot allocate create paths");
-        libxl__free_all(&gc);
-        return ERROR_FAIL;
+        rc = ERROR_FAIL;
+        goto out;
     }
 
     roperm[0].id = 0;
@@ -356,10 +356,8 @@ retry_transaction:
 
     xs_write(ctx->xsh, t, libxl__sprintf(&gc, "%s/vm", dom_path), vm_path, strlen(vm_path));
     rc = libxl_domain_rename(ctx, *domid, 0, info->name, t);
-    if (rc) {
-        libxl__free_all(&gc);
-        return rc;
-    }
+    if (rc)
+        goto out;
 
     for (i = 0; i < ARRAY_SIZE(rw_paths); i++) {
         char *path = libxl__sprintf(&gc, "%s/%s", dom_path, rw_paths[i]);
@@ -381,12 +379,23 @@ retry_transaction:
     libxl__xs_writev(&gc, t, libxl__sprintf(&gc, "%s/platform", dom_path), info->platformdata);
 
     xs_write(ctx->xsh, t, libxl__sprintf(&gc, "%s/control/platform-feature-multiprocessor-suspend", dom_path), "1", 1);
-    if (!xs_transaction_end(ctx->xsh, t, 0))
-        if (errno == EAGAIN)
+    if (!xs_transaction_end(ctx->xsh, t, 0)) {
+        if (errno == EAGAIN) {
+            t = 0;
             goto retry_transaction;
+        }
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "domain creation "
+                         "xenstore transaction commit failed");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    t = 0;
 
-    libxl__free_all(&gc);
-    return 0;
+    rc = 0;
+ out:
+    if (t) xs_transaction_end(ctx->xsh, t, 1);
+    libxl__free_all(&gc); /* fixme: should be done by caller */
+    return rc;
 }
 
 static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
-- 
1.5.6.5

[PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)

[Ian Jackson]

libxl__domain_make makes some assumptions about the way its caller
treats its uint32_t *domid parameter: specifically, if it fails it may
have partially created the domain and it does not every destroy it.
But it does not initialise it.  Document this assumption in a comment,
and assert on entry that domid not a guest domain id, to ensure that
the caller has properly initialised it.  This is not intended to
produce any functional change in current code as the only change is to
add an assertion.

libxl_create_stubdom calls libxl__domain_make and has no code to tear
down the domain again on error.  This is complicated to fix (since it
may even be possible for the the domain to be left in a state where
it's not possible to tell that it was going to be a stubdom for some
other domain).  So for now simply leave a fixme comment.

Finally, in 22739:d839631b6048 we introduced "-1" as a sentinel "no
such domain" value for domid.  However, domid is a uint32 so testing
it with "if (domid > 0)" as we do in 22740:ce208811f540 is wrong
because it always triggers Instead test it with "if (~domid)".  This
fix means that that if "xl create" fails, it will not try to destroy
the domain "-1".  Previously you'd see this message:
  libxl: error: libxl.c:697:libxl_domain_destroy non-existant domain -1
whose "-1" many readers may have thought was an error code, but which
is actually supposedly a domain id.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl_create.c |    6 +++++-
 tools/libxl/libxl_dm.c     |    2 ++
 tools/libxl/xl_cmdimpl.c   |    2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 0d0c84f..8aabc3c 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -25,6 +25,7 @@
 #include <xenctrl.h>
 #include <xc_dom.h>
 #include <xenguest.h>
+#include <assert.h>
 #include "libxl.h"
 #include "libxl_utils.h"
 #include "libxl_internal.h"
@@ -282,7 +283,8 @@ out:
 }
 
 int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
-                       uint32_t *domid)
+                       uint32_t *domid /* domid 0 or ~0 on entry; on exit
+                                          valid, perhaps >0 (even on error) */)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx); /* fixme: should be done by caller */
     int flags, ret, i, rc;
@@ -296,6 +298,8 @@ int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
     xs_transaction_t t = 0;
     xen_domain_handle_t handle;
 
+    assert(!*domid || !~*domid);
+
     uuid_string = libxl__uuid2string(&gc, info->uuid);
     if (!uuid_string) {
         rc = ERROR_NOMEM;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3cfebbf..3bef49a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -473,6 +473,8 @@ static int libxl_create_stubdom(libxl_ctx *ctx,
     b_info.u.pv.features = "";
     b_info.hvm = 0;
 
+    /* fixme: this function can leak the stubdom if it fails */
+
     ret = libxl__domain_make(ctx, &c_info, &domid);
     if (ret)
         goto out_free;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5826755..a4ffd72 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1649,7 +1649,7 @@ start:
 
 error_out:
     release_lock();
-    if (domid > 0)
+    if (~domid)
         libxl_domain_destroy(&ctx, domid, 0);
 
 out:
-- 
1.5.6.5

[PATCH 4/6] libxl: internals: document the error behaviour of various libxl__xs_* functions

[Ian Jackson]

Many of the functions in libxl_xshelp.c simply return 0 on error, and
leave the errno value from xenstore in errno.  Document this more
clearly.

Also fix a >75 column line.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl_internal.h |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d58b483..1e277ae 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -140,13 +140,21 @@ _hidden char *libxl__strdup(libxl__gc *gc, const char *c);
 _hidden char *libxl__dirname(libxl__gc *gc, const char *s);
 
 _hidden char **libxl__xs_kvs_of_flexarray(libxl__gc *gc, flexarray_t *array, int length);
+
 _hidden int libxl__xs_writev(libxl__gc *gc, xs_transaction_t t,
                     char *dir, char **kvs);
 _hidden int libxl__xs_write(libxl__gc *gc, xs_transaction_t t,
                    char *path, char *fmt, ...) PRINTF_ATTRIBUTE(4, 5);
-_hidden char *libxl__xs_get_dompath(libxl__gc *gc, uint32_t domid); // logs errs
+   /* Each fn returns 0 on success.
+    * On error: returns -1, sets errno (no logging) */
+
+_hidden char *libxl__xs_get_dompath(libxl__gc *gc, uint32_t domid);
+   /* On error: logs, returns NULL, sets errno. */
+
 _hidden char *libxl__xs_read(libxl__gc *gc, xs_transaction_t t, char *path);
-_hidden char **libxl__xs_directory(libxl__gc *gc, xs_transaction_t t, char *path, unsigned int *nb);
+_hidden char **libxl__xs_directory(libxl__gc *gc, xs_transaction_t t,
+                                   char *path, unsigned int *nb);
+   /* On error: returns NULL, sets errno (no logging) */
 
 /* from xl_dom */
 _hidden int libxl__domain_is_hvm(libxl_ctx *ctx, uint32_t domid);
-- 
1.5.6.5

[PATCH 5/6] libxl: during domain destruction, do not complain if no devices dir to destroy

[Ian Jackson]

Previously calling libxl__devices_destroy on a half-constructed or
half-destroyed domain would sometimes complain along these lines:
  libxl: error: libxl_device.c:327:libxl__devices_destroy /local/domain/29/device is empty
This is (a) not a reasonable thing to complain about and (b) not an
accurate description of all the things that that particular failure of
libxl__xs_directory might mean.

Change the code to check errno, so that if errno==ENOENT we silently
continue, not destroying any devices, and if errno!=ENOENT, properly
log the problem and fail.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl_device.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index cf694d2..a7f3bda 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -324,8 +324,12 @@ int libxl__devices_destroy(libxl_ctx *ctx, uint32_t domid, int force)
     path = libxl__sprintf(&gc, "/local/domain/%d/device", domid);
     l1 = libxl__xs_directory(&gc, XBT_NULL, path, &num1);
     if (!l1) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "%s is empty", path);
-        goto out;
+        if (errno != ENOENT) {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unable to get xenstore"
+                             " device listing %s", path);
+            goto out;
+        }
+        num1 = 0;
     }
     for (i = 0; i < num1; i++) {
         if (!strcmp("vfs", l1[i]))
-- 
1.5.6.5

[PATCH 6/6] libxl: prevent creation of domains with duplicate names

[Ian Jackson]

libxl_domain_rename is where domain names are assigned.  Therefore
this is where we check that no two domains have the same name.  As a
special exception, domains whose name is "" are not considered to
clash.

We also take special care not to mind if we try to rename a domain to
the name it already has.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl.c        |   22 ++++++++++++++++++++++
 tools/libxl/libxl_create.c |    1 +
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ccbc842..a88623a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -141,6 +141,28 @@ int libxl_domain_rename(libxl_ctx *ctx, uint32_t domid,
         }
     }
 
+    if (new_name[0]) {
+        /* nonempty names must be unique */
+        uint32_t domid_e;
+        rc = libxl_name_to_domid(ctx, new_name, &domid_e);
+        if (rc == ERROR_INVAL) {
+            /* no such domain, good */
+        } else if (rc != 0) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unexpected error"
+                       "checking for existing domain");
+            goto x_rc;
+        } else if (domid_e == domid) {
+            /* domain already has this name, ok (but we do still
+             * need the rest of the code as we may need to check
+             * old_name, for example). */
+        } else {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "domain with name \"%s\""
+                       " already exists.", new_name);
+            rc = ERROR_INVAL;
+            goto x_rc;
+        }
+    }
+
     if (old_name) {
         got_old_name = xs_read(ctx->xsh, trans, name_path, &got_old_len);
         if (!got_old_name) {
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 8aabc3c..f5a5cc4 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -350,6 +350,7 @@ int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
 
 retry_transaction:
     t = xs_transaction_start(ctx->xsh);
+
     xs_rm(ctx->xsh, t, dom_path);
     xs_mkdir(ctx->xsh, t, dom_path);
     xs_set_permissions(ctx->xsh, t, dom_path, roperm, ARRAY_SIZE(roperm));
-- 
1.5.6.5

Re: [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)

[Stefano Stabellini]

On Thu, 27 Jan 2011, Ian Jackson wrote:
> libxl__domain_make makes some assumptions about the way its caller
> treats its uint32_t *domid parameter: specifically, if it fails it may
> have partially created the domain and it does not every destroy it.
> But it does not initialise it.  Document this assumption in a comment,
> and assert on entry that domid not a guest domain id, to ensure that
> the caller has properly initialised it.  This is not intended to
> produce any functional change in current code as the only change is to
> add an assertion.
> 
> libxl_create_stubdom calls libxl__domain_make and has no code to tear
> down the domain again on error.  This is complicated to fix (since it
> may even be possible for the the domain to be left in a state where
> it's not possible to tell that it was going to be a stubdom for some
> other domain).  So for now simply leave a fixme comment.
> 
> Finally, in 22739:d839631b6048 we introduced "-1" as a sentinel "no
> such domain" value for domid.  However, domid is a uint32 so testing
> it with "if (domid > 0)" as we do in 22740:ce208811f540 is wrong
> because it always triggers Instead test it with "if (~domid)".  This
> fix means that that if "xl create" fails, it will not try to destroy
> the domain "-1".  Previously you'd see this message:
>   libxl: error: libxl.c:697:libxl_domain_destroy non-existant domain -1
> whose "-1" many readers may have thought was an error code, but which
> is actually supposedly a domain id.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  tools/libxl/libxl_create.c |    6 +++++-
>  tools/libxl/libxl_dm.c     |    2 ++
>  tools/libxl/xl_cmdimpl.c   |    2 +-
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 0d0c84f..8aabc3c 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -25,6 +25,7 @@
>  #include <xenctrl.h>
>  #include <xc_dom.h>
>  #include <xenguest.h>
> +#include <assert.h>
>  #include "libxl.h"
>  #include "libxl_utils.h"
>  #include "libxl_internal.h"
> @@ -282,7 +283,8 @@ out:
>  }
>  
>  int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
> -                       uint32_t *domid)
> +                       uint32_t *domid /* domid 0 or ~0 on entry; on exit
> +                                          valid, perhaps >0 (even on error) */)
>  {
>      libxl__gc gc = LIBXL_INIT_GC(ctx); /* fixme: should be done by caller */
>      int flags, ret, i, rc;
> @@ -296,6 +298,8 @@ int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
>      xs_transaction_t t = 0;
>      xen_domain_handle_t handle;
>  
> +    assert(!*domid || !~*domid);
> +
>      uuid_string = libxl__uuid2string(&gc, info->uuid);
>      if (!uuid_string) {
>          rc = ERROR_NOMEM;
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 3cfebbf..3bef49a 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -473,6 +473,8 @@ static int libxl_create_stubdom(libxl_ctx *ctx,
>      b_info.u.pv.features = "";
>      b_info.hvm = 0;
>  
> +    /* fixme: this function can leak the stubdom if it fails */
> +
>      ret = libxl__domain_make(ctx, &c_info, &domid);
>      if (ret)
>          goto out_free;
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5826755..a4ffd72 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1649,7 +1649,7 @@ start:
>  
>  error_out:
>      release_lock();
> -    if (domid > 0)
> +    if (~domid)
>          libxl_domain_destroy(&ctx, domid, 0);

It is non-trivial to understand what this code is supposed to check for.
You should use a more obvious expression or check for DOMID_INVALID.

Re: [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)"):
> It is non-trivial to understand what this code is supposed to check for.
> You should use a more obvious expression or check for DOMID_INVALID.

Hrm.  Perhaps a utility function for testing valid guest domids would
be a good idea.

Ian.

Re: [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)

[Ian Jackson]

I wrote:
> Hrm.  Perhaps a utility function for testing valid guest domids would
> be a good idea.

How about this.

Ian.

commit 2036fda8474df55879ce8b7dd2b0e20fe7e8e3eb
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date:   Thu Jan 27 17:22:41 2011 +0000

    libxl, xl: fixes to domain creation cleanup logic (domid values)
    
    libxl__domain_make makes some assumptions about the way its caller
    treats its uint32_t *domid parameter: specifically, if it fails it may
    have partially created the domain and it does not every destroy it.
    But it does not initialise it.  Document this assumption in a comment,
    and assert on entry that domid not a guest domain id, to ensure that
    the caller has properly initialised it.
    
    Introduce a function libxl_domid_valid_guest to help with this.
    
    This is not intended to produce any practical functional change in
    current code.
    
    Secondly, libxl_create_stubdom calls libxl__domain_make and has no
    code to tear down the domain again on error.  This is complicated to
    fix (since it may even be possible for the the domain to be left in a
    state where it's not possible to tell that it was going to be a
    stubdom for some other domain).  So for now simply leave a fixme
    comment.
    
    Finally, in 22739:d839631b6048 we introduced "-1" as a sentinel "no
    such domain" value for domid.  However, domid is a uint32 so testing
    it with "if (domid > 0)" as we do in 22740:ce208811f540 is wrong
    because it always triggers.  Instead use libxl_domid_valid_guest.
    This fix means that that if "xl create" fails, it will not try to
    destroy the domain "-1".  Previously you'd see this message:
      libxl: error: libxl.c:697:libxl_domain_destroy non-existant domain -1
    whose "-1" many readers may have thought was an error code, but which
    is actually supposedly a domain id.
    
    Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index d608e99..0851e66 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -554,6 +554,12 @@ int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu);
 int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
 int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t poolid, uint32_t domid);
 
+static inline int libxl_domid_valid_guest(uint32_t domid) {
+    /* returns 1 if the value _could_ be a valid guest domid, 0 otherwise
+     * does not check whether the domain actually exists */
+    return domid > 0 && domid < DOMID_FIRST_RESERVED;
+}
+
 /* common paths */
 const char *libxl_sbindir_path(void);
 const char *libxl_bindir_path(void);
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 0d0c84f..c3da845 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -25,6 +25,7 @@
 #include <xenctrl.h>
 #include <xc_dom.h>
 #include <xenguest.h>
+#include <assert.h>
 #include "libxl.h"
 #include "libxl_utils.h"
 #include "libxl_internal.h"
@@ -282,7 +283,8 @@ out:
 }
 
 int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
-                       uint32_t *domid)
+                       uint32_t *domid /* domid 0 or ~0 on entry; on exit
+                                          valid, perhaps >0 (even on error) */)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx); /* fixme: should be done by caller */
     int flags, ret, i, rc;
@@ -296,6 +298,8 @@ int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
     xs_transaction_t t = 0;
     xen_domain_handle_t handle;
 
+    assert(!libxl_domid_valid_guest(*domid));
+
     uuid_string = libxl__uuid2string(&gc, info->uuid);
     if (!uuid_string) {
         rc = ERROR_NOMEM;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3cfebbf..3bef49a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -473,6 +473,8 @@ static int libxl_create_stubdom(libxl_ctx *ctx,
     b_info.u.pv.features = "";
     b_info.hvm = 0;
 
+    /* fixme: this function can leak the stubdom if it fails */
+
     ret = libxl__domain_make(ctx, &c_info, &domid);
     if (ret)
         goto out_free;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5826755..703bb03 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1649,7 +1649,7 @@ start:
 
 error_out:
     release_lock();
-    if (domid > 0)
+    if (libxl_domid_valid_guest(domid))
         libxl_domain_destroy(&ctx, domid, 0);
 
 out:

Re: [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)

[Gianni Tedesco]

On Thu, 2011-01-27 at 20:01 +0000, Ian Jackson wrote:
> 
> +static inline int libxl_domid_valid_guest(uint32_t domid) {
> +    /* returns 1 if the value _could_ be a valid guest domid, 0 otherwise
> +     * does not check whether the domain actually exists */
> +    return domid > 0 && domid < DOMID_FIRST_RESERVED;
> +}

Looks good to me, apart from coding style (opening brace)...

Gianni

Re: [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)

[Stefano Stabellini]

On Thu, 27 Jan 2011, Ian Jackson wrote:
> I wrote:
> > Hrm.  Perhaps a utility function for testing valid guest domids would
> > be a good idea.
> 
> How about this.
> 
> Ian.
> 
> commit 2036fda8474df55879ce8b7dd2b0e20fe7e8e3eb
> Author: Ian Jackson <ian.jackson@eu.citrix.com>
> Date:   Thu Jan 27 17:22:41 2011 +0000
> 
>     libxl, xl: fixes to domain creation cleanup logic (domid values)
>     
>     libxl__domain_make makes some assumptions about the way its caller
>     treats its uint32_t *domid parameter: specifically, if it fails it may
>     have partially created the domain and it does not every destroy it.
>     But it does not initialise it.  Document this assumption in a comment,
>     and assert on entry that domid not a guest domain id, to ensure that
>     the caller has properly initialised it.
>     
>     Introduce a function libxl_domid_valid_guest to help with this.
>     
>     This is not intended to produce any practical functional change in
>     current code.
>     
>     Secondly, libxl_create_stubdom calls libxl__domain_make and has no
>     code to tear down the domain again on error.  This is complicated to
>     fix (since it may even be possible for the the domain to be left in a
>     state where it's not possible to tell that it was going to be a
>     stubdom for some other domain).  So for now simply leave a fixme
>     comment.
>     
>     Finally, in 22739:d839631b6048 we introduced "-1" as a sentinel "no
>     such domain" value for domid.  However, domid is a uint32 so testing
>     it with "if (domid > 0)" as we do in 22740:ce208811f540 is wrong
>     because it always triggers.  Instead use libxl_domid_valid_guest.
>     This fix means that that if "xl create" fails, it will not try to
>     destroy the domain "-1".  Previously you'd see this message:
>       libxl: error: libxl.c:697:libxl_domain_destroy non-existant domain -1
>     whose "-1" many readers may have thought was an error code, but which
>     is actually supposedly a domain id.
>     
>     Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index d608e99..0851e66 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -554,6 +554,12 @@ int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu);
>  int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
>  int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t poolid, uint32_t domid);
>  
> +static inline int libxl_domid_valid_guest(uint32_t domid) {
> +    /* returns 1 if the value _could_ be a valid guest domid, 0 otherwise
> +     * does not check whether the domain actually exists */
> +    return domid > 0 && domid < DOMID_FIRST_RESERVED;
> +}
> +

better

>  int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
> -                       uint32_t *domid)
> +                       uint32_t *domid /* domid 0 or ~0 on entry; on exit
> +                                          valid, perhaps >0 (even on error) */)

can you please change this comment too?

Re: [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)"):
> On Thu, 27 Jan 2011, Ian Jackson wrote:
> >  int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
> > -                       uint32_t *domid)
> > +                       uint32_t *domid /* domid 0 or ~0 on entry; on exit
> > +                                          valid, perhaps >0 (even on error) */)
> 
> can you please change this comment too?

What do you think would be clearer ?  -1 isn't right since a
uint32_t can't have the value -1.

Ian.

Re: [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)

[Stefano Stabellini]

On Fri, 28 Jan 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)"):
> > On Thu, 27 Jan 2011, Ian Jackson wrote:
> > >  int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
> > > -                       uint32_t *domid)
> > > +                       uint32_t *domid /* domid 0 or ~0 on entry; on exit
> > > +                                          valid, perhaps >0 (even on error) */)
> > 
> > can you please change this comment too?
> 
> What do you think would be clearer ?  -1 isn't right since a
> uint32_t can't have the value -1.
> 

domid 0 or domid > DOMID_FIRST_RESERVED on entry

Re: [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)"):
> domid 0 or domid > DOMID_FIRST_RESERVED on entry

How about this.  Also fixes Gianni's brace formatting complaint.

Ian.

commit b4a79d2621a7e21e7fcbbf605d650e106d8fc3d6
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date:   Thu Jan 27 17:22:41 2011 +0000

    libxl, xl: fixes to domain creation cleanup logic (domid values)
    
    libxl__domain_make makes some assumptions about the way its caller
    treats its uint32_t *domid parameter: specifically, if it fails it may
    have partially created the domain and it does not every destroy it.
    But it does not initialise it.  Document this assumption in a comment,
    and assert on entry that domid not a guest domain id, to ensure that
    the caller has properly initialised it.
    
    Introduce a function libxl_domid_valid_guest to help with this.
    
    This is not intended to produce any practical functional change in
    current code.
    
    Secondly, libxl_create_stubdom calls libxl__domain_make and has no
    code to tear down the domain again on error.  This is complicated to
    fix (since it may even be possible for the the domain to be left in a
    state where it's not possible to tell that it was going to be a
    stubdom for some other domain).  So for now simply leave a fixme
    comment.
    
    Finally, in 22739:d839631b6048 we introduced "-1" as a sentinel "no
    such domain" value for domid.  However, domid is a uint32 so testing
    it with "if (domid > 0)" as we do in 22740:ce208811f540 is wrong
    because it always triggers.  Instead use libxl_domid_valid_guest.
    This fix means that that if "xl create" fails, it will not try to
    destroy the domain "-1".  Previously you'd see this message:
      libxl: error: libxl.c:697:libxl_domain_destroy non-existant domain -1
    whose "-1" many readers may have thought was an error code, but which
    is actually supposedly a domain id.
    
    Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index d608e99..7b6e5c3 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -554,6 +554,13 @@ int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu);
 int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
 int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t poolid, uint32_t domid);
 
+static inline int libxl_domid_valid_guest(uint32_t domid)
+{
+    /* returns 1 if the value _could_ be a valid guest domid, 0 otherwise
+     * does not check whether the domain actually exists */
+    return domid > 0 && domid < DOMID_FIRST_RESERVED;
+}
+
 /* common paths */
 const char *libxl_sbindir_path(void);
 const char *libxl_bindir_path(void);
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 0d0c84f..1879de0 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -25,6 +25,7 @@
 #include <xenctrl.h>
 #include <xc_dom.h>
 #include <xenguest.h>
+#include <assert.h>
 #include "libxl.h"
 #include "libxl_utils.h"
 #include "libxl_internal.h"
@@ -283,6 +284,8 @@ out:
 
 int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
                        uint32_t *domid)
+ /* on entry, libxl_domid_valid_guest(domid) must be false;
+  * on exit (even error exit), domid may be valid and refer to a domain */
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx); /* fixme: should be done by caller */
     int flags, ret, i, rc;
@@ -296,6 +299,8 @@ int libxl__domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
     xs_transaction_t t = 0;
     xen_domain_handle_t handle;
 
+    assert(!libxl_domid_valid_guest(*domid));
+
     uuid_string = libxl__uuid2string(&gc, info->uuid);
     if (!uuid_string) {
         rc = ERROR_NOMEM;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3cfebbf..3bef49a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -473,6 +473,8 @@ static int libxl_create_stubdom(libxl_ctx *ctx,
     b_info.u.pv.features = "";
     b_info.hvm = 0;
 
+    /* fixme: this function can leak the stubdom if it fails */
+
     ret = libxl__domain_make(ctx, &c_info, &domid);
     if (ret)
         goto out_free;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5826755..703bb03 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1649,7 +1649,7 @@ start:
 
 error_out:
     release_lock();
-    if (domid > 0)
+    if (libxl_domid_valid_guest(domid))
         libxl_domain_destroy(&ctx, domid, 0);
 
 out:

Re: [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)

[Stefano Stabellini]

On Fri, 28 Jan 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)"):
> > domid 0 or domid > DOMID_FIRST_RESERVED on entry
> 
> How about this.  Also fixes Gianni's brace formatting complaint.
> 
> Ian.
> 
> commit b4a79d2621a7e21e7fcbbf605d650e106d8fc3d6
> Author: Ian Jackson <ian.jackson@eu.citrix.com>
> Date:   Thu Jan 27 17:22:41 2011 +0000
> 
>     libxl, xl: fixes to domain creation cleanup logic (domid values)
>     
>     libxl__domain_make makes some assumptions about the way its caller
>     treats its uint32_t *domid parameter: specifically, if it fails it may
>     have partially created the domain and it does not every destroy it.
>     But it does not initialise it.  Document this assumption in a comment,
>     and assert on entry that domid not a guest domain id, to ensure that
>     the caller has properly initialised it.
>     
>     Introduce a function libxl_domid_valid_guest to help with this.
>     
>     This is not intended to produce any practical functional change in
>     current code.
>     
>     Secondly, libxl_create_stubdom calls libxl__domain_make and has no
>     code to tear down the domain again on error.  This is complicated to
>     fix (since it may even be possible for the the domain to be left in a
>     state where it's not possible to tell that it was going to be a
>     stubdom for some other domain).  So for now simply leave a fixme
>     comment.
>     
>     Finally, in 22739:d839631b6048 we introduced "-1" as a sentinel "no
>     such domain" value for domid.  However, domid is a uint32 so testing
>     it with "if (domid > 0)" as we do in 22740:ce208811f540 is wrong
>     because it always triggers.  Instead use libxl_domid_valid_guest.
>     This fix means that that if "xl create" fails, it will not try to
>     destroy the domain "-1".  Previously you'd see this message:
>       libxl: error: libxl.c:697:libxl_domain_destroy non-existant domain -1
>     whose "-1" many readers may have thought was an error code, but which
>     is actually supposedly a domain id.
>     
>     Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Re: [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)"):
> On Fri, 28 Jan 2011, Ian Jackson wrote:
> > How about this.  Also fixes Gianni's brace formatting complaint.
...
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Thanks, I've applied 2-6/6.  (1/6 went in yesterday.)

Ian.