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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

# HG changeset patch
# User George Dunlap <george.dunlap@xxxxxxxxxxxxx>
# 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@xxxxxxxxxxxxx>

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;
         }
     }

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel