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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 14 Sep 2015, Wei Liu wrote:
> 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?

I know that QEMU naming is very confusing, but qemu-xen is the upstream
QEMU based tree which xen-unstable uses. We are only talking about
upstream QEMU based trees here.




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