[Qemu-devel] [PATCH] block: reject O_RDWR open for read-only images

[Qemu-devel] [PATCH] block: reject O_RDWR open for read-only images

[Christoph Hellwig]

Various obscure image format drivers do not allow write access.
Instead of silently falling back to read-only access reject attempts
to open these images for write access.

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

Index: qemu/block/bochs.c
===================================================================
--- qemu.orig/block/bochs.c	2010-04-07 13:48:34.448003761 +0200
+++ qemu/block/bochs.c	2010-04-07 13:49:21.088256030 +0200
@@ -116,13 +116,15 @@ static int bochs_open(BlockDriverState *
     struct bochs_header bochs;
     struct bochs_header_v1 header_v1;
 
+    if (flags & BDRV_O_RDWR) {
+        return -EROFS;
+    }
+
     fd = open(filename, O_RDONLY | O_BINARY);
     if (fd < 0) {
         return -1;
     }
 
-    bs->read_only = 1; // no write support yet
-
     s->fd = fd;
 
     if (read(fd, &bochs, sizeof(bochs)) != sizeof(bochs)) {
Index: qemu/block/cloop.c
===================================================================
--- qemu.orig/block/cloop.c	2010-04-07 13:49:35.420004250 +0200
+++ qemu/block/cloop.c	2010-04-07 13:50:01.978005226 +0200
@@ -56,10 +56,13 @@ static int cloop_open(BlockDriverState *
     BDRVCloopState *s = bs->opaque;
     uint32_t offsets_size,max_compressed_block_size=1,i;
 
+    if (flags & BDRV_O_RDWR) {
+        return -EROFS;
+    }
+
     s->fd = open(filename, O_RDONLY | O_BINARY);
     if (s->fd < 0)
         return -errno;
-    bs->read_only = 1;
 
     /* read header */
     if(lseek(s->fd,128,SEEK_SET)<0) {
Index: qemu/block/dmg.c
===================================================================
--- qemu.orig/block/dmg.c	2010-04-07 13:49:35.435254424 +0200
+++ qemu/block/dmg.c	2010-04-07 13:49:56.004013608 +0200
@@ -81,10 +81,13 @@ static int dmg_open(BlockDriverState *bs
     uint32_t count;
     uint32_t max_compressed_size=1,max_sectors_per_chunk=1,i;
 
+    if (flags & BDRV_O_RDWR) {
+        return -EROFS;
+    }
+
     s->fd = open(filename, O_RDONLY | O_BINARY);
     if (s->fd < 0)
         return -errno;
-    bs->read_only = 1;
     s->n_chunks = 0;
     s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
 
Index: qemu/block/parallels.c
===================================================================
--- qemu.orig/block/parallels.c	2010-04-07 13:49:35.455261059 +0200
+++ qemu/block/parallels.c	2010-04-07 13:49:59.009004250 +0200
@@ -74,13 +74,15 @@ static int parallels_open(BlockDriverSta
     int fd, i;
     struct parallels_header ph;
 
+    if (flags & BDRV_O_RDWR) {
+        return -EROFS;
+    }
+
     fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE);
     if (fd < 0) {
         return -1;
     }
 
-    bs->read_only = 1; // no write support yet
-
     s->fd = fd;
 
     if (read(fd, &ph, sizeof(ph)) != sizeof(ph))

Re: [Qemu-devel] [PATCH] block: reject O_RDWR open for read-only images

[Kevin Wolf]

Am 07.04.2010 13:57, schrieb Christoph Hellwig:
> Various obscure image format drivers do not allow write access.
> Instead of silently falling back to read-only access reject attempts
> to open these images for write access.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I'm not sure about this patch. I don't have any such images around, so I
can't test it easily, but it looks to me as if you'd need to explicitly
specify readonly=on if you want to use such formats. This in turn means
that old style shortcuts like -hda don't work any more. Is it that
important to avoid implicit fallbacks for obscure formats that we're
willing to sacrifice compatibility?

That said, personally I don't care about those format all that much
anyway and the change would make things more consistent. If people
consider it okay to break compatibility in this way, I won't stand in
your way.

But if we wanted to implement this, have you thought about doing it in
one place in block.c? You could check if a driver supports bdrv_write or
bdrv_aio_write, and if it doesn't support either I think you can assume
it's read-only.

Kevin

Re: [Qemu-devel] [PATCH] block: reject O_RDWR open for read-only images

[Stefan Hajnoczi]

On Wed, Apr 7, 2010 at 12:57 PM, Christoph Hellwig <hch@lst.de> wrote:
> Various obscure image format drivers do not allow write access.
> Instead of silently falling back to read-only access reject attempts
> to open these images for write access.

Does block/curl.c need this too?

Stefan

Re: [Qemu-devel] [PATCH] block: reject O_RDWR open for read-only images

[Kevin Wolf]

Am 07.04.2010 17:02, schrieb Stefan Hajnoczi:
> On Wed, Apr 7, 2010 at 12:57 PM, Christoph Hellwig <hch@lst.de> wrote:
>> Various obscure image format drivers do not allow write access.
>> Instead of silently falling back to read-only access reject attempts
>> to open these images for write access.
> 
> Does block/curl.c need this too?

Yes, looks like it. It even fails to declare itself readonly like those
other formats do currently. One more argument for doing it in a central
place where no driver would be missed.

Kevin

Re: [Qemu-devel] [PATCH] block: reject O_RDWR open for read-only images

[Kevin Wolf]

Am 07.04.2010 14:32, schrieb Kevin Wolf:
> Am 07.04.2010 13:57, schrieb Christoph Hellwig:
>> Various obscure image format drivers do not allow write access.
>> Instead of silently falling back to read-only access reject attempts
>> to open these images for write access.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> I'm not sure about this patch. I don't have any such images around, so I
> can't test it easily, but it looks to me as if you'd need to explicitly
> specify readonly=on if you want to use such formats. This in turn means
> that old style shortcuts like -hda don't work any more. Is it that
> important to avoid implicit fallbacks for obscure formats that we're
> willing to sacrifice compatibility?
> 
> That said, personally I don't care about those format all that much
> anyway and the change would make things more consistent. If people
> consider it okay to break compatibility in this way, I won't stand in
> your way.
> 
> But if we wanted to implement this, have you thought about doing it in
> one place in block.c? You could check if a driver supports bdrv_write or
> bdrv_aio_write, and if it doesn't support either I think you can assume
> it's read-only.

Christoph, what about this one? It's still in the block branch, but I
have excluded it from first pull request round because these questions
are still open.

What should I do with it? Drop it, wait for a new version or keep it as
it is? If you're not going to drop it, I'd definitely like to hear more
opinions on the compatibility thing before proposing it for a merge in a
pull request.

Kevin

Re: [Qemu-devel] [PATCH] block: reject O_RDWR open for read-only images

[Christoph Hellwig]

On Fri, Apr 23, 2010 at 06:09:09PM +0200, Kevin Wolf wrote:
> Christoph, what about this one? It's still in the block branch, but I
> have excluded it from first pull request round because these questions
> are still open.
> 
> What should I do with it? Drop it, wait for a new version or keep it as
> it is? If you're not going to drop it, I'd definitely like to hear more
> opinions on the compatibility thing before proposing it for a merge in a
> pull request.

Drop it for now.  I agree on the centralization so I will do it.  Then
we can decide if we want to implement the enforcement in the central
place ontop of it.