Xen devel - Jan 2011 - [PATCH] xl: avoid creating domains with duplicate names

Do not create the domain if another domain with the same name is already
running.

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

diff -r 829855751e19 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Tue Jan 25 15:05:20 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c	Tue Jan 25 15:12:21 2011 +0000
@@ -569,6 +569,7 @@ static void parse_config_data(const char
     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;
@@ -598,6 +599,11 @@ static void parse_config_data(const char
 
     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 (!xlu_cfg_get_string (config, "uuid", &buf) ) {
         if ( libxl_uuid_from_string(&c_info->uuid, buf) ) {

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

Stefano Stabellini writes ("[Xen-devel] [PATCH] xl: avoid creating domains
with duplicate names"):
> Do not create the domain if another domain with the same name is already
> running.
Thanks.  I approve of the principle of this patch, but:
> +    e = libxl_name_to_domid(&ctx, c_info->name, &domid_e);
> +    if (!e) {
You should explicitly check the actual error return value of
libxl_name_to_domid and check that it is the expected error code, and
not some other error code meaning "general failure" or something.

I went to look at the code for libxl_name_to_domid and it returns,
entirely ad-hoc, -1 (which is now ERROR_VERSION), for "no such
domain".

IMO it should return ERROR_INVAL.

I grepped the libxl source for "-1" and found that this practice is
widespread.  At this stage of the release I don''t want to risk
breaking everything by changing them all (since something may compare
with -1, or something).

So I suggest the attached fixup patch, and then a revised version of
your patch.  What do you think?

Ian.

libxl: band-aid for functions with return literal "-1"

Many libxl functions erroneously return "-1" on error, rather than
some ERROR_* value.

To deal with this, invent a new ERROR_NONSPECIFIC "-1" which indicates
that "the function which generated this error code is broken".

Fix up the one we care about for forthcoming duplicate domain
detection (libxl_name_to_domid) and the others following the same
pattern nearby; leave the rest for post-4.1.

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

diff -r 1d1eec7e1fb4 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Tue Jan 25 18:09:49 2011 +0000
+++ b/tools/libxl/libxl.h	Tue Jan 25 18:18:53 2011 +0000
@@ -232,12 +232,13 @@ typedef struct {
 } libxl_domain_suspend_info;
 
 enum {
-    ERROR_VERSION = -1,
-    ERROR_FAIL = -2,
-    ERROR_NI = -3,
-    ERROR_NOMEM = -4,
-    ERROR_INVAL = -5,
-    ERROR_BADFAIL = -6,
+    ERROR_NONSPECIFIC = -1,
+    ERROR_VERSION = -2,
+    ERROR_FAIL = -3,
+    ERROR_NI = -4,
+    ERROR_NOMEM = -5,
+    ERROR_INVAL = -6,
+    ERROR_BADFAIL = -7,
 };
 
 #define LIBXL_VERSION 0
diff -r 1d1eec7e1fb4 tools/libxl/libxl_utils.c
--- a/tools/libxl/libxl_utils.c	Tue Jan 25 18:09:49 2011 +0000
+++ b/tools/libxl/libxl_utils.c	Tue Jan 25 18:18:53 2011 +0000
@@ -93,7 +93,7 @@ int libxl_name_to_domid(libxl_ctx *ctx, 
     int i, nb_domains;
     char *domname;
     libxl_dominfo *dominfo;
-    int ret = -1;
+    int ret = ERROR_INVAL;
 
     dominfo = libxl_list_domain(ctx, &nb_domains);
     if (!dominfo)
@@ -142,7 +142,7 @@ int libxl_name_to_cpupoolid(libxl_ctx *c
     int i, nb_pools;
     char *poolname;
     libxl_cpupoolinfo *poolinfo;
-    int ret = -1;
+    int ret = ERROR_INVAL;
 
     poolinfo = libxl_list_cpupool(ctx, &nb_pools);
     if (!poolinfo)
@@ -171,7 +171,7 @@ int libxl_name_to_schedid(libxl_ctx *ctx
         if (strcmp(name, schedid_name[i].name) == 0)
             return schedid_name[i].id;
 
-    return -1;
+    return ERROR_INVAL;
 }
 
 char *libxl_schedid_to_name(libxl_ctx *ctx, int schedid)
@@ -287,7 +287,7 @@ int libxl_string_to_phystype(libxl_ctx *
     } else if (!strcmp(s, "tap")) {
         p = strchr(s, '':'');
         if (!p) {
-            rc = -1;
+            rc = ERROR_INVAL;
             goto out;
         }
         p++;

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

On Tue, 25 Jan 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH] xl: avoid creating
domains with duplicate names"):
> > Do not create the domain if another domain with the same name is
already
> > running.
> 
> Thanks.  I approve of the principle of this patch, but:
> 
> > +    e = libxl_name_to_domid(&ctx, c_info->name, &domid_e);
> > +    if (!e) {
> 
> You should explicitly check the actual error return value of
> libxl_name_to_domid and check that it is the expected error code, and
> not some other error code meaning "general failure" or something.
> 
> I went to look at the code for libxl_name_to_domid and it returns,
> entirely ad-hoc, -1 (which is now ERROR_VERSION), for "no such
> domain".
> 
> IMO it should return ERROR_INVAL.
> 
> I grepped the libxl source for "-1" and found that this practice
is
> widespread.  At this stage of the release I don''t want to risk
> breaking everything by changing them all (since something may compare
> with -1, or something).
> 
> So I suggest the attached fixup patch, and then a revised version of
> your patch.  What do you think?
I think is a good idea.


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

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] xl: avoid creating
domains with duplicate names"):
> On Tue, 25 Jan 2011, Ian Jackson wrote:
> > So I suggest the attached fixup patch, and then a revised version of
> > your patch.  What do you think?
> 
> I think is a good idea.
OK, thanks, I''ll take that as an ack.  I have applied my patch and
will post a revised version of yours on a moment.

Ian.

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

I wrote:
> OK, thanks, I''ll take that as an ack.  I have applied my patch and
> will post a revised version of yours on a moment.
TBH I think this restriction should be in libxl (in
libxl_domain_rename), rather than in xl.  But that would require too
intrusive a set of changes at this stage.


xl: avoid creating domains with duplicate names

Do not create the domain if another domain with the same name is already
running.

This is another error-checking function at rather too high a level:
this should be moved into libxl_domain_rename in 4.2.

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

diff -r 67d5b8004947 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Wed Jan 26 11:58:45 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c	Wed Jan 26 12:02:37 2011 +0000
@@ -583,6 +583,7 @@ static void parse_config_data(const char
     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;
@@ -612,6 +613,15 @@ static void parse_config_data(const char
 
     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) ) {

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

Looks like this patch is causing failures in guest-localmigrate:

2011-01-27 03:06:09 Z executing ssh ... root@10.80.249.56 xl migrate
debian.guest.osstest localhost
migration target: Ready to receive domain.
Saving to migration stream new xl format (info 0x0/0x0/774)
Loading new save file incoming migration stream (new xl fmt info 0x0/0x0/774)
 Savefile contains xl domain config
A domain with name "debian.guest.osstest" already exists.
xc: error: write: p2m_size (32 = Broken pipe): Internal error
libxl: error: libxl_dom.c:444:libxl__domain_suspend_common saving
domain: Broken pipe
migration sender: libxl_domain_suspend failed (rc=-3)
libxl: info: libxl_exec.c:70:libxl_report_child_exitstatus migration
target process [20216] exited with error status 1
Migration failed, resuming at sender.

Do we need to special-case local migration?

 -George

On Wed, Jan 26, 2011 at 12:07 PM, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:
> I wrote:
>> OK, thanks, I''ll take that as an ack.  I have applied my patch
and
>> will post a revised version of yours on a moment.
>
> TBH I think this restriction should be in libxl (in
> libxl_domain_rename), rather than in xl.  But that would require too
> intrusive a set of changes at this stage.
>
>
> xl: avoid creating domains with duplicate names
>
> Do not create the domain if another domain with the same name is already
> running.
>
> This is another error-checking function at rather too high a level:
> this should be moved into libxl_domain_rename in 4.2.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> diff -r 67d5b8004947 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c  Wed Jan 26 11:58:45 2011 +0000
> +++ b/tools/libxl/xl_cmdimpl.c  Wed Jan 26 12:02:37 2011 +0000
> @@ -583,6 +583,7 @@ static void parse_config_data(const char
>     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;
> @@ -612,6 +613,15 @@ static void parse_config_data(const char
>
>     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) ) {
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

George Dunlap writes ("Re: [Xen-devel] [PATCH] xl: avoid creating domains
with duplicate names"):
> Looks like this patch is causing failures in guest-localmigrate:
Thanks.  See my series of 6 small patches to fix this.  Following
consultations, I have already applied the first, which was simply to
revert the check for now pending a proper fix.

Ian.

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