George Dunlap
2012-Nov-23 15:56 UTC
[PATCH] libxl: Make an internal function explicitly check existence of expected paths
# 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;
}
}
Ian Campbell
2012-Nov-27 11:44 UTC
Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths
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.
George Dunlap
2012-Nov-27 11:50 UTC
Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths
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
Ian Campbell
2012-Nov-27 11:55 UTC
Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths
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.
George Dunlap
2012-Nov-30 17:13 UTC
[PATCH] libxl: Make an internal function explicitly check existence of expected paths
# 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;
}
}
Ian Campbell
2012-Dec-04 14:35 UTC
Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths
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 prettinessMore 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.
George Dunlap
2012-Dec-05 14:34 UTC
Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths
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. > >
George Dunlap
2012-Dec-05 18:28 UTC
[PATCH] 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)
George Dunlap
2012-Dec-05 18:29 UTC
Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths
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)