[Qemu-devel] [PATCH 00/15] block: Overriding the backing file with -drive

[Qemu-devel] [PATCH 00/15] block: Overriding the backing file with -drive

[Kevin Wolf]

This is the next part of the driver-specific options. Looks like we're getting
closer to pass file descriptors for the whole backing file chain. In fact, I
hope we're now only lacking QMP support before we can actually use it. :-)

This series adds support for the backing.* options namespace, and allows to use
the file.filename option to configure the filename string for non-top-level
BlockDriverStates. In the end you can use things like:

    -drive file=test.qcow2,backing.file.filename=/dev/fdset/1,
    backing.backing.file.driver=nbd,backing.backing.file.host=localhost


On another note, I'm still not sure whether I should leave all of this work
enabled for 1.5, or if we wouldn't be better off with disabling it for the
release so that we have some additional months before we commit to the
interface. Any opinions?


Kevin Wolf (15):
  block: Fail gracefully when using a format driver on protocol level
  block: Add driver-specific options for backing files
  block: Enable filename option
  raw-posix: Use bdrv_open options instead of filename
  raw-win32: Use bdrv_open options instead of filename
  blkdebug: Use bdrv_open options instead of filename
  blkverify: Use bdrv_open options instead of filename
  curl: Use bdrv_open options instead of filename
  gluster: Use bdrv_open options instead of filename
  iscsi: Use bdrv_open options instead of filename
  rbd: Use bdrv_open options instead of filename
  sheepdog: Use bdrv_open options instead of filename
  vvfat: Use bdrv_open options instead of filename
  block: Remove filename parameter from .bdrv_file_open()
  block: Allow overriding backing.file.filename

 block.c                    |  65 ++++++++++++--
 block/blkdebug.c           | 102 ++++++++++++++++------
 block/blkverify.c          |  93 ++++++++++++++++----
 block/curl.c               | 152 +++++++++++++++++++++-----------
 block/gluster.c            |  34 +++++++-
 block/iscsi.c              |  40 ++++++++-
 block/mirror.c             |   2 +-
 block/nbd.c                |   3 +-
 block/raw-posix.c          |  70 ++++++++++-----
 block/raw-win32.c          |  59 ++++++++++---
 block/rbd.c                |  32 ++++++-
 block/sheepdog.c           |  33 ++++++-
 block/vvfat.c              | 211 +++++++++++++++++++++++++++++++++------------
 include/block/block.h      |   2 +-
 include/block/block_int.h  |   3 +-
 tests/qemu-iotests/051     |   7 ++
 tests/qemu-iotests/051.out |  10 +++
 17 files changed, 713 insertions(+), 205 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 01/15] block: Fail gracefully when using a format driver on protocol level

[Kevin Wolf]

Specifying the wrong driver could fail an assertion:

$ qemu-system-x86_64 -drive file.driver=qcow2,file=x
qemu-system-x86_64: block.c:721: bdrv_open_common: Assertion `file !=
((void *)0)' failed.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                    |  7 +++++++
 tests/qemu-iotests/051     |  7 +++++++
 tests/qemu-iotests/051.out | 10 ++++++++++
 3 files changed, 24 insertions(+)

diff --git a/block.c b/block.c
index 602d8a4..f23bdcc 100644
--- a/block.c
+++ b/block.c
@@ -718,6 +718,13 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
         assert(drv->bdrv_parse_filename || filename != NULL);
         ret = drv->bdrv_file_open(bs, filename, options, open_flags);
     } else {
+        if (file == NULL) {
+            qerror_report(ERROR_CLASS_GENERIC_ERROR, "The '%s' block driver is "
+                          "not suitable for the bottom level",
+                          drv->format_name);
+            ret = -EINVAL;
+            goto free_and_fail;
+        }
         assert(file != NULL);
         bs->file = file;
         ret = drv->bdrv_open(bs, options, open_flags);
diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 17a1d92..8f96ef4 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -142,6 +142,13 @@ run_qemu -drive media=cdrom,cache=writethrough
 run_qemu -drive media=cdrom,cache=unsafe
 run_qemu -drive media=cdrom,cache=invalid_value
 
+echo
+echo === Specifying the protocol layer ===
+echo
+
+run_qemu -drive file=$TEST_IMG,file.driver=file
+run_qemu -drive file=$TEST_IMG,file.driver=qcow2
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index da0d18b..194f2d2 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -159,4 +159,14 @@ q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
 Testing: -drive media=cdrom,cache=invalid_value
 qemu: -drive media=cdrom,cache=invalid_value: invalid cache option
 
+
+=== Specifying the protocol layer ===
+
+Testing: -drive file=TEST_DIR/t.qcow2,file.driver=file
+q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
+
+Testing: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2
+qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: The 'qcow2' block driver is not suitable for the bottom level
+qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: could not open disk image TEST_DIR/t.qcow2: Invalid argument
+
 *** done
-- 
1.8.1.4

[Qemu-devel] [PATCH 02/15] block: Add driver-specific options for backing files

[Kevin Wolf]

Options starting in "backing." are passed to the backing file now. If
you don't need to specify the filename for the backing file, you can add
it on the command line instead of in the image file:

$ qemu-nbd -t /tmp/test.img
$ qemu-img create -f qcow2 empty.qcow2 1G
$ qemu-system-x86_64 -drive file=empty.qcow2,backing.file.driver=nbd,\
    backing.file.host=localhost

Note that this doesn't override the backing filename from the image. If
the image has one, this will fail because NBD doesn't want the options
and a filename at the same time.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c               | 27 +++++++++++++++++++++++----
 block/mirror.c        |  2 +-
 include/block/block.h |  2 +-
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index f23bdcc..fc19df1 100644
--- a/block.c
+++ b/block.c
@@ -845,18 +845,33 @@ fail:
     return ret;
 }
 
-int bdrv_open_backing_file(BlockDriverState *bs)
+/*
+ * Opens the backing file for a BlockDriverState if not yet open
+ *
+ * options is a QDict of options to pass to the block drivers, or NULL for an
+ * empty set of options. The reference to the QDict is transferred to this
+ * function (even on failure), so if the caller intends to reuse the dictionary,
+ * it needs to use QINCREF() before calling bdrv_file_open.
+ */
+int bdrv_open_backing_file(BlockDriverState *bs, QDict *options)
 {
     char backing_filename[PATH_MAX];
     int back_flags, ret;
     BlockDriver *back_drv = NULL;
 
     if (bs->backing_hd != NULL) {
+        QDECREF(options);
         return 0;
     }
 
+    /* NULL means an empty set of options */
+    if (options == NULL) {
+        options = qdict_new();
+    }
+
     bs->open_flags &= ~BDRV_O_NO_BACKING;
-    if (bs->backing_file[0] == '\0') {
+    if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
+        QDECREF(options);
         return 0;
     }
 
@@ -871,7 +886,8 @@ int bdrv_open_backing_file(BlockDriverState *bs)
     /* backing files always opened read-only */
     back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT);
 
-    ret = bdrv_open(bs->backing_hd, backing_filename, NULL,
+    ret = bdrv_open(bs->backing_hd,
+                    *backing_filename ? backing_filename : NULL, options,
                     back_flags, back_drv);
     if (ret < 0) {
         bdrv_delete(bs->backing_hd);
@@ -1027,7 +1043,10 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
 
     /* If there is a backing file, use it */
     if ((flags & BDRV_O_NO_BACKING) == 0) {
-        ret = bdrv_open_backing_file(bs);
+        QDict *backing_options;
+
+        extract_subqdict(options, &backing_options, "backing.");
+        ret = bdrv_open_backing_file(bs, backing_options);
         if (ret < 0) {
             goto close_and_fail;
         }
diff --git a/block/mirror.c b/block/mirror.c
index a62ad86..8b07dec 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -507,7 +507,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
     int ret;
 
-    ret = bdrv_open_backing_file(s->target);
+    ret = bdrv_open_backing_file(s->target, NULL);
     if (ret < 0) {
         char backing_filename[PATH_MAX];
         bdrv_get_full_backing_filename(s->target, backing_filename,
diff --git a/include/block/block.h b/include/block/block.h
index 9dc6aad..a512f1b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -137,7 +137,7 @@ int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_parse_discard_flags(const char *mode, int *flags);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename,
                    QDict *options, int flags);
-int bdrv_open_backing_file(BlockDriverState *bs);
+int bdrv_open_backing_file(BlockDriverState *bs, QDict *options);
 int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
               int flags, BlockDriver *drv);
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
-- 
1.8.1.4

[Qemu-devel] [PATCH 03/15] block: Enable filename option

[Kevin Wolf]

This allows using the file.filename option instead of the string that
comes from -drive file=... and is passed around as a separate parameter.
The goal is to get rid of this parameter and use the options QDict more
consistently.

With this option you can access not only the top-level image, but
specify a filename for the backing file (currently only if no backing
file exists, but we'll allow overriding it later)

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index fc19df1..a67365e 100644
--- a/block.c
+++ b/block.c
@@ -667,10 +667,10 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
  * Removes all processed options from *options.
  */
 static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
-    const char *filename, QDict *options,
-    int flags, BlockDriver *drv)
+    QDict *options, int flags, BlockDriver *drv)
 {
     int ret, open_flags;
+    const char *filename;
 
     assert(drv != NULL);
     assert(bs->file == NULL);
@@ -698,6 +698,12 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
         bdrv_enable_copy_on_read(bs);
     }
 
+    if (file != NULL) {
+        filename = file->filename;
+    } else {
+        filename = qdict_get_try_str(options, "filename");
+    }
+
     if (filename != NULL) {
         pstrcpy(bs->filename, sizeof(bs->filename), filename);
     } else {
@@ -780,6 +786,18 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
     bs->options = options;
     options = qdict_clone_shallow(options);
 
+    /* Fetch the file name from the options QDict if necessary */
+    if (!filename) {
+        filename = qdict_get_try_str(options, "filename");
+    } else if (filename && !qdict_haskey(options, "filename")) {
+        qdict_put(options, "filename", qstring_from_str(filename));
+    } else {
+        qerror_report(ERROR_CLASS_GENERIC_ERROR, "Can't specify 'file' and "
+                      "'filename' options at the same time");
+        ret = -EINVAL;
+        goto fail;
+    }
+
     /* Find the right block driver */
     drvname = qdict_get_try_str(options, "driver");
     if (drvname) {
@@ -816,11 +834,16 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
         goto fail;
     }
 
-    ret = bdrv_open_common(bs, NULL, filename, options, flags, drv);
+    ret = bdrv_open_common(bs, NULL, options, flags, drv);
     if (ret < 0) {
         goto fail;
     }
 
+    /* TODO Remove once all protocols know the filename option */
+    if (qdict_haskey(options, "filename")) {
+        qdict_del(options, "filename");
+    }
+
     /* Check if any unknown options were used */
     if (qdict_size(options) != 0) {
         const QDictEntry *entry = qdict_first(options);
@@ -1031,7 +1054,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     }
 
     /* Open the image */
-    ret = bdrv_open_common(bs, file, filename, options, flags, drv);
+    ret = bdrv_open_common(bs, file, options, flags, drv);
     if (ret < 0) {
         goto unlink_and_fail;
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 04/15] raw-posix: Use bdrv_open options instead of filename

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/raw-posix.c | 57 +++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 12 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 99ac869..9b7bc9c 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -262,15 +262,42 @@ error:
 }
 #endif
 
-static int raw_open_common(BlockDriverState *bs, const char *filename,
+static QemuOptsList raw_runtime_opts = {
+    .name = "raw",
+    .head = QTAILQ_HEAD_INITIALIZER(raw_runtime_opts.head),
+    .desc = {
+        {
+            .name = "filename",
+            .type = QEMU_OPT_STRING,
+            .help = "File name of the image",
+        },
+        { /* end of list */ }
+    },
+};
+
+static int raw_open_common(BlockDriverState *bs, QDict *options,
                            int bdrv_flags, int open_flags)
 {
     BDRVRawState *s = bs->opaque;
+    QemuOpts *opts;
+    Error *local_err = NULL;
+    const char* filename;
     int fd, ret;
 
+    opts = qemu_opts_create_nofail(&raw_runtime_opts);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    filename = qemu_opt_get(opts, "filename");
+
     ret = raw_normalize_devicepath(&filename);
     if (ret != 0) {
-        return ret;
+        goto fail;
     }
 
     s->open_flags = open_flags;
@@ -280,16 +307,18 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
     fd = qemu_open(filename, s->open_flags, 0644);
     if (fd < 0) {
         ret = -errno;
-        if (ret == -EROFS)
+        if (ret == -EROFS) {
             ret = -EACCES;
-        return ret;
+        }
+        goto fail;
     }
     s->fd = fd;
 
 #ifdef CONFIG_LINUX_AIO
     if (raw_set_aio(&s->aio_ctx, &s->use_aio, bdrv_flags)) {
         qemu_close(fd);
-        return -errno;
+        ret = -errno;
+        goto fail;
     }
 #endif
 
@@ -300,7 +329,10 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
     }
 #endif
 
-    return 0;
+    ret = 0;
+fail:
+    qemu_opts_del(opts);
+    return ret;
 }
 
 static int raw_open(BlockDriverState *bs, const char *filename,
@@ -309,7 +341,7 @@ static int raw_open(BlockDriverState *bs, const char *filename,
     BDRVRawState *s = bs->opaque;
 
     s->type = FTYPE_FILE;
-    return raw_open_common(bs, filename, flags, 0);
+    return raw_open_common(bs, options, flags, 0);
 }
 
 static int raw_reopen_prepare(BDRVReopenState *state,
@@ -1293,11 +1325,12 @@ static int check_hdev_writable(BDRVRawState *s)
     return 0;
 }
 
-static int hdev_open(BlockDriverState *bs, const char *filename,
+static int hdev_open(BlockDriverState *bs, const char *dummy,
                      QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
+    const char *filename = qdict_get_str(options, "filename");
 
 #if defined(__APPLE__) && defined(__MACH__)
     if (strstart(filename, "/dev/cdrom", NULL)) {
@@ -1338,7 +1371,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename,
     }
 #endif
 
-    ret = raw_open_common(bs, filename, flags, 0);
+    ret = raw_open_common(bs, options, flags, 0);
     if (ret < 0) {
         return ret;
     }
@@ -1541,7 +1574,7 @@ static int floppy_open(BlockDriverState *bs, const char *filename,
     s->type = FTYPE_FD;
 
     /* open will not fail even if no floppy is inserted, so add O_NONBLOCK */
-    ret = raw_open_common(bs, filename, flags, O_NONBLOCK);
+    ret = raw_open_common(bs, options, flags, O_NONBLOCK);
     if (ret)
         return ret;
 
@@ -1663,7 +1696,7 @@ static int cdrom_open(BlockDriverState *bs, const char *filename,
     s->type = FTYPE_CD;
 
     /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
-    return raw_open_common(bs, filename, flags, O_NONBLOCK);
+    return raw_open_common(bs, options, flags, O_NONBLOCK);
 }
 
 static int cdrom_probe_device(const char *filename)
@@ -1772,7 +1805,7 @@ static int cdrom_open(BlockDriverState *bs, const char *filename,
 
     s->type = FTYPE_CD;
 
-    ret = raw_open_common(bs, filename, flags, 0);
+    ret = raw_open_common(bs, options, flags, 0);
     if (ret)
         return ret;
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 05/15] raw-win32: Use bdrv_open options instead of filename

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/raw-win32.c | 57 +++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 47 insertions(+), 10 deletions(-)

diff --git a/block/raw-win32.c b/block/raw-win32.c
index ece2f1a..49d9234 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -221,21 +221,50 @@ static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped)
     }
 }
 
-static int raw_open(BlockDriverState *bs, const char *filename,
+static QemuOptsList raw_runtime_opts = {
+    .name = "raw",
+    .head = QTAILQ_HEAD_INITIALIZER(raw_runtime_opts.head),
+    .desc = {
+        {
+            .name = "filename",
+            .type = QEMU_OPT_STRING,
+            .help = "File name of the image",
+        },
+        { /* end of list */ }
+    },
+};
+
+static int raw_open(BlockDriverState *bs, const char *unused,
                     QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int access_flags;
     DWORD overlapped;
+    QemuOpts *opts;
+    Error *local_err = NULL;
+    const char* filename;
+    int ret;
 
     s->type = FTYPE_FILE;
 
+    opts = qemu_opts_create_nofail(&raw_runtime_opts);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    filename = qemu_opt_get(opts, "filename");
+
     raw_parse_flags(flags, &access_flags, &overlapped);
-    
+
     if ((flags & BDRV_O_NATIVE_AIO) && aio == NULL) {
         aio = win32_aio_init();
         if (aio == NULL) {
-            return -EINVAL;
+            ret = -EINVAL;
+            goto fail;
         }
     }
 
@@ -245,20 +274,27 @@ static int raw_open(BlockDriverState *bs, const char *filename,
     if (s->hfile == INVALID_HANDLE_VALUE) {
         int err = GetLastError();
 
-        if (err == ERROR_ACCESS_DENIED)
-            return -EACCES;
-        return -EINVAL;
+        if (err == ERROR_ACCESS_DENIED) {
+            ret = -EACCES;
+        } else {
+            ret = -EINVAL;
+        }
+        goto fail;
     }
 
     if (flags & BDRV_O_NATIVE_AIO) {
-        int ret = win32_aio_attach(aio, s->hfile);
+        ret = win32_aio_attach(aio, s->hfile);
         if (ret < 0) {
             CloseHandle(s->hfile);
-            return ret;
+            goto fail;
         }
         s->aio = aio;
     }
-    return 0;
+
+    ret = 0;
+fail:
+    qemu_opts_del(opts);
+    return ret;
 }
 
 static BlockDriverAIOCB *raw_aio_readv(BlockDriverState *bs,
@@ -495,13 +531,14 @@ static int hdev_probe_device(const char *filename)
     return 0;
 }
 
-static int hdev_open(BlockDriverState *bs, const char *filename,
+static int hdev_open(BlockDriverState *bs, const char *dummy,
                      QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int access_flags, create_flags;
     DWORD overlapped;
     char device_name[64];
+    const char *filename = qdict_get_str(options, "filename");
 
     if (strstart(filename, "/dev/cdrom", NULL)) {
         if (find_cdrom(device_name, sizeof(device_name)) < 0)
-- 
1.8.1.4

[Qemu-devel] [PATCH 06/15] blkdebug: Use bdrv_open options instead of filename

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkdebug.c | 103 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 77 insertions(+), 26 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 37cfbc7..da11de2 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -273,11 +273,6 @@ static int read_config(BDRVBlkdebugState *s, const char *filename)
     int ret;
     struct add_rule_data d;
 
-    /* Allow usage without config file */
-    if (!*filename) {
-        return 0;
-    }
-
     f = fopen(filename, "r");
     if (f == NULL) {
         return -errno;
@@ -304,44 +299,99 @@ fail:
 }
 
 /* Valid blkdebug filenames look like blkdebug:path/to/config:path/to/image */
-static int blkdebug_open(BlockDriverState *bs, const char *filename,
-                         QDict *options, int flags)
+static void blkdebug_parse_filename(const char *filename, QDict *options,
+                                    Error **errp)
 {
-    BDRVBlkdebugState *s = bs->opaque;
-    int ret;
-    char *config, *c;
+    const char *c;
 
     /* Parse the blkdebug: prefix */
-    if (strncmp(filename, "blkdebug:", strlen("blkdebug:"))) {
-        return -EINVAL;
+    if (!strstart(filename, "blkdebug:", &filename)) {
+        error_setg(errp, "File name string must start with 'blkdebug:'");
+        return;
     }
-    filename += strlen("blkdebug:");
 
-    /* Read rules from config file */
+    /* Parse config file path */
     c = strchr(filename, ':');
     if (c == NULL) {
-        return -EINVAL;
+        error_setg(errp, "blkdebug requires both config file and image path");
+        return;
     }
 
-    config = g_strdup(filename);
-    config[c - filename] = '\0';
-    ret = read_config(s, config);
-    g_free(config);
-    if (ret < 0) {
-        return ret;
+    if (c != filename) {
+        QString *config_path;
+        config_path = qstring_from_substr(filename, 0, c - filename - 1);
+        qdict_put(options, "config", config_path);
     }
+
+    /* TODO Allow multi-level nesting and set file.filename here */
     filename = c + 1;
+    qdict_put(options, "x-image", qstring_from_str(filename));
+}
+
+static QemuOptsList runtime_opts = {
+    .name = "blkdebug",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = "config",
+            .type = QEMU_OPT_STRING,
+            .help = "Path to the configuration file",
+        },
+        {
+            .name = "x-image",
+            .type = QEMU_OPT_STRING,
+            .help = "[internal use only, will be removed]",
+        },
+        { /* end of list */ }
+    },
+};
+
+static int blkdebug_open(BlockDriverState *bs, const char *dummy,
+                         QDict *options, int flags)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+    QemuOpts *opts;
+    Error *local_err = NULL;
+    const char *filename, *config;
+    int ret;
+
+    opts = qemu_opts_create_nofail(&runtime_opts);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    /* Read rules from config file */
+    config = qemu_opt_get(opts, "config");
+    if (config) {
+        ret = read_config(s, config);
+        if (ret < 0) {
+            goto fail;
+        }
+    }
 
     /* Set initial state */
     s->state = 1;
 
     /* Open the backing file */
+    filename = qemu_opt_get(opts, "x-image");
+    if (filename == NULL) {
+        ret = -EINVAL;
+        goto fail;
+    }
+
     ret = bdrv_file_open(&bs->file, filename, NULL, flags);
     if (ret < 0) {
-        return ret;
+        goto fail;
     }
 
-    return 0;
+    ret = 0;
+fail:
+    qemu_opts_del(opts);
+    return ret;
 }
 
 static void error_callback_bh(void *opaque)
@@ -574,9 +624,10 @@ static BlockDriver bdrv_blkdebug = {
 
     .instance_size      = sizeof(BDRVBlkdebugState),
 
-    .bdrv_file_open     = blkdebug_open,
-    .bdrv_close         = blkdebug_close,
-    .bdrv_getlength     = blkdebug_getlength,
+    .bdrv_parse_filename    = blkdebug_parse_filename,
+    .bdrv_file_open         = blkdebug_open,
+    .bdrv_close             = blkdebug_close,
+    .bdrv_getlength         = blkdebug_getlength,
 
     .bdrv_aio_readv     = blkdebug_aio_readv,
     .bdrv_aio_writev    = blkdebug_aio_writev,
-- 
1.8.1.4

[Qemu-devel] [PATCH 07/15] blkverify: Use bdrv_open options instead of filename

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkverify.c | 94 ++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 76 insertions(+), 18 deletions(-)

diff --git a/block/blkverify.c b/block/blkverify.c
index 59e3b05..37806e3 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -69,44 +69,101 @@ static void GCC_FMT_ATTR(2, 3) blkverify_err(BlkverifyAIOCB *acb,
 }
 
 /* Valid blkverify filenames look like blkverify:path/to/raw_image:path/to/image */
-static int blkverify_open(BlockDriverState *bs, const char *filename,
-                          QDict *options, int flags)
+static void blkverify_parse_filename(const char *filename, QDict *options,
+                                     Error **errp)
 {
-    BDRVBlkverifyState *s = bs->opaque;
-    int ret;
-    char *raw, *c;
+    const char *c;
+    QString *raw_path;
+
 
     /* Parse the blkverify: prefix */
-    if (strncmp(filename, "blkverify:", strlen("blkverify:"))) {
-        return -EINVAL;
+    if (!strstart(filename, "blkverify:", &filename)) {
+        error_setg(errp, "File name string must start with 'blkverify:'");
+        return;
     }
-    filename += strlen("blkverify:");
 
     /* Parse the raw image filename */
     c = strchr(filename, ':');
     if (c == NULL) {
-        return -EINVAL;
+        error_setg(errp, "blkdebug requires raw copy and original image path");
+        return;
+    }
+
+    /* TODO Implement option pass-through and set raw.filename here */
+    raw_path = qstring_from_substr(filename, 0, c - filename - 1);
+    qdict_put(options, "x-raw", raw_path);
+
+    /* TODO Allow multi-level nesting and set file.filename here */
+    filename = c + 1;
+    qdict_put(options, "x-image", qstring_from_str(filename));
+}
+
+static QemuOptsList runtime_opts = {
+    .name = "blkverify",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = "x-raw",
+            .type = QEMU_OPT_STRING,
+            .help = "[internal use only, will be removed]",
+        },
+        {
+            .name = "x-image",
+            .type = QEMU_OPT_STRING,
+            .help = "[internal use only, will be removed]",
+        },
+        { /* end of list */ }
+    },
+};
+
+static int blkverify_open(BlockDriverState *bs, const char *dummy,
+                          QDict *options, int flags)
+{
+    BDRVBlkverifyState *s = bs->opaque;
+    QemuOpts *opts;
+    Error *local_err = NULL;
+    const char *filename, *raw;
+    int ret;
+
+    opts = qemu_opts_create_nofail(&runtime_opts);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    /* Parse the raw image filename */
+    raw = qemu_opt_get(opts, "x-raw");
+    if (raw == NULL) {
+        ret = -EINVAL;
+        goto fail;
     }
 
-    raw = g_strdup(filename);
-    raw[c - filename] = '\0';
     ret = bdrv_file_open(&bs->file, raw, NULL, flags);
-    g_free(raw);
     if (ret < 0) {
-        return ret;
+        goto fail;
     }
-    filename = c + 1;
 
     /* Open the test file */
+    filename = qemu_opt_get(opts, "x-image");
+    if (filename == NULL) {
+        ret = -EINVAL;
+        goto fail;
+    }
+
     s->test_file = bdrv_new("");
     ret = bdrv_open(s->test_file, filename, NULL, flags, NULL);
     if (ret < 0) {
         bdrv_delete(s->test_file);
         s->test_file = NULL;
-        return ret;
+        goto fail;
     }
 
-    return 0;
+    ret = 0;
+fail:
+    return ret;
 }
 
 static void blkverify_close(BlockDriverState *bs)
@@ -351,8 +408,9 @@ static BlockDriver bdrv_blkverify = {
 
     .bdrv_getlength     = blkverify_getlength,
 
-    .bdrv_file_open     = blkverify_open,
-    .bdrv_close         = blkverify_close,
+    .bdrv_parse_filename    = blkverify_parse_filename,
+    .bdrv_file_open         = blkverify_open,
+    .bdrv_close             = blkverify_close,
 
     .bdrv_aio_readv     = blkverify_aio_readv,
     .bdrv_aio_writev    = blkverify_aio_writev,
-- 
1.8.1.4

[Qemu-devel] [PATCH 08/15] curl: Use bdrv_open options instead of filename

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/curl.c | 153 +++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 102 insertions(+), 51 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 186e3b0..61bc3db 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -335,12 +335,9 @@ static void curl_clean_state(CURLState *s)
     s->in_use = 0;
 }
 
-static int curl_open(BlockDriverState *bs, const char *filename,
-                     QDict *options, int flags)
+static void curl_parse_filename(const char *filename, QDict *options,
+                                Error **errp)
 {
-    BDRVCURLState *s = bs->opaque;
-    CURLState *state = NULL;
-    double d;
 
     #define RA_OPTSTR ":readahead="
     char *file;
@@ -348,19 +345,17 @@ static int curl_open(BlockDriverState *bs, const char *filename,
     const char *ra_val;
     int parse_state = 0;
 
-    static int inited = 0;
-
     file = g_strdup(filename);
-    s->readahead_size = READ_AHEAD_SIZE;
 
     /* Parse a trailing ":readahead=#:" param, if present. */
     ra = file + strlen(file) - 1;
     while (ra >= file) {
         if (parse_state == 0) {
-            if (*ra == ':')
+            if (*ra == ':') {
                 parse_state++;
-            else
+            } else {
                 break;
+            }
         } else if (parse_state == 1) {
             if (*ra > '9' || *ra < '0') {
                 char *opt_start = ra - strlen(RA_OPTSTR) + 1;
@@ -369,29 +364,78 @@ static int curl_open(BlockDriverState *bs, const char *filename,
                     ra_val = ra + 1;
                     ra -= strlen(RA_OPTSTR) - 1;
                     *ra = '\0';
-                    s->readahead_size = atoi(ra_val);
-                    break;
-                } else {
-                    break;
+                    qdict_put(options, "readahead", qstring_from_str(ra_val));
                 }
+                break;
             }
         }
         ra--;
     }
 
+    qdict_put(options, "url", qstring_from_str(file));
+
+    g_free(file);
+}
+
+static QemuOptsList runtime_opts = {
+    .name = "curl",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = "url",
+            .type = QEMU_OPT_STRING,
+            .help = "URL to open",
+        },
+        {
+            .name = "readahead",
+            .type = QEMU_OPT_SIZE,
+            .help = "Readahead size",
+        },
+        { /* end of list */ }
+    },
+};
+
+static int curl_open(BlockDriverState *bs, const char *dummy,
+                     QDict *options, int flags)
+{
+    BDRVCURLState *s = bs->opaque;
+    CURLState *state = NULL;
+    QemuOpts *opts;
+    Error *local_err = NULL;
+    const char *file;
+    double d;
+
+    static int inited = 0;
+
+    opts = qemu_opts_create_nofail(&runtime_opts);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        goto out_noclean;
+    }
+
+    s->readahead_size = qemu_opt_get_size(opts, "readahead", READ_AHEAD_SIZE);
     if ((s->readahead_size & 0x1ff) != 0) {
         fprintf(stderr, "HTTP_READAHEAD_SIZE %zd is not a multiple of 512\n",
                 s->readahead_size);
         goto out_noclean;
     }
 
+    file = qemu_opt_get(opts, "url");
+    if (file == NULL) {
+        qerror_report(ERROR_CLASS_GENERIC_ERROR, "curl block driver requires "
+                      "an 'url' option");
+        goto out_noclean;
+    }
+
     if (!inited) {
         curl_global_init(CURL_GLOBAL_ALL);
         inited = 1;
     }
 
     DPRINTF("CURL: Opening %s\n", file);
-    s->url = file;
+    s->url = g_strdup(file);
     state = curl_init_state(s);
     if (!state)
         goto out_noclean;
@@ -423,6 +467,7 @@ static int curl_open(BlockDriverState *bs, const char *filename,
     curl_multi_setopt( s->multi, CURLMOPT_SOCKETFUNCTION, curl_sock_cb ); 
     curl_multi_do(s);
 
+    qemu_opts_del(opts);
     return 0;
 
 out:
@@ -430,7 +475,8 @@ out:
     curl_easy_cleanup(state->curl);
     state->curl = NULL;
 out_noclean:
-    g_free(file);
+    g_free(s->url);
+    qemu_opts_del(opts);
     return -EINVAL;
 }
 
@@ -568,63 +614,68 @@ static int64_t curl_getlength(BlockDriverState *bs)
 }
 
 static BlockDriver bdrv_http = {
-    .format_name     = "http",
-    .protocol_name   = "http",
+    .format_name            = "http",
+    .protocol_name          = "http",
 
-    .instance_size   = sizeof(BDRVCURLState),
-    .bdrv_file_open  = curl_open,
-    .bdrv_close      = curl_close,
-    .bdrv_getlength  = curl_getlength,
+    .instance_size          = sizeof(BDRVCURLState),
+    .bdrv_parse_filename    = curl_parse_filename,
+    .bdrv_file_open         = curl_open,
+    .bdrv_close             = curl_close,
+    .bdrv_getlength         = curl_getlength,
 
-    .bdrv_aio_readv  = curl_aio_readv,
+    .bdrv_aio_readv         = curl_aio_readv,
 };
 
 static BlockDriver bdrv_https = {
-    .format_name     = "https",
-    .protocol_name   = "https",
+    .format_name            = "https",
+    .protocol_name          = "https",
 
-    .instance_size   = sizeof(BDRVCURLState),
-    .bdrv_file_open  = curl_open,
-    .bdrv_close      = curl_close,
-    .bdrv_getlength  = curl_getlength,
+    .instance_size          = sizeof(BDRVCURLState),
+    .bdrv_parse_filename    = curl_parse_filename,
+    .bdrv_file_open         = curl_open,
+    .bdrv_close             = curl_close,
+    .bdrv_getlength         = curl_getlength,
 
-    .bdrv_aio_readv  = curl_aio_readv,
+    .bdrv_aio_readv         = curl_aio_readv,
 };
 
 static BlockDriver bdrv_ftp = {
-    .format_name     = "ftp",
-    .protocol_name   = "ftp",
+    .format_name            = "ftp",
+    .protocol_name          = "ftp",
 
-    .instance_size   = sizeof(BDRVCURLState),
-    .bdrv_file_open  = curl_open,
-    .bdrv_close      = curl_close,
-    .bdrv_getlength  = curl_getlength,
+    .instance_size          = sizeof(BDRVCURLState),
+    .bdrv_parse_filename    = curl_parse_filename,
+    .bdrv_file_open         = curl_open,
+    .bdrv_close             = curl_close,
+    .bdrv_getlength         = curl_getlength,
 
-    .bdrv_aio_readv  = curl_aio_readv,
+    .bdrv_aio_readv         = curl_aio_readv,
 };
 
 static BlockDriver bdrv_ftps = {
-    .format_name     = "ftps",
-    .protocol_name   = "ftps",
+    .format_name            = "ftps",
+    .protocol_name          = "ftps",
 
-    .instance_size   = sizeof(BDRVCURLState),
-    .bdrv_file_open  = curl_open,
-    .bdrv_close      = curl_close,
-    .bdrv_getlength  = curl_getlength,
+    .instance_size          = sizeof(BDRVCURLState),
+    .bdrv_parse_filename    = curl_parse_filename,
+    .bdrv_file_open         = curl_open,
+    .bdrv_close             = curl_close,
+    .bdrv_getlength         = curl_getlength,
 
-    .bdrv_aio_readv  = curl_aio_readv,
+    .bdrv_aio_readv         = curl_aio_readv,
 };
 
 static BlockDriver bdrv_tftp = {
-    .format_name     = "tftp",
-    .protocol_name   = "tftp",
+    .format_name            = "tftp",
+    .protocol_name          = "tftp",
 
-    .instance_size   = sizeof(BDRVCURLState),
-    .bdrv_file_open  = curl_open,
-    .bdrv_close      = curl_close,
-    .bdrv_getlength  = curl_getlength,
+    .instance_size          = sizeof(BDRVCURLState),
+    .bdrv_parse_filename    = curl_parse_filename,
+    .bdrv_file_open         = curl_open,
+    .bdrv_close             = curl_close,
+    .bdrv_getlength         = curl_getlength,
 
-    .bdrv_aio_readv  = curl_aio_readv,
+    .bdrv_aio_readv         = curl_aio_readv,
 };
 
 static void curl_block_init(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 09/15] gluster: Use bdrv_open options instead of filename

[Kevin Wolf]

This is only to convert the internal interface that is used for passing
the "filename" to be parsed, but converting to actual fine grained
options is left for another day, as it doesn't look trivial.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/gluster.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/block/gluster.c b/block/gluster.c
index 9ccd4d4..3796da8 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -282,13 +282,42 @@ static int qemu_gluster_aio_flush_cb(void *opaque)
     return (s->qemu_aio_count > 0);
 }
 
-static int qemu_gluster_open(BlockDriverState *bs, const char *filename,
+/* TODO Convert to fine grained options */
+static QemuOptsList runtime_opts = {
+    .name = "gluster",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = "filename",
+            .type = QEMU_OPT_STRING,
+            .help = "URL to the gluster image",
+        },
+        { /* end of list */ }
+    },
+};
+
+static int qemu_gluster_open(BlockDriverState *bs, const char *dummy,
     QDict *options, int bdrv_flags)
 {
     BDRVGlusterState *s = bs->opaque;
     int open_flags = O_BINARY;
     int ret = 0;
     GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
+    QemuOpts *opts;
+    Error *local_err = NULL;
+    const char *filename;
+
+    opts = qemu_opts_create_nofail(&runtime_opts);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        ret = -EINVAL;
+        goto out;
+    }
+
+    filename = qemu_opt_get(opts, "filename");
+
 
     s->glfs = qemu_gluster_init(gconf, filename);
     if (!s->glfs) {
@@ -322,6 +351,7 @@ static int qemu_gluster_open(BlockDriverState *bs, const char *filename,
         qemu_gluster_aio_event_reader, NULL, qemu_gluster_aio_flush_cb, s);
 
 out:
+    qemu_opts_del(opts);
     qemu_gluster_gconf_free(gconf);
     if (!ret) {
         return ret;
-- 
1.8.1.4

[Qemu-devel] [PATCH 10/15] iscsi: Use bdrv_open options instead of filename

[Kevin Wolf]

This is only to convert the internal interface that is used for passing
the "filename" to be parsed, but converting to actual fine grained
options is left for another day, as it doesn't look trivial.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/iscsi.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 92d6eae..907beba 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1003,11 +1003,25 @@ out:
     return ret;
 }
 
+/* TODO Convert to fine grained options */
+static QemuOptsList runtime_opts = {
+    .name = "iscsi",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = "filename",
+            .type = QEMU_OPT_STRING,
+            .help = "URL to the iscsi image",
+        },
+        { /* end of list */ }
+    },
+};
+
 /*
  * We support iscsi url's on the form
  * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>
  */
-static int iscsi_open(BlockDriverState *bs, const char *filename,
+static int iscsi_open(BlockDriverState *bs, const char *dummy,
                       QDict *options, int flags)
 {
     IscsiLun *iscsilun = bs->opaque;
@@ -1016,6 +1030,9 @@ static int iscsi_open(BlockDriverState *bs, const char *filename,
     struct scsi_task *task = NULL;
     struct scsi_inquiry_standard *inq = NULL;
     char *initiator_name = NULL;
+    QemuOpts *opts;
+    Error *local_err = NULL;
+    const char *filename;
     int ret;
 
     if ((BDRV_SECTOR_SIZE % 512) != 0) {
@@ -1025,6 +1042,18 @@ static int iscsi_open(BlockDriverState *bs, const char *filename,
         return -EINVAL;
     }
 
+    opts = qemu_opts_create_nofail(&runtime_opts);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        ret = -EINVAL;
+        goto out;
+    }
+
+    filename = qemu_opt_get(opts, "filename");
+
+
     iscsi_url = iscsi_parse_full_url(iscsi, filename);
     if (iscsi_url == NULL) {
         error_report("Failed to parse URL : %s", filename);
@@ -1126,6 +1155,7 @@ static int iscsi_open(BlockDriverState *bs, const char *filename,
 #endif
 
 out:
+    qemu_opts_del(opts);
     if (initiator_name != NULL) {
         g_free(initiator_name);
     }
@@ -1190,6 +1220,7 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
     int64_t total_size = 0;
     BlockDriverState bs;
     IscsiLun *iscsilun = NULL;
+    QDict *bs_options;
 
     memset(&bs, 0, sizeof(BlockDriverState));
 
@@ -1204,7 +1235,11 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
     bs.opaque = g_malloc0(sizeof(struct IscsiLun));
     iscsilun = bs.opaque;
 
-    ret = iscsi_open(&bs, filename, NULL, 0);
+    bs_options = qdict_new();
+    qdict_put(bs_options, "filename", qstring_from_str(filename));
+    ret = iscsi_open(&bs, NULL, bs_options, 0);
+    QDECREF(bs_options);
+
     if (ret != 0) {
         goto out;
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 11/15] rbd: Use bdrv_open options instead of filename

[Kevin Wolf]

This is only to convert the internal interface that is used for passing
the "filename" to be parsed, but converting to actual fine grained
options is left for another day, as it doesn't look trivial.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/rbd.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/block/rbd.c b/block/rbd.c
index 1a8ea6d..938ceeb 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -441,7 +441,21 @@ static int qemu_rbd_aio_flush_cb(void *opaque)
     return (s->qemu_aio_count > 0);
 }
 
-static int qemu_rbd_open(BlockDriverState *bs, const char *filename,
+/* TODO Convert to fine grained options */
+static QemuOptsList runtime_opts = {
+    .name = "rbd",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = "filename",
+            .type = QEMU_OPT_STRING,
+            .help = "Specification of the rbd image",
+        },
+        { /* end of list */ }
+    },
+};
+
+static int qemu_rbd_open(BlockDriverState *bs, const char *dummy,
                          QDict *options, int flags)
 {
     BDRVRBDState *s = bs->opaque;
@@ -450,8 +464,23 @@ static int qemu_rbd_open(BlockDriverState *bs, const char *filename,
     char conf[RBD_MAX_CONF_SIZE];
     char clientname_buf[RBD_MAX_CONF_SIZE];
     char *clientname;
+    QemuOpts *opts;
+    Error *local_err = NULL;
+    const char *filename;
     int r;
 
+    opts = qemu_opts_create_nofail(&runtime_opts);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        qemu_opts_del(opts);
+        return -EINVAL;
+    }
+
+    filename = qemu_opt_get(opts, "filename");
+    qemu_opts_del(opts);
+
     if (qemu_rbd_parsename(filename, pool, sizeof(pool),
                            snap_buf, sizeof(snap_buf),
                            s->name, sizeof(s->name),
-- 
1.8.1.4

[Qemu-devel] [PATCH 12/15] sheepdog: Use bdrv_open options instead of filename

[Kevin Wolf]

This is only to convert the internal interface that is used for passing
the "filename" to be parsed, but converting to actual fine grained
options is left for another day, as it doesn't look trivial.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/sheepdog.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 987018e..40bc55a 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1126,7 +1126,21 @@ static int write_object(int fd, char *buf, uint64_t oid, int copies,
                              create, cache_flags);
 }
 
-static int sd_open(BlockDriverState *bs, const char *filename,
+/* TODO Convert to fine grained options */
+static QemuOptsList runtime_opts = {
+    .name = "sheepdog",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = "filename",
+            .type = QEMU_OPT_STRING,
+            .help = "URL to the sheepdog image",
+        },
+        { /* end of list */ }
+    },
+};
+
+static int sd_open(BlockDriverState *bs, const char *dummy,
                    QDict *options, int flags)
 {
     int ret, fd;
@@ -1135,6 +1149,20 @@ static int sd_open(BlockDriverState *bs, const char *filename,
     char vdi[SD_MAX_VDI_LEN], tag[SD_MAX_VDI_TAG_LEN];
     uint32_t snapid;
     char *buf = NULL;
+    QemuOpts *opts;
+    Error *local_err = NULL;
+    const char *filename;
+
+    opts = qemu_opts_create_nofail(&runtime_opts);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        ret = -EINVAL;
+        goto out;
+    }
+
+    filename = qemu_opt_get(opts, "filename");
 
     QLIST_INIT(&s->inflight_aio_head);
     QLIST_INIT(&s->pending_aio_head);
@@ -1199,6 +1227,7 @@ static int sd_open(BlockDriverState *bs, const char *filename,
     bs->total_sectors = s->inode.vdi_size / SECTOR_SIZE;
     pstrcpy(s->name, sizeof(s->name), vdi);
     qemu_co_mutex_init(&s->lock);
+    qemu_opts_del(opts);
     g_free(buf);
     return 0;
 out:
@@ -1206,6 +1235,7 @@ out:
     if (s->fd >= 0) {
         closesocket(s->fd);
     }
+    qemu_opts_del(opts);
     g_free(buf);
     return ret;
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH 13/15] vvfat: Use bdrv_open options instead of filename

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vvfat.c | 210 +++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 158 insertions(+), 52 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index ef74c30..ac56421 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -28,6 +28,8 @@
 #include "block/block_int.h"
 #include "qemu/module.h"
 #include "migration/migration.h"
+#include "qapi/qmp/qint.h"
+#include "qapi/qmp/qbool.h"
 
 #ifndef S_IWGRP
 #define S_IWGRP 0
@@ -988,11 +990,91 @@ static void vvfat_rebind(BlockDriverState *bs)
     s->bs = bs;
 }
 
-static int vvfat_open(BlockDriverState *bs, const char* dirname,
+static QemuOptsList runtime_opts = {
+    .name = "vvfat",
+    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
+    .desc = {
+        {
+            .name = "dir",
+            .type = QEMU_OPT_STRING,
+            .help = "Host directory to map to the vvfat device",
+        },
+        {
+            .name = "fat-type",
+            .type = QEMU_OPT_NUMBER,
+            .help = "FAT type (12, 16 or 32)",
+        },
+        {
+            .name = "floppy",
+            .type = QEMU_OPT_BOOL,
+            .help = "Create a floppy rather than a hard disk image",
+        },
+        {
+            .name = "rw",
+            .type = QEMU_OPT_BOOL,
+            .help = "Make the image writable",
+        },
+        { /* end of list */ }
+    },
+};
+
+static void vvfat_parse_filename(const char *filename, QDict *options,
+                                 Error **errp)
+{
+    int fat_type = 0;
+    bool floppy = false;
+    bool rw = false;
+    int i;
+
+    if (!strstart(filename, "fat:", NULL)) {
+        error_setg(errp, "File name string must start with 'fat:'");
+        return;
+    }
+
+    /* Parse options */
+    if (strstr(filename, ":32:")) {
+        fat_type = 32;
+    } else if (strstr(filename, ":16:")) {
+        fat_type = 16;
+    } else if (strstr(filename, ":12:")) {
+        fat_type = 12;
+    }
+
+    if (strstr(filename, ":floppy:")) {
+        floppy = true;
+    }
+
+    if (strstr(filename, ":rw:")) {
+        rw = true;
+    }
+
+    /* Get the directory name without options */
+    i = strrchr(filename, ':') - filename;
+    assert(i >= 3);
+    if (filename[i - 2] == ':' && qemu_isalpha(filename[i - 1])) {
+        /* workaround for DOS drive names */
+        filename += i - 1;
+    } else {
+        filename += i + 1;
+    }
+
+    /* Fill in the options QDict */
+    qdict_put(options, "dir", qstring_from_str(filename));
+    qdict_put(options, "fat-type", qint_from_int(fat_type));
+    qdict_put(options, "floppy", qbool_from_int(floppy));
+    qdict_put(options, "rw", qbool_from_int(rw));
+}
+
+static int vvfat_open(BlockDriverState *bs, const char* dummy,
                       QDict *options, int flags)
 {
     BDRVVVFATState *s = bs->opaque;
-    int i, cyls, heads, secs;
+    int cyls, heads, secs;
+    bool floppy;
+    const char *dirname;
+    QemuOpts *opts;
+    Error *local_err = NULL;
+    int ret;
 
 #ifdef DEBUG
     vvv = s;
@@ -1003,6 +1085,65 @@ DLOG(if (stderr == NULL) {
     setbuf(stderr, NULL);
 })
 
+    opts = qemu_opts_create_nofail(&runtime_opts);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    dirname = qemu_opt_get(opts, "dir");
+    if (!dirname) {
+        qerror_report(ERROR_CLASS_GENERIC_ERROR, "vvfat block driver requires "
+                      "a 'dir' option");
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    s->fat_type = qemu_opt_get_number(opts, "fat-type", 0);
+    floppy = qemu_opt_get_bool(opts, "floppy", false);
+
+    if (floppy) {
+        /* 1.44MB or 2.88MB floppy.  2.88MB can be FAT12 (default) or FAT16. */
+        if (!s->fat_type) {
+            s->fat_type = 12;
+            secs = 36;
+            s->sectors_per_cluster = 2;
+        } else {
+            secs = s->fat_type == 12 ? 18 : 36;
+            s->sectors_per_cluster = 1;
+        }
+        s->first_sectors_number = 1;
+        cyls = 80;
+        heads = 2;
+    } else {
+        /* 32MB or 504MB disk*/
+        if (!s->fat_type) {
+            s->fat_type = 16;
+        }
+        cyls = s->fat_type == 12 ? 64 : 1024;
+        heads = 16;
+        secs = 63;
+    }
+
+    switch (s->fat_type) {
+    case 32:
+	    fprintf(stderr, "Big fat greek warning: FAT32 has not been tested. "
+                "You are welcome to do so!\n");
+        break;
+    case 16:
+    case 12:
+        break;
+    default:
+        qerror_report(ERROR_CLASS_GENERIC_ERROR, "Valid FAT types are only "
+                      "12, 16 and 32");
+        ret = -EINVAL;
+        goto fail;
+    }
+
+
     s->bs = bs;
 
     /* LATER TODO: if FAT32, adjust */
@@ -1018,63 +1159,24 @@ DLOG(if (stderr == NULL) {
     s->fat2 = NULL;
     s->downcase_short_names = 1;
 
-    if (!strstart(dirname, "fat:", NULL))
-	return -1;
-
-    if (strstr(dirname, ":32:")) {
-	fprintf(stderr, "Big fat greek warning: FAT32 has not been tested. You are welcome to do so!\n");
-	s->fat_type = 32;
-    } else if (strstr(dirname, ":16:")) {
-	s->fat_type = 16;
-    } else if (strstr(dirname, ":12:")) {
-	s->fat_type = 12;
-    }
-
-    if (strstr(dirname, ":floppy:")) {
-	/* 1.44MB or 2.88MB floppy.  2.88MB can be FAT12 (default) or FAT16. */
-	if (!s->fat_type) {
-	    s->fat_type = 12;
-            secs = 36;
-	    s->sectors_per_cluster=2;
-	} else {
-            secs = s->fat_type == 12 ? 18 : 36;
-	    s->sectors_per_cluster=1;
-	}
-	s->first_sectors_number = 1;
-        cyls = 80;
-        heads = 2;
-    } else {
-	/* 32MB or 504MB disk*/
-	if (!s->fat_type) {
-	    s->fat_type = 16;
-	}
-        cyls = s->fat_type == 12 ? 64 : 1024;
-        heads = 16;
-        secs = 63;
-    }
     fprintf(stderr, "vvfat %s chs %d,%d,%d\n",
             dirname, cyls, heads, secs);
 
     s->sector_count = cyls * heads * secs - (s->first_sectors_number - 1);
 
-    if (strstr(dirname, ":rw:")) {
-	if (enable_write_target(s))
-	    return -1;
-	bs->read_only = 0;
+    if (qemu_opt_get_bool(opts, "rw", false)) {
+        if (enable_write_target(s)) {
+            ret = -EIO;
+            goto fail;
+        }
+        bs->read_only = 0;
     }
 
-    i = strrchr(dirname, ':') - dirname;
-    assert(i >= 3);
-    if (dirname[i-2] == ':' && qemu_isalpha(dirname[i-1]))
-	/* workaround for DOS drive names */
-	dirname += i-1;
-    else
-	dirname += i+1;
-
     bs->total_sectors = cyls * heads * secs;
 
     if (init_directories(s, dirname, heads, secs)) {
-	return -1;
+        ret = -EIO;
+        goto fail;
     }
 
     s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count;
@@ -1094,7 +1196,10 @@ DLOG(if (stderr == NULL) {
         migrate_add_blocker(s->migration_blocker);
     }
 
-    return 0;
+    ret = 0;
+fail:
+    qemu_opts_del(opts);
+    return ret;
 }
 
 static inline void vvfat_close_current_file(BDRVVVFATState *s)
@@ -2868,8 +2973,9 @@ static void vvfat_close(BlockDriverState *bs)
 static BlockDriver bdrv_vvfat = {
     .format_name	= "vvfat",
     .instance_size	= sizeof(BDRVVVFATState),
-    .bdrv_file_open	= vvfat_open,
-    .bdrv_rebind	= vvfat_rebind,
+    .bdrv_parse_filename    = vvfat_parse_filename,
+    .bdrv_file_open         = vvfat_open,
+    .bdrv_rebind            = vvfat_rebind,
     .bdrv_read          = vvfat_co_read,
     .bdrv_write         = vvfat_co_write,
     .bdrv_close		= vvfat_close,
-- 
1.8.1.4

[Qemu-devel] [PATCH 14/15] block: Remove filename parameter from .bdrv_file_open()

[Kevin Wolf]

It is unused now in all block drivers.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   |  8 ++------
 block/blkdebug.c          |  3 +--
 block/blkverify.c         |  3 +--
 block/curl.c              |  3 +--
 block/gluster.c           |  4 ++--
 block/iscsi.c             |  5 ++---
 block/nbd.c               |  3 +--
 block/raw-posix.c         | 15 +++++----------
 block/raw-win32.c         |  6 ++----
 block/rbd.c               |  3 +--
 block/sheepdog.c          |  3 +--
 block/vvfat.c             |  3 +--
 include/block/block_int.h |  3 +--
 13 files changed, 21 insertions(+), 41 deletions(-)

diff --git a/block.c b/block.c
index a67365e..c2d1d2d 100644
--- a/block.c
+++ b/block.c
@@ -722,7 +722,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     if (drv->bdrv_file_open) {
         assert(file == NULL);
         assert(drv->bdrv_parse_filename || filename != NULL);
-        ret = drv->bdrv_file_open(bs, filename, options, open_flags);
+        ret = drv->bdrv_file_open(bs, options, open_flags);
     } else {
         if (file == NULL) {
             qerror_report(ERROR_CLASS_GENERIC_ERROR, "The '%s' block driver is "
@@ -826,6 +826,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
             ret = -EINVAL;
             goto fail;
         }
+        qdict_del(options, "filename");
     } else if (!drv->bdrv_parse_filename && !filename) {
         qerror_report(ERROR_CLASS_GENERIC_ERROR,
                       "The '%s' block driver requires a file name",
@@ -839,11 +840,6 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
         goto fail;
     }
 
-    /* TODO Remove once all protocols know the filename option */
-    if (qdict_haskey(options, "filename")) {
-        qdict_del(options, "filename");
-    }
-
     /* Check if any unknown options were used */
     if (qdict_size(options) != 0) {
         const QDictEntry *entry = qdict_first(options);
diff --git a/block/blkdebug.c b/block/blkdebug.c
index da11de2..90f35e6 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -346,8 +346,7 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int blkdebug_open(BlockDriverState *bs, const char *dummy,
-                         QDict *options, int flags)
+static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVBlkdebugState *s = bs->opaque;
     QemuOpts *opts;
diff --git a/block/blkverify.c b/block/blkverify.c
index 37806e3..05685b6 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -116,8 +116,7 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int blkverify_open(BlockDriverState *bs, const char *dummy,
-                          QDict *options, int flags)
+static int blkverify_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVBlkverifyState *s = bs->opaque;
     QemuOpts *opts;
diff --git a/block/curl.c b/block/curl.c
index 61bc3db..b8935fd 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -395,8 +395,7 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int curl_open(BlockDriverState *bs, const char *dummy,
-                     QDict *options, int flags)
+static int curl_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVCURLState *s = bs->opaque;
     CURLState *state = NULL;
diff --git a/block/gluster.c b/block/gluster.c
index 3796da8..91acde2 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -296,8 +296,8 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int qemu_gluster_open(BlockDriverState *bs, const char *dummy,
-    QDict *options, int bdrv_flags)
+static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
+                             int bdrv_flags)
 {
     BDRVGlusterState *s = bs->opaque;
     int open_flags = O_BINARY;
diff --git a/block/iscsi.c b/block/iscsi.c
index 907beba..f7199c1 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1021,8 +1021,7 @@ static QemuOptsList runtime_opts = {
  * We support iscsi url's on the form
  * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>
  */
-static int iscsi_open(BlockDriverState *bs, const char *dummy,
-                      QDict *options, int flags)
+static int iscsi_open(BlockDriverState *bs, QDict *options, int flags)
 {
     IscsiLun *iscsilun = bs->opaque;
     struct iscsi_context *iscsi = NULL;
@@ -1237,7 +1236,7 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
 
     bs_options = qdict_new();
     qdict_put(bs_options, "filename", qstring_from_str(filename));
-    ret = iscsi_open(&bs, NULL, bs_options, 0);
+    ret = iscsi_open(&bs, bs_options, 0);
     QDECREF(bs_options);
 
     if (ret != 0) {
diff --git a/block/nbd.c b/block/nbd.c
index eff683c..61b7c9b 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -441,8 +441,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
     closesocket(s->sock);
 }
 
-static int nbd_open(BlockDriverState *bs, const char* filename,
-                    QDict *options, int flags)
+static int nbd_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVNBDState *s = bs->opaque;
     int result;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 9b7bc9c..5c78e53 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -335,8 +335,7 @@ fail:
     return ret;
 }
 
-static int raw_open(BlockDriverState *bs, const char *filename,
-                    QDict *options, int flags)
+static int raw_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
 
@@ -1325,8 +1324,7 @@ static int check_hdev_writable(BDRVRawState *s)
     return 0;
 }
 
-static int hdev_open(BlockDriverState *bs, const char *dummy,
-                     QDict *options, int flags)
+static int hdev_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
@@ -1565,8 +1563,7 @@ static BlockDriver bdrv_host_device = {
 };
 
 #ifdef __linux__
-static int floppy_open(BlockDriverState *bs, const char *filename,
-                       QDict *options, int flags)
+static int floppy_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
@@ -1688,8 +1685,7 @@ static BlockDriver bdrv_host_floppy = {
     .bdrv_eject         = floppy_eject,
 };
 
-static int cdrom_open(BlockDriverState *bs, const char *filename,
-                      QDict *options, int flags)
+static int cdrom_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
 
@@ -1797,8 +1793,7 @@ static BlockDriver bdrv_host_cdrom = {
 #endif /* __linux__ */
 
 #if defined (__FreeBSD__) || defined(__FreeBSD_kernel__)
-static int cdrom_open(BlockDriverState *bs, const char *filename,
-                      QDict *options, int flags)
+static int cdrom_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 49d9234..581d830 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -234,8 +234,7 @@ static QemuOptsList raw_runtime_opts = {
     },
 };
 
-static int raw_open(BlockDriverState *bs, const char *unused,
-                    QDict *options, int flags)
+static int raw_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int access_flags;
@@ -531,8 +530,7 @@ static int hdev_probe_device(const char *filename)
     return 0;
 }
 
-static int hdev_open(BlockDriverState *bs, const char *dummy,
-                     QDict *options, int flags)
+static int hdev_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int access_flags, create_flags;
diff --git a/block/rbd.c b/block/rbd.c
index 938ceeb..eb93235 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -455,8 +455,7 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int qemu_rbd_open(BlockDriverState *bs, const char *dummy,
-                         QDict *options, int flags)
+static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRBDState *s = bs->opaque;
     char pool[RBD_MAX_POOL_NAME_SIZE];
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 40bc55a..917738c 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1140,8 +1140,7 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int sd_open(BlockDriverState *bs, const char *dummy,
-                   QDict *options, int flags)
+static int sd_open(BlockDriverState *bs, QDict *options, int flags)
 {
     int ret, fd;
     uint32_t vid = 0;
diff --git a/block/vvfat.c b/block/vvfat.c
index ac56421..be85076 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1065,8 +1065,7 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
     qdict_put(options, "rw", qbool_from_int(rw));
 }
 
-static int vvfat_open(BlockDriverState *bs, const char* dummy,
-                      QDict *options, int flags)
+static int vvfat_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVVVFATState *s = bs->opaque;
     int cyls, heads, secs;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 9aa98b5..4e76633 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -87,8 +87,7 @@ struct BlockDriver {
     void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
 
     int (*bdrv_open)(BlockDriverState *bs, QDict *options, int flags);
-    int (*bdrv_file_open)(BlockDriverState *bs, const char *filename,
-                          QDict *options, int flags);
+    int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags);
     int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
                      uint8_t *buf, int nb_sectors);
     int (*bdrv_write)(BlockDriverState *bs, int64_t sector_num,
-- 
1.8.1.4

[Qemu-devel] [PATCH 15/15] block: Allow overriding backing.file.filename

[Kevin Wolf]

If a filename is passed in the driver-specific options from the command
line, the backing file path from the image is ignored now.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index c2d1d2d..3a45c2a 100644
--- a/block.c
+++ b/block.c
@@ -889,7 +889,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options)
     }
 
     bs->open_flags &= ~BDRV_O_NO_BACKING;
-    if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
+    if (qdict_haskey(options, "file.filename")) {
+        backing_filename[0] = '\0';
+    } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
         QDECREF(options);
         return 0;
     }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 01/15] block: Fail gracefully when using a format driver on protocol level

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2153 bytes --]

On 04/12/2013 02:47 PM, Kevin Wolf wrote:
> Specifying the wrong driver could fail an assertion:
> 
> $ qemu-system-x86_64 -drive file.driver=qcow2,file=x
> qemu-system-x86_64: block.c:721: bdrv_open_common: Assertion `file !=
> ((void *)0)' failed.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                    |  7 +++++++
>  tests/qemu-iotests/051     |  7 +++++++
>  tests/qemu-iotests/051.out | 10 ++++++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 602d8a4..f23bdcc 100644
> --- a/block.c
> +++ b/block.c
> @@ -718,6 +718,13 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>          assert(drv->bdrv_parse_filename || filename != NULL);
>          ret = drv->bdrv_file_open(bs, filename, options, open_flags);
>      } else {
> +        if (file == NULL) {
> +            qerror_report(ERROR_CLASS_GENERIC_ERROR, "The '%s' block driver is "
> +                          "not suitable for the bottom level",
> +                          drv->format_name);
> +            ret = -EINVAL;
> +            goto free_and_fail;
> +        }
>          assert(file != NULL);

Is it really necessary to leave the assert in place, now that you have a
check for NULL followed by unconditional goto?

Just reading that error message, I'm not quite sure what you meant by
"not suitable for the bottom level".  I guess the intent is that
file.driver specifies the protocol, and that both raw and qcow2 are
formats possible on the file protocol, rather than qcow2 being a file
protocol itself.

> +Testing: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2
> +qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: The 'qcow2' block driver is not suitable for the bottom level
> +qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: could not open disk image TEST_DIR/t.qcow2: Invalid argument

Maybe a better error message would be this?

Attempt to use format driver 'qcow2' where a protocol driver was expected

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 01/15] block: Fail gracefully when using a format driver on protocol level

[Kevin Wolf]

Am 13.04.2013 um 00:50 hat Eric Blake geschrieben:
> On 04/12/2013 02:47 PM, Kevin Wolf wrote:
> > Specifying the wrong driver could fail an assertion:
> > 
> > $ qemu-system-x86_64 -drive file.driver=qcow2,file=x
> > qemu-system-x86_64: block.c:721: bdrv_open_common: Assertion `file !=
> > ((void *)0)' failed.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c                    |  7 +++++++
> >  tests/qemu-iotests/051     |  7 +++++++
> >  tests/qemu-iotests/051.out | 10 ++++++++++
> >  3 files changed, 24 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index 602d8a4..f23bdcc 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -718,6 +718,13 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
> >          assert(drv->bdrv_parse_filename || filename != NULL);
> >          ret = drv->bdrv_file_open(bs, filename, options, open_flags);
> >      } else {
> > +        if (file == NULL) {
> > +            qerror_report(ERROR_CLASS_GENERIC_ERROR, "The '%s' block driver is "
> > +                          "not suitable for the bottom level",
> > +                          drv->format_name);
> > +            ret = -EINVAL;
> > +            goto free_and_fail;
> > +        }
> >          assert(file != NULL);
> 
> Is it really necessary to leave the assert in place, now that you have a
> check for NULL followed by unconditional goto?

Not really. I'll send a cleanup.

> Just reading that error message, I'm not quite sure what you meant by
> "not suitable for the bottom level".  I guess the intent is that
> file.driver specifies the protocol, and that both raw and qcow2 are
> formats possible on the file protocol, rather than qcow2 being a file
> protocol itself.
> 
> > +Testing: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2
> > +qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: The 'qcow2' block driver is not suitable for the bottom level
> > +qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: could not open disk image TEST_DIR/t.qcow2: Invalid argument
> 
> Maybe a better error message would be this?
> 
> Attempt to use format driver 'qcow2' where a protocol driver was expected

I was trying to avoid the format/protocol discussion this time by
choosing a different phrasing in the first place. Seems this approach
doesn't work better either...

Problems with this terminology start when you have more than two drivers
involved. Currently, this is only with the more exotic cases like when
you have qcow2 -> blkdebug -> file, but if we follow through with our
-blockdev plans, arbitrary stacking of BlockDriverStates will become
more common.

Markus likes to describe it as a graph of nodes of the same kind, where
the leaves (i.e. the protocols) just happen to not have a .file option.

Kevin

Re: [Qemu-devel] [PATCH 01/15] block: Fail gracefully when using a format driver on protocol level

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 13.04.2013 um 00:50 hat Eric Blake geschrieben:
>> On 04/12/2013 02:47 PM, Kevin Wolf wrote:
>> > Specifying the wrong driver could fail an assertion:
>> > 
>> > $ qemu-system-x86_64 -drive file.driver=qcow2,file=x
>> > qemu-system-x86_64: block.c:721: bdrv_open_common: Assertion `file !=
>> > ((void *)0)' failed.
>> > 
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> >  block.c                    |  7 +++++++
>> >  tests/qemu-iotests/051     |  7 +++++++
>> >  tests/qemu-iotests/051.out | 10 ++++++++++
>> >  3 files changed, 24 insertions(+)
>> > 
>> > diff --git a/block.c b/block.c
>> > index 602d8a4..f23bdcc 100644
>> > --- a/block.c
>> > +++ b/block.c
>> > @@ -718,6 +718,13 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>> >          assert(drv->bdrv_parse_filename || filename != NULL);
>> >          ret = drv->bdrv_file_open(bs, filename, options, open_flags);
>> >      } else {
>> > +        if (file == NULL) {
>> > +            qerror_report(ERROR_CLASS_GENERIC_ERROR, "The '%s' block driver is "
>> > +                          "not suitable for the bottom level",
>> > +                          drv->format_name);
>> > +            ret = -EINVAL;
>> > +            goto free_and_fail;
>> > +        }
>> >          assert(file != NULL);
>> 
>> Is it really necessary to leave the assert in place, now that you have a
>> check for NULL followed by unconditional goto?
>
> Not really. I'll send a cleanup.
>
>> Just reading that error message, I'm not quite sure what you meant by
>> "not suitable for the bottom level".  I guess the intent is that
>> file.driver specifies the protocol, and that both raw and qcow2 are
>> formats possible on the file protocol, rather than qcow2 being a file
>> protocol itself.
>> 
>> > +Testing: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2
>> > +qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: The 'qcow2'
>> > block driver is not suitable for the bottom level
>> > +qemu: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: could not
>> > open disk image TEST_DIR/t.qcow2: Invalid argument
>> 
>> Maybe a better error message would be this?
>> 
>> Attempt to use format driver 'qcow2' where a protocol driver was expected

It's an improvement, but it's still gobbledygook :)

I don't think users understand "format" vs. "protocol".  Heck, I don't
half of the time!

> I was trying to avoid the format/protocol discussion this time by
> choosing a different phrasing in the first place. Seems this approach
> doesn't work better either...
>
> Problems with this terminology start when you have more than two drivers
> involved. Currently, this is only with the more exotic cases like when
> you have qcow2 -> blkdebug -> file, but if we follow through with our
> -blockdev plans, arbitrary stacking of BlockDriverStates will become
> more common.

Exactly.

> Markus likes to describe it as a graph of nodes of the same kind, where
> the leaves (i.e. the protocols) just happen to not have a .file option.

I view block drivers as building blocks for building block backends.  A
block driver has one plug and may have sockets.  You combine them by
plugging plugs into sockets.  Sockets need to be plugged for the thing
to work[*].  The root plug plugs into the block frontend, via qdev drive
property.

What the error message above is trying to report is "I got an empty
socket here, and I need it filled".

Begs the question which socket.

In simple cases, there's just one.  Block driver "raw", for instance.
The current code is designed for this case, and provides this socket as
BlockDriverState member file.

Block driver "qcow2" actually has two sockets, one for the QCOW2 image,
one for its backing image.  But we don't (yet) expose the backing image
as a first class pluggable socket, so we can pretend there's just one.

For truly general block backend configuration, we need to ditch this
single socket mindset.

In the context of such general block backend configuration, where users
combine block drivers into backends by plugging plugs into named
sockets, the most useful way to report an empty socket is to identify it
by name.

How to best report it when plugs and sockets are obscured by a heavy
layer of syntactic sugar isn't obvious to me.


[*] Unless we find a use for optional sockets.  Then only the mandatory
sockets need to be plugged.

Re: [Qemu-devel] [PATCH 02/15] block: Add driver-specific options for backing files

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1157 bytes --]

On 04/12/2013 02:47 PM, Kevin Wolf wrote:
> Options starting in "backing." are passed to the backing file now. If
> you don't need to specify the filename for the backing file, you can add
> it on the command line instead of in the image file:
> 
> $ qemu-nbd -t /tmp/test.img
> $ qemu-img create -f qcow2 empty.qcow2 1G
> $ qemu-system-x86_64 -drive file=empty.qcow2,backing.file.driver=nbd,\
>     backing.file.host=localhost

So effectively empty.qcow2 is booted as though it had an explicit
nbd://localhost in its backing metadata?  Looks interesting!

> 
> Note that this doesn't override the backing filename from the image. If
> the image has one, this will fail because NBD doesn't want the options
> and a filename at the same time.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c               | 27 +++++++++++++++++++++++----
>  block/mirror.c        |  2 +-
>  include/block/block.h |  2 +-
>  3 files changed, 25 insertions(+), 6 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 03/15] block: Enable filename option

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

On 04/12/2013 02:47 PM, Kevin Wolf wrote:
> This allows using the file.filename option instead of the string that
> comes from -drive file=... and is passed around as a separate parameter.
> The goal is to get rid of this parameter and use the options QDict more
> consistently.
> 
> With this option you can access not only the top-level image, but
> specify a filename for the backing file (currently only if no backing
> file exists, but we'll allow overriding it later)
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)

Quite a few lines added for a net reduction in parameters, but the
results look nice.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 04/15] raw-posix: Use bdrv_open options instead of filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

On 04/12/2013 02:47 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/raw-posix.c | 57 +++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 12 deletions(-)

> +static int raw_open_common(BlockDriverState *bs, QDict *options,
>                             int bdrv_flags, int open_flags)
>  {
>      BDRVRawState *s = bs->opaque;
> +    QemuOpts *opts;
> +    Error *local_err = NULL;
> +    const char* filename;

Spacing is off for this line (should be before *, not after).  But
that's trivial, so I have no qualms with adding this to a fixed version:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 05/15] raw-win32: Use bdrv_open options instead of filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 820 bytes --]

On 04/12/2013 02:47 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/raw-win32.c | 57 +++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 47 insertions(+), 10 deletions(-)
> 

> +static int raw_open(BlockDriverState *bs, const char *unused,
>                      QDict *options, int flags)
>  {
>      BDRVRawState *s = bs->opaque;
>      int access_flags;
>      DWORD overlapped;
> +    QemuOpts *opts;
> +    Error *local_err = NULL;
> +    const char* filename;

Copy-and-paste formatting of this line from 4/15.  Same caveat about it
being trivial enough to add this once fixed:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 06/15] blkdebug: Use bdrv_open options instead of filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 397 bytes --]

On 04/12/2013 02:47 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/blkdebug.c | 103 +++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 77 insertions(+), 26 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 07/15] blkverify: Use bdrv_open options instead of filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

On 04/12/2013 02:48 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/blkverify.c | 94 ++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 76 insertions(+), 18 deletions(-)
> 

>      /* Parse the blkverify: prefix */
> -    if (strncmp(filename, "blkverify:", strlen("blkverify:"))) {
> -        return -EINVAL;
> +    if (!strstart(filename, "blkverify:", &filename)) {
> +        error_setg(errp, "File name string must start with 'blkverify:'");
> +        return;
>      }
> -    filename += strlen("blkverify:");
>  
>      /* Parse the raw image filename */
>      c = strchr(filename, ':');
>      if (c == NULL) {
> -        return -EINVAL;
> +        error_setg(errp, "blkdebug requires raw copy and original image path");

s/blkdebug/blkverify/

Simple enough fix that I'm still comfortable if you include:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 08/15] curl: Use bdrv_open options instead of filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2991 bytes --]

On 04/12/2013 02:48 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/curl.c | 153 +++++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 102 insertions(+), 51 deletions(-)
> 

> @@ -369,29 +364,78 @@ static int curl_open(BlockDriverState *bs, const char *filename,
>                      ra_val = ra + 1;
>                      ra -= strlen(RA_OPTSTR) - 1;
>                      *ra = '\0';
> -                    s->readahead_size = atoi(ra_val);

Good riddance - atoi() is evil because it can't detect overflow.

> -                    break;
> -                } else {
> -                    break;
> +                    qdict_put(options, "readahead", qstring_from_str(ra_val));

So now we just pass the string on to the option parser,...

> +static QemuOptsList runtime_opts = {
> +    .name = "curl",
> +    .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> +    .desc = {
> +        {
> +            .name = "url",
> +            .type = QEMU_OPT_STRING,
> +            .help = "URL to open",
> +        },
> +        {
> +            .name = "readahead",
> +            .type = QEMU_OPT_SIZE,
> +            .help = "Readahead size",

...and this new option lets QEMU_OPT_SIZE detect crazy strings that were
previously treated as '0' by the old atoi().  You should probably
mention this bug fix in the commit message as being intentional.

> @@ -568,63 +614,68 @@ static int64_t curl_getlength(BlockDriverState *bs)
>  }
>  
>  static BlockDriver bdrv_http = {
> -    .format_name     = "http",
> -    .protocol_name   = "http",
> +    .format_name            = "http",
> +    .protocol_name          = "http",
>  
> -    .instance_size   = sizeof(BDRVCURLState),
> -    .bdrv_file_open  = curl_open,
> -    .bdrv_close      = curl_close,
> -    .bdrv_getlength  = curl_getlength,
> +    .instance_size          = sizeof(BDRVCURLState),
> +    .bdrv_parse_filename    = curl_parse_filename,
> +    .bdrv_file_open         = curl_open,
> +    .bdrv_close             = curl_close,
> +    .bdrv_getlength         = curl_getlength,
>  
> -    .bdrv_aio_readv  = curl_aio_readv,
> +    .bdrv_aio_readv         = curl_aio_readv,

On this patch, you reindented ALL callbacks to the same width.  But in
6/15 and 7/15, you reindented only the callbacks in the
.bdrv_parse_filename section.  You should probably be consistent between
patches (I actually prefer indenting the entire BlockDriver
initialization consistently, as done in this patch, but didn't ding
patch 6 or 7 at the time because they looked innocuous enough in isolation).

Since I only called out whitespace issues (and even then, only for
commits other than this) and a weakness in the commit message, the code
itself deserves:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 09/15] gluster: Use bdrv_open options instead of filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]

On 04/12/2013 02:48 PM, Kevin Wolf wrote:
> This is only to convert the internal interface that is used for passing
> the "filename" to be parsed, but converting to actual fine grained
> options is left for another day, as it doesn't look trivial.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/gluster.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> +
> +static int qemu_gluster_open(BlockDriverState *bs, const char *dummy,
>      QDict *options, int bdrv_flags)

As long as you are touching this code, should you re-indent the second
line of this signature?

> +
> +    filename = qemu_opt_get(opts, "filename");
> +
>  
>      s->glfs = qemu_gluster_init(gconf, filename);

Why the two blank lines?

Comments deal only with whitespace, so feel free to add:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 10/15] iscsi: Use bdrv_open options instead of filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 589 bytes --]

On 04/12/2013 02:48 PM, Kevin Wolf wrote:
> This is only to convert the internal interface that is used for passing
> the "filename" to be parsed, but converting to actual fine grained
> options is left for another day, as it doesn't look trivial.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/iscsi.c | 39 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 11/15] rbd: Use bdrv_open options instead of filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]

On 04/12/2013 02:48 PM, Kevin Wolf wrote:
> This is only to convert the internal interface that is used for passing
> the "filename" to be parsed, but converting to actual fine grained
> options is left for another day, as it doesn't look trivial.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/rbd.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 12/15] sheepdog: Use bdrv_open options instead of filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 584 bytes --]

On 04/12/2013 02:48 PM, Kevin Wolf wrote:
> This is only to convert the internal interface that is used for passing
> the "filename" to be parsed, but converting to actual fine grained
> options is left for another day, as it doesn't look trivial.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/sheepdog.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 13/15] vvfat: Use bdrv_open options instead of filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2267 bytes --]

On 04/12/2013 02:48 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/vvfat.c | 210 +++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 158 insertions(+), 52 deletions(-)
> 

> +    if (!strstart(filename, "fat:", NULL)) {
> +        error_setg(errp, "File name string must start with 'fat:'");
> +        return;
> +    }
> +
> +    /* Parse options */
> +    if (strstr(filename, ":32:")) {
> +        fat_type = 32;
> +    } else if (strstr(filename, ":16:")) {
> +        fat_type = 16;
> +    } else if (strstr(filename, ":12:")) {
> +        fat_type = 12;
> +    }
> +
> +    if (strstr(filename, ":floppy:")) {
> +        floppy = true;
> +    }
> +
> +    if (strstr(filename, ":rw:")) {
> +        rw = true;
> +    }
> +
> +    /* Get the directory name without options */
> +    i = strrchr(filename, ':') - filename;

This parser is rather loose, in that it ignores unknown options (for
example, if I did fat:1:file, it would happily ignore option "1").  But
that's no worse than the old code.

> +    switch (s->fat_type) {
> +    case 32:
> +	    fprintf(stderr, "Big fat greek warning: FAT32 has not been tested. "
> +                "You are welcome to do so!\n");

Should we s/greek/Greek/ in the message, or otherwise change its
contents?  But you just moved the pre-existing choice of spelling, so it
doesn't reflect on you.

> @@ -2868,8 +2973,9 @@ static void vvfat_close(BlockDriverState *bs)
>  static BlockDriver bdrv_vvfat = {
>      .format_name	= "vvfat",
>      .instance_size	= sizeof(BDRVVVFATState),
> -    .bdrv_file_open	= vvfat_open,
> -    .bdrv_rebind	= vvfat_rebind,
> +    .bdrv_parse_filename    = vvfat_parse_filename,
> +    .bdrv_file_open         = vvfat_open,
> +    .bdrv_rebind            = vvfat_rebind,
>      .bdrv_read          = vvfat_co_read,
>      .bdrv_write         = vvfat_co_write,
>      .bdrv_close		= vvfat_close,

Back to my comments on 9/15 on indenting the callbacks consistently.

No major errors, so I'm fine if you use:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 14/15] block: Remove filename parameter from .bdrv_file_open()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 791 bytes --]

On 04/12/2013 02:48 PM, Kevin Wolf wrote:
> It is unused now in all block drivers.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> +++ b/block/gluster.c
> @@ -296,8 +296,8 @@ static QemuOptsList runtime_opts = {
>      },
>  };
>  
> -static int qemu_gluster_open(BlockDriverState *bs, const char *dummy,
> -    QDict *options, int bdrv_flags)
> +static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
> +                             int bdrv_flags)

Ah, one of my comments about inconsistent indentation in 9/15 has been
solved here (so it's fine to avoid the churn of touching it up twice).

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 15/15] block: Allow overriding backing.file.filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1448 bytes --]

On 04/12/2013 02:48 PM, Kevin Wolf wrote:
> If a filename is passed in the driver-specific options from the command
> line, the backing file path from the image is ignored now.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index c2d1d2d..3a45c2a 100644
> --- a/block.c
> +++ b/block.c
> @@ -889,7 +889,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options)
>      }
>  
>      bs->open_flags &= ~BDRV_O_NO_BACKING;
> -    if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
> +    if (qdict_haskey(options, "file.filename")) {
> +        backing_filename[0] = '\0';
> +    } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {

Of course, the user can abuse this at their peril, but the flexibility
it adds (being able to pass in a /dev/fdset to let the command line
specify an open fd as backing file even when open() is forbidden on NFS)
is just AWESOME!

Looking forward to a QMP counterpart; I'll definitely review that, as
libvirt is still reluctant to use fdsets for image manipulation on the
command line if we can't follow it up with hot-plug manipulation, but I
love where this is heading.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 00/15] block: Overriding the backing file with -drive

[Stefan Hajnoczi]

On Fri, Apr 12, 2013 at 10:47:53PM +0200, Kevin Wolf wrote:
> This is the next part of the driver-specific options. Looks like we're getting
> closer to pass file descriptors for the whole backing file chain. In fact, I
> hope we're now only lacking QMP support before we can actually use it. :-)
> 
> This series adds support for the backing.* options namespace, and allows to use
> the file.filename option to configure the filename string for non-top-level
> BlockDriverStates. In the end you can use things like:
> 
>     -drive file=test.qcow2,backing.file.filename=/dev/fdset/1,
>     backing.backing.file.driver=nbd,backing.backing.file.host=localhost

Nice series.

> On another note, I'm still not sure whether I should leave all of this work
> enabled for 1.5, or if we wouldn't be better off with disabling it for the
> release so that we have some additional months before we commit to the
> interface. Any opinions?

I suggest we leave this series and the io_flush series I sent recently
out of QEMU 1.5.  Once they are reviewed, let's merge them into
block-next.  When 1.6 opens they'll be the first things to get merged.

Unless there's an urgent need to get this into 1.5, I don't see the big
win - QMP changes are still missing anyway.  Let's not rush it.

Also, I guess documentation changes are missing to really make this
feature ready for end-users :).

Stefan

Re: [Qemu-devel] [PATCH 00/15] block: Overriding the backing file with -drive

[Kevin Wolf]

Am 18.04.2013 um 13:42 hat Stefan Hajnoczi geschrieben:
> On Fri, Apr 12, 2013 at 10:47:53PM +0200, Kevin Wolf wrote:
> > This is the next part of the driver-specific options. Looks like we're getting
> > closer to pass file descriptors for the whole backing file chain. In fact, I
> > hope we're now only lacking QMP support before we can actually use it. :-)
> > 
> > This series adds support for the backing.* options namespace, and allows to use
> > the file.filename option to configure the filename string for non-top-level
> > BlockDriverStates. In the end you can use things like:
> > 
> >     -drive file=test.qcow2,backing.file.filename=/dev/fdset/1,
> >     backing.backing.file.driver=nbd,backing.backing.file.host=localhost
> 
> Nice series.
> 
> > On another note, I'm still not sure whether I should leave all of this work
> > enabled for 1.5, or if we wouldn't be better off with disabling it for the
> > release so that we have some additional months before we commit to the
> > interface. Any opinions?
> 
> I suggest we leave this series and the io_flush series I sent recently
> out of QEMU 1.5.  Once they are reviewed, let's merge them into
> block-next.  When 1.6 opens they'll be the first things to get merged.

There have already two other series gone in that deal with driver
specific options. If we decide to not enable this in 1.5 (which I agree
is probably better), then I think I'd be inclined to merge this series
and put another patch in that re-enables the old strict option checking
in drive_init() and therefore effectively disables all of these changes.

> Unless there's an urgent need to get this into 1.5, I don't see the big
> win - QMP changes are still missing anyway.  Let's not rush it.
> 
> Also, I guess documentation changes are missing to really make this
> feature ready for end-users :).

Yes, there are definitely enough missing pieces. Though I must admit
that the part that really worries me is committing to an external API
before we are sure that the syntax can stays as it is when all pieces
are there. I wouldn't have much problems with exposing it if we were
sure that it's the right thing and it's just incomplete yet.

Kevin

[Qemu-devel] [PATCH v2 14/15] block: Remove filename parameter from .bdrv_file_open()

[Kevin Wolf]

It is unused now in all block drivers.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
v2:
- Added a hunk for the new ssh block driver
---
 block.c                   |  8 ++------
 block/blkdebug.c          |  3 +--
 block/blkverify.c         |  3 +--
 block/curl.c              |  3 +--
 block/gluster.c           |  4 ++--
 block/iscsi.c             |  5 ++---
 block/nbd.c               |  3 +--
 block/raw-posix.c         | 15 +++++----------
 block/raw-win32.c         |  6 ++----
 block/rbd.c               |  3 +--
 block/sheepdog.c          |  3 +--
 block/ssh.c               |  3 +--
 block/vvfat.c             |  3 +--
 include/block/block_int.h |  3 +--
 14 files changed, 22 insertions(+), 43 deletions(-)

diff --git a/block.c b/block.c
index e3464d6..6e07f45 100644
--- a/block.c
+++ b/block.c
@@ -722,7 +722,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     if (drv->bdrv_file_open) {
         assert(file == NULL);
         assert(drv->bdrv_parse_filename || filename != NULL);
-        ret = drv->bdrv_file_open(bs, filename, options, open_flags);
+        ret = drv->bdrv_file_open(bs, options, open_flags);
     } else {
         if (file == NULL) {
             qerror_report(ERROR_CLASS_GENERIC_ERROR, "Can't use '%s' as a "
@@ -826,6 +826,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
             ret = -EINVAL;
             goto fail;
         }
+        qdict_del(options, "filename");
     } else if (!drv->bdrv_parse_filename && !filename) {
         qerror_report(ERROR_CLASS_GENERIC_ERROR,
                       "The '%s' block driver requires a file name",
@@ -839,11 +840,6 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
         goto fail;
     }
 
-    /* TODO Remove once all protocols know the filename option */
-    if (qdict_haskey(options, "filename")) {
-        qdict_del(options, "filename");
-    }
-
     /* Check if any unknown options were used */
     if (qdict_size(options) != 0) {
         const QDictEntry *entry = qdict_first(options);
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 3d03fcb..71f99e4 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -346,8 +346,7 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int blkdebug_open(BlockDriverState *bs, const char *dummy,
-                         QDict *options, int flags)
+static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVBlkdebugState *s = bs->opaque;
     QemuOpts *opts;
diff --git a/block/blkverify.c b/block/blkverify.c
index d63158f..1d58cc3 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -116,8 +116,7 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int blkverify_open(BlockDriverState *bs, const char *dummy,
-                          QDict *options, int flags)
+static int blkverify_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVBlkverifyState *s = bs->opaque;
     QemuOpts *opts;
diff --git a/block/curl.c b/block/curl.c
index 61bc3db..b8935fd 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -395,8 +395,7 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int curl_open(BlockDriverState *bs, const char *dummy,
-                     QDict *options, int flags)
+static int curl_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVCURLState *s = bs->opaque;
     CURLState *state = NULL;
diff --git a/block/gluster.c b/block/gluster.c
index 3796da8..91acde2 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -296,8 +296,8 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int qemu_gluster_open(BlockDriverState *bs, const char *dummy,
-    QDict *options, int bdrv_flags)
+static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
+                             int bdrv_flags)
 {
     BDRVGlusterState *s = bs->opaque;
     int open_flags = O_BINARY;
diff --git a/block/iscsi.c b/block/iscsi.c
index 907beba..f7199c1 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1021,8 +1021,7 @@ static QemuOptsList runtime_opts = {
  * We support iscsi url's on the form
  * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>
  */
-static int iscsi_open(BlockDriverState *bs, const char *dummy,
-                      QDict *options, int flags)
+static int iscsi_open(BlockDriverState *bs, QDict *options, int flags)
 {
     IscsiLun *iscsilun = bs->opaque;
     struct iscsi_context *iscsi = NULL;
@@ -1237,7 +1236,7 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
 
     bs_options = qdict_new();
     qdict_put(bs_options, "filename", qstring_from_str(filename));
-    ret = iscsi_open(&bs, NULL, bs_options, 0);
+    ret = iscsi_open(&bs, bs_options, 0);
     QDECREF(bs_options);
 
     if (ret != 0) {
diff --git a/block/nbd.c b/block/nbd.c
index eff683c..61b7c9b 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -441,8 +441,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
     closesocket(s->sock);
 }
 
-static int nbd_open(BlockDriverState *bs, const char* filename,
-                    QDict *options, int flags)
+static int nbd_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVNBDState *s = bs->opaque;
     int result;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index afd5385..c0ccf27 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -335,8 +335,7 @@ fail:
     return ret;
 }
 
-static int raw_open(BlockDriverState *bs, const char *filename,
-                    QDict *options, int flags)
+static int raw_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
 
@@ -1325,8 +1324,7 @@ static int check_hdev_writable(BDRVRawState *s)
     return 0;
 }
 
-static int hdev_open(BlockDriverState *bs, const char *dummy,
-                     QDict *options, int flags)
+static int hdev_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
@@ -1565,8 +1563,7 @@ static BlockDriver bdrv_host_device = {
 };
 
 #ifdef __linux__
-static int floppy_open(BlockDriverState *bs, const char *filename,
-                       QDict *options, int flags)
+static int floppy_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
@@ -1688,8 +1685,7 @@ static BlockDriver bdrv_host_floppy = {
     .bdrv_eject         = floppy_eject,
 };
 
-static int cdrom_open(BlockDriverState *bs, const char *filename,
-                      QDict *options, int flags)
+static int cdrom_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
 
@@ -1797,8 +1793,7 @@ static BlockDriver bdrv_host_cdrom = {
 #endif /* __linux__ */
 
 #if defined (__FreeBSD__) || defined(__FreeBSD_kernel__)
-static int cdrom_open(BlockDriverState *bs, const char *filename,
-                      QDict *options, int flags)
+static int cdrom_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
diff --git a/block/raw-win32.c b/block/raw-win32.c
index be33cc1..7c03b6d 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -234,8 +234,7 @@ static QemuOptsList raw_runtime_opts = {
     },
 };
 
-static int raw_open(BlockDriverState *bs, const char *unused,
-                    QDict *options, int flags)
+static int raw_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int access_flags;
@@ -531,8 +530,7 @@ static int hdev_probe_device(const char *filename)
     return 0;
 }
 
-static int hdev_open(BlockDriverState *bs, const char *dummy,
-                     QDict *options, int flags)
+static int hdev_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int access_flags, create_flags;
diff --git a/block/rbd.c b/block/rbd.c
index 0d68145..1826411 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -455,8 +455,7 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int qemu_rbd_open(BlockDriverState *bs, const char *dummy,
-                         QDict *options, int flags)
+static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRBDState *s = bs->opaque;
     char pool[RBD_MAX_POOL_NAME_SIZE];
diff --git a/block/sheepdog.c b/block/sheepdog.c
index e224c51..20b5d06 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1140,8 +1140,7 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int sd_open(BlockDriverState *bs, const char *dummy,
-                   QDict *options, int flags)
+static int sd_open(BlockDriverState *bs, QDict *options, int flags)
 {
     int ret, fd;
     uint32_t vid = 0;
diff --git a/block/ssh.c b/block/ssh.c
index 93a8b53..246a70d 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -608,8 +608,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     return ret;
 }
 
-static int ssh_file_open(BlockDriverState *bs, const char *filename,
-                         QDict *options, int bdrv_flags)
+static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags)
 {
     BDRVSSHState *s = bs->opaque;
     int ret;
diff --git a/block/vvfat.c b/block/vvfat.c
index f4c06b9..87b0279 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1065,8 +1065,7 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
     qdict_put(options, "rw", qbool_from_int(rw));
 }
 
-static int vvfat_open(BlockDriverState *bs, const char* dummy,
-                      QDict *options, int flags)
+static int vvfat_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVVVFATState *s = bs->opaque;
     int cyls, heads, secs;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 458cde3..6078dd3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -87,8 +87,7 @@ struct BlockDriver {
     void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
 
     int (*bdrv_open)(BlockDriverState *bs, QDict *options, int flags);
-    int (*bdrv_file_open)(BlockDriverState *bs, const char *filename,
-                          QDict *options, int flags);
+    int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags);
     int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
                      uint8_t *buf, int nb_sectors);
     int (*bdrv_write)(BlockDriverState *bs, int64_t sector_num,
-- 
1.8.1.4