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

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
---
 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel