| Message ID | 20100111175209.GB7571@lst.de |
|---|---|
| State | New |
| Headers | show |
Am 11.01.2010 18:52, schrieb Christoph Hellwig: > Untangle the open flag manipulation in bdrv_open2 and document why we > are clearing the various flags in the different flag combinations. > > > Signed-off-by: Christoph Hellwig <hch@lst.de> Nice cleanup. Acked-by: Kevin Wolf <kwolf@redhat.com> > + /* > + * Currently BDRV_O_CREAT is not supported by any image format, > + * but I'm not sure that's reason enough to always clear it for > + * the !BDRV_O_FILE case.. > + */ > + open_flags &= ~(BDRV_O_CREAT); Is there even a theoretical use case for this flag? When you want to create an image, you use bdrv_create. I'd suggest another patch to remove BDRV_O_CREAT entirely. Maybe except if we want to support protocols with bdrv_create, so we would go for the block layer functions rather than the POSIX functions. However, I don't think there is a need to do so. Kevin
On Tue, Jan 12, 2010 at 06:32:13PM +0100, Kevin Wolf wrote: > Am 11.01.2010 18:52, schrieb Christoph Hellwig: > > Untangle the open flag manipulation in bdrv_open2 and document why we > > are clearing the various flags in the different flag combinations. > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > Nice cleanup. > > Acked-by: Kevin Wolf <kwolf@redhat.com> > > > + /* > > + * Currently BDRV_O_CREAT is not supported by any image format, > > + * but I'm not sure that's reason enough to always clear it for > > + * the !BDRV_O_FILE case.. > > + */ > > + open_flags &= ~(BDRV_O_CREAT); > > Is there even a theoretical use case for this flag? When you want to > create an image, you use bdrv_create. I'd suggest another patch to > remove BDRV_O_CREAT entirely. Sounds good. Currently the only user of it is qemu-io. I tried to look up the history of it but with all the renamings of the block files I haven't managed to find what it used to be for.
Index: qemu/block.c =================================================================== --- qemu.orig/block.c 2010-01-11 17:52:54.273003789 +0100 +++ qemu/block.c 2010-01-11 18:19:22.485006278 +0100 @@ -356,7 +356,7 @@ int bdrv_open(BlockDriverState *bs, cons int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, BlockDriver *drv) { - int ret, open_flags, try_rw; + int ret, open_flags; char tmp_filename[PATH_MAX]; char backing_filename[PATH_MAX]; @@ -450,19 +450,39 @@ int bdrv_open2(BlockDriverState *bs, con if (flags & (BDRV_O_CACHE_WB|BDRV_O_NOCACHE)) bs->enable_write_cache = 1; - /* Note: for compatibility, we open disk image files as RDWR, and - RDONLY as fallback */ - try_rw = !bs->read_only || bs->is_temporary; - if (!(flags & BDRV_O_FILE)) - open_flags = (try_rw ? BDRV_O_RDWR : 0) | - (flags & (BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO)); - else - open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT); + /* + * Always clear these flags as they are not destined for the drivers. + */ + open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT); + + if (!(flags & BDRV_O_FILE)) { + /* + * For historical reasons we always try to open drive images as + * read-write first and only fall back to read-only later. + * + * This can be overriden using readonly suboption for the + * drive option. + */ + if (bs->read_only && !bs->is_temporary) { + open_flags &= ~BDRV_O_RDWR; + } else { + open_flags |= BDRV_O_RDWR; + } - ret = drv->bdrv_open(bs, filename, open_flags); - if ((ret == -EACCES || ret == -EPERM) && !(flags & BDRV_O_FILE)) { - ret = drv->bdrv_open(bs, filename, open_flags & ~BDRV_O_RDWR); - bs->read_only = 1; + /* + * Currently BDRV_O_CREAT is not supported by any image format, + * but I'm not sure that's reason enough to always clear it for + * the !BDRV_O_FILE case.. + */ + open_flags &= ~(BDRV_O_CREAT); + + ret = drv->bdrv_open(bs, filename, open_flags); + if (ret == -EACCES || ret == -EPERM) { + ret = drv->bdrv_open(bs, filename, open_flags & ~BDRV_O_RDWR); + bs->read_only = 1; + } + } else { + ret = drv->bdrv_open(bs, filename, open_flags); } if (ret < 0) {
Untangle the open flag manipulation in bdrv_open2 and document why we are clearing the various flags in the different flag combinations. Signed-off-by: Christoph Hellwig <hch@lst.de>