[Qemu-devel] Regression opening read-only cdroms

[Qemu-devel] Regression opening read-only cdroms

[Avi Kivity]

qemu used to be quite happy opening read-only cdrom images, and I was 
quite happy feeding kvm-autotest a library of read-only iso images.  
However since

    commit 0e1d8f4c549e51fd19793a154862979fdc199477
    Author: Christoph Hellwig <hch@lst.de>
    Date:   Mon Jun 15 13:53:26 2009 +0200

         raw-posix: always store open flags

         Both the Linux floppy and the FreeBSD CDROM host device need to
    store
         the open flags so that they can re-open the device later. 
    Store the
         open flags unconditionally to remove the ifdef mess and simply the
         calling conventions for the later patches in the series.

         Signed-off-by: Christoph Hellwig <hch@lst.de>

this state of happiness no longer exists.

Can you look at what caused this?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Regression opening read-only cdroms

[Jamie Lokier]

Avi Kivity wrote:
> qemu used to be quite happy opening read-only cdrom images, and I was 
> quite happy feeding kvm-autotest a library of read-only iso images.

1. While we're here, an _option_ to open an image read-only even when
   you have write permission would be useful, for those occasions when
   you want to boot from some valuable image and be certain you aren't
   modifying it - without having to chmod back and forth in
   Qemu-wrapper scripts, or copy the image first.

2. Would it make sense to open CD-ROM images read-only all the time,
   when they are opened with media=cdrom, or can QEMU's CD-ROM devices
   write to the images?

-- Jamie

Re: [Qemu-devel] Regression opening read-only cdroms

[Stefano Stabellini]

Jamie Lokier wrote:

> Avi Kivity wrote:
>> qemu used to be quite happy opening read-only cdrom images, and I was 
>> quite happy feeding kvm-autotest a library of read-only iso images.
> 
> 1. While we're here, an _option_ to open an image read-only even when
>    you have write permission would be useful, for those occasions when
>    you want to boot from some valuable image and be certain you aren't
>    modifying it - without having to chmod back and forth in
>    Qemu-wrapper scripts, or copy the image first.


Indeed.
I have a patch that adds bdrv_set_read_only that set bs->read_only = 1
so that bdrv_open2 can check if the flag is set and act accordingly.
The problem is that in xen we read whether the device should be opened
read only in the VM config file, I don't think the user can specify to
open a drive read-only in qemu.

 
> 2. Would it make sense to open CD-ROM images read-only all the time,
>    when they are opened with media=cdrom, or can QEMU's CD-ROM devices
>    write to the images?
> 

not at the moment, it may  be possible in the future.

Re: [Qemu-devel] Regression opening read-only cdroms

[Jamie Lokier]

Stefano Stabellini wrote:
> Jamie Lokier wrote:
> 
> > Avi Kivity wrote:
> >> qemu used to be quite happy opening read-only cdrom images, and I was 
> >> quite happy feeding kvm-autotest a library of read-only iso images.
> > 
> > 1. While we're here, an _option_ to open an image read-only even when
> >    you have write permission would be useful, for those occasions when
> >    you want to boot from some valuable image and be certain you aren't
> >    modifying it - without having to chmod back and forth in
> >    Qemu-wrapper scripts, or copy the image first.
> 
> 
> Indeed.
> I have a patch that adds bdrv_set_read_only that set bs->read_only = 1
> so that bdrv_open2 can check if the flag is set and act accordingly.
> The problem is that in xen we read whether the device should be opened
> read only in the VM config file, I don't think the user can specify to
> open a drive read-only in qemu.

What's missing is a "ro" sub-option to "-drive".

-- Jamie

Re: [Qemu-devel] Regression opening read-only cdroms

[Anthony Liguori]

Jamie Lokier wrote:
>> Indeed.
>> I have a patch that adds bdrv_set_read_only that set bs->read_only = 1
>> so that bdrv_open2 can check if the flag is set and act accordingly.
>> The problem is that in xen we read whether the device should be opened
>> read only in the VM config file, I don't think the user can specify to
>> open a drive read-only in qemu.
>>     
>
> What's missing is a "ro" sub-option to "-drive".
>   

It only is useful if we can expose that read-only attribute to the 
guest.  You can't do that consistently with all block devices so the 
result would be that you'll fail write operations causing a guest to crash.

This is not friendly to a user who expects that ro would Just Work.

Regards,

Anthony Liguori

> -- Jamie
>
>
>   

Re: [Qemu-devel] Regression opening read-only cdroms

[Stefano Stabellini]

Anthony Liguori wrote:

> Jamie Lokier wrote:
>>> Indeed.
>>> I have a patch that adds bdrv_set_read_only that set bs->read_only = 1
>>> so that bdrv_open2 can check if the flag is set and act accordingly.
>>> The problem is that in xen we read whether the device should be opened
>>> read only in the VM config file, I don't think the user can specify to
>>> open a drive read-only in qemu.
>>>     
>> What's missing is a "ro" sub-option to "-drive".
>>   
> 
> It only is useful if we can expose that read-only attribute to the 
> guest.  You can't do that consistently with all block devices so the 
> result would be that you'll fail write operations causing a guest to crash.
> 
> This is not friendly to a user who expects that ro would Just Work.
> 

Well, it could Just Work for cdrom.

Re: [Qemu-devel] Regression opening read-only cdroms

[Jamie Lokier]

Stefano Stabellini wrote:
> Anthony Liguori wrote:
> 
> > Jamie Lokier wrote:
> >>> Indeed.
> >>> I have a patch that adds bdrv_set_read_only that set bs->read_only = 1
> >>> so that bdrv_open2 can check if the flag is set and act accordingly.
> >>> The problem is that in xen we read whether the device should be opened
> >>> read only in the VM config file, I don't think the user can specify to
> >>> open a drive read-only in qemu.
> >>>     
> >> What's missing is a "ro" sub-option to "-drive".
> >>   
> > 
> > It only is useful if we can expose that read-only attribute to the 
> > guest.  You can't do that consistently with all block devices so the 
> > result would be that you'll fail write operations causing a guest to crash.

You should get disk write errors, rather than a simple crash.  Any
decent operating system will spew a lot of printks, which is a clue.

> > This is not friendly to a user who expects that ro would Just Work.

I don't agree that it's not friendly, because I think it's quite
obvious what behaviour to expect.  Users who don't know what a
read-only disk would do can use virt-manager and that can warn them
when they try to tick the option, or equally likely hide the option so
they can never find it :-)

We already have a user interface for it: chmod.  That's less friendly,
but it's the same result.

Users who expect things to just work will be even more surprised that
"-hda image" where image is read-only does not give any error from
QEMU, but their guest crashes.  Or that "-hda image" works as usual,
and their guest crashes, and eventually they discover it's because
their disk image is not writable, and it's always worked before
because they were using -snapshot or something like that, and QEMU
didn't warn them it would be a problem...

> Well, it could Just Work for cdrom.

It would Just Work for several media types (cdrom, floppy), and
for hard disks on certain interfaces (virtio, SCSI, USB).

Yes, virtio, SCSI and USB have read-only storage flags.

It is also appropriate for disks that you intent to always mount
read-only in the guest anyway, even if the interface doesn't have a
flag, just to protect the image from aberrant guest behaviour.

-- Jamie

Re: [Qemu-devel] Regression opening read-only cdroms

[Avi Kivity]

On 06/16/2009 08:46 PM, Jamie Lokier wrote:
>>> It only is useful if we can expose that read-only attribute to the
>>> guest.  You can't do that consistently with all block devices so the
>>> result would be that you'll fail write operations causing a guest to crash.
>>>        
>
> You should get disk write errors, rather than a simple crash.  Any
> decent operating system will spew a lot of printks, which is a clue.
>    

I don't consider this useful behaviour and see no motivation to support it.

> Users who expect things to just work will be even more surprised that
> "-hda image" where image is read-only does not give any error from
> QEMU, but their guest crashes.  Or that "-hda image" works as usual,
> and their guest crashes, and eventually they discover it's because
> their disk image is not writable, and it's always worked before
> because they were using -snapshot or something like that, and QEMU
> didn't warn them it would be a problem...
>    

I agree, for non-cdroms/floppies, non-backing store files we should 
require write access.


> It is also appropriate for disks that you intent to always mount
> read-only in the guest anyway, even if the interface doesn't have a
> flag, just to protect the image from aberrant guest behaviour.
>    

Maybe -drive ...,snapshot.  The disk will be writable, just not persistent.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] Regression opening read-only cdroms

[Jamie Lokier]

Avi Kivity wrote:
> On 06/16/2009 08:46 PM, Jamie Lokier wrote:
> >Users who expect things to just work will be even more surprised that
> >"-hda image" where image is read-only does not give any error from
> >QEMU, but their guest crashes.  Or that "-hda image" works as usual,
> >and their guest crashes, and eventually they discover it's because
> >their disk image is not writable, and it's always worked before
> >because they were using -snapshot or something like that, and QEMU
> >didn't warn them it would be a problem...
> 
> I agree, for non-cdroms/floppies, non-backing store files we should 
> require write access.

Did you miss that read-only disks exist, as seen through
SCSI/USB/virtio, and that guests will mount them read-only automatically?

There are real physical storage devices like that.  Just think of your
favourite USB flash drive with a write-protection switch.  It's just a
SCSI disk with the read-only flag set, to the guest.  Modern guests
know about them.

-- Jamie

Re: [Qemu-devel] Regression opening read-only cdroms

[Jamie Lokier]

Jamie Lokier wrote:
> Avi Kivity wrote:
> > On 06/16/2009 08:46 PM, Jamie Lokier wrote:
> > >Users who expect things to just work will be even more surprised that
> > >"-hda image" where image is read-only does not give any error from
> > >QEMU, but their guest crashes.  Or that "-hda image" works as usual,
> > >and their guest crashes, and eventually they discover it's because
> > >their disk image is not writable, and it's always worked before
> > >because they were using -snapshot or something like that, and QEMU
> > >didn't warn them it would be a problem...
> > 
> > I agree, for non-cdroms/floppies, non-backing store files we should 
> > require write access.
> 
> Did you miss that read-only disks exist, as seen through
> SCSI/USB/virtio, and that guests will mount them read-only automatically?

I see that you didn't; our mails passed in the mist.

Ok. :-)

-- Jamie

Re: [Qemu-devel] Regression opening read-only cdroms

[Gerd Hoffmann]

On 06/16/09 18:28, Anthony Liguori wrote:
> Jamie Lokier wrote:
>> What's missing is a "ro" sub-option to "-drive".
>
> It only is useful if we can expose that read-only attribute to the
> guest. You can't do that consistently with all block devices so the
> result would be that you'll fail write operations causing a guest to crash.

Well, I think all except ide-disk can signal r/o medium to the guest.

> This is not friendly to a user who expects that ro would Just Work.

chmod a-w $diskimage
And you'll run into the same issue today.

cheers,
   Gerd

Re: [Qemu-devel] Regression opening read-only cdroms

[Avi Kivity]

On 06/17/2009 12:11 AM, Gerd Hoffmann wrote:
>> This is not friendly to a user who expects that ro would Just Work.
>
> chmod a-w $diskimage
> And you'll run into the same issue today.

That should error on IDE and warn on scsi (if ,readonly not supplied) 
unless -snapshot.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] Regression opening read-only cdroms

[Avi Kivity]

On 06/16/2009 05:33 PM, Jamie Lokier wrote:
> Avi Kivity wrote:
>    
>> qemu used to be quite happy opening read-only cdrom images, and I was
>> quite happy feeding kvm-autotest a library of read-only iso images.
>>      
>
> 1. While we're here, an _option_ to open an image read-only even when
>     you have write permission would be useful, for those occasions when
>     you want to boot from some valuable image and be certain you aren't
>     modifying it - without having to chmod back and forth in
>     Qemu-wrapper scripts, or copy the image first.
>    

read-only disk images don't make much sense.

Using -snapshot will generally ensure the image is not modified, while 
allowing the guest to write.

> 2. Would it make sense to open CD-ROM images read-only all the time,
>     when they are opened with media=cdrom, or can QEMU's CD-ROM devices
>     write to the images?
>    

We don't emulate cd writers yet, so it makes sense to restict ourselves 
to O_RDONLY for now.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Regression opening read-only cdroms

[Jamie Lokier]

Avi Kivity wrote:
> On 06/16/2009 05:33 PM, Jamie Lokier wrote:
> >Avi Kivity wrote:
> >   
> >>qemu used to be quite happy opening read-only cdrom images, and I was
> >>quite happy feeding kvm-autotest a library of read-only iso images.
> >>     
> >
> >1. While we're here, an _option_ to open an image read-only even when
> >    you have write permission would be useful, for those occasions when
> >    you want to boot from some valuable image and be certain you aren't
> >    modifying it - without having to chmod back and forth in
> >    Qemu-wrapper scripts, or copy the image first.
> >   
> 
> read-only disk images don't make much sense.

And yet "chmod 444 image; qemu ..." works.
If you're booting from a disk you don't need to write to, obviously.
Generally it'll need to be mounted read-only in the guest.

> Using -snapshot will generally ensure the image is not modified, while 
> allowing the guest to write.

I never do that with _valuable_ images because:

   - Valuable images are expensive/difficult/impossible to recreate.
     But too large to copy about casually.

   - I don't have that much faith in QEMU's correctness, having
     already been bitten by a number it's bugs, or in the guest's
     correctness if I were to rely on the guest doing read-only mount
     instead of using -snapshot.

   - It's too easy to accidentally write back the changes over the
     the original image.  Man page:

     "the raw disk image you use is not written back. You can however
     force the write back by pressing C-a s".

And I don't do it when booting a guest where I have _both_ a disk a do
want to write, and another valuable image that I don't want written,
because:

   - How would I use -snapshot and then commit changes to the disk I
     do want to write (either C-a s or "commit" in the monitor), but
     never write changes to the disk I don't want written?

   - "commit" has always been a bit ambiguous when applied to a
     combination of -snapshot and a qcow2 delta image.

Finally, QEMU clearly does support read-only images, so it's always
struck me as odd that the only way to invoke this support is using
"chmod" outside QEMU.  (Actually I use "chattr +i" as well.  That's
how paranoid I am about difficult to recreate images).

-- Jamie

Re: [Qemu-devel] Regression opening read-only cdroms

[Avi Kivity]

On 06/16/2009 06:54 PM, Jamie Lokier wrote:
> read-only disk images don't make much sense.
>    
>
> And yet "chmod 444 image; qemu ..." works.
> If you're booting from a disk you don't need to write to, obviously.
> Generally it'll need to be mounted read-only in the guest.
>    

It will eventually fail.  Open the ext3 log, update atime, or 
something.  The guest expects the disk to be writeable.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Regression opening read-only cdroms

[Jamie Lokier]

Avi Kivity wrote:
> On 06/16/2009 06:54 PM, Jamie Lokier wrote:
> >read-only disk images don't make much sense.
> >   
> >
> >And yet "chmod 444 image; qemu ..." works.
> >If you're booting from a disk you don't need to write to, obviously.
> >Generally it'll need to be mounted read-only in the guest.
> >   
> 
> It will eventually fail.  Open the ext3 log, update atime, or 
> something.  The guest expects the disk to be writeable.

No.  Obviously if you _want_ to run a guest with the disk mounted
writable, you'll use snapshot=on instead because that's what it's for.

Otherwise, a read-only disk should works fine using virtio/SCSI/USB,
as the guest will mount it read-only, as those interfaces all have a
read-only media flag which Linux guests (at least) look at.

Which is the desired behaviour.

-- Jamie

Re: [Qemu-devel] Regression opening read-only cdroms

[Avi Kivity]

On 06/16/2009 08:51 PM, Jamie Lokier wrote:
> Avi Kivity wrote:
>    
>> On 06/16/2009 06:54 PM, Jamie Lokier wrote:
>>      
>>> read-only disk images don't make much sense.
>>>
>>>
>>> And yet "chmod 444 image; qemu ..." works.
>>> If you're booting from a disk you don't need to write to, obviously.
>>> Generally it'll need to be mounted read-only in the guest.
>>>
>>>        
>> It will eventually fail.  Open the ext3 log, update atime, or
>> something.  The guest expects the disk to be writeable.
>>      
>
> No.  Obviously if you _want_ to run a guest with the disk mounted
> writable, you'll use snapshot=on instead because that's what it's for.
>
> Otherwise, a read-only disk should works fine using virtio/SCSI/USB,
> as the guest will mount it read-only, as those interfaces all have a
> read-only media flag which Linux guests (at least) look at.
>
> Which is the desired behaviour.
>
>    

I didn't consider a read-only media flag.  I retract my remarks, -drive 
...,readonly then makes sense for those interfaces.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] Regression opening read-only cdroms

[Richard W.M. Jones]

On Tue, Jun 16, 2009 at 03:33:00PM +0100, Jamie Lokier wrote:
> Avi Kivity wrote:
> > qemu used to be quite happy opening read-only cdrom images, and I was 
> > quite happy feeding kvm-autotest a library of read-only iso images.
> 
> 1. While we're here, an _option_ to open an image read-only even when
>    you have write permission would be useful, for those occasions when
>    you want to boot from some valuable image and be certain you aren't
>    modifying it - without having to chmod back and forth in
>    Qemu-wrapper scripts, or copy the image first.

FWIW we went round the houses on this one for libguestfs and settled
on using '-drive ...,snapshot=on' instead.  The guest sees the drive
as writable, and any writes are discarded when libguestfs/qemu quits.

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v

Re: [Qemu-devel] Regression opening read-only cdroms

[Jamie Lokier]

Richard W.M. Jones wrote:
> On Tue, Jun 16, 2009 at 03:33:00PM +0100, Jamie Lokier wrote:
> > Avi Kivity wrote:
> > > qemu used to be quite happy opening read-only cdrom images, and I was 
> > > quite happy feeding kvm-autotest a library of read-only iso images.
> > 
> > 1. While we're here, an _option_ to open an image read-only even when
> >    you have write permission would be useful, for those occasions when
> >    you want to boot from some valuable image and be certain you aren't
> >    modifying it - without having to chmod back and forth in
> >    Qemu-wrapper scripts, or copy the image first.
> 
> FWIW we went round the houses on this one for libguestfs and settled
> on using '-drive ...,snapshot=on' instead.  The guest sees the drive
> as writable, and any writes are discarded when libguestfs/qemu quits.

It's fine for CD-ROMs of course.

For disks, there's that slight problem of running out of RAM or disk
space for the /tmp snapshot files.  Better not to write anything.

It'd be nice to enforce it, instead of depending on good behaviour
from the guest.

-- Jamie

Re: [Qemu-devel] Regression opening read-only cdroms

[Richard W.M. Jones]

On Wed, Jun 24, 2009 at 09:23:53PM +0100, Jamie Lokier wrote:
> It'd be nice to enforce it, instead of depending on good behaviour
> from the guest.

Right - but in libguestfs everything is controlled by the host so we
can rely on good behaviour in our case.

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw

Re: [Qemu-devel] Regression opening read-only cdroms

[Jamie Lokier]

Richard W.M. Jones wrote:
> On Wed, Jun 24, 2009 at 09:23:53PM +0100, Jamie Lokier wrote:
> > It'd be nice to enforce it, instead of depending on good behaviour
> > from the guest.
> 
> Right - but in libguestfs everything is controlled by the host so we
> can rely on good behaviour in our case.

In that case, why do you need the ",snapshot=on"?

-- Jamie

Re: [Qemu-devel] Regression opening read-only cdroms

[Richard W.M. Jones]

On Thu, Jun 25, 2009 at 01:50:32AM +0100, Jamie Lokier wrote:
> Richard W.M. Jones wrote:
> > On Wed, Jun 24, 2009 at 09:23:53PM +0100, Jamie Lokier wrote:
> > > It'd be nice to enforce it, instead of depending on good behaviour
> > > from the guest.
> > 
> > Right - but in libguestfs everything is controlled by the host so we
> > can rely on good behaviour in our case.
> 
> In that case, why do you need the ",snapshot=on"?

Did I say that?  I meant that we use that only for read-only access to
hard drives, not for CD-ROMs.  Here's the actual code:

http://git.et.redhat.com/?p=libguestfs.git;a=blob;f=src/guestfs.c;h=fdf5cd369842557b744bac2a80fdbd97fa4e8d60;hb=13229b1d54c03be098ab75e6d451b5b46a98550d#l661

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 75 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora

Re: [Qemu-devel] Regression opening read-only cdroms

[Blue Swirl]

On 6/16/09, Avi Kivity <avi@redhat.com> wrote:
> qemu used to be quite happy opening read-only cdrom images, and I was quite
> happy feeding kvm-autotest a library of read-only iso images.  However since
>
>    commit 0e1d8f4c549e51fd19793a154862979fdc199477
>    Author: Christoph Hellwig <hch@lst.de>
>    Date:   Mon Jun 15 13:53:26 2009 +0200
>
>         raw-posix: always store open flags
>
>         Both the Linux floppy and the FreeBSD CDROM host device need to
>    store
>         the open flags so that they can re-open the device later.   Store
> the
>         open flags unconditionally to remove the ifdef mess and simply the
>         calling conventions for the later patches in the series.
>
>         Signed-off-by: Christoph Hellwig <hch@lst.de>
>
>  this state of happiness no longer exists.
>
>  Can you look at what caused this?

How about these:
- open_flags = O_BINARY;
+ s->open_flags |= O_BINARY;
- open_flags = O_BINARY;
+ s->open_flags |= O_BINARY;

Re: [Qemu-devel] Regression opening read-only cdroms

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

On 6/16/09, Blue Swirl <blauwirbel@gmail.com> wrote:
> On 6/16/09, Avi Kivity <avi@redhat.com> wrote:
>  > qemu used to be quite happy opening read-only cdrom images, and I was quite
>  > happy feeding kvm-autotest a library of read-only iso images.  However since
>  >
>  >    commit 0e1d8f4c549e51fd19793a154862979fdc199477
>  >    Author: Christoph Hellwig <hch@lst.de>
>  >    Date:   Mon Jun 15 13:53:26 2009 +0200
>  >
>  >         raw-posix: always store open flags
>  >
>  >         Both the Linux floppy and the FreeBSD CDROM host device need to
>  >    store
>  >         the open flags so that they can re-open the device later.   Store
>  > the
>  >         open flags unconditionally to remove the ifdef mess and simply the
>  >         calling conventions for the later patches in the series.
>  >
>  >         Signed-off-by: Christoph Hellwig <hch@lst.de>
>  >
>  >  this state of happiness no longer exists.
>  >
>  >  Can you look at what caused this?
>
>
> How about these:
>  - open_flags = O_BINARY;
>  + s->open_flags |= O_BINARY;
>  - open_flags = O_BINARY;
>  + s->open_flags |= O_BINARY;

Fix attached, could you test it?

[-- Attachment #2: 0001-Fix-opening-of-read-only-raw-images.patch --]
[-- Type: application/x-patch, Size: 4031 bytes --]

Re: [Qemu-devel] Regression opening read-only cdroms

[Christoph Hellwig]

[-- Attachment #1: Type: text/plain, Size: 148 bytes --]

Yes, explicitly passing the open flags is probably cleaner then
re-using the struct field.  This passes my newly added test for
the open behaviour.

[-- Attachment #2: 0001-Fix-opening-of-read-only-raw-images.patch --]
[-- Type: application/x-patch, Size: 4031 bytes --]