[Qemu-devel] [PATCH v2 0/9] bdrv_open() cleanups, part 1

[Qemu-devel] [PATCH v2 0/9] bdrv_open() cleanups, part 1

[Kevin Wolf]

This is the first part of an attempt for disentangling bdrv_open(). At the end
of this series, bdrv_open() code is somewhat easier to read, but the real
changes (including some bug fixes and changes of behaviour) haven't happened
yet.

Just sending out the first part now to get this merged early and avoid
conflicts.

v2:
- Rebased on current git master
- Patch 1: Removed redundant if condition [Benoît]
           Replaced redundant error check with assertion [Eric]
- Patch 2: Fixed leak in error path [Benoît]

Kevin Wolf (9):
  block: Create bdrv_fill_options()
  block: Move bdrv_fill_options() call to bdrv_open()
  block: Move json: parsing to bdrv_fill_options()
  block: Always pass driver name through options QDict
  block: Use common driver selection code for bdrv_open_file()
  block: Inline bdrv_file_open()
  block: Remove second bdrv_open() recursion
  block: Catch backing files assigned to non-COW drivers
  block: Remove a special case for protocols

 block.c                    | 280 ++++++++++++++++++++++-----------------------
 block/cow.c                |   1 +
 block/qcow.c               |   1 +
 block/qcow2.c              |   1 +
 block/qed.c                |   1 +
 block/vmdk.c               |   1 +
 include/block/block_int.h  |   3 +
 tests/qemu-iotests/051     |   6 +
 tests/qemu-iotests/051.out |  14 ++-
 9 files changed, 164 insertions(+), 144 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/9] block: Create bdrv_fill_options()

[Kevin Wolf]

The idea of bdrv_fill_options() is to convert every parameter for
opening images, in particular the filename and flags, to entries in the
options QDict.

This patch starts with moving the filename parsing and driver probing
part from bdrv_file_open() to the new function.

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

diff --git a/block.c b/block.c
index a3d09fa..f1c3b57 100644
--- a/block.c
+++ b/block.c
@@ -1006,77 +1006,103 @@ free_and_fail:
 }
 
 /*
- * Opens a file using a protocol (file, host_device, nbd, ...)
- *
- * options is an indirect pointer to a QDict of options to pass to the block
- * drivers, or pointer to NULL for an empty set of options. If this function
- * takes ownership of the QDict reference, it will set *options to NULL;
- * otherwise, it will contain unused/unrecognized options after this function
- * returns. Then, the caller is responsible for freeing it. If it intends to
- * reuse the QDict, QINCREF() should be called beforehand.
+ * Fills in default options for opening images and converts the legacy
+ * filename/flags pair to option QDict entries.
  */
-static int bdrv_file_open(BlockDriverState *bs, const char *filename,
-                          QDict **options, int flags, Error **errp)
+static int bdrv_fill_options(QDict **options, const char *filename,
+                             Error **errp)
 {
-    BlockDriver *drv;
     const char *drvname;
     bool parse_filename = false;
     Error *local_err = NULL;
-    int ret;
+    BlockDriver *drv;
 
     /* 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));
-        parse_filename = true;
-    } else {
-        error_setg(errp, "Can't specify 'file' and 'filename' options at the "
-                   "same time");
-        ret = -EINVAL;
-        goto fail;
+    if (filename) {
+        if (!qdict_haskey(*options, "filename")) {
+            qdict_put(*options, "filename", qstring_from_str(filename));
+            parse_filename = true;
+        } else {
+            error_setg(errp, "Can't specify 'file' and 'filename' options at "
+                             "the same time");
+            return -EINVAL;
+        }
     }
 
     /* Find the right block driver */
+    filename = qdict_get_try_str(*options, "filename");
     drvname = qdict_get_try_str(*options, "driver");
-    if (drvname) {
-        drv = bdrv_find_format(drvname);
-        if (!drv) {
-            error_setg(errp, "Unknown driver '%s'", drvname);
-        }
-        qdict_del(*options, "driver");
-    } else if (filename) {
-        drv = bdrv_find_protocol(filename, parse_filename);
-        if (!drv) {
-            error_setg(errp, "Unknown protocol");
+
+    if (!drvname) {
+        if (filename) {
+            drv = bdrv_find_protocol(filename, parse_filename);
+            if (!drv) {
+                error_setg(errp, "Unknown protocol");
+                return -EINVAL;
+            }
+
+            drvname = drv->format_name;
+            qdict_put(*options, "driver", qstring_from_str(drvname));
+        } else {
+            error_setg(errp, "Must specify either driver or file");
+            return -EINVAL;
         }
-    } else {
-        error_setg(errp, "Must specify either driver or file");
-        drv = NULL;
     }
 
+    drv = bdrv_find_format(drvname);
     if (!drv) {
-        /* errp has been set already */
-        ret = -ENOENT;
-        goto fail;
+        error_setg(errp, "Unknown driver '%s'", drvname);
+        return -ENOENT;
     }
 
-    /* Parse the filename and open it */
+    /* Driver-specific filename parsing */
     if (drv->bdrv_parse_filename && parse_filename) {
         drv->bdrv_parse_filename(filename, *options, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
-            ret = -EINVAL;
-            goto fail;
+            return -EINVAL;
         }
 
         if (!drv->bdrv_needs_filename) {
             qdict_del(*options, "filename");
-        } else {
-            filename = qdict_get_str(*options, "filename");
         }
     }
 
+    return 0;
+}
+
+/*
+ * Opens a file using a protocol (file, host_device, nbd, ...)
+ *
+ * options is an indirect pointer to a QDict of options to pass to the block
+ * drivers, or pointer to NULL for an empty set of options. If this function
+ * takes ownership of the QDict reference, it will set *options to NULL;
+ * otherwise, it will contain unused/unrecognized options after this function
+ * returns. Then, the caller is responsible for freeing it. If it intends to
+ * reuse the QDict, QINCREF() should be called beforehand.
+ */
+static int bdrv_file_open(BlockDriverState *bs, const char *filename,
+                          QDict **options, int flags, Error **errp)
+{
+    BlockDriver *drv;
+    const char *drvname;
+    Error *local_err = NULL;
+    int ret;
+
+    ret = bdrv_fill_options(options, filename, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto fail;
+    }
+
+    filename = qdict_get_try_str(*options, "filename");
+    drvname = qdict_get_str(*options, "driver");
+
+    drv = bdrv_find_format(drvname);
+    assert(drv);
+    qdict_del(*options, "driver");
+
+    /* Open the file */
     if (!drv->bdrv_file_open) {
         ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err);
         *options = NULL;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 1/9] block: Create bdrv_fill_options()

[Eric Blake]

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

On 06/25/2014 08:35 AM, Kevin Wolf wrote:
> The idea of bdrv_fill_options() is to convert every parameter for
> opening images, in particular the filename and flags, to entries in the
> options QDict.
> 
> This patch starts with moving the filename parsing and driver probing
> part from bdrv_file_open() to the new function.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 112 +++++++++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 69 insertions(+), 43 deletions(-)
> 
> + * Fills in default options for opening images and converts the legacy
> + * filename/flags pair to option QDict entries.
>   */
> -static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> -                          QDict **options, int flags, Error **errp)
> +static int bdrv_fill_options(QDict **options, const char *filename,
> +                             Error **errp)

Still looks odd to reference filename/flags pair in the comments when
there is no flags argument, but it gets fixed later in the series so I
can overlook it.

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: 604 bytes --]

[Qemu-devel] [PATCH v2 2/9] block: Move bdrv_fill_options() call to bdrv_open()

[Kevin Wolf]

bs->options now contains the modified version of the options.

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

diff --git a/block.c b/block.c
index f1c3b57..b1087e6 100644
--- a/block.c
+++ b/block.c
@@ -1009,14 +1009,19 @@ free_and_fail:
  * Fills in default options for opening images and converts the legacy
  * filename/flags pair to option QDict entries.
  */
-static int bdrv_fill_options(QDict **options, const char *filename,
+static int bdrv_fill_options(QDict **options, const char *filename, int flags,
                              Error **errp)
 {
     const char *drvname;
+    bool protocol = flags & BDRV_O_PROTOCOL;
     bool parse_filename = false;
     Error *local_err = NULL;
     BlockDriver *drv;
 
+    if (!protocol) {
+        return 0;
+    }
+
     /* Fetch the file name from the options QDict if necessary */
     if (filename) {
         if (!qdict_haskey(*options, "filename")) {
@@ -1081,20 +1086,15 @@ static int bdrv_fill_options(QDict **options, const char *filename,
  * returns. Then, the caller is responsible for freeing it. If it intends to
  * reuse the QDict, QINCREF() should be called beforehand.
  */
-static int bdrv_file_open(BlockDriverState *bs, const char *filename,
-                          QDict **options, int flags, Error **errp)
+static int bdrv_file_open(BlockDriverState *bs, QDict **options, int flags,
+                          Error **errp)
 {
     BlockDriver *drv;
+    const char *filename;
     const char *drvname;
     Error *local_err = NULL;
     int ret;
 
-    ret = bdrv_fill_options(options, filename, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        goto fail;
-    }
-
     filename = qdict_get_try_str(*options, "filename");
     drvname = qdict_get_str(*options, "driver");
 
@@ -1436,12 +1436,17 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         filename = NULL;
     }
 
+    ret = bdrv_fill_options(&options, filename, flags, &local_err);
+    if (local_err) {
+        goto fail;
+    }
+
     bs->options = options;
     options = qdict_clone_shallow(options);
 
     if (flags & BDRV_O_PROTOCOL) {
         assert(!drv);
-        ret = bdrv_file_open(bs, filename, &options, flags & ~BDRV_O_PROTOCOL,
+        ret = bdrv_file_open(bs, &options, flags & ~BDRV_O_PROTOCOL,
                              &local_err);
         if (!ret) {
             drv = bs->drv;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 2/9] block: Move bdrv_fill_options() call to bdrv_open()

[Eric Blake]

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

On 06/25/2014 08:35 AM, Kevin Wolf wrote:
> bs->options now contains the modified version of the options.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/block.c b/block.c
> index f1c3b57..b1087e6 100644
> --- a/block.c
> +++ b/block.c
> @@ -1009,14 +1009,19 @@ free_and_fail:
>   * Fills in default options for opening images and converts the legacy
>   * filename/flags pair to option QDict entries.
>   */
> -static int bdrv_fill_options(QDict **options, const char *filename,
> +static int bdrv_fill_options(QDict **options, const char *filename, int flags,
>                               Error **errp)

Yep, that cleans up my comment in 1/9.

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: 604 bytes --]

[Qemu-devel] [PATCH v2 3/9] block: Move json: parsing to bdrv_fill_options()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c | 88 +++++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 45 insertions(+), 43 deletions(-)

diff --git a/block.c b/block.c
index b1087e6..f71e88b 100644
--- a/block.c
+++ b/block.c
@@ -1005,19 +1005,62 @@ free_and_fail:
     return ret;
 }
 
+static QDict *parse_json_filename(const char *filename, Error **errp)
+{
+    QObject *options_obj;
+    QDict *options;
+    int ret;
+
+    ret = strstart(filename, "json:", &filename);
+    assert(ret);
+
+    options_obj = qobject_from_json(filename);
+    if (!options_obj) {
+        error_setg(errp, "Could not parse the JSON options");
+        return NULL;
+    }
+
+    if (qobject_type(options_obj) != QTYPE_QDICT) {
+        qobject_decref(options_obj);
+        error_setg(errp, "Invalid JSON object given");
+        return NULL;
+    }
+
+    options = qobject_to_qdict(options_obj);
+    qdict_flatten(options);
+
+    return options;
+}
+
 /*
  * Fills in default options for opening images and converts the legacy
  * filename/flags pair to option QDict entries.
  */
-static int bdrv_fill_options(QDict **options, const char *filename, int flags,
+static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
                              Error **errp)
 {
+    const char *filename = *pfilename;
     const char *drvname;
     bool protocol = flags & BDRV_O_PROTOCOL;
     bool parse_filename = false;
     Error *local_err = NULL;
     BlockDriver *drv;
 
+    /* Parse json: pseudo-protocol */
+    if (filename && g_str_has_prefix(filename, "json:")) {
+        QDict *json_options = parse_json_filename(filename, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return -EINVAL;
+        }
+
+        /* Options given in the filename have lower priority than options
+         * specified directly */
+        qdict_join(*options, json_options, false);
+        QDECREF(json_options);
+        *pfilename = filename = NULL;
+    }
+
     if (!protocol) {
         return 0;
     }
@@ -1332,33 +1375,6 @@ out:
     g_free(tmp_filename);
 }
 
-static QDict *parse_json_filename(const char *filename, Error **errp)
-{
-    QObject *options_obj;
-    QDict *options;
-    int ret;
-
-    ret = strstart(filename, "json:", &filename);
-    assert(ret);
-
-    options_obj = qobject_from_json(filename);
-    if (!options_obj) {
-        error_setg(errp, "Could not parse the JSON options");
-        return NULL;
-    }
-
-    if (qobject_type(options_obj) != QTYPE_QDICT) {
-        qobject_decref(options_obj);
-        error_setg(errp, "Invalid JSON object given");
-        return NULL;
-    }
-
-    options = qobject_to_qdict(options_obj);
-    qdict_flatten(options);
-
-    return options;
-}
-
 /*
  * Opens a disk image (raw, qcow2, vmdk, ...)
  *
@@ -1422,21 +1438,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         options = qdict_new();
     }
 
-    if (filename && g_str_has_prefix(filename, "json:")) {
-        QDict *json_options = parse_json_filename(filename, &local_err);
-        if (local_err) {
-            ret = -EINVAL;
-            goto fail;
-        }
-
-        /* Options given in the filename have lower priority than options
-         * specified directly */
-        qdict_join(options, json_options, false);
-        QDECREF(json_options);
-        filename = NULL;
-    }
-
-    ret = bdrv_fill_options(&options, filename, flags, &local_err);
+    ret = bdrv_fill_options(&options, &filename, flags, &local_err);
     if (local_err) {
         goto fail;
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 4/9] block: Always pass driver name through options QDict

[Kevin Wolf]

The "driver" entry in the options QDict is now only missing if we're
opening an image with format probing.

We also catch cases now where both the drv argument and a "driver"
option is specified, e.g. by specifying -drive format=qcow2,driver=raw

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c                    | 76 ++++++++++++++++++++++++----------------------
 tests/qemu-iotests/051     |  1 +
 tests/qemu-iotests/051.out |  5 ++-
 3 files changed, 45 insertions(+), 37 deletions(-)

diff --git a/block.c b/block.c
index f71e88b..47ec746 100644
--- a/block.c
+++ b/block.c
@@ -1037,14 +1037,13 @@ static QDict *parse_json_filename(const char *filename, Error **errp)
  * filename/flags pair to option QDict entries.
  */
 static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
-                             Error **errp)
+                             BlockDriver *drv, Error **errp)
 {
     const char *filename = *pfilename;
     const char *drvname;
     bool protocol = flags & BDRV_O_PROTOCOL;
     bool parse_filename = false;
     Error *local_err = NULL;
-    BlockDriver *drv;
 
     /* Parse json: pseudo-protocol */
     if (filename && g_str_has_prefix(filename, "json:")) {
@@ -1061,12 +1060,8 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
         *pfilename = filename = NULL;
     }
 
-    if (!protocol) {
-        return 0;
-    }
-
     /* Fetch the file name from the options QDict if necessary */
-    if (filename) {
+    if (protocol && filename) {
         if (!qdict_haskey(*options, "filename")) {
             qdict_put(*options, "filename", qstring_from_str(filename));
             parse_filename = true;
@@ -1081,30 +1076,41 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
     filename = qdict_get_try_str(*options, "filename");
     drvname = qdict_get_try_str(*options, "driver");
 
-    if (!drvname) {
-        if (filename) {
-            drv = bdrv_find_protocol(filename, parse_filename);
-            if (!drv) {
-                error_setg(errp, "Unknown protocol");
+    if (drv) {
+        if (drvname) {
+            error_setg(errp, "Driver specified twice");
+            return -EINVAL;
+        }
+        drvname = drv->format_name;
+        qdict_put(*options, "driver", qstring_from_str(drvname));
+    } else {
+        if (!drvname && protocol) {
+            if (filename) {
+                drv = bdrv_find_protocol(filename, parse_filename);
+                if (!drv) {
+                    error_setg(errp, "Unknown protocol");
+                    return -EINVAL;
+                }
+
+                drvname = drv->format_name;
+                qdict_put(*options, "driver", qstring_from_str(drvname));
+            } else {
+                error_setg(errp, "Must specify either driver or file");
                 return -EINVAL;
             }
-
-            drvname = drv->format_name;
-            qdict_put(*options, "driver", qstring_from_str(drvname));
-        } else {
-            error_setg(errp, "Must specify either driver or file");
-            return -EINVAL;
+        } else if (drvname) {
+            drv = bdrv_find_format(drvname);
+            if (!drv) {
+                error_setg(errp, "Unknown driver '%s'", drvname);
+                return -ENOENT;
+            }
         }
     }
 
-    drv = bdrv_find_format(drvname);
-    if (!drv) {
-        error_setg(errp, "Unknown driver '%s'", drvname);
-        return -ENOENT;
-    }
+    assert(drv || !protocol);
 
     /* Driver-specific filename parsing */
-    if (drv->bdrv_parse_filename && parse_filename) {
+    if (drv && drv->bdrv_parse_filename && parse_filename) {
         drv->bdrv_parse_filename(filename, *options, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
@@ -1438,7 +1444,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         options = qdict_new();
     }
 
-    ret = bdrv_fill_options(&options, &filename, flags, &local_err);
+    ret = bdrv_fill_options(&options, &filename, flags, drv, &local_err);
     if (local_err) {
         goto fail;
     }
@@ -1478,7 +1484,10 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     }
 
     /* Find the right image format driver */
+    drv = NULL;
     drvname = qdict_get_try_str(options, "driver");
+    assert(drvname || !(flags & BDRV_O_PROTOCOL));
+
     if (drvname) {
         drv = bdrv_find_format(drvname);
         qdict_del(options, "driver");
@@ -1487,19 +1496,14 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
             ret = -EINVAL;
             goto fail;
         }
-    }
-
-    if (!drv) {
-        if (file) {
-            ret = find_image_format(file, filename, &drv, &local_err);
-        } else {
-            error_setg(errp, "Must specify either driver or file");
-            ret = -EINVAL;
+    } else if (file) {
+        ret = find_image_format(file, filename, &drv, &local_err);
+        if (ret < 0) {
             goto fail;
         }
-    }
-
-    if (!drv) {
+    } else {
+        error_setg(errp, "Must specify either driver or file");
+        ret = -EINVAL;
         goto fail;
     }
 
diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index c4af131..30a712f 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -92,6 +92,7 @@ echo
 
 run_qemu -drive file="$TEST_IMG",format=foo
 run_qemu -drive file="$TEST_IMG",driver=foo
+run_qemu -drive file="$TEST_IMG",driver=raw,format=qcow2
 
 echo
 echo === Overriding backing file ===
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index ad60107..37cb049 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -38,7 +38,10 @@ Testing: -drive file=TEST_DIR/t.qcow2,format=foo
 QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=foo: 'foo' invalid format
 
 Testing: -drive file=TEST_DIR/t.qcow2,driver=foo
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=foo: could not open disk image TEST_DIR/t.qcow2: Invalid driver: 'foo'
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=foo: could not open disk image TEST_DIR/t.qcow2: Unknown driver 'foo'
+
+Testing: -drive file=TEST_DIR/t.qcow2,driver=raw,format=qcow2
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=raw,format=qcow2: could not open disk image TEST_DIR/t.qcow2: Driver specified twice
 
 
 === Overriding backing file ===
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 5/9] block: Use common driver selection code for bdrv_open_file()

[Kevin Wolf]

This moves the bdrv_open_file() call a bit down so that it can use the
bdrv_open() code that selects the right block driver.

The code between the old and the new call site is either common code
(the error message for an unknown driver has been unified now) or
doesn't run with cleared BDRV_O_PROTOCOL (added an if block in one
place, whereas the right path was already asserted in another place)

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 block.c | 67 +++++++++++++++++++++++++++++------------------------------------
 1 file changed, 30 insertions(+), 37 deletions(-)

diff --git a/block.c b/block.c
index 47ec746..412f023 100644
--- a/block.c
+++ b/block.c
@@ -1135,21 +1135,14 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
  * returns. Then, the caller is responsible for freeing it. If it intends to
  * reuse the QDict, QINCREF() should be called beforehand.
  */
-static int bdrv_file_open(BlockDriverState *bs, QDict **options, int flags,
-                          Error **errp)
+static int bdrv_file_open(BlockDriverState *bs, BlockDriver *drv,
+                          QDict **options, int flags, Error **errp)
 {
-    BlockDriver *drv;
     const char *filename;
-    const char *drvname;
     Error *local_err = NULL;
     int ret;
 
     filename = qdict_get_try_str(*options, "filename");
-    drvname = qdict_get_str(*options, "driver");
-
-    drv = bdrv_find_format(drvname);
-    assert(drv);
-    qdict_del(*options, "driver");
 
     /* Open the file */
     if (!drv->bdrv_file_open) {
@@ -1452,35 +1445,23 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     bs->options = options;
     options = qdict_clone_shallow(options);
 
-    if (flags & BDRV_O_PROTOCOL) {
-        assert(!drv);
-        ret = bdrv_file_open(bs, &options, flags & ~BDRV_O_PROTOCOL,
-                             &local_err);
-        if (!ret) {
-            drv = bs->drv;
-            goto done;
-        } else if (bs->drv) {
-            goto close_and_fail;
-        } else {
-            goto fail;
-        }
-    }
-
     /* Open image file without format layer */
-    if (flags & BDRV_O_RDWR) {
-        flags |= BDRV_O_ALLOW_RDWR;
-    }
-    if (flags & BDRV_O_SNAPSHOT) {
-        snapshot_flags = bdrv_temp_snapshot_flags(flags);
-        flags = bdrv_backing_flags(flags);
-    }
+    if ((flags & BDRV_O_PROTOCOL) == 0) {
+        if (flags & BDRV_O_RDWR) {
+            flags |= BDRV_O_ALLOW_RDWR;
+        }
+        if (flags & BDRV_O_SNAPSHOT) {
+            snapshot_flags = bdrv_temp_snapshot_flags(flags);
+            flags = bdrv_backing_flags(flags);
+        }
 
-    assert(file == NULL);
-    ret = bdrv_open_image(&file, filename, options, "file",
-                          bdrv_inherited_flags(flags),
-                          true, &local_err);
-    if (ret < 0) {
-        goto fail;
+        assert(file == NULL);
+        ret = bdrv_open_image(&file, filename, options, "file",
+                              bdrv_inherited_flags(flags),
+                              true, &local_err);
+        if (ret < 0) {
+            goto fail;
+        }
     }
 
     /* Find the right image format driver */
@@ -1492,7 +1473,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         drv = bdrv_find_format(drvname);
         qdict_del(options, "driver");
         if (!drv) {
-            error_setg(errp, "Invalid driver: '%s'", drvname);
+            error_setg(errp, "Unknown driver: '%s'", drvname);
             ret = -EINVAL;
             goto fail;
         }
@@ -1508,6 +1489,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     }
 
     /* Open the image */
+    if (flags & BDRV_O_PROTOCOL) {
+        ret = bdrv_file_open(bs, drv, &options, flags & ~BDRV_O_PROTOCOL,
+                             &local_err);
+        if (!ret) {
+            goto done;
+        } else if (bs->drv) {
+            goto close_and_fail;
+        } else {
+            goto fail;
+        }
+    }
+
     ret = bdrv_open_common(bs, file, options, flags, drv, &local_err);
     if (ret < 0) {
         goto fail;
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 6/9] block: Inline bdrv_file_open()

[Kevin Wolf]

It doesn't do much any more, we can move the code to bdrv_open() now.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 block.c | 51 +++++++++++----------------------------------------
 1 file changed, 11 insertions(+), 40 deletions(-)

diff --git a/block.c b/block.c
index 412f023..64e1176 100644
--- a/block.c
+++ b/block.c
@@ -1125,44 +1125,6 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
     return 0;
 }
 
-/*
- * Opens a file using a protocol (file, host_device, nbd, ...)
- *
- * options is an indirect pointer to a QDict of options to pass to the block
- * drivers, or pointer to NULL for an empty set of options. If this function
- * takes ownership of the QDict reference, it will set *options to NULL;
- * otherwise, it will contain unused/unrecognized options after this function
- * returns. Then, the caller is responsible for freeing it. If it intends to
- * reuse the QDict, QINCREF() should be called beforehand.
- */
-static int bdrv_file_open(BlockDriverState *bs, BlockDriver *drv,
-                          QDict **options, int flags, Error **errp)
-{
-    const char *filename;
-    Error *local_err = NULL;
-    int ret;
-
-    filename = qdict_get_try_str(*options, "filename");
-
-    /* Open the file */
-    if (!drv->bdrv_file_open) {
-        ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err);
-        *options = NULL;
-    } else {
-        ret = bdrv_open_common(bs, NULL, *options, flags, drv, &local_err);
-    }
-    if (ret < 0) {
-        error_propagate(errp, local_err);
-        goto fail;
-    }
-
-    bs->growable = 1;
-    return 0;
-
-fail:
-    return ret;
-}
-
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
 {
 
@@ -1490,9 +1452,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
 
     /* Open the image */
     if (flags & BDRV_O_PROTOCOL) {
-        ret = bdrv_file_open(bs, drv, &options, flags & ~BDRV_O_PROTOCOL,
-                             &local_err);
+        if (!drv->bdrv_file_open) {
+            const char *filename;
+            filename = qdict_get_try_str(options, "filename");
+            ret = bdrv_open(&bs, filename, NULL, options,
+                            flags & ~BDRV_O_PROTOCOL, drv, &local_err);
+            options = NULL;
+        } else {
+            ret = bdrv_open_common(bs, NULL, options,
+                                   flags & ~BDRV_O_PROTOCOL, drv, &local_err);
+        }
         if (!ret) {
+            bs->growable = 1;
             goto done;
         } else if (bs->drv) {
             goto close_and_fail;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 6/9] block: Inline bdrv_file_open()

[Eric Blake]

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

On 06/25/2014 08:35 AM, Kevin Wolf wrote:
> It doesn't do much any more, we can move the code to bdrv_open() now.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c | 51 +++++++++++----------------------------------------
>  1 file changed, 11 insertions(+), 40 deletions(-)
> 

>          if (!ret) {
> +            bs->growable = 1;

Is it worth converting bs->growable to bool at some point in this
series?  But saving it for a separate patch is wise, so:

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: 604 bytes --]

[Qemu-devel] [PATCH v2 7/9] block: Remove second bdrv_open() recursion

[Kevin Wolf]

This recursion was introduced in commit 505d7583 in order to allow
nesting image formats. It only ever takes effect when the user
explicitly specifies a driver name and that driver isn't suitable for
the protocol level.

We can check this earlier in bdrv_open() and if the explicitly
requested driver is a format driver, clear BDRV_O_PROTOCOL so that
another bs->file layer is opened.

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

diff --git a/block.c b/block.c
index 64e1176..b179dd6 100644
--- a/block.c
+++ b/block.c
@@ -1404,6 +1404,26 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         goto fail;
     }
 
+    /* Find the right image format driver */
+    drv = NULL;
+    drvname = qdict_get_try_str(options, "driver");
+    if (drvname) {
+        drv = bdrv_find_format(drvname);
+        qdict_del(options, "driver");
+        if (!drv) {
+            error_setg(errp, "Unknown driver: '%s'", drvname);
+            ret = -EINVAL;
+            goto fail;
+        }
+    }
+
+    assert(drvname || !(flags & BDRV_O_PROTOCOL));
+    if (drv && !drv->bdrv_file_open) {
+        /* If the user explicitly wants a format driver here, we'll need to add
+         * another layer for the protocol in bs->file */
+        flags &= ~BDRV_O_PROTOCOL;
+    }
+
     bs->options = options;
     options = qdict_clone_shallow(options);
 
@@ -1426,25 +1446,13 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         }
     }
 
-    /* Find the right image format driver */
-    drv = NULL;
-    drvname = qdict_get_try_str(options, "driver");
-    assert(drvname || !(flags & BDRV_O_PROTOCOL));
-
-    if (drvname) {
-        drv = bdrv_find_format(drvname);
-        qdict_del(options, "driver");
-        if (!drv) {
-            error_setg(errp, "Unknown driver: '%s'", drvname);
-            ret = -EINVAL;
-            goto fail;
-        }
-    } else if (file) {
+    /* Image format probing */
+    if (!drv && file) {
         ret = find_image_format(file, filename, &drv, &local_err);
         if (ret < 0) {
             goto fail;
         }
-    } else {
+    } else if (!drv) {
         error_setg(errp, "Must specify either driver or file");
         ret = -EINVAL;
         goto fail;
@@ -1452,16 +1460,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
 
     /* Open the image */
     if (flags & BDRV_O_PROTOCOL) {
-        if (!drv->bdrv_file_open) {
-            const char *filename;
-            filename = qdict_get_try_str(options, "filename");
-            ret = bdrv_open(&bs, filename, NULL, options,
-                            flags & ~BDRV_O_PROTOCOL, drv, &local_err);
-            options = NULL;
-        } else {
-            ret = bdrv_open_common(bs, NULL, options,
-                                   flags & ~BDRV_O_PROTOCOL, drv, &local_err);
-        }
+        ret = bdrv_open_common(bs, NULL, options,
+                               flags & ~BDRV_O_PROTOCOL, drv, &local_err);
         if (!ret) {
             bs->growable = 1;
             goto done;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 7/9] block: Remove second bdrv_open() recursion

[Eric Blake]

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

On 06/25/2014 08:35 AM, Kevin Wolf wrote:
> This recursion was introduced in commit 505d7583 in order to allow
> nesting image formats. It only ever takes effect when the user
> explicitly specifies a driver name and that driver isn't suitable for
> the protocol level.
> 
> We can check this earlier in bdrv_open() and if the explicitly
> requested driver is a format driver, clear BDRV_O_PROTOCOL so that
> another bs->file layer is opened.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 50 +++++++++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 25 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: 604 bytes --]

[Qemu-devel] [PATCH v2 8/9] block: Catch backing files assigned to non-COW drivers

[Kevin Wolf]

Since we parse backing.* options to add a backing file from the command
line when the driver didn't assign one, it has been possible to have a
backing file for e.g. raw images (it just was never accessed).

This is obvious nonsense and should be rejected.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                    | 7 +++++++
 block/cow.c                | 1 +
 block/qcow.c               | 1 +
 block/qcow2.c              | 1 +
 block/qed.c                | 1 +
 block/vmdk.c               | 1 +
 include/block/block_int.h  | 3 +++
 tests/qemu-iotests/051     | 5 +++++
 tests/qemu-iotests/051.out | 9 +++++++++
 9 files changed, 29 insertions(+)

diff --git a/block.c b/block.c
index b179dd6..abe81b9 100644
--- a/block.c
+++ b/block.c
@@ -1192,6 +1192,13 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
         bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX);
     }
 
+    if (!bs->drv || !bs->drv->supports_backing) {
+        ret = -EINVAL;
+        error_setg(errp, "Driver doesn't support backing files");
+        QDECREF(options);
+        goto free_exit;
+    }
+
     backing_hd = bdrv_new("", errp);
 
     if (bs->backing_format[0] != '\0') {
diff --git a/block/cow.c b/block/cow.c
index a05a92c..8f81ee6 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -414,6 +414,7 @@ static BlockDriver bdrv_cow = {
     .bdrv_close     = cow_close,
     .bdrv_create    = cow_create,
     .bdrv_has_zero_init     = bdrv_has_zero_init_1,
+    .supports_backing       = true,
 
     .bdrv_read              = cow_co_read,
     .bdrv_write             = cow_co_write,
diff --git a/block/qcow.c b/block/qcow.c
index 1f2bac8..a874056 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -941,6 +941,7 @@ static BlockDriver bdrv_qcow = {
     .bdrv_reopen_prepare    = qcow_reopen_prepare,
     .bdrv_create            = qcow_create,
     .bdrv_has_zero_init     = bdrv_has_zero_init_1,
+    .supports_backing       = true,
 
     .bdrv_co_readv          = qcow_co_readv,
     .bdrv_co_writev         = qcow_co_writev,
diff --git a/block/qcow2.c b/block/qcow2.c
index b9d2fa6..67e55c9 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2418,6 +2418,7 @@ static BlockDriver bdrv_qcow2 = {
     .bdrv_save_vmstate    = qcow2_save_vmstate,
     .bdrv_load_vmstate    = qcow2_load_vmstate,
 
+    .supports_backing           = true,
     .bdrv_change_backing_file   = qcow2_change_backing_file,
 
     .bdrv_refresh_limits        = qcow2_refresh_limits,
diff --git a/block/qed.c b/block/qed.c
index 092e6fb..eddae92 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1652,6 +1652,7 @@ static BlockDriver bdrv_qed = {
     .format_name              = "qed",
     .instance_size            = sizeof(BDRVQEDState),
     .create_opts              = &qed_create_opts,
+    .supports_backing         = true,
 
     .bdrv_probe               = bdrv_qed_probe,
     .bdrv_rebind              = bdrv_qed_rebind,
diff --git a/block/vmdk.c b/block/vmdk.c
index 83dd6fe..d0de019 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2180,6 +2180,7 @@ static BlockDriver bdrv_vmdk = {
     .bdrv_detach_aio_context      = vmdk_detach_aio_context,
     .bdrv_attach_aio_context      = vmdk_attach_aio_context,
 
+    .supports_backing             = true,
     .create_opts                  = &vmdk_create_opts,
 };
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a1885d3..a015df6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -100,6 +100,9 @@ struct BlockDriver {
      */
     bool bdrv_needs_filename;
 
+    /* Set if a driver can support backing files */
+    bool supports_backing;
+
     /* For handling image reopen for split or non-split files */
     int (*bdrv_reopen_prepare)(BDRVReopenState *reopen_state,
                                BlockReopenQueue *queue, Error **errp);
diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 30a712f..a41334e 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -100,6 +100,11 @@ echo
 
 echo "info block" | run_qemu -drive file="$TEST_IMG",driver=qcow2,backing.file.filename="$TEST_IMG.orig" -nodefaults
 
+# Drivers that don't support backing files
+run_qemu -drive file="$TEST_IMG",driver=raw,backing.file.filename="$TEST_IMG.orig"
+run_qemu -drive file="$TEST_IMG",file.backing.driver=file,file.backing.filename="$TEST_IMG.orig"
+run_qemu -drive file="$TEST_IMG",file.backing.driver=qcow2,file.backing.file.filename="$TEST_IMG.orig"
+
 echo
 echo === Enable and disable lazy refcounting on the command line, plus some invalid values ===
 echo
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 37cb049..791956f 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -54,6 +54,15 @@ ide0-hd0: TEST_DIR/t.qcow2 (qcow2)
     Backing file:     TEST_DIR/t.qcow2.orig (chain depth: 1)
 (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
 
+Testing: -drive file=TEST_DIR/t.qcow2,driver=raw,backing.file.filename=TEST_DIR/t.qcow2.orig
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=raw,backing.file.filename=TEST_DIR/t.qcow2.orig: could not open disk image TEST_DIR/t.qcow2: Driver doesn't support backing files
+
+Testing: -drive file=TEST_DIR/t.qcow2,file.backing.driver=file,file.backing.filename=TEST_DIR/t.qcow2.orig
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,file.backing.driver=file,file.backing.filename=TEST_DIR/t.qcow2.orig: could not open disk image TEST_DIR/t.qcow2: Driver doesn't support backing files
+
+Testing: -drive file=TEST_DIR/t.qcow2,file.backing.driver=qcow2,file.backing.file.filename=TEST_DIR/t.qcow2.orig
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,file.backing.driver=qcow2,file.backing.file.filename=TEST_DIR/t.qcow2.orig: could not open disk image TEST_DIR/t.qcow2: Driver doesn't support backing files
+
 
 === Enable and disable lazy refcounting on the command line, plus some invalid values ===
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 8/9] block: Catch backing files assigned to non-COW drivers

[Eric Blake]

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

On 06/25/2014 08:35 AM, Kevin Wolf wrote:
> Since we parse backing.* options to add a backing file from the command
> line when the driver didn't assign one, it has been possible to have a
> backing file for e.g. raw images (it just was never accessed).
> 
> This is obvious nonsense and should be rejected.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                    | 7 +++++++
>  block/cow.c                | 1 +
>  block/qcow.c               | 1 +
>  block/qcow2.c              | 1 +
>  block/qed.c                | 1 +
>  block/vmdk.c               | 1 +
>  include/block/block_int.h  | 3 +++
>  tests/qemu-iotests/051     | 5 +++++
>  tests/qemu-iotests/051.out | 9 +++++++++
>  9 files changed, 29 insertions(+)

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: 604 bytes --]

[Qemu-devel] [PATCH v2 9/9] block: Remove a special case for protocols

[Kevin Wolf]

The only semantic change is that bs->open_flags gets BDRV_O_PROTOCOL set
now. This isn't useful, but it doesn't hurt either. The code that was
previously skipped by 'goto done' is automatically disabled because
protocol drivers don't support backing files (and if they did, this
would probably be a fix) and can't have snapshot_flags set.

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

diff --git a/block.c b/block.c
index abe81b9..186a4e9 100644
--- a/block.c
+++ b/block.c
@@ -831,7 +831,7 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
      * Clear flags that are internal to the block layer before opening the
      * image.
      */
-    open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
+    open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_PROTOCOL);
 
     /*
      * Snapshots should be writable.
@@ -928,6 +928,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     bs->zero_beyond_eof = true;
     open_flags = bdrv_open_flags(bs, flags);
     bs->read_only = !(open_flags & BDRV_O_RDWR);
+    bs->growable = !!(flags & BDRV_O_PROTOCOL);
 
     if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
         error_setg(errp,
@@ -1466,19 +1467,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     }
 
     /* Open the image */
-    if (flags & BDRV_O_PROTOCOL) {
-        ret = bdrv_open_common(bs, NULL, options,
-                               flags & ~BDRV_O_PROTOCOL, drv, &local_err);
-        if (!ret) {
-            bs->growable = 1;
-            goto done;
-        } else if (bs->drv) {
-            goto close_and_fail;
-        } else {
-            goto fail;
-        }
-    }
-
     ret = bdrv_open_common(bs, file, options, flags, drv, &local_err);
     if (ret < 0) {
         goto fail;
@@ -1510,8 +1498,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         }
     }
 
-
-done:
     /* Check if any unknown options were used */
     if (options && (qdict_size(options) != 0)) {
         const QDictEntry *entry = qdict_first(options);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 9/9] block: Remove a special case for protocols

[Eric Blake]

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

On 06/25/2014 08:35 AM, Kevin Wolf wrote:
> The only semantic change is that bs->open_flags gets BDRV_O_PROTOCOL set
> now. This isn't useful, but it doesn't hurt either. The code that was
> previously skipped by 'goto done' is automatically disabled because
> protocol drivers don't support backing files (and if they did, this
> would probably be a fix) and can't have snapshot_flags set.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 18 ++----------------
>  1 file changed, 2 insertions(+), 16 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: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 0/9] bdrv_open() cleanups, part 1

[Kevin Wolf]

Am 25.06.2014 um 16:35 hat Kevin Wolf geschrieben:
> This is the first part of an attempt for disentangling bdrv_open(). At the end
> of this series, bdrv_open() code is somewhat easier to read, but the real
> changes (including some bug fixes and changes of behaviour) haven't happened
> yet.
> 
> Just sending out the first part now to get this merged early and avoid
> conflicts.
> 
> v2:
> - Rebased on current git master
> - Patch 1: Removed redundant if condition [Benoît]
>            Replaced redundant error check with assertion [Eric]
> - Patch 2: Fixed leak in error path [Benoît]

Applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH v2 0/9] bdrv_open() cleanups, part 1

[Max Reitz]

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

On 25.06.2014 16:35, Kevin Wolf wrote:
> This is the first part of an attempt for disentangling bdrv_open(). At the end
> of this series, bdrv_open() code is somewhat easier to read, but the real
> changes (including some bug fixes and changes of behaviour) haven't happened
> yet.
>
> Just sending out the first part now to get this merged early and avoid
> conflicts.
>
> v2:
> - Rebased on current git master
> - Patch 1: Removed redundant if condition [Benoît]
>             Replaced redundant error check with assertion [Eric]
> - Patch 2: Fixed leak in error path [Benoît]
>
> Kevin Wolf (9):
>    block: Create bdrv_fill_options()
>    block: Move bdrv_fill_options() call to bdrv_open()
>    block: Move json: parsing to bdrv_fill_options()
>    block: Always pass driver name through options QDict
>    block: Use common driver selection code for bdrv_open_file()
>    block: Inline bdrv_file_open()
>    block: Remove second bdrv_open() recursion
>    block: Catch backing files assigned to non-COW drivers
>    block: Remove a special case for protocols
>
>   block.c                    | 280 ++++++++++++++++++++++-----------------------
>   block/cow.c                |   1 +
>   block/qcow.c               |   1 +
>   block/qcow2.c              |   1 +
>   block/qed.c                |   1 +
>   block/vmdk.c               |   1 +
>   include/block/block_int.h  |   3 +
>   tests/qemu-iotests/051     |   6 +
>   tests/qemu-iotests/051.out |  14 ++-
>   9 files changed, 164 insertions(+), 144 deletions(-)

With this series, iotests 040, 041 and 067 fail for me. I attached the 
result.

Max

[-- Attachment #2: log --]
[-- Type: text/plain, Size: 29830 bytes --]

QEMU          -- /home/mreitz/projects/qemu/tests/qemu-iotests/qemu
QEMU_IMG      -- /home/mreitz/projects/qemu/tests/qemu-iotests/qemu-img
QEMU_IO       -- /home/mreitz/projects/qemu/tests/qemu-iotests/qemu-io 
QEMU_NBD      -- /home/mreitz/projects/qemu/tests/qemu-iotests/qemu-nbd
IMGFMT        -- qcow2 (compat=1.1)
IMGPROTO      -- file
PLATFORM      -- Linux/x86_64 dresden 3.14.5-200.fc20.x86_64
SOCKET_SCM_HELPER -- 

040 10s ...        [23:15:41] [23:15:52] [failed, exit status 1] - output mismatch (see 040.out.bad)
--- 040.out	2014-06-26 23:10:00.591073323 +0200
+++ 040.out.bad	2014-06-26 23:15:52.102465884 +0200
@@ -1,5 +1,47 @@
-........................
+.....F.......F.......F..
+======================================================================
+FAIL: test_top_is_active (__main__.TestActiveZeroLengthImage)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./040", line 112, in test_top_is_active
+    self.run_commit_test(test_img, backing_img, need_ready=True)
+  File "./040", line 57, in run_commit_test
+    self.assert_qmp(event, 'data/type', 'commit')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_top_is_active (__main__.TestRelativePaths)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./040", line 202, in test_top_is_active
+    self.run_commit_test(self.test_img, self.backing_img)
+  File "./040", line 57, in run_commit_test
+    self.assert_qmp(event, 'data/type', 'commit')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_top_is_active (__main__.TestSingleDrive)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./040", line 112, in test_top_is_active
+    self.run_commit_test(test_img, backing_img, need_ready=True)
+  File "./040", line 57, in run_commit_test
+    self.assert_qmp(event, 'data/type', 'commit')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
 ----------------------------------------------------------------------
 Ran 24 tests
 
-OK
+FAILED (failures=3)
041 17s ...        [23:15:52] [23:16:21] [failed, exit status 1] - output mismatch (see 041.out.bad)
--- 041.out	2014-06-26 23:10:00.591073323 +0200
+++ 041.out.bad	2014-06-26 23:16:21.502833277 +0200
@@ -1,5 +1,399 @@
-...................................
+FEFFFFFF..FF.FF..F.FFF.FF...FFFFFFFF
+======================================================================
+ERROR: test_cancel (__main__.TestMirrorNoBacking)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 235, in tearDown
+    os.remove(target_backing_img)
+OSError: [Errno 2] No such file or directory: '/home/mreitz/projects/qemu/tests/qemu-iotests/scratch/target-backing.img'
+
+======================================================================
+FAIL: test_cancel (__main__.TestMirrorNoBacking)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 261, in test_cancel
+    self.wait_ready_and_cancel()
+  File "./041", line 45, in wait_ready_and_cancel
+    self.wait_ready(drive)
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_complete (__main__.TestMirrorNoBacking)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 246, in test_complete
+    self.complete_and_wait()
+  File "./041", line 219, in complete_and_wait
+    return ImageMirroringTestCase.complete_and_wait(self, drive, wait_ready)
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_large_cluster (__main__.TestMirrorNoBacking)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 282, in test_large_cluster
+    self.complete_and_wait()
+  File "./041", line 219, in complete_and_wait
+    return ImageMirroringTestCase.complete_and_wait(self, drive, wait_ready)
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_complete_full (__main__.TestMirrorResized)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 330, in test_complete_full
+    self.complete_and_wait()
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_complete_top (__main__.TestMirrorResized)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 316, in test_complete_top
+    self.complete_and_wait()
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_ignore_read (__main__.TestReadErrors)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 428, in test_ignore_read
+    self.complete_and_wait()
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_large_cluster (__main__.TestReadErrors)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 449, in test_large_cluster
+    self.complete_and_wait()
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_set_speed (__main__.TestSetSpeed)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 654, in test_set_speed
+    self.wait_ready_and_cancel()
+  File "./041", line 45, in wait_ready_and_cancel
+    self.wait_ready(drive)
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_set_speed_invalid (__main__.TestSetSpeed)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 683, in test_set_speed_invalid
+    self.wait_ready_and_cancel()
+  File "./041", line 45, in wait_ready_and_cancel
+    self.wait_ready(drive)
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_cancel_after_ready (__main__.TestSingleDrive)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 114, in test_cancel_after_ready
+    self.wait_ready_and_cancel()
+  File "./041", line 45, in wait_ready_and_cancel
+    self.wait_ready(drive)
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_complete (__main__.TestSingleDrive)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 88, in test_complete
+    self.complete_and_wait()
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_large_cluster (__main__.TestSingleDrive)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 187, in test_large_cluster
+    self.complete_and_wait()
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_pause (__main__.TestSingleDrive)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 142, in test_pause
+    self.complete_and_wait()
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_small_buffer (__main__.TestSingleDrive)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 155, in test_small_buffer
+    self.complete_and_wait()
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_small_buffer2 (__main__.TestSingleDrive)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 171, in test_small_buffer2
+    self.complete_and_wait()
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_cancel_after_ready (__main__.TestSingleDriveZeroLength)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 114, in test_cancel_after_ready
+    self.wait_ready_and_cancel()
+  File "./041", line 45, in wait_ready_and_cancel
+    self.wait_ready(drive)
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_complete (__main__.TestSingleDriveZeroLength)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 88, in test_complete
+    self.complete_and_wait()
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_pause (__main__.TestSingleDriveZeroLength)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 142, in test_pause
+    self.complete_and_wait()
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_small_buffer (__main__.TestSingleDriveZeroLength)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 155, in test_small_buffer
+    self.complete_and_wait()
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_absolute_paths_full (__main__.TestUnbackedSource)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 705, in test_absolute_paths_full
+    self.complete_and_wait()
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_absolute_paths_none (__main__.TestUnbackedSource)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 723, in test_absolute_paths_none
+    self.complete_and_wait()
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_absolute_paths_top (__main__.TestUnbackedSource)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 714, in test_absolute_paths_top
+    self.complete_and_wait()
+  File "./041", line 55, in complete_and_wait
+    self.wait_ready()
+  File "./041", line 40, in wait_ready
+    self.assert_qmp(event, 'data/type', 'mirror')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 232, in assert_qmp
+    result = self.dictpath(d, path)
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 211, in dictpath
+    self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+AssertionError: failed path traversal for "data/type" in "{u'device': u'drive0'}"
+
+======================================================================
+FAIL: test_ignore_write (__main__.TestWriteErrors)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 576, in test_ignore_write
+    self.assert_qmp(event, 'data/device', 'drive0')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 233, in assert_qmp
+    self.assertEqual(result, value, 'values not equal "%s" and "%s"' % (str(result), str(value)))
+AssertionError: values not equal "" and "drive0"
+
+======================================================================
+FAIL: test_report_write (__main__.TestWriteErrors)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 550, in test_report_write
+    self.assert_qmp(event, 'data/device', 'drive0')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 233, in assert_qmp
+    self.assertEqual(result, value, 'values not equal "%s" and "%s"' % (str(result), str(value)))
+AssertionError: values not equal "" and "drive0"
+
+======================================================================
+FAIL: test_stop_write (__main__.TestWriteErrors)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "./041", line 596, in test_stop_write
+    self.assert_qmp(event, 'data/device', 'drive0')
+  File "/home/mreitz/projects/qemu/tests/qemu-iotests/iotests.py", line 233, in assert_qmp
+    self.assertEqual(result, value, 'values not equal "%s" and "%s"' % (str(result), str(value)))
+AssertionError: values not equal "" and "drive0"
+
 ----------------------------------------------------------------------
 Ran 35 tests
 
-OK
+FAILED (failures=25, errors=1)
067 1s ...        [23:16:21] [23:16:21] - output mismatch (see 067.out.bad)
--- 067.out	2014-06-26 23:10:00.593073348 +0200
+++ 067.out.bad	2014-06-26 23:16:21.883838039 +0200
@@ -9,7 +9,6 @@
 {"return": [{"io-status": "ok", "device": "disk", "locked": false, "removable": false, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": SIZE, "format-specific": {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "type": "unknown"}, {"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
 {"return": {}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"path": "/machine/peripheral/virtio0/virtio-backend"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"device": "virtio0", "path": "/machine/peripheral/virtio0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "RESET"}
 {"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
@@ -28,7 +27,6 @@
 {"return": {}}
 {"return": {}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"path": "/machine/peripheral/virtio0/virtio-backend"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"device": "virtio0", "path": "/machine/peripheral/virtio0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "RESET"}
 {"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
@@ -48,7 +46,6 @@
 {"return": {}}
 {"return": {}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"path": "/machine/peripheral/virtio0/virtio-backend"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"device": "virtio0", "path": "/machine/peripheral/virtio0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "RESET"}
 {"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
@@ -68,7 +65,6 @@
 {"return": {}}
 {"return": {}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"path": "/machine/peripheral/virtio0/virtio-backend"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_DELETED", "data": {"device": "virtio0", "path": "/machine/peripheral/virtio0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "RESET"}
 {"return": [{"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"io-status": "ok", "device": "disk", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster-size": 65536, "format": "qcow2", "actual-size": SIZE, "format-specific": {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "tray_open": false, "type": "unknown"}]}
Failures: 040 041 067
Failed 3 of 3 tests

Re: [Qemu-devel] [PATCH v2 0/9] bdrv_open() cleanups, part 1

[Eric Blake]

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

On 06/26/2014 03:17 PM, Max Reitz wrote:
> On 25.06.2014 16:35, Kevin Wolf wrote:
>> This is the first part of an attempt for disentangling bdrv_open(). At
>> the end
>> of this series, bdrv_open() code is somewhat easier to read, but the real
>> changes (including some bug fixes and changes of behaviour) haven't
>> happened
>> yet.
>>

> With this series, iotests 040, 041 and 067 fail for me. I attached the
> result.

Actually, it wasn't this series, but the qapi-as-event series.  See here
for more diagnosis and potential fixes:
https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg06554.html
https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg06499.html

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 0/9] bdrv_open() cleanups, part 1

[Max Reitz]

On 26.06.2014 23:46, Eric Blake wrote:
> On 06/26/2014 03:17 PM, Max Reitz wrote:
>> On 25.06.2014 16:35, Kevin Wolf wrote:
>>> This is the first part of an attempt for disentangling bdrv_open(). At
>>> the end
>>> of this series, bdrv_open() code is somewhat easier to read, but the real
>>> changes (including some bug fixes and changes of behaviour) haven't
>>> happened
>>> yet.
>>>
>> With this series, iotests 040, 041 and 067 fail for me. I attached the
>> result.
> Actually, it wasn't this series, but the qapi-as-event series.  See here
> for more diagnosis and potential fixes:
> https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg06554.html
> https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg06499.html

Ah, great, looking for the error here would have probably been rather 
interesting.

Max