George Dunlap
2012-Dec-05 18:29 UTC
[PATCH v3] libxl: Make an internal function explicitly check existence of expected paths
# 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)
Ian Campbell
2012-Dec-06 10:19 UTC
Re: [PATCH v3] libxl: Make an internal function explicitly check existence of expected paths
On Wed, 2012-12-05 at 18:29 +0000, 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.[...]> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>Acked + applied, thanks.