[PATCH V2] libxl: don't add cache mode for qdisk cdrom drives

[PATCH V2] libxl: don't add cache mode for qdisk cdrom drives

[Jim Fehlig]

qemu commit 91a097e7 forbids specifying cache mode for empty
drives. Attempting to create a domain with an empty qdisk cdrom
drive results in

qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom,
   cache=writeback,id=ide-832: Must specify either driver or file

libxl only allows an empty 'target=' for cdroms. By default, cdroms
are readonly (see the 'access' parameter in xl-disk-configuration.txt)
and forced to readonly by any tools (e.g. xl) using libxlutil's
xlu_disk_parse() function. With cdroms always marked readonly,
explicitly specifying the cache mode for cdrom drives can be dropped.
The drive's 'readonly=on' option can also be set unconditionally.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---

V2:
Drop explicitly setting cache mode since cdrom devices are
readonly.
Unconditionally add 'readonly=on' drive option for cdroms.

 tools/libxl/libxl_dm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index fd12844..959e292 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1368,8 +1368,8 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
 
             if (disks[i].is_cdrom) {
                 drive = libxl__sprintf(gc,
-                         "if=ide,index=%d,readonly=%s,media=cdrom,cache=writeback,id=ide-%i",
-                         disk, disks[i].readwrite ? "off" : "on", dev_number);
+                         "if=ide,index=%d,readonly=on,media=cdrom,id=ide-%i",
+                         disk, dev_number);
 
                 if (target_path)
                     drive = libxl__sprintf(gc, "%s,file=%s,format=%s",
-- 
1.8.0.1


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

Re: [PATCH V2] libxl: don't add cache mode for qdisk cdrom drives

[Wei Liu]

On Thu, Apr 28, 2016 at 03:20:46PM -0600, Jim Fehlig wrote:
> qemu commit 91a097e7 forbids specifying cache mode for empty
> drives. Attempting to create a domain with an empty qdisk cdrom
> drive results in
> 
> qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom,
>    cache=writeback,id=ide-832: Must specify either driver or file
> 

We need to fix this one way or another.

> libxl only allows an empty 'target=' for cdroms. By default, cdroms
> are readonly (see the 'access' parameter in xl-disk-configuration.txt)
> and forced to readonly by any tools (e.g. xl) using libxlutil's
> xlu_disk_parse() function. With cdroms always marked readonly,
> explicitly specifying the cache mode for cdrom drives can be dropped.
> The drive's 'readonly=on' option can also be set unconditionally.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>

CC Stefano who made the change to use writeback in
e9a327bbbcab127625b0917a2780cb3601a81d01 . Also CC Anthony.

Ian, Stefano and Anthony, do you have opinion on this patch?

> ---
> 
> V2:
> Drop explicitly setting cache mode since cdrom devices are
> readonly.
> Unconditionally add 'readonly=on' drive option for cdroms.
> 
>  tools/libxl/libxl_dm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index fd12844..959e292 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1368,8 +1368,8 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>  
>              if (disks[i].is_cdrom) {
>                  drive = libxl__sprintf(gc,
> -                         "if=ide,index=%d,readonly=%s,media=cdrom,cache=writeback,id=ide-%i",
> -                         disk, disks[i].readwrite ? "off" : "on", dev_number);
> +                         "if=ide,index=%d,readonly=on,media=cdrom,id=ide-%i",
> +                         disk, dev_number);
>  
>                  if (target_path)
>                      drive = libxl__sprintf(gc, "%s,file=%s,format=%s",
> -- 
> 1.8.0.1
> 

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

Re: [PATCH V2] libxl: don't add cache mode for qdisk cdrom drives

[Stefano Stabellini]

On Mon, 9 May 2016, Wei Liu wrote:
> On Thu, Apr 28, 2016 at 03:20:46PM -0600, Jim Fehlig wrote:
> > qemu commit 91a097e7 forbids specifying cache mode for empty
> > drives. Attempting to create a domain with an empty qdisk cdrom
> > drive results in
> > 
> > qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom,
> >    cache=writeback,id=ide-832: Must specify either driver or file
> > 
> 
> We need to fix this one way or another.
> 
> > libxl only allows an empty 'target=' for cdroms. By default, cdroms
> > are readonly (see the 'access' parameter in xl-disk-configuration.txt)
> > and forced to readonly by any tools (e.g. xl) using libxlutil's
> > xlu_disk_parse() function. With cdroms always marked readonly,
> > explicitly specifying the cache mode for cdrom drives can be dropped.
> > The drive's 'readonly=on' option can also be set unconditionally.
> > 
> > Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> 
> CC Stefano who made the change to use writeback in
> e9a327bbbcab127625b0917a2780cb3601a81d01 . Also CC Anthony.
> 
> Ian, Stefano and Anthony, do you have opinion on this patch?

It looks good to me. We could consider to backport it, given that we
only have a loose relationship with QEMU and older versions of Xen could
end up running with newer versions of QEMU.


> > 
> > V2:
> > Drop explicitly setting cache mode since cdrom devices are
> > readonly.
> > Unconditionally add 'readonly=on' drive option for cdroms.
> > 
> >  tools/libxl/libxl_dm.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index fd12844..959e292 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -1368,8 +1368,8 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
> >  
> >              if (disks[i].is_cdrom) {
> >                  drive = libxl__sprintf(gc,
> > -                         "if=ide,index=%d,readonly=%s,media=cdrom,cache=writeback,id=ide-%i",
> > -                         disk, disks[i].readwrite ? "off" : "on", dev_number);
> > +                         "if=ide,index=%d,readonly=on,media=cdrom,id=ide-%i",
> > +                         disk, dev_number);
> >  
> >                  if (target_path)
> >                      drive = libxl__sprintf(gc, "%s,file=%s,format=%s",
> > -- 
> > 1.8.0.1
> > 
> 
> _______________________________________________
> 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

Re: [PATCH V2] libxl: don't add cache mode for qdisk cdrom drives

[Jim Fehlig]

On 05/10/2016 04:06 AM, Stefano Stabellini wrote:
> On Mon, 9 May 2016, Wei Liu wrote:
>> On Thu, Apr 28, 2016 at 03:20:46PM -0600, Jim Fehlig wrote:
>>> qemu commit 91a097e7 forbids specifying cache mode for empty
>>> drives. Attempting to create a domain with an empty qdisk cdrom
>>> drive results in
>>>
>>> qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom,
>>>    cache=writeback,id=ide-832: Must specify either driver or file
>>>
>> We need to fix this one way or another.
>>
>>> libxl only allows an empty 'target=' for cdroms. By default, cdroms
>>> are readonly (see the 'access' parameter in xl-disk-configuration.txt)
>>> and forced to readonly by any tools (e.g. xl) using libxlutil's
>>> xlu_disk_parse() function. With cdroms always marked readonly,
>>> explicitly specifying the cache mode for cdrom drives can be dropped.
>>> The drive's 'readonly=on' option can also be set unconditionally.
>>>
>>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> CC Stefano who made the change to use writeback in
>> e9a327bbbcab127625b0917a2780cb3601a81d01 . Also CC Anthony.
>>
>> Ian, Stefano and Anthony, do you have opinion on this patch?
> It looks good to me. We could consider to backport it, given that we
> only have a loose relationship with QEMU and older versions of Xen could
> end up running with newer versions of QEMU.

Wei,

Will this patch make 4.7? It allows empty cdroms with freshly released QEMU 2.6 :-).

Regards,
Jim


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

Re: [PATCH V2] libxl: don't add cache mode for qdisk cdrom drives

[Wei Liu]

On Fri, May 13, 2016 at 02:36:18PM -0600, Jim Fehlig wrote:
> On 05/10/2016 04:06 AM, Stefano Stabellini wrote:
> > On Mon, 9 May 2016, Wei Liu wrote:
> >> On Thu, Apr 28, 2016 at 03:20:46PM -0600, Jim Fehlig wrote:
> >>> qemu commit 91a097e7 forbids specifying cache mode for empty
> >>> drives. Attempting to create a domain with an empty qdisk cdrom
> >>> drive results in
> >>>
> >>> qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom,
> >>>    cache=writeback,id=ide-832: Must specify either driver or file
> >>>
> >> We need to fix this one way or another.
> >>
> >>> libxl only allows an empty 'target=' for cdroms. By default, cdroms
> >>> are readonly (see the 'access' parameter in xl-disk-configuration.txt)
> >>> and forced to readonly by any tools (e.g. xl) using libxlutil's
> >>> xlu_disk_parse() function. With cdroms always marked readonly,
> >>> explicitly specifying the cache mode for cdrom drives can be dropped.
> >>> The drive's 'readonly=on' option can also be set unconditionally.
> >>>
> >>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> >> CC Stefano who made the change to use writeback in
> >> e9a327bbbcab127625b0917a2780cb3601a81d01 . Also CC Anthony.
> >>
> >> Ian, Stefano and Anthony, do you have opinion on this patch?
> > It looks good to me. We could consider to backport it, given that we
> > only have a loose relationship with QEMU and older versions of Xen could
> > end up running with newer versions of QEMU.
> 
> Wei,
> 
> Will this patch make 4.7? It allows empty cdroms with freshly released QEMU 2.6 :-).
> 

Yes, it's on my radar. I wanted to give some time for other people to
look at it.

Now two weeks has passed, I've queued it up for committing.

Wei.


> Regards,
> Jim
> 

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