| Message ID | 4AD32550.8040901@redhat.com |
|---|---|
| State | Rejected |
| Headers | show |
Am 12.10.2009 14:47, schrieb Naphtali Sprei: > In order to safely share an image between guests (as read only drive), add a 'readonly' flag > to the -drive command (qemu command line and monitor). > > Still missing passing the read only attribute to the guest, where possible. I don't know which device types supports > read only, and don't know how to pass this information to guests. > > Also not sure what to do when qemu cannot open the file as writeable. Currently it opens it as read only. > We might change it to give a warning or even an error. > > From 6e297da79a4c015555e3927e6d28744933a31ebe Mon Sep 17 00:00:00 2001 > From: Naphtali Sprei <nsprei@redhat.com> > Date: Mon, 12 Oct 2009 14:25:25 +0200 > Subject: [PATCH] Added readonly flag to -drive command. > This enables sharing same image between guests, with readonly access. > Implementaion mark the drive as read_only and changes the flags when actually opening the file. Is this enough? Basically none of the block drivers know that their image could be read-only, so we'll likely trigger some unexpected error cases there. For a simple write I guess we'll be okay (not sure if we'll return the right error code, though), but I have no idea what, say, savevm would do with a read-only image. What cases have you tested? > TODO: > 1. Pass the readonly attribute to the guest (write-protected drive ??) I agree. To be useful the read-only attribute should be exposed to the guest. I think most devices support some sort of write protection. > 2. Re-visit the scheme where qemu open a file (silently) in read only mode when it can't open for write. > Now that user can specify read only (and didn't), might give a warning when not writeable, or even > give an error. > > Signed-off-by: Naphtali Sprei <nsprei@redhat.com> > --- > block.c | 14 ++++++++++++-- > block.h | 1 + > qemu-config.c | 3 +++ > vl.c | 6 ++++++ > 4 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/block.c b/block.c > index 33f3d65..01fd289 100644 > --- a/block.c > +++ b/block.c > @@ -335,7 +335,8 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, > char tmp_filename[PATH_MAX]; > char backing_filename[PATH_MAX]; > > - bs->read_only = 0; > + /* don't mess with it, should have been zeros, anyway */ > + /* bs->read_only = 0; */ Why leave that comment instead of just removing it if it's not necessary? Kevin
Kevin Wolf wrote: > Am 12.10.2009 14:47, schrieb Naphtali Sprei: > > In order to safely share an image between guests (as read only drive), add a 'readonly' flag > > to the -drive command (qemu command line and monitor). Heh. I've been sharing images between guests for ages - using "chmod -r" :-) > > Still missing passing the read only attribute to the guest, where possible. I don't know which device types supports > > read only, and don't know how to pass this information to guests. This was discussed in a thread some months ago. You might look it up. SCSI, USB, and floppy can pass the information to guests - see the Linux kernel for how the flag is read. CD-ROMs are read-only already of course. I don't know if virtio-blk can; if not, it would be good to add it. Not sure about the latest iterations of PATA/SATA. Read-only images are particularly useful for the backing file for a qcow2 image or -snapshot. Then the guest doesn't need to know anything, but 'savevm', 'delvm' and 'commit' need to be stopped from trying to modify the backing file. (But it's fine for -snapshot + commit to modify a writable qcow2 file which _itself_ has a read-only backing file, as long as 'commit' doesn't try to push the commit further back than one level). > > Also not sure what to do when qemu cannot open the file as > > writeable. Currently it opens it as read only. We might change it > > to give a warning or even an error. I think that's a good plan, but please don't break the current situation, where it's possible to "chmod -r" an image file and then share it safely, until the 'readonly' flag is a usable replacement. > Is this enough? Basically none of the block drivers know that their > image could be read-only, so we'll likely trigger some unexpected error > cases there. For a simple write I guess we'll be okay (not sure if we'll > return the right error code, though), but I have no idea what, say, > savevm would do with a read-only image. Right now there is already a 'readonly' flag called 'chmod -r image', because QEMU opens a file read-only if it can't open it writable, so it's not a new case. Just moving it from the filesystem into QEMU. When opened read-only, it would be better for the block drivers to return an error themselves, instead of trying to write and (hopefully) getting a host OS error. I doubt if read-only errors have been tested or accomodated, or if they are reported well to guests as the right sort of error. I'm pretty sure the read-only _flag_ isn't passed through guest interfaces for the guest OS to use yet, but that should be quite easy. -- Jamie
Kevin Wolf wrote: > Am 12.10.2009 14:47, schrieb Naphtali Sprei: > >> In order to safely share an image between guests (as read only drive), add a 'readonly' flag >> to the -drive command (qemu command line and monitor). >> >> Still missing passing the read only attribute to the guest, where possible. I don't know which device types supports >> read only, and don't know how to pass this information to guests. >> >> Also not sure what to do when qemu cannot open the file as writeable. Currently it opens it as read only. >> We might change it to give a warning or even an error. >> >> From 6e297da79a4c015555e3927e6d28744933a31ebe Mon Sep 17 00:00:00 2001 >> From: Naphtali Sprei <nsprei@redhat.com> >> Date: Mon, 12 Oct 2009 14:25:25 +0200 >> Subject: [PATCH] Added readonly flag to -drive command. >> This enables sharing same image between guests, with readonly access. >> Implementaion mark the drive as read_only and changes the flags when actually opening the file. >> > > Is this enough? Basically none of the block drivers know that their > image could be read-only, so we'll likely trigger some unexpected error > cases there. For a simple write I guess we'll be okay (not sure if we'll > return the right error code, though), but I have no idea what, say, > savevm would do with a read-only image. > Yes, this is why patches like this have been rejected in the past. This concept needs to be plumbed to the device emulation so that a guest realizes it has a read-only disk. Throwing away writes without telling the guest is a recipe for disaster. Regards, Anthony Liguori
Am 12.10.2009 15:50, schrieb Jamie Lokier: > Right now there is already a 'readonly' flag called 'chmod -r image', > because QEMU opens a file read-only if it can't open it writable, so > it's not a new case. Just moving it from the filesystem into QEMU. Right, but I'm not sure how well it is tested. I think, when moving it into qemu we should take the opportunity to do it right. If everything works right now, the right description of it is probably "pure luck". And I assume you didn't do stupid things like savevm on a read-only image which we still should be able to handle. > When opened read-only, it would be better for the block drivers to > return an error themselves, instead of trying to write and (hopefully) > getting a host OS error. For simple read/write operations, the generic block layer is doing the check already. Kevin
Jamie Lokier wrote: > Kevin Wolf wrote: > >> Am 12.10.2009 14:47, schrieb Naphtali Sprei: >> >>> In order to safely share an image between guests (as read only drive), add a 'readonly' flag >>> to the -drive command (qemu command line and monitor). >>> > > Heh. I've been sharing images between guests for ages - using "chmod -r" :-) > Were it not for backwards compatibility, I would remove that and have it error out. I've had multiple people end up with strangely broken guests because they didn't realize the image was read only. >>> Still missing passing the read only attribute to the guest, where possible. I don't know which device types supports >>> read only, and don't know how to pass this information to guests. >>> > > This was discussed in a thread some months ago. You might look it up. > SCSI, USB, and floppy can pass the information to guests - see the > Linux kernel for how the flag is read. CD-ROMs are read-only already > of course. I don't know if virtio-blk can; if not, it would be good to > add it. > virtio-blk can support read-only images now. Regards, Anthony Liguori
Anthony Liguori wrote: > Jamie Lokier wrote: > >Kevin Wolf wrote: > > > >>Am 12.10.2009 14:47, schrieb Naphtali Sprei: > >> > >>>In order to safely share an image between guests (as read only drive), > >>>add a 'readonly' flag > >>>to the -drive command (qemu command line and monitor). > >>> > > > >Heh. I've been sharing images between guests for ages - using "chmod -r" > >:-) > > > > Were it not for backwards compatibility, I would remove that and have it > error out. > > I've had multiple people end up with strangely broken guests because > they didn't realize the image was read only. I agree, that's ugly - I've wasted time on a strangely broken guest in that situation too! Backward compatibility might be an issue with the idea to change QEMU to complain if it can't open a file writable. Not with regular disk images so much, but floppy/USB images. Perhaps the answer to that is 'readonly=auto', 'readonly=yes', 'readonly=no'? Sharing images with -snapshot, and backing images with qcow2, is a very useful feature for launching multiple test VMs in parallel, and for making "forked" images for testing different guest OS variations, so I'm glad to see the 'readonly' flag appearing as an option. (You can't use qcow2 snapshots to implement "forked" images running in parallel because you can't safely open different snapshots in the same qcow2 file at the same time). Taking a read-only (shared) or writeable (exclusive) lock on the file would be a useful addition I think, to catch mistakes. -- Jamie
Anthony Liguori wrote: > Jamie Lokier wrote: >> Kevin Wolf wrote: >> >>> Am 12.10.2009 14:47, schrieb Naphtali Sprei: >>> >>>> In order to safely share an image between guests (as read only >>>> drive), add a 'readonly' flag >>>> to the -drive command (qemu command line and monitor). >> >> Heh. I've been sharing images between guests for ages - using "chmod -r" :-) > > Were it not for backwards compatibility, I would remove that and have it > error out. > > I've had multiple people end up with strangely broken guests because > they didn't realize the image was read only. And I had this issue too. But for me it's still not clear. Some disk drives out there can be switched to read-only mode at runtime by using appropriate command. Certain (USB and especially SD) flash drives can be read-only too (usually a tiny swith does that). Even old good floppy drives had a "write protect" switch. How the system determines if such devices are read-only? For a HDD or a flash drive, I *guess* the drive return appropriate error message, no? But qemu - apparently - just ignores writes in this case, which results in stalled guest which expects some answer to the write command.. I guess, because it was always the stalls. /mjt
Am 12.10.2009 18:15, schrieb Michael Tokarev: > Anthony Liguori wrote: >> Jamie Lokier wrote: >>> Kevin Wolf wrote: >>> >>>> Am 12.10.2009 14:47, schrieb Naphtali Sprei: >>>> >>>>> In order to safely share an image between guests (as read only >>>>> drive), add a 'readonly' flag >>>>> to the -drive command (qemu command line and monitor). >>> >>> Heh. I've been sharing images between guests for ages - using "chmod -r" :-) >> >> Were it not for backwards compatibility, I would remove that and have it >> error out. >> >> I've had multiple people end up with strangely broken guests because >> they didn't realize the image was read only. > > And I had this issue too. > > But for me it's still not clear. Some disk drives out there can be > switched to read-only mode at runtime by using appropriate command. > Certain (USB and especially SD) flash drives can be read-only too > (usually a tiny swith does that). Even old good floppy drives had > a "write protect" switch. How the system determines if such devices > are read-only? For a HDD or a flash drive, I *guess* the drive return > appropriate error message, no? > > But qemu - apparently - just ignores writes in this case, which > results in stalled guest which expects some answer to the write > command.. I guess, because it was always the stalls. The qemu block layer returns errors. For good old bdrv_write it's -EACCES, for bdrv_aio_writev it's NULL (and therefore cannot be distinguished from other errors). Not sure what the devices make out of it. Probably not the right thing in all cases. Kevin
diff --git a/block.c b/block.c index 33f3d65..01fd289 100644 --- a/block.c +++ b/block.c @@ -335,7 +335,8 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, char tmp_filename[PATH_MAX]; char backing_filename[PATH_MAX]; - bs->read_only = 0; + /* don't mess with it, should have been zeros, anyway */ + /* bs->read_only = 0; */ bs->is_temporary = 0; bs->encrypted = 0; bs->valid_key = 0; @@ -423,7 +424,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, /* Note: for compatibility, we open disk image files as RDWR, and RDONLY as fallback */ if (!(flags & BDRV_O_FILE)) - open_flags = BDRV_O_RDWR | + open_flags = (bs->read_only ? 0 : BDRV_O_RDWR) | (flags & (BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO)); else open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT); @@ -453,6 +454,8 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, /* if there is a backing file, use it */ BlockDriver *back_drv = NULL; bs->backing_hd = bdrv_new(""); + /* pass on read_only property to the backing_hd */ + bs->backing_hd->read_only = bs->read_only; path_combine(backing_filename, sizeof(backing_filename), filename, bs->backing_file); if (bs->backing_format[0] != '\0') @@ -925,6 +928,13 @@ int bdrv_is_read_only(BlockDriverState *bs) return bs->read_only; } +int bdrv_set_read_only(BlockDriverState *bs, int read_only) +{ + int ret = bs->read_only; + bs->read_only = read_only; + return ret; +} + int bdrv_is_sg(BlockDriverState *bs) { return bs->sg; diff --git a/block.h b/block.h index a966afb..302010d 100644 --- a/block.h +++ b/block.h @@ -136,6 +136,7 @@ int bdrv_get_type_hint(BlockDriverState *bs); int bdrv_get_translation_hint(BlockDriverState *bs); int bdrv_is_removable(BlockDriverState *bs); int bdrv_is_read_only(BlockDriverState *bs); +int bdrv_set_read_only(BlockDriverState *bs, int read_only); int bdrv_is_sg(BlockDriverState *bs); int bdrv_enable_write_cache(BlockDriverState *bs); int bdrv_is_inserted(BlockDriverState *bs); diff --git a/qemu-config.c b/qemu-config.c index bafaea2..5eb8838 100644 --- a/qemu-config.c +++ b/qemu-config.c @@ -71,6 +71,9 @@ QemuOptsList qemu_drive_opts = { .name = "addr", .type = QEMU_OPT_STRING, .help = "pci address (virtio only)", + },{ + .name = "readonly", + .type = QEMU_OPT_BOOL, }, { /* end if list */ } }, diff --git a/vl.c b/vl.c index 374f85b..3763911 100644 --- a/vl.c +++ b/vl.c @@ -2007,6 +2007,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, int index; int cache; int aio = 0; + int ro = 0; int bdrv_flags, onerror; const char *devaddr; DriveInfo *dinfo; @@ -2038,6 +2039,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, secs = qemu_opt_get_number(opts, "secs", 0); snapshot = qemu_opt_get_bool(opts, "snapshot", 0); + ro = qemu_opt_get_bool(opts, "readonly", 0); file = qemu_opt_get(opts, "file"); serial = qemu_opt_get(opts, "serial"); @@ -2329,6 +2331,10 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, bdrv_flags &= ~BDRV_O_NATIVE_AIO; } + if (ro == 1) { + (void)bdrv_set_read_only(dinfo->bdrv, 1); + } + if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv) < 0) { fprintf(stderr, "qemu: could not open disk image %s: %s\n", file, strerror(errno));