George Dunlap
2012-Nov-21 17:29 UTC
[PATCH] libxl: Fix bug in libxl_cdrom_insert, make more robust against bad xenstore data
# HG changeset patch
# User George Dunlap <george.dunlap@eu.citrix.com>
# Date 1353518844 0
# Node ID a4f707f6049a4a8152d2886f1b1d49f9e70ef5eb
# Parent ae6fb202b233af815466055d9f1a635802a50855
libxl: Fix bug in libxl_cdrom_insert, make more robust against bad xenstore data
libxl_cdrom_insert was failing to initialize the backend type,
resulting in the wrong default backend. The result was not only that
the CD was not inserted properly, but also that some improper xenstore
entries were created, causing further block commands to fail.
This patch fixes the bug by setting the disk backend type based on the
type of the existing device.
It also makes the system more robust by:
* Checking to see that it has got a valid path before proceeding to
write a partial xenstore entry
* Handling non-existent nodes in the backend xenstore entries more
gracefully
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
@@ -2201,14 +2201,14 @@ static void libxl__device_disk_from_xs_b
disk->removable = 0;
tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/mode",
be_path));
- if (!strcmp(tmp, "w"))
+ if (tmp && !strcmp(tmp, "w"))
disk->readwrite = 1;
else
disk->readwrite = 0;
tmp = libxl__xs_read(gc, XBT_NULL,
libxl__sprintf(gc, "%s/device-type",
be_path));
- disk->is_cdrom = !strcmp(tmp, "cdrom");
+ disk->is_cdrom = tmp && !strcmp(tmp, "cdrom");
disk->format = LIBXL_DISK_FORMAT_UNKNOWN;
}
@@ -2353,6 +2353,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
libxl__device device;
const char * path;
+ char * tmp;
flexarray_t *insert = NULL;
@@ -2383,8 +2384,11 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
disks = libxl_device_disk_list(ctx, domid, &num);
for (i = 0; i < num; i++) {
if (disks[i].is_cdrom && !strcmp(disk->vdev, disks[i].vdev))
- /* found */
+ {
+ /* Found. Set backend type appropriately. */
+ disk->backend=disks[i].backend;
break;
+ }
}
if (i == num) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual device not
found");
@@ -2410,6 +2414,17 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
path = libxl__device_backend_path(gc, &device);
+ /* Sanity check: make sure the backend exists before writing here */
+ tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
"%s/frontend", path));
+ if (!tmp)
+ {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Internal error: %s does not
exist",
+ libxl__sprintf(gc, "%s/frontend", path));
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+
insert = flexarray_make(gc, 4, 1);
flexarray_append_pair(insert, "type",
Fabio Fantoni
2012-Nov-22 14:16 UTC
Re: [PATCH] libxl: Fix bug in libxl_cdrom_insert, make more robust against bad xenstore data
Ian Campbell
2012-Nov-23 10:05 UTC
Re: [PATCH] libxl: Fix bug in libxl_cdrom_insert, make more robust against bad xenstore data
On Wed, 2012-11-21 at 17:29 +0000, George Dunlap wrote:> # HG changeset patch > # User George Dunlap <george.dunlap@eu.citrix.com> > # Date 1353518844 0 > # Node ID a4f707f6049a4a8152d2886f1b1d49f9e70ef5eb > # Parent ae6fb202b233af815466055d9f1a635802a50855 > libxl: Fix bug in libxl_cdrom_insert, make more robust against bad xenstore data > > libxl_cdrom_insert was failing to initialize the backend type, > resulting in the wrong default backend. The result was not only that > the CD was not inserted properly, but also that some improper xenstore > entries were created, causing further block commands to fail. > > This patch fixes the bug by setting the disk backend type based on the > type of the existing device. > > It also makes the system more robust by: > * Checking to see that it has got a valid path before proceeding to > write a partial xenstore entryThe above all looked good to me. Acked-by: Ian Campbell <ian.campbell@citrix.com> for those bits.> * Handling non-existent nodes in the backend xenstore entries more > gracefullyI wonder if it should error out here rather than assume a default -- the nodes in question should have been written by libxl itself (I think, I don''t think these are backend provided keys) so if they are missing something has gone pretty wrong in the library, I think it''s worth at least a log message and more than likely an error return.> 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 > @@ -2201,14 +2201,14 @@ static void libxl__device_disk_from_xs_b > disk->removable = 0; > > tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/mode", be_path)); > - if (!strcmp(tmp, "w")) > + if (tmp && !strcmp(tmp, "w")) > disk->readwrite = 1; > else > disk->readwrite = 0; > > tmp = libxl__xs_read(gc, XBT_NULL, > libxl__sprintf(gc, "%s/device-type", be_path)); > - disk->is_cdrom = !strcmp(tmp, "cdrom"); > + disk->is_cdrom = tmp && !strcmp(tmp, "cdrom"); > > disk->format = LIBXL_DISK_FORMAT_UNKNOWN; > } > @@ -2353,6 +2353,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u > > libxl__device device; > const char * path; > + char * tmp; > > flexarray_t *insert = NULL; > > @@ -2383,8 +2384,11 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u > disks = libxl_device_disk_list(ctx, domid, &num); > for (i = 0; i < num; i++) { > if (disks[i].is_cdrom && !strcmp(disk->vdev, disks[i].vdev)) > - /* found */ > + { > + /* Found. Set backend type appropriately. */ > + disk->backend=disks[i].backend; > break; > + } > } > if (i == num) { > LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual device not found"); > @@ -2410,6 +2414,17 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u > > path = libxl__device_backend_path(gc, &device); > > + /* Sanity check: make sure the backend exists before writing here */ > + tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/frontend", path)); > + if (!tmp) > + { > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Internal error: %s does not exist", > + libxl__sprintf(gc, "%s/frontend", path)); > + rc = ERROR_FAIL; > + goto out; > + } > + > + > insert = flexarray_make(gc, 4, 1); > > flexarray_append_pair(insert, "type", > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
George Dunlap
2012-Nov-23 11:37 UTC
Re: [PATCH] libxl: Fix bug in libxl_cdrom_insert, make more robust against bad xenstore data
On Fri, Nov 23, 2012 at 10:05 AM, Ian Campbell <Ian.Campbell@citrix.com>wrote:> > > * Handling non-existent nodes in the backend xenstore entries more > > gracefully > > I wonder if it should error out here rather than assume a default -- the > nodes in question should have been written by libxl itself (I think, I > don''t think these are backend provided keys) so if they are missing > something has gone pretty wrong in the library, I think it''s worth at > least a log message and more than likely an error return. >I would have returned an error, but the fuction returns void. :-) That leaves us the options of: 1. Erroring and calling exit() in the middle of the function 2. Just doing a warning 3. Modifying the function to return success / failure #3 seems like the it shouldn''t be too very difficult. If that sounds good to you I''ll re-post this patch with only the bits you''ve acked, then post another patch with #3. -George> > > 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 > > @@ -2201,14 +2201,14 @@ static void libxl__device_disk_from_xs_b > > disk->removable = 0; > > > > tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/mode", > be_path)); > > - if (!strcmp(tmp, "w")) > > + if (tmp && !strcmp(tmp, "w")) > > disk->readwrite = 1; > > else > > disk->readwrite = 0; > > > > tmp = libxl__xs_read(gc, XBT_NULL, > > libxl__sprintf(gc, "%s/device-type", be_path)); > > - disk->is_cdrom = !strcmp(tmp, "cdrom"); > > + disk->is_cdrom = tmp && !strcmp(tmp, "cdrom"); > > > > disk->format = LIBXL_DISK_FORMAT_UNKNOWN; > > } > > @@ -2353,6 +2353,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u > > > > libxl__device device; > > const char * path; > > + char * tmp; > > > > flexarray_t *insert = NULL; > > > > @@ -2383,8 +2384,11 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u > > disks = libxl_device_disk_list(ctx, domid, &num); > > for (i = 0; i < num; i++) { > > if (disks[i].is_cdrom && !strcmp(disk->vdev, disks[i].vdev)) > > - /* found */ > > + { > > + /* Found. Set backend type appropriately. */ > > + disk->backend=disks[i].backend; > > break; > > + } > > } > > if (i == num) { > > LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual device not found"); > > @@ -2410,6 +2414,17 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u > > > > path = libxl__device_backend_path(gc, &device); > > > > + /* Sanity check: make sure the backend exists before writing here */ > > + tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, > "%s/frontend", path)); > > + if (!tmp) > > + { > > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Internal error: %s does not > exist", > > + libxl__sprintf(gc, "%s/frontend", path)); > > + rc = ERROR_FAIL; > > + goto out; > > + } > > + > > + > > insert = flexarray_make(gc, 4, 1); > > > > flexarray_append_pair(insert, "type", > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2012-Nov-23 11:48 UTC
Re: [PATCH] libxl: Fix bug in libxl_cdrom_insert, make more robust against bad xenstore data
On Fri, 2012-11-23 at 11:37 +0000, George Dunlap wrote:> I would have returned an error, but the fuction returns void. :-) > That leaves us the options of: > 1. Erroring and calling exit() in the middle of the function > 2. Just doing a warning > 3. Modifying the function to return success / failure > > #3 seems like the it shouldn''t be too very difficult.Yes, it looks like both the callers should manage to do something sane pretty easily.> If that sounds good to you I''ll re-post this patch with only the > bits you''ve acked, then post another patch with #3.Sounds good. Ian.