* [PATCH] libxl: Make an internal function explicitly check existence of expected paths
@ 2012-12-05 18:28 George Dunlap
2012-12-05 18:29 ` George Dunlap
0 siblings, 1 reply; 9+ messages in thread
From: George Dunlap @ 2012-12-05 18:28 UTC (permalink / raw)
To: xen-devel; +Cc: 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)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths
2012-12-05 18:28 [PATCH] libxl: Make an internal function explicitly check existence of expected paths George Dunlap
@ 2012-12-05 18:29 ` George Dunlap
0 siblings, 0 replies; 9+ messages in thread
From: George Dunlap @ 2012-12-05 18:29 UTC (permalink / raw)
To: xen-devel@lists.xensource.com
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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] libxl: Make an internal function explicitly check existence of expected paths
@ 2012-11-30 17:13 George Dunlap
2012-12-04 14:35 ` Ian Campbell
0 siblings, 1 reply; 9+ messages in thread
From: George Dunlap @ 2012-11-30 17:13 UTC (permalink / raw)
To: xen-devel; +Cc: 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;
}
}
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths
2012-11-30 17:13 George Dunlap
@ 2012-12-04 14:35 ` Ian Campbell
2012-12-05 14:34 ` George Dunlap
0 siblings, 1 reply; 9+ messages in thread
From: Ian Campbell @ 2012-12-04 14:35 UTC (permalink / raw)
To: George Dunlap; +Cc: xen-devel@lists.xensource.com
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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths
2012-12-04 14:35 ` Ian Campbell
@ 2012-12-05 14:34 ` George Dunlap
0 siblings, 0 replies; 9+ messages in thread
From: George Dunlap @ 2012-12-05 14:34 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel@lists.xensource.com
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.
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] libxl: Make an internal function explicitly check existence of expected paths
@ 2012-11-23 15:56 George Dunlap
2012-11-27 11:44 ` Ian Campbell
0 siblings, 1 reply; 9+ messages in thread
From: George Dunlap @ 2012-11-23 15:56 UTC (permalink / raw)
To: xen-devel; +Cc: 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;
}
}
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths
2012-11-23 15:56 George Dunlap
@ 2012-11-27 11:44 ` Ian Campbell
2012-11-27 11:50 ` George Dunlap
0 siblings, 1 reply; 9+ messages in thread
From: Ian Campbell @ 2012-11-27 11:44 UTC (permalink / raw)
To: George Dunlap; +Cc: xen-devel@lists.xensource.com
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths
2012-11-27 11:44 ` Ian Campbell
@ 2012-11-27 11:50 ` George Dunlap
2012-11-27 11:55 ` Ian Campbell
0 siblings, 1 reply; 9+ messages in thread
From: George Dunlap @ 2012-11-27 11:50 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel@lists.xensource.com
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths
2012-11-27 11:50 ` George Dunlap
@ 2012-11-27 11:55 ` Ian Campbell
0 siblings, 0 replies; 9+ messages in thread
From: Ian Campbell @ 2012-11-27 11:55 UTC (permalink / raw)
To: George Dunlap; +Cc: xen-devel@lists.xensource.com
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-12-05 18:29 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-05 18:28 [PATCH] libxl: Make an internal function explicitly check existence of expected paths George Dunlap
2012-12-05 18:29 ` George Dunlap
-- strict thread matches above, loose matches on Subject: below --
2012-11-30 17:13 George Dunlap
2012-12-04 14:35 ` Ian Campbell
2012-12-05 14:34 ` George Dunlap
2012-11-23 15:56 George Dunlap
2012-11-27 11:44 ` Ian Campbell
2012-11-27 11:50 ` George Dunlap
2012-11-27 11:55 ` Ian Campbell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).