| Message ID | 20100407115753.GB12452@lst.de |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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.
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))
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>