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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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

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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel