Patchwork [2/2] block: untangle open flag manipulation in bdrv_open2

login
register
mail settings
Submitter Christoph Hellwig
Date Jan. 11, 2010, 5:52 p.m.
Message ID <20100111175209.GB7571@lst.de>
Download mbox | patch
Permalink /patch/42649/
State New
Headers show

Comments

Christoph Hellwig - Jan. 11, 2010, 5:52 p.m.
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>
Kevin Wolf - Jan. 12, 2010, 5:32 p.m.
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
Christoph Hellwig - Jan. 12, 2010, 5:37 p.m.
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.

Patch

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) {