[Qemu-devel] [PATCH 2/2] block: untangle open flag manipulation in bdrv_open2

[Qemu-devel] [PATCH 2/2] block: untangle open flag manipulation in bdrv_open2

[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>

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

Re: [Qemu-devel] [PATCH 2/2] block: untangle open flag manipulation in bdrv_open2

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 2/2] block: untangle open flag manipulation in bdrv_open2

[Christoph Hellwig]

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.

[Qemu-devel] PATCH 3/2] block: kill BDRV_O_CREAT

[Christoph Hellwig]

The BDRV_O_CREAT option is unused inside qemu and partially duplicates
the bdrv_create method.  Remove it, and the -C option to qemu-io which
isn't used in qemu-iotests anyway.


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

Index: qemu/block.c
===================================================================
--- qemu.orig/block.c	2010-01-19 21:45:08.901004272 +0100
+++ qemu/block.c	2010-01-19 21:45:44.105254367 +0100
@@ -469,13 +469,6 @@ int bdrv_open2(BlockDriverState *bs, con
             open_flags |= BDRV_O_RDWR;
         }
 
-        /*
-         * 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);
Index: qemu/block.h
===================================================================
--- qemu.orig/block.h	2010-01-19 21:47:10.720003392 +0100
+++ qemu/block.h	2010-01-19 21:47:29.261004392 +0100
@@ -30,7 +30,7 @@ typedef struct QEMUSnapshotInfo {
 #define BDRV_O_RDONLY      0x0000
 #define BDRV_O_RDWR        0x0002
 #define BDRV_O_ACCESS      0x0003
-#define BDRV_O_CREAT       0x0004 /* create an empty file */
+/* 0x0004 was BDRV_O_CREAT */
 #define BDRV_O_SNAPSHOT    0x0008 /* open the file read only and save writes in a snapshot */
 #define BDRV_O_FILE        0x0010 /* open as a raw file (do not try to
                                      use a disk image format on top of
Index: qemu/block/nbd.c
===================================================================
--- qemu.orig/block/nbd.c	2010-01-19 21:45:52.925004048 +0100
+++ qemu/block/nbd.c	2010-01-19 21:45:59.117254240 +0100
@@ -49,9 +49,6 @@ static int nbd_open(BlockDriverState *bs
     size_t blocksize;
     int ret;
 
-    if ((flags & BDRV_O_CREAT))
-        return -EINVAL;
-
     if (!strstart(filename, "nbd:", &host))
         return -EINVAL;
 
Index: qemu/block/raw-posix.c
===================================================================
--- qemu.orig/block/raw-posix.c	2010-01-19 21:46:40.265260294 +0100
+++ qemu/block/raw-posix.c	2010-01-19 21:47:06.080254365 +0100
@@ -205,13 +205,9 @@ out_close:
 static int raw_open(BlockDriverState *bs, const char *filename, int flags)
 {
     BDRVRawState *s = bs->opaque;
-    int open_flags = 0;
 
     s->type = FTYPE_FILE;
-    if (flags & BDRV_O_CREAT)
-        open_flags = O_CREAT | O_TRUNC;
-
-    return raw_open_common(bs, filename, flags, open_flags);
+    return raw_open_common(bs, filename, flags, 0);
 }
 
 /* XXX: use host sector size if necessary with:
Index: qemu/block/raw-win32.c
===================================================================
--- qemu.orig/block/raw-win32.c	2010-01-19 21:46:05.928253941 +0100
+++ qemu/block/raw-win32.c	2010-01-19 21:46:36.202254338 +0100
@@ -76,7 +76,7 @@ static int set_sparse(int fd)
 static int raw_open(BlockDriverState *bs, const char *filename, int flags)
 {
     BDRVRawState *s = bs->opaque;
-    int access_flags, create_flags;
+    int access_flags;
     DWORD overlapped;
 
     s->type = FTYPE_FILE;
@@ -86,11 +86,7 @@ static int raw_open(BlockDriverState *bs
     } else {
         access_flags = GENERIC_READ;
     }
-    if (flags & BDRV_O_CREAT) {
-        create_flags = CREATE_ALWAYS;
-    } else {
-        create_flags = OPEN_EXISTING;
-    }
+
     overlapped = FILE_ATTRIBUTE_NORMAL;
     if ((flags & BDRV_O_NOCACHE))
         overlapped |= FILE_FLAG_NO_BUFFERING | FILE_FLAG_WRITE_THROUGH;
@@ -98,7 +94,7 @@ static int raw_open(BlockDriverState *bs
         overlapped |= FILE_FLAG_WRITE_THROUGH;
     s->hfile = CreateFile(filename, access_flags,
                           FILE_SHARE_READ, NULL,
-                          create_flags, overlapped, NULL);
+                          OPEN_EXISTING, overlapped, NULL);
     if (s->hfile == INVALID_HANDLE_VALUE) {
         int err = GetLastError();
 
Index: qemu/qemu-io.c
===================================================================
--- qemu.orig/qemu-io.c	2010-01-19 21:47:37.065011538 +0100
+++ qemu/qemu-io.c	2010-01-19 21:48:59.664317026 +0100
@@ -1307,7 +1307,6 @@ open_help(void)
 " 'open -Cn /tmp/data' - creates/opens data file read-write and uncached\n"
 "\n"
 " Opens a file for subsequent use by all of the other qemu-io commands.\n"
-" -C, -- create new file if it doesn't exist\n"
 " -r, -- open file read-only\n"
 " -s, -- use snapshot file\n"
 " -n, -- disable host cache\n"
@@ -1337,7 +1336,7 @@ open_f(int argc, char **argv)
 	int growable = 0;
 	int c;
 
-	while ((c = getopt(argc, argv, "snCrg")) != EOF) {
+	while ((c = getopt(argc, argv, "snrg")) != EOF) {
 		switch (c) {
 		case 's':
 			flags |= BDRV_O_SNAPSHOT;
@@ -1345,9 +1344,6 @@ open_f(int argc, char **argv)
 		case 'n':
 			flags |= BDRV_O_NOCACHE;
 			break;
-		case 'C':
-			flags |= BDRV_O_CREAT;
-			break;
 		case 'r':
 			readonly = 1;
 			break;
@@ -1396,10 +1392,9 @@ init_check_command(
 static void usage(const char *name)
 {
 	printf(
-"Usage: %s [-h] [-V] [-Crsnm] [-c cmd] ... [file]\n"
+"Usage: %s [-h] [-V] [-rsnm] [-c cmd] ... [file]\n"
 "QEMU Disk exerciser\n"
 "\n"
-"  -C, --create         create new file if it doesn't exist\n"
 "  -c, --cmd            command to execute\n"
 "  -r, --read-only      export read-only\n"
 "  -s, --snapshot       use snapshot file\n"
@@ -1418,13 +1413,12 @@ int main(int argc, char **argv)
 {
 	int readonly = 0;
 	int growable = 0;
-	const char *sopt = "hVc:Crsnmgk";
+	const char *sopt = "hVc:rsnmgk";
         const struct option lopt[] = {
 		{ "help", 0, NULL, 'h' },
 		{ "version", 0, NULL, 'V' },
 		{ "offset", 1, NULL, 'o' },
 		{ "cmd", 1, NULL, 'c' },
-		{ "create", 0, NULL, 'C' },
 		{ "read-only", 0, NULL, 'r' },
 		{ "snapshot", 0, NULL, 's' },
 		{ "nocache", 0, NULL, 'n' },
@@ -1450,9 +1444,6 @@ int main(int argc, char **argv)
 		case 'c':
 			add_user_command(optarg);
 			break;
-		case 'C':
-			flags |= BDRV_O_CREAT;
-			break;
 		case 'r':
 			readonly = 1;
 			break;