Re: [Xen-devel] [PATCH] xl: support empty CDROM devices

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-07-25 at 17:06 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH] xl: support empty CDROM 
> devices"):
> > On Wed, 2012-07-25 at 11:15 +0100, Ian Campbell wrote:
> > > xl: support empty CDROM devices
> > 
> > It seems like I forgot to qrefresh before sending this.
> > 
> > I supect that given your updates these differences are now irrelevant
> > but FTR the incremental bit which was omitted because of this was:
> 
> Thanks.
> 
> > diff -r 097bf63027e0 docs/misc/xl-disk-configuration.txt
> > --- a/docs/misc/xl-disk-configuration.txt   Wed Jul 25 11:21:55 2012 +0100
> > +++ b/docs/misc/xl-disk-configuration.txt   Wed Jul 25 17:00:00 2012 +0100
> > @@ -91,8 +91,9 @@ Supported values:      raw, qcow, qcow2,
> >  Deprecated values:     None
> >  Default value:         raw
> >  
> > -Format cannot be specified as a positional parameter when target is
> > -not provided (the empty CDROM case)
> > +When "target" has been provided as a positional parameter and is empty
> > +(the empty CDROM case referred to above) then this field is implicitly
> > +"empty" and must not be specified as a positional parameter.
> 
> This is no longer true in my patch.
> 
> >  expected <<EOF
> > +EOF
> > +one 255 ,,hdc:cdrom,r
> > +
> > +expected <<EOF
> 
> I have this already.
> 
> > diff -r 097bf63027e0 tools/libxl/libxlu_disk.c
> > --- a/tools/libxl/libxlu_disk.c     Wed Jul 25 11:21:55 2012 +0100
> > +++ b/tools/libxl/libxlu_disk.c     Wed Jul 25 17:00:00 2012 +0100
> > @@ -78,6 +78,8 @@ int xlu_disk_parse(XLU_Config *cfg,
> >          disk->readwrite = 0;
> >          if (!disk->pdev_path || !strcmp(disk->pdev_path, ""))
> >              disk->format = LIBXL_DISK_FORMAT_EMPTY;
> > +    } else if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
> > +        disk->format = LIBXL_DISK_FORMAT_RAW;
> >      }
> 
> This is rather odd.  It appears to turn empty non-cdroms into RAW.  Is
> that actually correct ?  It doesn't seem likely to me that it is.  I
> think my new arrangements don't generate empty non-cdroms unless the
> user explicitly specifies `empty' as the format or uses the xend
> compatibility syntax and explicitly specifies `:disk'.

I think empty is meaningless for anything except cdroms. This was here
because in my version the parser didn't know it had a cdrom at the point
where it had to decide to make the device empty so I fixed it up here. I
think you are right that your version doesn't require it.

> 
> Ian.



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