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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@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);

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.

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