block: reject O_RDWR open for read-only images
PATCH 1/1 block: reject O_RDWR open for read-only images
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.
block/bochs.c +4 -2
@@ -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)) {
block/cloop.c +4 -1
@@ -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 ) {
block/dmg.c +4 -1
@@ -81,10 +81,13 @@ static int dmg_open(BlockDriverState *bsuint32_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 ;
block/parallels.c +4 -2
@@ -74,13 +74,15 @@ static int parallels_open(BlockDriverStaint 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))
Am 07.04.2010 13:57, schrieb Christoph Hellwig:
Quoted 5 lines
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 <[email protected]>
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
Am 07.04.2010 14:32, schrieb Kevin Wolf:
Quoted 19 lines
Am 07.04.2010 13:57, schrieb Christoph Hellwig:
Quoted 5 lines
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 <[email protected]>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.
Quoted 4 lines
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 supportsbdrv_writeorbdrv_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
Quoted 8 lines · Kevin Wolf
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.
Quoted 3 lines · 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.
Does block/curl.c need this too?
Stefan
Am 07.04.2010 17:02, schrieb Stefan Hajnoczi:
Quoted 6 lines · Christoph Hellwig
Quoted 3 lines
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.cneed 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