Re: [Xen-devel] [PATCH for-4.6] libxl: handle read-only drives with qemu-xen

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Sep 11, 2015 at 06:00:29PM +0100, Stefano Stabellini wrote:
> The current libxl code doesn't deal with read-only drives at all.
> 
> Upstream QEMU and qemu-xen only support read-only cdrom drives: make
> sure to specify "readonly=on" for cdrom drives and return error in case
> the user requested a non-cdrom read-only drive.
> 

The commit message speaks about both upstream QEMU and traditional QEMU,
but the code only touches upstream routine. I know that QEMU-trad uses
different method to get disk parameters so _args_old is not an option,
but perhaps something else is missing? Or is it because QEMU-trad is
doing the right thing already?

> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> ---
>  tools/libxl/libxl_dm.c |   13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 02c0162..468ff9c 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1110,13 +1110,18 @@ static int 
> libxl__build_device_model_args_new(libxl__gc *gc,
>              if (disks[i].is_cdrom) {
>                  if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY)
>                      drive = libxl__sprintf
> -                        (gc, 
> "if=ide,index=%d,media=cdrom,cache=writeback,id=ide-%i",
> -                         disk, dev_number);
> +                        (gc, 
> "if=ide,index=%d,readonly=%s,media=cdrom,cache=writeback,id=ide-%i",
> +                         disk, disks[i].readwrite ? "off" : "on", 
> dev_number);
>                  else
>                      drive = libxl__sprintf
> -                        (gc, 
> "file=%s,if=ide,index=%d,media=cdrom,format=%s,cache=writeback,id=ide-%i",
> -                         disks[i].pdev_path, disk, format, dev_number);
> +                        (gc, 
> "file=%s,if=ide,index=%d,readonly=%s,media=cdrom,format=%s,cache=writeback,id=ide-%i",
> +                         disks[i].pdev_path, disk, disks[i].readwrite ? 
> "off" : "on", format, dev_number);
>              } else {
> +                if (!disks[i].readwrite) {
> +                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "QEMU doesn't support 
> read-only disk drivers");

I think the "new" way of doing thing is to use LOG() macro. But for the
sake of consistency I think this is fine.

I shall look into writing a spatch to covert all LIBXL__LOG macros to
LOG.

Wei.

> +                    return ERROR_INVAL;
> +                }
> +
>                  if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
>                      LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "cannot support"
>                                 " empty disk format for %s", disks[i].vdev);
> -- 
> 1.7.10.4

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