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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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

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:

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