Re: [Xen-devel] [PATCH] libxl: Fix bug in libxl_cdrom_insert, make more robust against bad xenstore data

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-11-21 at 17:29 +0000, George Dunlap wrote:
> # HG changeset patch
> # User George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> # 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

The above all looked good to me.
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
for those bits.

> * 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.

> 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
> @@ -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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



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