[PATCH] libxl: Make an internal function explicitly check existence of expected paths

[PATCH] libxl: Make an internal function explicitly check existence of expected paths

[George Dunlap]

# HG changeset patch
# User George Dunlap <george.dunlap@eu.citrix.com>
# Date 1353686127 0
# Node ID 85552b648b3cabebd4f5bfb484ab329fe2bb290d
# Parent  54d1bcd211d14cfbae0782f2bf04f8942c2eb726
libxl: Make an internal function explicitly check existence of expected paths

libxl__device_disk_from_xs_be() was failing without error for some
missing xenstore nodes in a backend, while assuming (without checking)
that other nodes were valid, causing a crash when another internal
error wrote these nodes in the wrong place.

Make this function consistent by:
* Checking the existence of all nodes before using
* Choosing a default only when the node is not written in device_disk_add()
* Failing with log msg if any node written by device_disk_add() is not present
* Returning an error on failure

Also make the callers of the function pay attention to the error and
behave appropriately.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2168,9 +2168,9 @@ void libxl__device_disk_add(libxl__egc *
     device_disk_add(egc, domid, disk, aodev, NULL, NULL);
 }
 
-static void libxl__device_disk_from_xs_be(libxl__gc *gc,
-                                          const char *be_path,
-                                          libxl_device_disk *disk)
+static int libxl__device_disk_from_xs_be(libxl__gc *gc,
+                                         const char *be_path,
+                                         libxl_device_disk *disk)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned int len;
@@ -2178,6 +2178,7 @@ static void libxl__device_disk_from_xs_b
 
     libxl_device_disk_init(disk);
 
+    /* "params" may not be present; but everything else must be. */
     tmp = xs_read(ctx->xsh, XBT_NULL,
                   libxl__sprintf(gc, "%s/params", be_path), &len);
     if (tmp && strchr(tmp, ':')) {
@@ -2186,21 +2187,40 @@ static void libxl__device_disk_from_xs_b
     } else {
         disk->pdev_path = tmp;
     }
-    libxl_string_to_backend(ctx,
-                        libxl__xs_read(gc, XBT_NULL,
-                                       libxl__sprintf(gc, "%s/type", be_path)),
-                        &(disk->backend));
+
+    
+    tmp = libxl__xs_read(gc, XBT_NULL,
+                         libxl__sprintf(gc, "%s/type", be_path));
+    if (!tmp) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                   "Internal error: Missing xenstore node %s/type", be_path);
+        return ERROR_FAIL;
+    }
+    libxl_string_to_backend(ctx, tmp, &(disk->backend));
+
     disk->vdev = xs_read(ctx->xsh, XBT_NULL,
                          libxl__sprintf(gc, "%s/dev", be_path), &len);
+    if (!disk->vdev) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                   "Internal error: Missing xenstore node %s/dev", be_path);
+        return ERROR_FAIL;
+    }
+
     tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf
                          (gc, "%s/removable", be_path));
-
-    if (tmp)
-        disk->removable = atoi(tmp);
-    else
-        disk->removable = 0;
+    if (!tmp) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                   "Internal error: Missing xenstore node %s/removable", be_path);
+        return ERROR_FAIL;
+    }
+    disk->removable = atoi(tmp);
 
     tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/mode", be_path));
+    if (!tmp) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                   "Internal error: Missing xenstore node %s/mode", be_path);
+        return ERROR_FAIL;
+    }
     if (!strcmp(tmp, "w"))
         disk->readwrite = 1;
     else
@@ -2208,9 +2228,16 @@ static void libxl__device_disk_from_xs_b
 
     tmp = libxl__xs_read(gc, XBT_NULL,
                          libxl__sprintf(gc, "%s/device-type", be_path));
+    if (!tmp) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                   "Internal error: Missing xenstore node %s/device-type", be_path);
+        return ERROR_FAIL;
+    }
     disk->is_cdrom = !strcmp(tmp, "cdrom");
 
     disk->format = LIBXL_DISK_FORMAT_UNKNOWN;
+
+    return 0;
 }
 
 int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t domid,
@@ -2236,9 +2263,7 @@ int libxl_vdev_to_device_disk(libxl_ctx 
     if (!path)
         goto out;
 
-    libxl__device_disk_from_xs_be(gc, path, disk);
-
-    rc = 0;
+    rc = libxl__device_disk_from_xs_be(gc, path, disk);
 out:
     GC_FREE;
     return rc;
@@ -2255,6 +2280,7 @@ static int libxl__append_disk_list_of_ty
     char **dir = NULL;
     unsigned int n = 0;
     libxl_device_disk *pdisk = NULL, *pdisk_end = NULL;
+    int rc=0;
 
     be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
                              libxl__xs_get_dompath(gc, 0), type, domid);
@@ -2271,7 +2297,8 @@ static int libxl__append_disk_list_of_ty
         for (; pdisk < pdisk_end; pdisk++, dir++) {
             const char *p;
             p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
-            libxl__device_disk_from_xs_be(gc, p, pdisk);
+            if ((rc=libxl__device_disk_from_xs_be(gc, p, pdisk)))
+                return rc;
             pdisk->backend_domid = 0;
         }
     }

Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths

[Ian Campbell]

On Fri, 2012-11-23 at 15:56 +0000, George Dunlap wrote:
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +                   "Internal error: Missing xenstore node %s/removable", be_path);

One or two of these new log lines are > 80 columns. You might find that
using the shorthand LOG*() macros helps with this.

Personally I don't think the "Internal error" tag is necessary either,
but maybe a new return code ERROR_INTERNAL might make sense? or even an
assert() if this is really a "libxl failed to maintain internal
consistency" ?

Ian.

Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths

[George Dunlap]

On 27/11/12 11:44, Ian Campbell wrote:
> On Fri, 2012-11-23 at 15:56 +0000, George Dunlap wrote:
>> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
>> +                   "Internal error: Missing xenstore node %s/removable", be_path);
>
> One or two of these new log lines are > 80 columns. You might find that
> using the shorthand LOG*() macros helps with this.

Oops -- I'll fix that up.

>
> Personally I don't think the "Internal error" tag is necessary either,
> but maybe a new return code ERROR_INTERNAL might make sense? or even an
> assert() if this is really a "libxl failed to maintain internal
> consistency" ?

The idea behind the "Internal" tag is to tell users, "Unless you've been 
manually messing around with xenstore removing nodes, this is definitely 
not your fault."

I guess normally an ASSERT communicates that pretty well, but I guess 
I'm used to ASSERTs that disappear when DEBUG is turned off. :-)  Is 
that not the case for libxl?

  -George

Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths

[Ian Campbell]

On Tue, 2012-11-27 at 11:50 +0000, George Dunlap wrote:
> On 27/11/12 11:44, Ian Campbell wrote:
> > On Fri, 2012-11-23 at 15:56 +0000, George Dunlap wrote:
> >> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> >> +                   "Internal error: Missing xenstore node %s/removable", be_path);
> >
> > One or two of these new log lines are > 80 columns. You might find that
> > using the shorthand LOG*() macros helps with this.
> 
> Oops -- I'll fix that up.
> 
> >
> > Personally I don't think the "Internal error" tag is necessary either,
> > but maybe a new return code ERROR_INTERNAL might make sense? or even an
> > assert() if this is really a "libxl failed to maintain internal
> > consistency" ?
> 
> The idea behind the "Internal" tag is to tell users, "Unless you've been 
> manually messing around with xenstore removing nodes, this is definitely 
> not your fault."

OK. Perhaps in order to do this consistently we could have a libxl
helper which logs a message with this tag added and then aborts?
(libxl_assert?)

Might be worth leaving some time for others to weigh in before
implementing that though, since adding aborts is something which might
be considered controversial.

> I guess normally an ASSERT communicates that pretty well, but I guess 
> I'm used to ASSERTs that disappear when DEBUG is turned off. :-)  Is 
> that not the case for libxl?

I don't think assert(3) has this property?

Ian.

[PATCH] libxl: Make an internal function explicitly check existence of expected paths

[George Dunlap]

# HG changeset patch
# User George Dunlap <george.dunlap@eu.citrix.com>
# Date 1354294821 0
# Node ID bc2a7645dc2be4a01f2b5ee30d7453cc3d7339aa
# Parent  bd041b7426fe10a730994edd98708ff98ae1cb74
libxl: Make an internal function explicitly check existence of expected paths

libxl__device_disk_from_xs_be() was failing without error for some
missing xenstore nodes in a backend, while assuming (without checking)
that other nodes were valid, causing a crash when another internal
error wrote these nodes in the wrong place.

Make this function consistent by:
* Checking the existence of all nodes before using
* Choosing a default only when the node is not written in device_disk_add()
* Failing with log msg if any node written by device_disk_add() is not present
* Returning an error on failure

Also make the callers of the function pay attention to the error and
behave appropriately.

v2:
 * Remove "Internal error", as the failure will most likely look internal
 * Use LOG(ERROR...) macros for incrased prettiness

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2168,9 +2168,9 @@ void libxl__device_disk_add(libxl__egc *
     device_disk_add(egc, domid, disk, aodev, NULL, NULL);
 }
 
-static void libxl__device_disk_from_xs_be(libxl__gc *gc,
-                                          const char *be_path,
-                                          libxl_device_disk *disk)
+static int libxl__device_disk_from_xs_be(libxl__gc *gc,
+                                         const char *be_path,
+                                         libxl_device_disk *disk)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned int len;
@@ -2178,6 +2178,7 @@ static void libxl__device_disk_from_xs_b
 
     libxl_device_disk_init(disk);
 
+    /* "params" may not be present; but everything else must be. */
     tmp = xs_read(ctx->xsh, XBT_NULL,
                   libxl__sprintf(gc, "%s/params", be_path), &len);
     if (tmp && strchr(tmp, ':')) {
@@ -2186,21 +2187,36 @@ static void libxl__device_disk_from_xs_b
     } else {
         disk->pdev_path = tmp;
     }
-    libxl_string_to_backend(ctx,
-                        libxl__xs_read(gc, XBT_NULL,
-                                       libxl__sprintf(gc, "%s/type", be_path)),
-                        &(disk->backend));
+
+    
+    tmp = libxl__xs_read(gc, XBT_NULL,
+                         libxl__sprintf(gc, "%s/type", be_path));
+    if (!tmp) {
+        LOG(ERROR, "Missing xenstore node %s/type", be_path);
+        return ERROR_FAIL;
+    }
+    libxl_string_to_backend(ctx, tmp, &(disk->backend));
+
     disk->vdev = xs_read(ctx->xsh, XBT_NULL,
                          libxl__sprintf(gc, "%s/dev", be_path), &len);
+    if (!disk->vdev) {
+        LOG(ERROR, "Missing xenstore node %s/dev", be_path);
+        return ERROR_FAIL;
+    }
+
     tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf
                          (gc, "%s/removable", be_path));
-
-    if (tmp)
-        disk->removable = atoi(tmp);
-    else
-        disk->removable = 0;
+    if (!tmp) {
+        LOG(ERROR, "Missing xenstore node %s/removable", be_path);
+        return ERROR_FAIL;
+    }
+    disk->removable = atoi(tmp);
 
     tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/mode", be_path));
+    if (!tmp) {
+        LOG(ERROR, "Missing xenstore node %s/mode", be_path);
+        return ERROR_FAIL;
+    }
     if (!strcmp(tmp, "w"))
         disk->readwrite = 1;
     else
@@ -2208,9 +2224,15 @@ static void libxl__device_disk_from_xs_b
 
     tmp = libxl__xs_read(gc, XBT_NULL,
                          libxl__sprintf(gc, "%s/device-type", be_path));
+    if (!tmp) {
+        LOG(ERROR, "Missing xenstore node %s/device-type", be_path);
+        return ERROR_FAIL;
+    }
     disk->is_cdrom = !strcmp(tmp, "cdrom");
 
     disk->format = LIBXL_DISK_FORMAT_UNKNOWN;
+
+    return 0;
 }
 
 int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t domid,
@@ -2236,9 +2258,7 @@ int libxl_vdev_to_device_disk(libxl_ctx 
     if (!path)
         goto out;
 
-    libxl__device_disk_from_xs_be(gc, path, disk);
-
-    rc = 0;
+    rc = libxl__device_disk_from_xs_be(gc, path, disk);
 out:
     GC_FREE;
     return rc;
@@ -2255,6 +2275,7 @@ static int libxl__append_disk_list_of_ty
     char **dir = NULL;
     unsigned int n = 0;
     libxl_device_disk *pdisk = NULL, *pdisk_end = NULL;
+    int rc=0;
 
     be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
                              libxl__xs_get_dompath(gc, 0), type, domid);
@@ -2271,7 +2292,8 @@ static int libxl__append_disk_list_of_ty
         for (; pdisk < pdisk_end; pdisk++, dir++) {
             const char *p;
             p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
-            libxl__device_disk_from_xs_be(gc, p, pdisk);
+            if ((rc=libxl__device_disk_from_xs_be(gc, p, pdisk)))
+                return rc;
             pdisk->backend_domid = 0;
         }
     }

Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths

[Ian Campbell]

On Fri, 2012-11-30 at 17:13 +0000, George Dunlap wrote:
> # HG changeset patch
> # User George Dunlap <george.dunlap@eu.citrix.com>
> # Date 1354294821 0
> # Node ID bc2a7645dc2be4a01f2b5ee30d7453cc3d7339aa
> # Parent  bd041b7426fe10a730994edd98708ff98ae1cb74
> libxl: Make an internal function explicitly check existence of expected paths
> 
> libxl__device_disk_from_xs_be() was failing without error for some
> missing xenstore nodes in a backend, while assuming (without checking)
> that other nodes were valid, causing a crash when another internal
> error wrote these nodes in the wrong place.
> 
> Make this function consistent by:
> * Checking the existence of all nodes before using
> * Choosing a default only when the node is not written in device_disk_add()
> * Failing with log msg if any node written by device_disk_add() is not present
> * Returning an error on failure
> 
> Also make the callers of the function pay attention to the error and
> behave appropriately.

If libxl__device_disk_from_xs_be returns an error then someone needs to
cleanup the partial allocations in the disk (pdev_path) probably by
calling libxl_device_disk_dispose.

It's probably easiest to do this in libxl__device_disk_from_xs_be on
error rather than modifying all the callers?

Also libxl__append_disk_list_of_type updates *ndisks early, so if you
abort half way through initialising the elements of the disks array
using libxl__device_disk_from_xs_be then the caller will try and free
some stuff which hasn't been initialised. I think the code needs to
remember ndisks-in-array separately from
ndisks-which-I-have-initialised, with the latter becoming the returned
*ndisks.

> v2:
>  * Remove "Internal error", as the failure will most likely look internal
>  * Use LOG(ERROR...) macros for incrased prettiness

More crass?

> @@ -2186,21 +2187,36 @@ static void libxl__device_disk_from_xs_b
>      } else {
>          disk->pdev_path = tmp;
>      }
> -    libxl_string_to_backend(ctx,
> -                        libxl__xs_read(gc, XBT_NULL,
> -                                       libxl__sprintf(gc, "%s/type", be_path)),
> -                        &(disk->backend));
> +
> +    
> +    tmp = libxl__xs_read(gc, XBT_NULL,
> +                         libxl__sprintf(gc, "%s/type", be_path));
> +    if (!tmp) {
> +        LOG(ERROR, "Missing xenstore node %s/type", be_path);
> +        return ERROR_FAIL;
> +    }

I've just remembered about libxl__xs_read_checked which effectively
implements the error reporting for you.

Oh, but it accepts ENOENT, so not quite what you need -- nevermind!

Ian.

Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths

[George Dunlap]

On 04/12/12 14:35, Ian Campbell wrote:
> On Fri, 2012-11-30 at 17:13 +0000, George Dunlap wrote:
>> # HG changeset patch
>> # User George Dunlap <george.dunlap@eu.citrix.com>
>> # Date 1354294821 0
>> # Node ID bc2a7645dc2be4a01f2b5ee30d7453cc3d7339aa
>> # Parent  bd041b7426fe10a730994edd98708ff98ae1cb74
>> libxl: Make an internal function explicitly check existence of expected paths
>>
>> libxl__device_disk_from_xs_be() was failing without error for some
>> missing xenstore nodes in a backend, while assuming (without checking)
>> that other nodes were valid, causing a crash when another internal
>> error wrote these nodes in the wrong place.
>>
>> Make this function consistent by:
>> * Checking the existence of all nodes before using
>> * Choosing a default only when the node is not written in device_disk_add()
>> * Failing with log msg if any node written by device_disk_add() is not present
>> * Returning an error on failure
>>
>> Also make the callers of the function pay attention to the error and
>> behave appropriately.
> If libxl__device_disk_from_xs_be returns an error then someone needs to
> cleanup the partial allocations in the disk (pdev_path) probably by
> calling libxl_device_disk_dispose.
>
> It's probably easiest to do this in libxl__device_disk_from_xs_be on
> error rather than modifying all the callers?

Well, there are only two callers, only one of which (it looks like) 
needs a clean-up.  It seems like better design to make each caller do 
its own clean-up.  Let me take a look at that.

  -George

>
> Also libxl__append_disk_list_of_type updates *ndisks early, so if you
> abort half way through initialising the elements of the disks array
> using libxl__device_disk_from_xs_be then the caller will try and free
> some stuff which hasn't been initialised. I think the code needs to
> remember ndisks-in-array separately from
> ndisks-which-I-have-initialised, with the latter becoming the returned
> *ndisks.
>
>> v2:
>>   * Remove "Internal error", as the failure will most likely look internal
>>   * Use LOG(ERROR...) macros for incrased prettiness
> More crass?
>
>> @@ -2186,21 +2187,36 @@ static void libxl__device_disk_from_xs_b
>>       } else {
>>           disk->pdev_path = tmp;
>>       }
>> -    libxl_string_to_backend(ctx,
>> -                        libxl__xs_read(gc, XBT_NULL,
>> -                                       libxl__sprintf(gc, "%s/type", be_path)),
>> -                        &(disk->backend));
>> +
>> +
>> +    tmp = libxl__xs_read(gc, XBT_NULL,
>> +                         libxl__sprintf(gc, "%s/type", be_path));
>> +    if (!tmp) {
>> +        LOG(ERROR, "Missing xenstore node %s/type", be_path);
>> +        return ERROR_FAIL;
>> +    }
> I've just remembered about libxl__xs_read_checked which effectively
> implements the error reporting for you.
>
> Oh, but it accepts ENOENT, so not quite what you need -- nevermind!
>
> Ian.
>
>

[PATCH] libxl: Make an internal function explicitly check existence of expected paths

[George Dunlap]

# HG changeset patch
# User George Dunlap <george.dunlap@eu.citrix.com>
# Date 1354732124 0
# Node ID 21504ec56304ada2f093c30b290ac33c28381ae1
# Parent  670b07e8d7382229639af0d1df30071e6c1ebb19
libxl: Make an internal function explicitly check existence of expected paths

libxl__device_disk_from_xs_be() was failing without error for some
missing xenstore nodes in a backend, while assuming (without checking)
that other nodes were valid, causing a crash when another internal
error wrote these nodes in the wrong place.

Make this function consistent by:
* Checking the existence of all nodes before using
* Choosing a default only when the node is not written in device_disk_add()
* Failing with log msg if any node written by device_disk_add() is not present
* Returning an error on failure
* Disposing of the structure before returning using libxl_device_disk_displose()

Also make the callers of the function pay attention to the error and
behave appropriately.  In the case of libxl__append_disk_list_of_type(),
this means only incrementing *ndisks as the disk structures are
successfully initialized.

v3:
 * Make a failure path in libxl__device_disk_from_be() to free allocations
   from half-initialized disks
 * Modify libxl__append_disk_list_of_type to only update *ndisks as they are
   completed.  This will allow the only caller (libxl_device_disk_list()) to
   clean up the completed disks properly on an error.
v2:
 * Remove "Internal error", as the failure will most likely look internal
 * Use LOG(ERROR...) macros for incrased prettiness

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2169,9 +2169,9 @@ void libxl__device_disk_add(libxl__egc *
     device_disk_add(egc, domid, disk, aodev, NULL, NULL);
 }
 
-static void libxl__device_disk_from_xs_be(libxl__gc *gc,
-                                          const char *be_path,
-                                          libxl_device_disk *disk)
+static int libxl__device_disk_from_xs_be(libxl__gc *gc,
+                                         const char *be_path,
+                                         libxl_device_disk *disk)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned int len;
@@ -2179,6 +2179,7 @@ static void libxl__device_disk_from_xs_b
 
     libxl_device_disk_init(disk);
 
+    /* "params" may not be present; but everything else must be. */
     tmp = xs_read(ctx->xsh, XBT_NULL,
                   libxl__sprintf(gc, "%s/params", be_path), &len);
     if (tmp && strchr(tmp, ':')) {
@@ -2187,21 +2188,36 @@ static void libxl__device_disk_from_xs_b
     } else {
         disk->pdev_path = tmp;
     }
-    libxl_string_to_backend(ctx,
-                        libxl__xs_read(gc, XBT_NULL,
-                                       libxl__sprintf(gc, "%s/type", be_path)),
-                        &(disk->backend));
+
+    
+    tmp = libxl__xs_read(gc, XBT_NULL,
+                         libxl__sprintf(gc, "%s/type", be_path));
+    if (!tmp) {
+        LOG(ERROR, "Missing xenstore node %s/type", be_path);
+        goto cleanup;
+    }
+    libxl_string_to_backend(ctx, tmp, &(disk->backend));
+
     disk->vdev = xs_read(ctx->xsh, XBT_NULL,
                          libxl__sprintf(gc, "%s/dev", be_path), &len);
+    if (!disk->vdev) {
+        LOG(ERROR, "Missing xenstore node %s/dev", be_path);
+        goto cleanup;
+    }
+
     tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf
                          (gc, "%s/removable", be_path));
-
-    if (tmp)
-        disk->removable = atoi(tmp);
-    else
-        disk->removable = 0;
+    if (!tmp) {
+        LOG(ERROR, "Missing xenstore node %s/removable", be_path);
+        goto cleanup;
+    }
+    disk->removable = atoi(tmp);
 
     tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/mode", be_path));
+    if (!tmp) {
+        LOG(ERROR, "Missing xenstore node %s/mode", be_path);
+        goto cleanup;
+    }
     if (!strcmp(tmp, "w"))
         disk->readwrite = 1;
     else
@@ -2209,9 +2225,18 @@ static void libxl__device_disk_from_xs_b
 
     tmp = libxl__xs_read(gc, XBT_NULL,
                          libxl__sprintf(gc, "%s/device-type", be_path));
+    if (!tmp) {
+        LOG(ERROR, "Missing xenstore node %s/device-type", be_path);
+        goto cleanup;
+    }
     disk->is_cdrom = !strcmp(tmp, "cdrom");
 
     disk->format = LIBXL_DISK_FORMAT_UNKNOWN;
+
+    return 0;
+cleanup:
+    libxl_device_disk_dispose(disk);
+    return ERROR_FAIL;
 }
 
 int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t domid,
@@ -2237,9 +2262,7 @@ int libxl_vdev_to_device_disk(libxl_ctx 
     if (!path)
         goto out;
 
-    libxl__device_disk_from_xs_be(gc, path, disk);
-
-    rc = 0;
+    rc = libxl__device_disk_from_xs_be(gc, path, disk);
 out:
     GC_FREE;
     return rc;
@@ -2256,6 +2279,8 @@ static int libxl__append_disk_list_of_ty
     char **dir = NULL;
     unsigned int n = 0;
     libxl_device_disk *pdisk = NULL, *pdisk_end = NULL;
+    int rc=0;
+    int initial_disks = *ndisks;
 
     be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
                              libxl__xs_get_dompath(gc, 0), type, domid);
@@ -2266,17 +2291,19 @@ static int libxl__append_disk_list_of_ty
         if (tmp == NULL)
             return ERROR_NOMEM;
         *disks = tmp;
-        pdisk = *disks + *ndisks;
-        *ndisks += n;
-        pdisk_end = *disks + *ndisks;
+        pdisk = *disks + initial_disks;
+        pdisk_end = *disks + initial_disks + n;
         for (; pdisk < pdisk_end; pdisk++, dir++) {
             const char *p;
             p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
-            libxl__device_disk_from_xs_be(gc, p, pdisk);
+            if ((rc=libxl__device_disk_from_xs_be(gc, p, pdisk)))
+                goto out;
             pdisk->backend_domid = 0;
+            *ndisks += 1;
         }
     }
-    return 0;
+out:
+    return rc;
 }
 
 libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx, uint32_t domid, int *num)

Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths

[George Dunlap]

I've sent another one with a "v3" to help make it clear what's going on...

  -George

On 05/12/12 18:28, George Dunlap wrote:
> # HG changeset patch
> # User George Dunlap <george.dunlap@eu.citrix.com>
> # Date 1354732124 0
> # Node ID 21504ec56304ada2f093c30b290ac33c28381ae1
> # Parent  670b07e8d7382229639af0d1df30071e6c1ebb19
> libxl: Make an internal function explicitly check existence of expected paths
>
> libxl__device_disk_from_xs_be() was failing without error for some
> missing xenstore nodes in a backend, while assuming (without checking)
> that other nodes were valid, causing a crash when another internal
> error wrote these nodes in the wrong place.
>
> Make this function consistent by:
> * Checking the existence of all nodes before using
> * Choosing a default only when the node is not written in device_disk_add()
> * Failing with log msg if any node written by device_disk_add() is not present
> * Returning an error on failure
> * Disposing of the structure before returning using libxl_device_disk_displose()
>
> Also make the callers of the function pay attention to the error and
> behave appropriately.  In the case of libxl__append_disk_list_of_type(),
> this means only incrementing *ndisks as the disk structures are
> successfully initialized.
>
> v3:
>   * Make a failure path in libxl__device_disk_from_be() to free allocations
>     from half-initialized disks
>   * Modify libxl__append_disk_list_of_type to only update *ndisks as they are
>     completed.  This will allow the only caller (libxl_device_disk_list()) to
>     clean up the completed disks properly on an error.
> v2:
>   * Remove "Internal error", as the failure will most likely look internal
>   * Use LOG(ERROR...) macros for incrased prettiness
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2169,9 +2169,9 @@ void libxl__device_disk_add(libxl__egc *
>       device_disk_add(egc, domid, disk, aodev, NULL, NULL);
>   }
>   
> -static void libxl__device_disk_from_xs_be(libxl__gc *gc,
> -                                          const char *be_path,
> -                                          libxl_device_disk *disk)
> +static int libxl__device_disk_from_xs_be(libxl__gc *gc,
> +                                         const char *be_path,
> +                                         libxl_device_disk *disk)
>   {
>       libxl_ctx *ctx = libxl__gc_owner(gc);
>       unsigned int len;
> @@ -2179,6 +2179,7 @@ static void libxl__device_disk_from_xs_b
>   
>       libxl_device_disk_init(disk);
>   
> +    /* "params" may not be present; but everything else must be. */
>       tmp = xs_read(ctx->xsh, XBT_NULL,
>                     libxl__sprintf(gc, "%s/params", be_path), &len);
>       if (tmp && strchr(tmp, ':')) {
> @@ -2187,21 +2188,36 @@ static void libxl__device_disk_from_xs_b
>       } else {
>           disk->pdev_path = tmp;
>       }
> -    libxl_string_to_backend(ctx,
> -                        libxl__xs_read(gc, XBT_NULL,
> -                                       libxl__sprintf(gc, "%s/type", be_path)),
> -                        &(disk->backend));
> +
> +
> +    tmp = libxl__xs_read(gc, XBT_NULL,
> +                         libxl__sprintf(gc, "%s/type", be_path));
> +    if (!tmp) {
> +        LOG(ERROR, "Missing xenstore node %s/type", be_path);
> +        goto cleanup;
> +    }
> +    libxl_string_to_backend(ctx, tmp, &(disk->backend));
> +
>       disk->vdev = xs_read(ctx->xsh, XBT_NULL,
>                            libxl__sprintf(gc, "%s/dev", be_path), &len);
> +    if (!disk->vdev) {
> +        LOG(ERROR, "Missing xenstore node %s/dev", be_path);
> +        goto cleanup;
> +    }
> +
>       tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf
>                            (gc, "%s/removable", be_path));
> -
> -    if (tmp)
> -        disk->removable = atoi(tmp);
> -    else
> -        disk->removable = 0;
> +    if (!tmp) {
> +        LOG(ERROR, "Missing xenstore node %s/removable", be_path);
> +        goto cleanup;
> +    }
> +    disk->removable = atoi(tmp);
>   
>       tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/mode", be_path));
> +    if (!tmp) {
> +        LOG(ERROR, "Missing xenstore node %s/mode", be_path);
> +        goto cleanup;
> +    }
>       if (!strcmp(tmp, "w"))
>           disk->readwrite = 1;
>       else
> @@ -2209,9 +2225,18 @@ static void libxl__device_disk_from_xs_b
>   
>       tmp = libxl__xs_read(gc, XBT_NULL,
>                            libxl__sprintf(gc, "%s/device-type", be_path));
> +    if (!tmp) {
> +        LOG(ERROR, "Missing xenstore node %s/device-type", be_path);
> +        goto cleanup;
> +    }
>       disk->is_cdrom = !strcmp(tmp, "cdrom");
>   
>       disk->format = LIBXL_DISK_FORMAT_UNKNOWN;
> +
> +    return 0;
> +cleanup:
> +    libxl_device_disk_dispose(disk);
> +    return ERROR_FAIL;
>   }
>   
>   int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t domid,
> @@ -2237,9 +2262,7 @@ int libxl_vdev_to_device_disk(libxl_ctx
>       if (!path)
>           goto out;
>   
> -    libxl__device_disk_from_xs_be(gc, path, disk);
> -
> -    rc = 0;
> +    rc = libxl__device_disk_from_xs_be(gc, path, disk);
>   out:
>       GC_FREE;
>       return rc;
> @@ -2256,6 +2279,8 @@ static int libxl__append_disk_list_of_ty
>       char **dir = NULL;
>       unsigned int n = 0;
>       libxl_device_disk *pdisk = NULL, *pdisk_end = NULL;
> +    int rc=0;
> +    int initial_disks = *ndisks;
>   
>       be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
>                                libxl__xs_get_dompath(gc, 0), type, domid);
> @@ -2266,17 +2291,19 @@ static int libxl__append_disk_list_of_ty
>           if (tmp == NULL)
>               return ERROR_NOMEM;
>           *disks = tmp;
> -        pdisk = *disks + *ndisks;
> -        *ndisks += n;
> -        pdisk_end = *disks + *ndisks;
> +        pdisk = *disks + initial_disks;
> +        pdisk_end = *disks + initial_disks + n;
>           for (; pdisk < pdisk_end; pdisk++, dir++) {
>               const char *p;
>               p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
> -            libxl__device_disk_from_xs_be(gc, p, pdisk);
> +            if ((rc=libxl__device_disk_from_xs_be(gc, p, pdisk)))
> +                goto out;
>               pdisk->backend_domid = 0;
> +            *ndisks += 1;
>           }
>       }
> -    return 0;
> +out:
> +    return rc;
>   }
>   
>   libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx, uint32_t domid, int *num)