Ian Jackson
2011-Jan-27 17:40 UTC
[Xen-devel] [PATCH 0/6] libxl, xl: fix domain name uniqueness check
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> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-27 17:40 UTC
[Xen-devel] [PATCH 1/6] xl: Revert "xl: avoid creating domains with duplicate names"
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-27 17:41 UTC
[Xen-devel] [PATCH 2/6] libxl: fix error handling (xenstore transaction leak) in libxl__domain_make
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-27 17:41 UTC
[Xen-devel] [PATCH 3/6] 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. 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-27 17:41 UTC
[Xen-devel] [PATCH 4/6] libxl: internals: document the error behaviour of various libxl__xs_* functions
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-27 17:41 UTC
[Xen-devel] [PATCH 5/6] libxl: during domain destruction, do not complain if no devices dir to destroy
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-27 17:41 UTC
[Xen-devel] [PATCH 6/6] libxl: prevent creation of domains with duplicate names
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jan-27 18:33 UTC
Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-27 18:59 UTC
Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-27 20:01 UTC
Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)
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: _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2011-Jan-27 20:13 UTC
Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jan-28 12:22 UTC
Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)
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? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-28 12:28 UTC
Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jan-28 13:27 UTC
Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-28 17:38 UTC
Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)
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 entryHow 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: _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jan-28 17:52 UTC
Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)
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> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-28 18:39 UTC
Re: [Xen-devel] [PATCH 3/6] libxl, xl: fixes to domain creation cleanup logic (domid values)
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel