[Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open()

[Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open()

[Max Reitz]

bdrv_file_open() is now nearly a subset of bdrv_open(), except for the
fact that bdrv_file_open() is for protocols and bdrv_open() for block
drivers. It is possible to use bdrv_file_open() with a block driver, but
in that case that block driver must be explicitly specified.

Due to these great similarities, bdrv_file_open() can be integrated and
made a special case of bdrv_open(). If the flag BDRV_O_PROTOCOL is
specified, bdrv_open() will now do what bdrv_file_open() used to do:
Auto-detecting a protocol instead of a block driver.

This series implements this and changes all calls to bdrv_file_open() to
bdrv_open() calls with BDRV_O_PROTOCOL specified.

Note that this flag cannot be discerned automatically since it is
impossible for bdrv_open() to know by itself whether a given file should
be opened with or without the format layer involved: Both are valid
alternatives. Therefore, it still has to be specified by the user.


v2:
 - patch 1:
   - added appropriate assertions before every bdrv_open() and
     bdrv_open_image() call where the BDS should be NULL (i.e., where a
     new BDS should be created) and it is not obvious
   - don't initialize BDS pointers to NULL if that is necessary for a
     bdrv_open(); rather, set them to NULL directly before calling
     bdrv_open() in order to make the expected behavior of bdrv_open()
     (create a new BDS) more obvious
   - make bdrv_open_image() behave the same way as bdrv_open() in regard
     to BDS creation/reuse (completed in patch 8)
   - keep "options == NULL" check in bdrv_open() together with the block
     for setting "bs->options"
 - patch 2:
   - due to options now possibly being NULL in the reference handling
     block of bdrv_open(), the condition for "options_non_empty" has to
     be adjusted
   - contextual changes due to patch 1
 - patch 3:
   - adjust comment for BDRV_O_PROTOCOL
   - contextual changes due to the previous patches
 - patch 4 (previously 5):
   - contextual changes due to the previous patches
 - patch 5 (previously 6):
   - bdrv_close() is unnecessary if the BDS could not be opened
 - patch 6:
   - melded together from previous patches 4, 7 and 8
 - patch 7 (previously 9):
   - make the "options" parameter of bdrv_file_open() an indirect
     pointer; if bdrv_file_open() takes the reference, it will be set to
     NULL; otherwise, the caller is responsible for freeing the QDict
 - patch 8 (previously 10):
   - make bdrv_open_image() behave the same way as bdrv_open() in regard
     to BDS creation/reuse
   - contextual changes due to the previous patches


Max Reitz (8):
  block: Change BDS parameter of bdrv_open() to **
  block: Add reference parameter to bdrv_open()
  block: Make bdrv_file_open() static
  block: Reuse reference handling from bdrv_open()
  block: Remove bdrv_new() from bdrv_file_open()
  block: Handle bs->options in bdrv_open() only
  block: Reuse success path from bdrv_open()
  block: Remove bdrv_open_image()'s force_raw option

 block.c               | 229 +++++++++++++++++++++++++-------------------------
 block/blkdebug.c      |   3 +-
 block/blkverify.c     |   6 +-
 block/cow.c           |   6 +-
 block/qcow.c          |   6 +-
 block/qcow2.c         |  19 +++--
 block/qed.c           |   5 +-
 block/sheepdog.c      |   8 +-
 block/vhdx.c          |   5 +-
 block/vmdk.c          |  17 ++--
 block/vvfat.c         |   6 +-
 blockdev.c            |  22 +++--
 hw/block/xen_disk.c   |   4 +-
 include/block/block.h |  13 +--
 qemu-img.c            |  12 ++-
 qemu-io.c             |   8 +-
 qemu-nbd.c            |   2 +-
 17 files changed, 194 insertions(+), 177 deletions(-)

-- 
1.8.5.4

[Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to **

[Max Reitz]

Make bdrv_open() take a pointer to a BDS pointer, similarly to
bdrv_file_open(). If a pointer to a NULL pointer is given, bdrv_open()
will create a new BDS with an empty name; if the BDS pointer is not
NULL, that existing BDS will be reused (in the same way as bdrv_open()
already did).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 64 +++++++++++++++++++++++++++++++--------------------
 block/blkdebug.c      |  1 +
 block/blkverify.c     |  2 ++
 block/qcow2.c         | 14 +++++++----
 block/vmdk.c          |  5 ++--
 block/vvfat.c         |  6 ++---
 blockdev.c            | 20 ++++++++--------
 hw/block/xen_disk.c   |  2 +-
 include/block/block.h |  2 +-
 qemu-img.c            | 10 ++++----
 qemu-io.c             |  2 +-
 qemu-nbd.c            |  2 +-
 12 files changed, 72 insertions(+), 58 deletions(-)

diff --git a/block.c b/block.c
index 636aa11..40a585a 100644
--- a/block.c
+++ b/block.c
@@ -1040,7 +1040,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
     }
 
     if (!drv->bdrv_file_open) {
-        ret = bdrv_open(bs, filename, options, flags, drv, &local_err);
+        ret = bdrv_open(&bs, filename, options, flags, drv, &local_err);
         options = NULL;
     } else {
         ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
@@ -1109,8 +1109,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
                                        sizeof(backing_filename));
     }
 
-    bs->backing_hd = bdrv_new("");
-
     if (bs->backing_format[0] != '\0') {
         back_drv = bdrv_find_format(bs->backing_format);
     }
@@ -1119,11 +1117,11 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
     back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
                                     BDRV_O_COPY_ON_READ);
 
-    ret = bdrv_open(bs->backing_hd,
+    assert(bs->backing_hd == NULL);
+    ret = bdrv_open(&bs->backing_hd,
                     *backing_filename ? backing_filename : NULL, options,
                     back_flags, back_drv, &local_err);
     if (ret < 0) {
-        bdrv_unref(bs->backing_hd);
         bs->backing_hd = NULL;
         bs->open_flags |= BDRV_O_NO_BACKING;
         error_setg(errp, "Could not open backing file: %s",
@@ -1160,6 +1158,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
  * BlockdevRef.
  *
  * The BlockdevRef will be removed from the options QDict.
+ *
+ * As with bdrv_open(), if *pbs is NULL, a new BDS will be created with a
+ * pointer to it stored there. If it is not NULL, the referenced BDS will
+ * be reused.
  */
 int bdrv_open_image(BlockDriverState **pbs, const char *filename,
                     QDict *options, const char *bdref_key, int flags,
@@ -1190,8 +1192,6 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
         /* If a filename is given and the block driver should be detected
            automatically (instead of using none), use bdrv_open() in order to do
            that auto-detection. */
-        BlockDriverState *bs;
-
         if (reference) {
             error_setg(errp, "Cannot reference an existing block device while "
                        "giving a filename");
@@ -1199,13 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
             goto done;
         }
 
-        bs = bdrv_new("");
-        ret = bdrv_open(bs, filename, image_options, flags, NULL, errp);
-        if (ret < 0) {
-            bdrv_unref(bs);
-        } else {
-            *pbs = bs;
-        }
+        ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
     } else {
         ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
                              errp);
@@ -1223,22 +1217,32 @@ done:
  * empty set of options. The reference to the QDict belongs to the block layer
  * after the call (even on failure), so if the caller intends to reuse the
  * dictionary, it needs to use QINCREF() before calling bdrv_open.
+ *
+ * If *pbs is NULL, a new BDS will be created with a pointer to it stored there.
+ * If it is not NULL, the referenced BDS will be reused.
  */
-int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
+int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
               int flags, BlockDriver *drv, Error **errp)
 {
     int ret;
     /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
     char tmp_filename[PATH_MAX + 1];
-    BlockDriverState *file = NULL;
+    BlockDriverState *file = NULL, *bs;
     const char *drvname;
     Error *local_err = NULL;
 
+    assert(pbs);
+
+    if (*pbs) {
+        bs = *pbs;
+    } else {
+        bs = bdrv_new("");
+    }
+
     /* NULL means an empty set of options */
     if (options == NULL) {
         options = qdict_new();
     }
-
     bs->options = options;
     options = qdict_clone_shallow(options);
 
@@ -1254,12 +1258,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
            instead of opening 'filename' directly */
 
         /* Get the required size from the image */
-        bs1 = bdrv_new("");
         QINCREF(options);
-        ret = bdrv_open(bs1, filename, options, BDRV_O_NO_BACKING,
+        bs1 = NULL;
+        ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING,
                         drv, &local_err);
         if (ret < 0) {
-            bdrv_unref(bs1);
             goto fail;
         }
         total_size = bdrv_getlength(bs1) & BDRV_SECTOR_MASK;
@@ -1316,6 +1319,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
         flags |= BDRV_O_ALLOW_RDWR;
     }
 
+    assert(file == NULL);
     ret = bdrv_open_image(&file, filename, options, "file",
                           bdrv_open_flags(bs, flags | BDRV_O_UNMAP), true, true,
                           &local_err);
@@ -1387,6 +1391,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
         bdrv_dev_change_media_cb(bs, true);
     }
 
+    *pbs = bs;
     return 0;
 
 unlink_and_fail:
@@ -1400,13 +1405,24 @@ fail:
     QDECREF(bs->options);
     QDECREF(options);
     bs->options = NULL;
+    if (!*pbs) {
+        /* If *pbs is NULL, a new BDS has been created in this function and
+           needs to be freed now. Otherwise, it does not need to be closed,
+           since it has not really been opened yet. */
+        bdrv_unref(bs);
+    }
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
     }
     return ret;
 
 close_and_fail:
-    bdrv_close(bs);
+    /* See fail path, but now the BDS has to be always closed */
+    if (*pbs) {
+        bdrv_close(bs);
+    } else {
+        bdrv_unref(bs);
+    }
     QDECREF(options);
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
@@ -5288,9 +5304,8 @@ void bdrv_img_create(const char *filename, const char *fmt,
             back_flags =
                 flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
 
-            bs = bdrv_new("");
-
-            ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags,
+            bs = NULL;
+            ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags,
                             backing_drv, &local_err);
             if (ret < 0) {
                 error_setg_errno(errp, -ret, "Could not open '%s': %s",
@@ -5298,7 +5313,6 @@ void bdrv_img_create(const char *filename, const char *fmt,
                                  error_get_pretty(local_err));
                 error_free(local_err);
                 local_err = NULL;
-                bdrv_unref(bs);
                 goto out;
             }
             bdrv_get_geometry(bs, &size);
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 8eb0db0..053fa4c 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -410,6 +410,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     s->state = 1;
 
     /* Open the backing file */
+    assert(bs->file == NULL);
     ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, "image",
                           flags, true, false, &local_err);
     if (ret < 0) {
diff --git a/block/blkverify.c b/block/blkverify.c
index cfcbcf4..86585e7 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -135,6 +135,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Open the raw file */
+    assert(bs->file == NULL);
     ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-raw"), options,
                           "raw", flags, true, false, &local_err);
     if (ret < 0) {
@@ -143,6 +144,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Open the test file */
+    assert(s->test_file == NULL);
     ret = bdrv_open_image(&s->test_file, qemu_opt_get(opts, "x-image"), options,
                           "test", flags, false, false, &local_err);
     if (ret < 0) {
diff --git a/block/qcow2.c b/block/qcow2.c
index 0b4335c..c8e8ba7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1543,7 +1543,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         goto out;
     }
 
-    bdrv_close(bs);
+    bdrv_unref(bs);
+    bs = NULL;
 
     /*
      * And now open the image and make it consistent first (i.e. increase the
@@ -1552,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
      */
     BlockDriver* drv = bdrv_find_format("qcow2");
     assert(drv != NULL);
-    ret = bdrv_open(bs, filename, NULL,
+    ret = bdrv_open(&bs, filename, NULL,
         BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
@@ -1599,10 +1600,11 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         }
     }
 
-    bdrv_close(bs);
+    bdrv_unref(bs);
+    bs = NULL;
 
     /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
-    ret = bdrv_open(bs, filename, NULL,
+    ret = bdrv_open(&bs, filename, NULL,
                     BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
                     drv, &local_err);
     if (error_is_set(&local_err)) {
@@ -1612,7 +1614,9 @@ static int qcow2_create2(const char *filename, int64_t total_size,
 
     ret = 0;
 out:
-    bdrv_unref(bs);
+    if (bs) {
+        bdrv_unref(bs);
+    }
     return ret;
 }
 
diff --git a/block/vmdk.c b/block/vmdk.c
index e809e2e..52cd12a 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1755,10 +1755,9 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
         goto exit;
     }
     if (backing_file) {
-        BlockDriverState *bs = bdrv_new("");
-        ret = bdrv_open(bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
+        BlockDriverState *bs = NULL;
+        ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
         if (ret != 0) {
-            bdrv_unref(bs);
             goto exit;
         }
         if (strcmp(bs->drv->format_name, "vmdk")) {
diff --git a/block/vvfat.c b/block/vvfat.c
index 664941c..ae7bc6f 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2936,15 +2936,13 @@ static int enable_write_target(BDRVVVFATState *s)
         goto err;
     }
 
-    s->qcow = bdrv_new("");
-
-    ret = bdrv_open(s->qcow, s->qcow_filename, NULL,
+    s->qcow = NULL;
+    ret = bdrv_open(&s->qcow, s->qcow_filename, NULL,
             BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
             &local_err);
     if (ret < 0) {
         qerror_report_err(local_err);
         error_free(local_err);
-        bdrv_unref(s->qcow);
         goto err;
     }
 
diff --git a/blockdev.c b/blockdev.c
index 36ceece..6dd351c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -510,7 +510,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
     bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
 
     QINCREF(bs_opts);
-    ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
+    ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
 
     if (ret < 0) {
         error_setg(errp, "could not open disk image %s: %s",
@@ -1301,12 +1301,12 @@ static void external_snapshot_prepare(BlkTransactionState *common,
                   qstring_from_str(snapshot_node_name));
     }
 
-    /* We will manually add the backing_hd field to the bs later */
-    state->new_bs = bdrv_new("");
     /* TODO Inherit bs->options or only take explicit options with an
      * extended QMP command? */
-    ret = bdrv_open(state->new_bs, new_image_file, options,
+    assert(state->new_bs == NULL);
+    ret = bdrv_open(&state->new_bs, new_image_file, options,
                     flags | BDRV_O_NO_BACKING, drv, &local_err);
+    /* We will manually add the backing_hd field to the bs later */
     if (ret != 0) {
         error_propagate(errp, local_err);
     }
@@ -1555,7 +1555,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
     Error *local_err = NULL;
     int ret;
 
-    ret = bdrv_open(bs, filename, NULL, bdrv_flags, drv, &local_err);
+    ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         return;
@@ -1991,10 +1991,9 @@ void qmp_drive_backup(const char *device, const char *target,
         return;
     }
 
-    target_bs = bdrv_new("");
-    ret = bdrv_open(target_bs, target, NULL, flags, drv, &local_err);
+    target_bs = NULL;
+    ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
     if (ret < 0) {
-        bdrv_unref(target_bs);
         error_propagate(errp, local_err);
         return;
     }
@@ -2135,11 +2134,10 @@ void qmp_drive_mirror(const char *device, const char *target,
     /* Mirroring takes care of copy-on-write using the source's backing
      * file.
      */
-    target_bs = bdrv_new("");
-    ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
+    target_bs = NULL;
+    ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
                     &local_err);
     if (ret < 0) {
-        bdrv_unref(target_bs);
         error_propagate(errp, local_err);
         return;
     }
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 098f6c6..14e6d0b 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -813,7 +813,7 @@ static int blk_connect(struct XenDevice *xendev)
             Error *local_err = NULL;
             BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
                                                            readonly);
-            if (bdrv_open(blkdev->bs,
+            if (bdrv_open(&blkdev->bs,
                           blkdev->filename, NULL, qflags, drv, &local_err) != 0)
             {
                 xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
diff --git a/include/block/block.h b/include/block/block.h
index 963a61f..980869d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -190,7 +190,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
                     QDict *options, const char *bdref_key, int flags,
                     bool force_raw, bool allow_none, Error **errp);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
-int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
+int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
               int flags, BlockDriver *drv, Error **errp);
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
                                     BlockDriverState *bs, int flags);
diff --git a/qemu-img.c b/qemu-img.c
index c989850..897aa56 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
         drv = NULL;
     }
 
-    ret = bdrv_open(bs, filename, NULL, flags, drv, &local_err);
+    ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err);
     if (ret < 0) {
         error_report("Could not open '%s': %s", filename,
                      error_get_pretty(local_err));
@@ -310,9 +310,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
     }
     return bs;
 fail:
-    if (bs) {
-        bdrv_unref(bs);
-    }
+    bdrv_unref(bs);
     return NULL;
 }
 
@@ -2314,7 +2312,7 @@ static int img_rebase(int argc, char **argv)
 
         bs_old_backing = bdrv_new("old_backing");
         bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
-        ret = bdrv_open(bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
+        ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
                         old_backing_drv, &local_err);
         if (ret) {
             error_report("Could not open old backing file '%s': %s",
@@ -2324,7 +2322,7 @@ static int img_rebase(int argc, char **argv)
         }
         if (out_baseimg[0]) {
             bs_new_backing = bdrv_new("new_backing");
-            ret = bdrv_open(bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
+            ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
                         new_backing_drv, &local_err);
             if (ret) {
                 error_report("Could not open new backing file '%s': %s",
diff --git a/qemu-io.c b/qemu-io.c
index 7f459d8..8da8f6e 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -68,7 +68,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
     } else {
         qemuio_bs = bdrv_new("hda");
 
-        if (bdrv_open(qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
+        if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
             fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
                     error_get_pretty(local_err));
             error_free(local_err);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 136e8c9..0cf123c 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -597,7 +597,7 @@ int main(int argc, char **argv)
 
     bs = bdrv_new("hda");
     srcpath = argv[optind];
-    ret = bdrv_open(bs, srcpath, NULL, flags, drv, &local_err);
+    ret = bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err);
     if (ret < 0) {
         errno = -ret;
         err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind],
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to **

[Kevin Wolf]

Am 08.02.2014 um 18:39 hat Max Reitz geschrieben:
> Make bdrv_open() take a pointer to a BDS pointer, similarly to
> bdrv_file_open(). If a pointer to a NULL pointer is given, bdrv_open()
> will create a new BDS with an empty name; if the BDS pointer is not
> NULL, that existing BDS will be reused (in the same way as bdrv_open()
> already did).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c               | 64 +++++++++++++++++++++++++++++++--------------------
>  block/blkdebug.c      |  1 +
>  block/blkverify.c     |  2 ++
>  block/qcow2.c         | 14 +++++++----
>  block/vmdk.c          |  5 ++--
>  block/vvfat.c         |  6 ++---
>  blockdev.c            | 20 ++++++++--------
>  hw/block/xen_disk.c   |  2 +-
>  include/block/block.h |  2 +-
>  qemu-img.c            | 10 ++++----
>  qemu-io.c             |  2 +-
>  qemu-nbd.c            |  2 +-
>  12 files changed, 72 insertions(+), 58 deletions(-)

> @@ -1160,6 +1158,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>   * BlockdevRef.
>   *
>   * The BlockdevRef will be removed from the options QDict.
> + *
> + * As with bdrv_open(), if *pbs is NULL, a new BDS will be created with a
> + * pointer to it stored there. If it is not NULL, the referenced BDS will
> + * be reused.
>   */
>  int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>                      QDict *options, const char *bdref_key, int flags,

There are no callers that make use of *pbs != NULL. Are you planning to
add such users? Otherwise, we could just assert() it here instead of
documenting behaviour that is never used.

Kevin

Re: [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to **

[Max Reitz]

On 10.02.2014 13:42, Kevin Wolf wrote:
> Am 08.02.2014 um 18:39 hat Max Reitz geschrieben:
>> Make bdrv_open() take a pointer to a BDS pointer, similarly to
>> bdrv_file_open(). If a pointer to a NULL pointer is given, bdrv_open()
>> will create a new BDS with an empty name; if the BDS pointer is not
>> NULL, that existing BDS will be reused (in the same way as bdrv_open()
>> already did).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c               | 64 +++++++++++++++++++++++++++++++--------------------
>>   block/blkdebug.c      |  1 +
>>   block/blkverify.c     |  2 ++
>>   block/qcow2.c         | 14 +++++++----
>>   block/vmdk.c          |  5 ++--
>>   block/vvfat.c         |  6 ++---
>>   blockdev.c            | 20 ++++++++--------
>>   hw/block/xen_disk.c   |  2 +-
>>   include/block/block.h |  2 +-
>>   qemu-img.c            | 10 ++++----
>>   qemu-io.c             |  2 +-
>>   qemu-nbd.c            |  2 +-
>>   12 files changed, 72 insertions(+), 58 deletions(-)
>> @@ -1160,6 +1158,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>>    * BlockdevRef.
>>    *
>>    * The BlockdevRef will be removed from the options QDict.
>> + *
>> + * As with bdrv_open(), if *pbs is NULL, a new BDS will be created with a
>> + * pointer to it stored there. If it is not NULL, the referenced BDS will
>> + * be reused.
>>    */
>>   int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>>                       QDict *options, const char *bdref_key, int flags,
> There are no callers that make use of *pbs != NULL. Are you planning to
> add such users? Otherwise, we could just assert() it here instead of
> documenting behaviour that is never used.

No, currently, there aren't. Since we're planning to eventually adjust 
everything to give a pointer to a NULL pointer to these functions (i.e., 
nobody except bdrv_open() uses bdrv_new()), adding an assert() in order 
to prevent anyone from "exploiting" this in a way which would 
technically be fine but actually not what we want (i.e., reusing an 
already existing BDS), is fine with me. I'll add it and adjust the comment.

Max

Re: [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to **

[Benoît Canet]

Le Saturday 08 Feb 2014 à 18:39:12 (+0100), Max Reitz a écrit :
> Make bdrv_open() take a pointer to a BDS pointer, similarly to
> bdrv_file_open(). If a pointer to a NULL pointer is given, bdrv_open()
> will create a new BDS with an empty name; if the BDS pointer is not
> NULL, that existing BDS will be reused (in the same way as bdrv_open()
> already did).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c               | 64 +++++++++++++++++++++++++++++++--------------------
>  block/blkdebug.c      |  1 +
>  block/blkverify.c     |  2 ++
>  block/qcow2.c         | 14 +++++++----
>  block/vmdk.c          |  5 ++--
>  block/vvfat.c         |  6 ++---
>  blockdev.c            | 20 ++++++++--------
>  hw/block/xen_disk.c   |  2 +-
>  include/block/block.h |  2 +-
>  qemu-img.c            | 10 ++++----
>  qemu-io.c             |  2 +-
>  qemu-nbd.c            |  2 +-
>  12 files changed, 72 insertions(+), 58 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 636aa11..40a585a 100644
> --- a/block.c
> +++ b/block.c
> @@ -1040,7 +1040,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>      }
>  
>      if (!drv->bdrv_file_open) {
> -        ret = bdrv_open(bs, filename, options, flags, drv, &local_err);
> +        ret = bdrv_open(&bs, filename, options, flags, drv, &local_err);
>          options = NULL;
>      } else {
>          ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
> @@ -1109,8 +1109,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>                                         sizeof(backing_filename));
>      }
>  
> -    bs->backing_hd = bdrv_new("");
> -
>      if (bs->backing_format[0] != '\0') {
>          back_drv = bdrv_find_format(bs->backing_format);
>      }
> @@ -1119,11 +1117,11 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>      back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
>                                      BDRV_O_COPY_ON_READ);
>  
> -    ret = bdrv_open(bs->backing_hd,
> +    assert(bs->backing_hd == NULL);
> +    ret = bdrv_open(&bs->backing_hd,
>                      *backing_filename ? backing_filename : NULL, options,
>                      back_flags, back_drv, &local_err);
>      if (ret < 0) {
> -        bdrv_unref(bs->backing_hd);
>          bs->backing_hd = NULL;
>          bs->open_flags |= BDRV_O_NO_BACKING;
>          error_setg(errp, "Could not open backing file: %s",
> @@ -1160,6 +1158,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>   * BlockdevRef.
>   *
>   * The BlockdevRef will be removed from the options QDict.
> + *
> + * As with bdrv_open(), if *pbs is NULL, a new BDS will be created with a
> + * pointer to it stored there. If it is not NULL, the referenced BDS will
> + * be reused.
>   */
>  int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>                      QDict *options, const char *bdref_key, int flags,
> @@ -1190,8 +1192,6 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>          /* If a filename is given and the block driver should be detected
>             automatically (instead of using none), use bdrv_open() in order to do
>             that auto-detection. */
> -        BlockDriverState *bs;
> -
>          if (reference) {
>              error_setg(errp, "Cannot reference an existing block device while "
>                         "giving a filename");
> @@ -1199,13 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>              goto done;
>          }
>  
> -        bs = bdrv_new("");
> -        ret = bdrv_open(bs, filename, image_options, flags, NULL, errp);
> -        if (ret < 0) {
> -            bdrv_unref(bs);
> -        } else {
> -            *pbs = bs;
> -        }
> +        ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
>      } else {
>          ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
>                               errp);
> @@ -1223,22 +1217,32 @@ done:
>   * empty set of options. The reference to the QDict belongs to the block layer
>   * after the call (even on failure), so if the caller intends to reuse the
>   * dictionary, it needs to use QINCREF() before calling bdrv_open.
> + *
> + * If *pbs is NULL, a new BDS will be created with a pointer to it stored there.
> + * If it is not NULL, the referenced BDS will be reused.
>   */
> -int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> +int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>                int flags, BlockDriver *drv, Error **errp)
>  {
>      int ret;
>      /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
>      char tmp_filename[PATH_MAX + 1];
> -    BlockDriverState *file = NULL;
> +    BlockDriverState *file = NULL, *bs;
>      const char *drvname;
>      Error *local_err = NULL;
>  
> +    assert(pbs);
> +
> +    if (*pbs) {
> +        bs = *pbs;
> +    } else {
> +        bs = bdrv_new("");
> +    }
> +
>      /* NULL means an empty set of options */
>      if (options == NULL) {
>          options = qdict_new();
>      }
> -
nitpicking: spurious line removal.

>      bs->options = options;
>      options = qdict_clone_shallow(options);
>  
> @@ -1254,12 +1258,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>             instead of opening 'filename' directly */
>  
>          /* Get the required size from the image */
> -        bs1 = bdrv_new("");
>          QINCREF(options);
> -        ret = bdrv_open(bs1, filename, options, BDRV_O_NO_BACKING,
> +        bs1 = NULL;
> +        ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING,
>                          drv, &local_err);
>          if (ret < 0) {
> -            bdrv_unref(bs1);
>              goto fail;
>          }
>          total_size = bdrv_getlength(bs1) & BDRV_SECTOR_MASK;
> @@ -1316,6 +1319,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>          flags |= BDRV_O_ALLOW_RDWR;
>      }
>  
> +    assert(file == NULL);
>      ret = bdrv_open_image(&file, filename, options, "file",
>                            bdrv_open_flags(bs, flags | BDRV_O_UNMAP), true, true,
>                            &local_err);

is_temporary can already be set to one the goto after the bdrv_open_image should
be unlink_and_fail yet I don't know if the fix belong to this patch.

> @@ -1387,6 +1391,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>          bdrv_dev_change_media_cb(bs, true);
>      }
>  
> +    *pbs = bs;
>      return 0;
>  
>  unlink_and_fail:
> @@ -1400,13 +1405,24 @@ fail:
>      QDECREF(bs->options);
>      QDECREF(options);
>      bs->options = NULL;
> +    if (!*pbs) {
> +        /* If *pbs is NULL, a new BDS has been created in this function and
> +           needs to be freed now. Otherwise, it does not need to be closed,
> +           since it has not really been opened yet. */
> +        bdrv_unref(bs);
> +    }
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
>      }
>      return ret;
>  
>  close_and_fail:
> -    bdrv_close(bs);
> +    /* See fail path, but now the BDS has to be always closed */
> +    if (*pbs) {
> +        bdrv_close(bs);
> +    } else {
> +        bdrv_unref(bs);
> +    }
>      QDECREF(options);
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
> @@ -5288,9 +5304,8 @@ void bdrv_img_create(const char *filename, const char *fmt,
>              back_flags =
>                  flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>  
> -            bs = bdrv_new("");
> -
> -            ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags,
> +            bs = NULL;
> +            ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags,
>                              backing_drv, &local_err);
>              if (ret < 0) {
>                  error_setg_errno(errp, -ret, "Could not open '%s': %s",
> @@ -5298,7 +5313,6 @@ void bdrv_img_create(const char *filename, const char *fmt,
>                                   error_get_pretty(local_err));
>                  error_free(local_err);
>                  local_err = NULL;
> -                bdrv_unref(bs);
>                  goto out;
>              }
>              bdrv_get_geometry(bs, &size);
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 8eb0db0..053fa4c 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -410,6 +410,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>      s->state = 1;
>  
>      /* Open the backing file */
> +    assert(bs->file == NULL);
>      ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, "image",
>                            flags, true, false, &local_err);
>      if (ret < 0) {
> diff --git a/block/blkverify.c b/block/blkverify.c
> index cfcbcf4..86585e7 100644
> --- a/block/blkverify.c
> +++ b/block/blkverify.c
> @@ -135,6 +135,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>  
>      /* Open the raw file */
> +    assert(bs->file == NULL);
>      ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-raw"), options,
>                            "raw", flags, true, false, &local_err);
>      if (ret < 0) {
> @@ -143,6 +144,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>  
>      /* Open the test file */
> +    assert(s->test_file == NULL);
>      ret = bdrv_open_image(&s->test_file, qemu_opt_get(opts, "x-image"), options,
>                            "test", flags, false, false, &local_err);
>      if (ret < 0) {
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 0b4335c..c8e8ba7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1543,7 +1543,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>          goto out;
>      }
>  
> -    bdrv_close(bs);
> +    bdrv_unref(bs);
> +    bs = NULL;
>  
>      /*
>       * And now open the image and make it consistent first (i.e. increase the
> @@ -1552,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>       */
>      BlockDriver* drv = bdrv_find_format("qcow2");
>      assert(drv != NULL);
> -    ret = bdrv_open(bs, filename, NULL,
> +    ret = bdrv_open(&bs, filename, NULL,
>          BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
> @@ -1599,10 +1600,11 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>          }
>      }
>  
> -    bdrv_close(bs);
> +    bdrv_unref(bs);
> +    bs = NULL;
>  
>      /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
> -    ret = bdrv_open(bs, filename, NULL,
> +    ret = bdrv_open(&bs, filename, NULL,
>                      BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
>                      drv, &local_err);
>      if (error_is_set(&local_err)) {
> @@ -1612,7 +1614,9 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>  
>      ret = 0;
>  out:
> -    bdrv_unref(bs);
> +    if (bs) {
> +        bdrv_unref(bs);
> +    }
>      return ret;
>  }
>  
> diff --git a/block/vmdk.c b/block/vmdk.c
> index e809e2e..52cd12a 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1755,10 +1755,9 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>          goto exit;
>      }
>      if (backing_file) {
> -        BlockDriverState *bs = bdrv_new("");
> -        ret = bdrv_open(bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
> +        BlockDriverState *bs = NULL;
> +        ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
>          if (ret != 0) {
> -            bdrv_unref(bs);
>              goto exit;
>          }
>          if (strcmp(bs->drv->format_name, "vmdk")) {
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 664941c..ae7bc6f 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2936,15 +2936,13 @@ static int enable_write_target(BDRVVVFATState *s)
>          goto err;
>      }
>  
> -    s->qcow = bdrv_new("");
> -
> -    ret = bdrv_open(s->qcow, s->qcow_filename, NULL,
> +    s->qcow = NULL;
> +    ret = bdrv_open(&s->qcow, s->qcow_filename, NULL,
>              BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
>              &local_err);
>      if (ret < 0) {
>          qerror_report_err(local_err);
>          error_free(local_err);
> -        bdrv_unref(s->qcow);
>          goto err;
>      }
>  
> diff --git a/blockdev.c b/blockdev.c
> index 36ceece..6dd351c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -510,7 +510,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>      bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>  
>      QINCREF(bs_opts);
> -    ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
> +    ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
>  
>      if (ret < 0) {
>          error_setg(errp, "could not open disk image %s: %s",
> @@ -1301,12 +1301,12 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>                    qstring_from_str(snapshot_node_name));
>      }
>  
> -    /* We will manually add the backing_hd field to the bs later */
> -    state->new_bs = bdrv_new("");
>      /* TODO Inherit bs->options or only take explicit options with an
>       * extended QMP command? */
> -    ret = bdrv_open(state->new_bs, new_image_file, options,
> +    assert(state->new_bs == NULL);
> +    ret = bdrv_open(&state->new_bs, new_image_file, options,
>                      flags | BDRV_O_NO_BACKING, drv, &local_err);
> +    /* We will manually add the backing_hd field to the bs later */
>      if (ret != 0) {
>          error_propagate(errp, local_err);
>      }
> @@ -1555,7 +1555,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
>      Error *local_err = NULL;
>      int ret;
>  
> -    ret = bdrv_open(bs, filename, NULL, bdrv_flags, drv, &local_err);
> +    ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return;
> @@ -1991,10 +1991,9 @@ void qmp_drive_backup(const char *device, const char *target,
>          return;
>      }
>  
> -    target_bs = bdrv_new("");
> -    ret = bdrv_open(target_bs, target, NULL, flags, drv, &local_err);
> +    target_bs = NULL;
> +    ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
>      if (ret < 0) {
> -        bdrv_unref(target_bs);
>          error_propagate(errp, local_err);
>          return;
>      }
> @@ -2135,11 +2134,10 @@ void qmp_drive_mirror(const char *device, const char *target,
>      /* Mirroring takes care of copy-on-write using the source's backing
>       * file.
>       */
> -    target_bs = bdrv_new("");
> -    ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
> +    target_bs = NULL;
> +    ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
>                      &local_err);
>      if (ret < 0) {
> -        bdrv_unref(target_bs);
>          error_propagate(errp, local_err);
>          return;
>      }
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 098f6c6..14e6d0b 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -813,7 +813,7 @@ static int blk_connect(struct XenDevice *xendev)
>              Error *local_err = NULL;
>              BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
>                                                             readonly);
> -            if (bdrv_open(blkdev->bs,
> +            if (bdrv_open(&blkdev->bs,
>                            blkdev->filename, NULL, qflags, drv, &local_err) != 0)
>              {
>                  xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
> diff --git a/include/block/block.h b/include/block/block.h
> index 963a61f..980869d 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -190,7 +190,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>                      QDict *options, const char *bdref_key, int flags,
>                      bool force_raw, bool allow_none, Error **errp);
>  int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
> -int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> +int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>                int flags, BlockDriver *drv, Error **errp);
>  BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>                                      BlockDriverState *bs, int flags);
> diff --git a/qemu-img.c b/qemu-img.c
> index c989850..897aa56 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
>          drv = NULL;
>      }
>  
> -    ret = bdrv_open(bs, filename, NULL, flags, drv, &local_err);
> +    ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err);
>      if (ret < 0) {
>          error_report("Could not open '%s': %s", filename,
>                       error_get_pretty(local_err));
> @@ -310,9 +310,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
>      }
>      return bs;
>  fail:
> -    if (bs) {
> -        bdrv_unref(bs);
> -    }
> +    bdrv_unref(bs);
>      return NULL;
>  }
>  
> @@ -2314,7 +2312,7 @@ static int img_rebase(int argc, char **argv)
>  
>          bs_old_backing = bdrv_new("old_backing");
>          bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
> -        ret = bdrv_open(bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
> +        ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
>                          old_backing_drv, &local_err);
>          if (ret) {
>              error_report("Could not open old backing file '%s': %s",
> @@ -2324,7 +2322,7 @@ static int img_rebase(int argc, char **argv)
>          }
>          if (out_baseimg[0]) {
>              bs_new_backing = bdrv_new("new_backing");
> -            ret = bdrv_open(bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
> +            ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
>                          new_backing_drv, &local_err);
>              if (ret) {
>                  error_report("Could not open new backing file '%s': %s",
> diff --git a/qemu-io.c b/qemu-io.c
> index 7f459d8..8da8f6e 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -68,7 +68,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
>      } else {
>          qemuio_bs = bdrv_new("hda");
>  
> -        if (bdrv_open(qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
> +        if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
>              fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
>                      error_get_pretty(local_err));
>              error_free(local_err);
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 136e8c9..0cf123c 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -597,7 +597,7 @@ int main(int argc, char **argv)
>  
>      bs = bdrv_new("hda");
>      srcpath = argv[optind];
> -    ret = bdrv_open(bs, srcpath, NULL, flags, drv, &local_err);
> +    ret = bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err);
>      if (ret < 0) {
>          errno = -ret;
>          err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind],
> -- 
> 1.8.5.4
> 
> 

Re: [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to **

[Max Reitz]

On 10.02.2014 14:17, Benoît Canet wrote:
> Le Saturday 08 Feb 2014 à 18:39:12 (+0100), Max Reitz a écrit :
>> Make bdrv_open() take a pointer to a BDS pointer, similarly to
>> bdrv_file_open(). If a pointer to a NULL pointer is given, bdrv_open()
>> will create a new BDS with an empty name; if the BDS pointer is not
>> NULL, that existing BDS will be reused (in the same way as bdrv_open()
>> already did).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c               | 64 +++++++++++++++++++++++++++++++--------------------
>>   block/blkdebug.c      |  1 +
>>   block/blkverify.c     |  2 ++
>>   block/qcow2.c         | 14 +++++++----
>>   block/vmdk.c          |  5 ++--
>>   block/vvfat.c         |  6 ++---
>>   blockdev.c            | 20 ++++++++--------
>>   hw/block/xen_disk.c   |  2 +-
>>   include/block/block.h |  2 +-
>>   qemu-img.c            | 10 ++++----
>>   qemu-io.c             |  2 +-
>>   qemu-nbd.c            |  2 +-
>>   12 files changed, 72 insertions(+), 58 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 636aa11..40a585a 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1040,7 +1040,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>>       }
>>   
>>       if (!drv->bdrv_file_open) {
>> -        ret = bdrv_open(bs, filename, options, flags, drv, &local_err);
>> +        ret = bdrv_open(&bs, filename, options, flags, drv, &local_err);
>>           options = NULL;
>>       } else {
>>           ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
>> @@ -1109,8 +1109,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>>                                          sizeof(backing_filename));
>>       }
>>   
>> -    bs->backing_hd = bdrv_new("");
>> -
>>       if (bs->backing_format[0] != '\0') {
>>           back_drv = bdrv_find_format(bs->backing_format);
>>       }
>> @@ -1119,11 +1117,11 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>>       back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
>>                                       BDRV_O_COPY_ON_READ);
>>   
>> -    ret = bdrv_open(bs->backing_hd,
>> +    assert(bs->backing_hd == NULL);
>> +    ret = bdrv_open(&bs->backing_hd,
>>                       *backing_filename ? backing_filename : NULL, options,
>>                       back_flags, back_drv, &local_err);
>>       if (ret < 0) {
>> -        bdrv_unref(bs->backing_hd);
>>           bs->backing_hd = NULL;
>>           bs->open_flags |= BDRV_O_NO_BACKING;
>>           error_setg(errp, "Could not open backing file: %s",
>> @@ -1160,6 +1158,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>>    * BlockdevRef.
>>    *
>>    * The BlockdevRef will be removed from the options QDict.
>> + *
>> + * As with bdrv_open(), if *pbs is NULL, a new BDS will be created with a
>> + * pointer to it stored there. If it is not NULL, the referenced BDS will
>> + * be reused.
>>    */
>>   int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>>                       QDict *options, const char *bdref_key, int flags,
>> @@ -1190,8 +1192,6 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>>           /* If a filename is given and the block driver should be detected
>>              automatically (instead of using none), use bdrv_open() in order to do
>>              that auto-detection. */
>> -        BlockDriverState *bs;
>> -
>>           if (reference) {
>>               error_setg(errp, "Cannot reference an existing block device while "
>>                          "giving a filename");
>> @@ -1199,13 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>>               goto done;
>>           }
>>   
>> -        bs = bdrv_new("");
>> -        ret = bdrv_open(bs, filename, image_options, flags, NULL, errp);
>> -        if (ret < 0) {
>> -            bdrv_unref(bs);
>> -        } else {
>> -            *pbs = bs;
>> -        }
>> +        ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
>>       } else {
>>           ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
>>                                errp);
>> @@ -1223,22 +1217,32 @@ done:
>>    * empty set of options. The reference to the QDict belongs to the block layer
>>    * after the call (even on failure), so if the caller intends to reuse the
>>    * dictionary, it needs to use QINCREF() before calling bdrv_open.
>> + *
>> + * If *pbs is NULL, a new BDS will be created with a pointer to it stored there.
>> + * If it is not NULL, the referenced BDS will be reused.
>>    */
>> -int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>> +int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>>                 int flags, BlockDriver *drv, Error **errp)
>>   {
>>       int ret;
>>       /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
>>       char tmp_filename[PATH_MAX + 1];
>> -    BlockDriverState *file = NULL;
>> +    BlockDriverState *file = NULL, *bs;
>>       const char *drvname;
>>       Error *local_err = NULL;
>>   
>> +    assert(pbs);
>> +
>> +    if (*pbs) {
>> +        bs = *pbs;
>> +    } else {
>> +        bs = bdrv_new("");
>> +    }
>> +
>>       /* NULL means an empty set of options */
>>       if (options == NULL) {
>>           options = qdict_new();
>>       }
>> -
> nitpicking: spurious line removal.

Right, I didn't notice because I mostly compared v2 against v1. :-)

>>       bs->options = options;
>>       options = qdict_clone_shallow(options);
>>   
>> @@ -1254,12 +1258,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>>              instead of opening 'filename' directly */
>>   
>>           /* Get the required size from the image */
>> -        bs1 = bdrv_new("");
>>           QINCREF(options);
>> -        ret = bdrv_open(bs1, filename, options, BDRV_O_NO_BACKING,
>> +        bs1 = NULL;
>> +        ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING,
>>                           drv, &local_err);
>>           if (ret < 0) {
>> -            bdrv_unref(bs1);
>>               goto fail;
>>           }
>>           total_size = bdrv_getlength(bs1) & BDRV_SECTOR_MASK;
>> @@ -1316,6 +1319,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>>           flags |= BDRV_O_ALLOW_RDWR;
>>       }
>>   
>> +    assert(file == NULL);
>>       ret = bdrv_open_image(&file, filename, options, "file",
>>                             bdrv_open_flags(bs, flags | BDRV_O_UNMAP), true, true,
>>                             &local_err);
> is_temporary can already be set to one the goto after the bdrv_open_image should
> be unlink_and_fail yet I don't know if the fix belong to this patch.

Oh, right, good catch. I don't think this belongs to this series, 
though. Do you want to fix this or shall I?

Max

>> @@ -1387,6 +1391,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>>           bdrv_dev_change_media_cb(bs, true);
>>       }
>>   
>> +    *pbs = bs;
>>       return 0;
>>   
>>   unlink_and_fail:
>> @@ -1400,13 +1405,24 @@ fail:
>>       QDECREF(bs->options);
>>       QDECREF(options);
>>       bs->options = NULL;
>> +    if (!*pbs) {
>> +        /* If *pbs is NULL, a new BDS has been created in this function and
>> +           needs to be freed now. Otherwise, it does not need to be closed,
>> +           since it has not really been opened yet. */
>> +        bdrv_unref(bs);
>> +    }
>>       if (error_is_set(&local_err)) {
>>           error_propagate(errp, local_err);
>>       }
>>       return ret;
>>   
>>   close_and_fail:
>> -    bdrv_close(bs);
>> +    /* See fail path, but now the BDS has to be always closed */
>> +    if (*pbs) {
>> +        bdrv_close(bs);
>> +    } else {
>> +        bdrv_unref(bs);
>> +    }
>>       QDECREF(options);
>>       if (error_is_set(&local_err)) {
>>           error_propagate(errp, local_err);
>> @@ -5288,9 +5304,8 @@ void bdrv_img_create(const char *filename, const char *fmt,
>>               back_flags =
>>                   flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>>   
>> -            bs = bdrv_new("");
>> -
>> -            ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags,
>> +            bs = NULL;
>> +            ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags,
>>                               backing_drv, &local_err);
>>               if (ret < 0) {
>>                   error_setg_errno(errp, -ret, "Could not open '%s': %s",
>> @@ -5298,7 +5313,6 @@ void bdrv_img_create(const char *filename, const char *fmt,
>>                                    error_get_pretty(local_err));
>>                   error_free(local_err);
>>                   local_err = NULL;
>> -                bdrv_unref(bs);
>>                   goto out;
>>               }
>>               bdrv_get_geometry(bs, &size);
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 8eb0db0..053fa4c 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -410,6 +410,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>>       s->state = 1;
>>   
>>       /* Open the backing file */
>> +    assert(bs->file == NULL);
>>       ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, "image",
>>                             flags, true, false, &local_err);
>>       if (ret < 0) {
>> diff --git a/block/blkverify.c b/block/blkverify.c
>> index cfcbcf4..86585e7 100644
>> --- a/block/blkverify.c
>> +++ b/block/blkverify.c
>> @@ -135,6 +135,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
>>       }
>>   
>>       /* Open the raw file */
>> +    assert(bs->file == NULL);
>>       ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-raw"), options,
>>                             "raw", flags, true, false, &local_err);
>>       if (ret < 0) {
>> @@ -143,6 +144,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
>>       }
>>   
>>       /* Open the test file */
>> +    assert(s->test_file == NULL);
>>       ret = bdrv_open_image(&s->test_file, qemu_opt_get(opts, "x-image"), options,
>>                             "test", flags, false, false, &local_err);
>>       if (ret < 0) {
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 0b4335c..c8e8ba7 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1543,7 +1543,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>>           goto out;
>>       }
>>   
>> -    bdrv_close(bs);
>> +    bdrv_unref(bs);
>> +    bs = NULL;
>>   
>>       /*
>>        * And now open the image and make it consistent first (i.e. increase the
>> @@ -1552,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>>        */
>>       BlockDriver* drv = bdrv_find_format("qcow2");
>>       assert(drv != NULL);
>> -    ret = bdrv_open(bs, filename, NULL,
>> +    ret = bdrv_open(&bs, filename, NULL,
>>           BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err);
>>       if (ret < 0) {
>>           error_propagate(errp, local_err);
>> @@ -1599,10 +1600,11 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>>           }
>>       }
>>   
>> -    bdrv_close(bs);
>> +    bdrv_unref(bs);
>> +    bs = NULL;
>>   
>>       /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
>> -    ret = bdrv_open(bs, filename, NULL,
>> +    ret = bdrv_open(&bs, filename, NULL,
>>                       BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
>>                       drv, &local_err);
>>       if (error_is_set(&local_err)) {
>> @@ -1612,7 +1614,9 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>>   
>>       ret = 0;
>>   out:
>> -    bdrv_unref(bs);
>> +    if (bs) {
>> +        bdrv_unref(bs);
>> +    }
>>       return ret;
>>   }
>>   
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index e809e2e..52cd12a 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -1755,10 +1755,9 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>>           goto exit;
>>       }
>>       if (backing_file) {
>> -        BlockDriverState *bs = bdrv_new("");
>> -        ret = bdrv_open(bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
>> +        BlockDriverState *bs = NULL;
>> +        ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
>>           if (ret != 0) {
>> -            bdrv_unref(bs);
>>               goto exit;
>>           }
>>           if (strcmp(bs->drv->format_name, "vmdk")) {
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index 664941c..ae7bc6f 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -2936,15 +2936,13 @@ static int enable_write_target(BDRVVVFATState *s)
>>           goto err;
>>       }
>>   
>> -    s->qcow = bdrv_new("");
>> -
>> -    ret = bdrv_open(s->qcow, s->qcow_filename, NULL,
>> +    s->qcow = NULL;
>> +    ret = bdrv_open(&s->qcow, s->qcow_filename, NULL,
>>               BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
>>               &local_err);
>>       if (ret < 0) {
>>           qerror_report_err(local_err);
>>           error_free(local_err);
>> -        bdrv_unref(s->qcow);
>>           goto err;
>>       }
>>   
>> diff --git a/blockdev.c b/blockdev.c
>> index 36ceece..6dd351c 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -510,7 +510,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>>       bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>>   
>>       QINCREF(bs_opts);
>> -    ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
>> +    ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
>>   
>>       if (ret < 0) {
>>           error_setg(errp, "could not open disk image %s: %s",
>> @@ -1301,12 +1301,12 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>>                     qstring_from_str(snapshot_node_name));
>>       }
>>   
>> -    /* We will manually add the backing_hd field to the bs later */
>> -    state->new_bs = bdrv_new("");
>>       /* TODO Inherit bs->options or only take explicit options with an
>>        * extended QMP command? */
>> -    ret = bdrv_open(state->new_bs, new_image_file, options,
>> +    assert(state->new_bs == NULL);
>> +    ret = bdrv_open(&state->new_bs, new_image_file, options,
>>                       flags | BDRV_O_NO_BACKING, drv, &local_err);
>> +    /* We will manually add the backing_hd field to the bs later */
>>       if (ret != 0) {
>>           error_propagate(errp, local_err);
>>       }
>> @@ -1555,7 +1555,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
>>       Error *local_err = NULL;
>>       int ret;
>>   
>> -    ret = bdrv_open(bs, filename, NULL, bdrv_flags, drv, &local_err);
>> +    ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
>>       if (ret < 0) {
>>           error_propagate(errp, local_err);
>>           return;
>> @@ -1991,10 +1991,9 @@ void qmp_drive_backup(const char *device, const char *target,
>>           return;
>>       }
>>   
>> -    target_bs = bdrv_new("");
>> -    ret = bdrv_open(target_bs, target, NULL, flags, drv, &local_err);
>> +    target_bs = NULL;
>> +    ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
>>       if (ret < 0) {
>> -        bdrv_unref(target_bs);
>>           error_propagate(errp, local_err);
>>           return;
>>       }
>> @@ -2135,11 +2134,10 @@ void qmp_drive_mirror(const char *device, const char *target,
>>       /* Mirroring takes care of copy-on-write using the source's backing
>>        * file.
>>        */
>> -    target_bs = bdrv_new("");
>> -    ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
>> +    target_bs = NULL;
>> +    ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
>>                       &local_err);
>>       if (ret < 0) {
>> -        bdrv_unref(target_bs);
>>           error_propagate(errp, local_err);
>>           return;
>>       }
>> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
>> index 098f6c6..14e6d0b 100644
>> --- a/hw/block/xen_disk.c
>> +++ b/hw/block/xen_disk.c
>> @@ -813,7 +813,7 @@ static int blk_connect(struct XenDevice *xendev)
>>               Error *local_err = NULL;
>>               BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
>>                                                              readonly);
>> -            if (bdrv_open(blkdev->bs,
>> +            if (bdrv_open(&blkdev->bs,
>>                             blkdev->filename, NULL, qflags, drv, &local_err) != 0)
>>               {
>>                   xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 963a61f..980869d 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -190,7 +190,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>>                       QDict *options, const char *bdref_key, int flags,
>>                       bool force_raw, bool allow_none, Error **errp);
>>   int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
>> -int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>> +int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>>                 int flags, BlockDriver *drv, Error **errp);
>>   BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>>                                       BlockDriverState *bs, int flags);
>> diff --git a/qemu-img.c b/qemu-img.c
>> index c989850..897aa56 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
>>           drv = NULL;
>>       }
>>   
>> -    ret = bdrv_open(bs, filename, NULL, flags, drv, &local_err);
>> +    ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err);
>>       if (ret < 0) {
>>           error_report("Could not open '%s': %s", filename,
>>                        error_get_pretty(local_err));
>> @@ -310,9 +310,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
>>       }
>>       return bs;
>>   fail:
>> -    if (bs) {
>> -        bdrv_unref(bs);
>> -    }
>> +    bdrv_unref(bs);
>>       return NULL;
>>   }
>>   
>> @@ -2314,7 +2312,7 @@ static int img_rebase(int argc, char **argv)
>>   
>>           bs_old_backing = bdrv_new("old_backing");
>>           bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
>> -        ret = bdrv_open(bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
>> +        ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
>>                           old_backing_drv, &local_err);
>>           if (ret) {
>>               error_report("Could not open old backing file '%s': %s",
>> @@ -2324,7 +2322,7 @@ static int img_rebase(int argc, char **argv)
>>           }
>>           if (out_baseimg[0]) {
>>               bs_new_backing = bdrv_new("new_backing");
>> -            ret = bdrv_open(bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
>> +            ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
>>                           new_backing_drv, &local_err);
>>               if (ret) {
>>                   error_report("Could not open new backing file '%s': %s",
>> diff --git a/qemu-io.c b/qemu-io.c
>> index 7f459d8..8da8f6e 100644
>> --- a/qemu-io.c
>> +++ b/qemu-io.c
>> @@ -68,7 +68,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
>>       } else {
>>           qemuio_bs = bdrv_new("hda");
>>   
>> -        if (bdrv_open(qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
>> +        if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
>>               fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
>>                       error_get_pretty(local_err));
>>               error_free(local_err);
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 136e8c9..0cf123c 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -597,7 +597,7 @@ int main(int argc, char **argv)
>>   
>>       bs = bdrv_new("hda");
>>       srcpath = argv[optind];
>> -    ret = bdrv_open(bs, srcpath, NULL, flags, drv, &local_err);
>> +    ret = bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err);
>>       if (ret < 0) {
>>           errno = -ret;
>>           err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind],
>> -- 
>> 1.8.5.4
>>
>>

[Qemu-devel] [PATCH v2 2/8] block: Add reference parameter to bdrv_open()

[Max Reitz]

Allow bdrv_open() to handle references to existing block devices just as
bdrv_file_open() is already capable of.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 41 ++++++++++++++++++++++++++++++++++-------
 block/qcow2.c         |  4 ++--
 block/vmdk.c          |  3 ++-
 block/vvfat.c         |  2 +-
 blockdev.c            | 12 ++++++------
 hw/block/xen_disk.c   |  4 ++--
 include/block/block.h |  5 +++--
 qemu-img.c            |  8 ++++----
 qemu-io.c             |  4 +++-
 qemu-nbd.c            |  2 +-
 10 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/block.c b/block.c
index 40a585a..3a32c37 100644
--- a/block.c
+++ b/block.c
@@ -1040,7 +1040,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
     }
 
     if (!drv->bdrv_file_open) {
-        ret = bdrv_open(&bs, filename, options, flags, drv, &local_err);
+        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);
@@ -1119,7 +1119,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
 
     assert(bs->backing_hd == NULL);
     ret = bdrv_open(&bs->backing_hd,
-                    *backing_filename ? backing_filename : NULL, options,
+                    *backing_filename ? backing_filename : NULL, NULL, options,
                     back_flags, back_drv, &local_err);
     if (ret < 0) {
         bs->backing_hd = NULL;
@@ -1199,7 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
             goto done;
         }
 
-        ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
+        ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
     } else {
         ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
                              errp);
@@ -1221,8 +1221,9 @@ done:
  * If *pbs is NULL, a new BDS will be created with a pointer to it stored there.
  * If it is not NULL, the referenced BDS will be reused.
  */
-int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
-              int flags, BlockDriver *drv, Error **errp)
+int bdrv_open(BlockDriverState **pbs, const char *filename,
+              const char *reference, QDict *options, int flags,
+              BlockDriver *drv, Error **errp)
 {
     int ret;
     /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
@@ -1233,6 +1234,32 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
 
     assert(pbs);
 
+    if (reference) {
+        bool options_non_empty = options ? qdict_size(options) : false;
+        QDECREF(options);
+
+        if (*pbs) {
+            error_setg(errp, "Cannot reuse an existing BDS when referencing "
+                       "another block device");
+            return -EINVAL;
+        }
+
+        if (filename || options_non_empty) {
+            error_setg(errp, "Cannot reference an existing block device with "
+                       "additional options or a new filename");
+            return -EINVAL;
+        }
+
+        bs = bdrv_find(reference);
+        if (!bs) {
+            error_setg(errp, "Cannot find block device '%s'", reference);
+            return -ENODEV;
+        }
+        bdrv_ref(bs);
+        *pbs = bs;
+        return 0;
+    }
+
     if (*pbs) {
         bs = *pbs;
     } else {
@@ -1260,7 +1287,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
         /* Get the required size from the image */
         QINCREF(options);
         bs1 = NULL;
-        ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING,
+        ret = bdrv_open(&bs1, filename, NULL, options, BDRV_O_NO_BACKING,
                         drv, &local_err);
         if (ret < 0) {
             goto fail;
@@ -5305,7 +5332,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
                 flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
 
             bs = NULL;
-            ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags,
+            ret = bdrv_open(&bs, backing_file->value.s, NULL, NULL, back_flags,
                             backing_drv, &local_err);
             if (ret < 0) {
                 error_setg_errno(errp, -ret, "Could not open '%s': %s",
diff --git a/block/qcow2.c b/block/qcow2.c
index c8e8ba7..6996276 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1553,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
      */
     BlockDriver* drv = bdrv_find_format("qcow2");
     assert(drv != NULL);
-    ret = bdrv_open(&bs, filename, NULL,
+    ret = bdrv_open(&bs, filename, NULL, NULL,
         BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
@@ -1604,7 +1604,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     bs = NULL;
 
     /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
-    ret = bdrv_open(&bs, filename, NULL,
+    ret = bdrv_open(&bs, filename, NULL, NULL,
                     BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
                     drv, &local_err);
     if (error_is_set(&local_err)) {
diff --git a/block/vmdk.c b/block/vmdk.c
index 52cd12a..e007793 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1756,7 +1756,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
     }
     if (backing_file) {
         BlockDriverState *bs = NULL;
-        ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
+        ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_NO_BACKING, NULL,
+                        errp);
         if (ret != 0) {
             goto exit;
         }
diff --git a/block/vvfat.c b/block/vvfat.c
index ae7bc6f..d7a830e 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2937,7 +2937,7 @@ static int enable_write_target(BDRVVVFATState *s)
     }
 
     s->qcow = NULL;
-    ret = bdrv_open(&s->qcow, s->qcow_filename, NULL,
+    ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, NULL,
             BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
             &local_err);
     if (ret < 0) {
diff --git a/blockdev.c b/blockdev.c
index 6dd351c..7b7e349 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -510,7 +510,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
     bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
 
     QINCREF(bs_opts);
-    ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
+    ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, &error);
 
     if (ret < 0) {
         error_setg(errp, "could not open disk image %s: %s",
@@ -1304,7 +1304,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
     /* TODO Inherit bs->options or only take explicit options with an
      * extended QMP command? */
     assert(state->new_bs == NULL);
-    ret = bdrv_open(&state->new_bs, new_image_file, options,
+    ret = bdrv_open(&state->new_bs, new_image_file, NULL, options,
                     flags | BDRV_O_NO_BACKING, drv, &local_err);
     /* We will manually add the backing_hd field to the bs later */
     if (ret != 0) {
@@ -1555,7 +1555,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
     Error *local_err = NULL;
     int ret;
 
-    ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
+    ret = bdrv_open(&bs, filename, NULL, NULL, bdrv_flags, drv, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         return;
@@ -1992,7 +1992,7 @@ void qmp_drive_backup(const char *device, const char *target,
     }
 
     target_bs = NULL;
-    ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
+    ret = bdrv_open(&target_bs, target, NULL, NULL, flags, drv, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         return;
@@ -2135,8 +2135,8 @@ void qmp_drive_mirror(const char *device, const char *target,
      * file.
      */
     target_bs = NULL;
-    ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
-                    &local_err);
+    ret = bdrv_open(&target_bs, target, NULL, NULL, flags | BDRV_O_NO_BACKING,
+                    drv, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         return;
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 14e6d0b..61e6ff3 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -813,8 +813,8 @@ static int blk_connect(struct XenDevice *xendev)
             Error *local_err = NULL;
             BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
                                                            readonly);
-            if (bdrv_open(&blkdev->bs,
-                          blkdev->filename, NULL, qflags, drv, &local_err) != 0)
+            if (bdrv_open(&blkdev->bs, blkdev->filename, NULL, NULL, qflags,
+                          drv, &local_err) != 0)
             {
                 xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
                               error_get_pretty(local_err));
diff --git a/include/block/block.h b/include/block/block.h
index 980869d..a421041 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -190,8 +190,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
                     QDict *options, const char *bdref_key, int flags,
                     bool force_raw, bool allow_none, Error **errp);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
-int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
-              int flags, BlockDriver *drv, Error **errp);
+int bdrv_open(BlockDriverState **pbs, const char *filename,
+              const char *reference, QDict *options, int flags,
+              BlockDriver *drv, Error **errp);
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
                                     BlockDriverState *bs, int flags);
 int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
diff --git a/qemu-img.c b/qemu-img.c
index 897aa56..b834d52 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
         drv = NULL;
     }
 
-    ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err);
+    ret = bdrv_open(&bs, filename, NULL, NULL, flags, drv, &local_err);
     if (ret < 0) {
         error_report("Could not open '%s': %s", filename,
                      error_get_pretty(local_err));
@@ -2312,7 +2312,7 @@ static int img_rebase(int argc, char **argv)
 
         bs_old_backing = bdrv_new("old_backing");
         bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
-        ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
+        ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, BDRV_O_FLAGS,
                         old_backing_drv, &local_err);
         if (ret) {
             error_report("Could not open old backing file '%s': %s",
@@ -2322,8 +2322,8 @@ static int img_rebase(int argc, char **argv)
         }
         if (out_baseimg[0]) {
             bs_new_backing = bdrv_new("new_backing");
-            ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
-                        new_backing_drv, &local_err);
+            ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL,
+                            BDRV_O_FLAGS, new_backing_drv, &local_err);
             if (ret) {
                 error_report("Could not open new backing file '%s': %s",
                              out_baseimg, error_get_pretty(local_err));
diff --git a/qemu-io.c b/qemu-io.c
index 8da8f6e..61d54c0 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -68,7 +68,9 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
     } else {
         qemuio_bs = bdrv_new("hda");
 
-        if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
+        if (bdrv_open(&qemuio_bs, name, NULL, opts, flags, NULL, &local_err)
+            < 0)
+        {
             fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
                     error_get_pretty(local_err));
             error_free(local_err);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 0cf123c..711162c 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -597,7 +597,7 @@ int main(int argc, char **argv)
 
     bs = bdrv_new("hda");
     srcpath = argv[optind];
-    ret = bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err);
+    ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err);
     if (ret < 0) {
         errno = -ret;
         err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind],
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH v2 2/8] block: Add reference parameter to bdrv_open()

[Benoît Canet]

Le Saturday 08 Feb 2014 à 18:39:13 (+0100), Max Reitz a écrit :
> Allow bdrv_open() to handle references to existing block devices just as
> bdrv_file_open() is already capable of.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c               | 41 ++++++++++++++++++++++++++++++++++-------
>  block/qcow2.c         |  4 ++--
>  block/vmdk.c          |  3 ++-
>  block/vvfat.c         |  2 +-
>  blockdev.c            | 12 ++++++------
>  hw/block/xen_disk.c   |  4 ++--
>  include/block/block.h |  5 +++--
>  qemu-img.c            |  8 ++++----
>  qemu-io.c             |  4 +++-
>  qemu-nbd.c            |  2 +-
>  10 files changed, 58 insertions(+), 27 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 40a585a..3a32c37 100644
> --- a/block.c
> +++ b/block.c
> @@ -1040,7 +1040,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>      }
>  
>      if (!drv->bdrv_file_open) {
> -        ret = bdrv_open(&bs, filename, options, flags, drv, &local_err);
> +        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);
> @@ -1119,7 +1119,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>  
>      assert(bs->backing_hd == NULL);
>      ret = bdrv_open(&bs->backing_hd,
> -                    *backing_filename ? backing_filename : NULL, options,
> +                    *backing_filename ? backing_filename : NULL, NULL, options,
>                      back_flags, back_drv, &local_err);
>      if (ret < 0) {
>          bs->backing_hd = NULL;
> @@ -1199,7 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>              goto done;
>          }
>  
> -        ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
> +        ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
>      } else {
>          ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
>                               errp);
> @@ -1221,8 +1221,9 @@ done:
>   * If *pbs is NULL, a new BDS will be created with a pointer to it stored there.
>   * If it is not NULL, the referenced BDS will be reused.
>   */
> -int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
> -              int flags, BlockDriver *drv, Error **errp)
> +int bdrv_open(BlockDriverState **pbs, const char *filename,
> +              const char *reference, QDict *options, int flags,
> +              BlockDriver *drv, Error **errp)
Maybe a little reference to the reference parameters in the comments would help
?

Aside from that:

Reviewed-by: Benoit Canet <benoit@irqsave.net>

>  {
>      int ret;
>      /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> @@ -1233,6 +1234,32 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>  
>      assert(pbs);
>  
> +    if (reference) {
> +        bool options_non_empty = options ? qdict_size(options) : false;
> +        QDECREF(options);
> +
> +        if (*pbs) {
> +            error_setg(errp, "Cannot reuse an existing BDS when referencing "
> +                       "another block device");
> +            return -EINVAL;
> +        }
> +
> +        if (filename || options_non_empty) {
> +            error_setg(errp, "Cannot reference an existing block device with "
> +                       "additional options or a new filename");
> +            return -EINVAL;
> +        }
> +
> +        bs = bdrv_find(reference);
> +        if (!bs) {
> +            error_setg(errp, "Cannot find block device '%s'", reference);
> +            return -ENODEV;
> +        }
> +        bdrv_ref(bs);
> +        *pbs = bs;
> +        return 0;
> +    }
> +
>      if (*pbs) {
>          bs = *pbs;
>      } else {
> @@ -1260,7 +1287,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>          /* Get the required size from the image */
>          QINCREF(options);
>          bs1 = NULL;
> -        ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING,
> +        ret = bdrv_open(&bs1, filename, NULL, options, BDRV_O_NO_BACKING,
>                          drv, &local_err);
>          if (ret < 0) {
>              goto fail;
> @@ -5305,7 +5332,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>                  flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>  
>              bs = NULL;
> -            ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags,
> +            ret = bdrv_open(&bs, backing_file->value.s, NULL, NULL, back_flags,
>                              backing_drv, &local_err);
>              if (ret < 0) {
>                  error_setg_errno(errp, -ret, "Could not open '%s': %s",
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c8e8ba7..6996276 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1553,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>       */
>      BlockDriver* drv = bdrv_find_format("qcow2");
>      assert(drv != NULL);
> -    ret = bdrv_open(&bs, filename, NULL,
> +    ret = bdrv_open(&bs, filename, NULL, NULL,
>          BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
> @@ -1604,7 +1604,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>      bs = NULL;
>  
>      /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
> -    ret = bdrv_open(&bs, filename, NULL,
> +    ret = bdrv_open(&bs, filename, NULL, NULL,
>                      BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
>                      drv, &local_err);
>      if (error_is_set(&local_err)) {
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 52cd12a..e007793 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1756,7 +1756,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>      }
>      if (backing_file) {
>          BlockDriverState *bs = NULL;
> -        ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
> +        ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_NO_BACKING, NULL,
> +                        errp);
>          if (ret != 0) {
>              goto exit;
>          }
> diff --git a/block/vvfat.c b/block/vvfat.c
> index ae7bc6f..d7a830e 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2937,7 +2937,7 @@ static int enable_write_target(BDRVVVFATState *s)
>      }
>  
>      s->qcow = NULL;
> -    ret = bdrv_open(&s->qcow, s->qcow_filename, NULL,
> +    ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, NULL,
>              BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
>              &local_err);
>      if (ret < 0) {
> diff --git a/blockdev.c b/blockdev.c
> index 6dd351c..7b7e349 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -510,7 +510,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>      bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>  
>      QINCREF(bs_opts);
> -    ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
> +    ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, &error);
>  
>      if (ret < 0) {
>          error_setg(errp, "could not open disk image %s: %s",
> @@ -1304,7 +1304,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>      /* TODO Inherit bs->options or only take explicit options with an
>       * extended QMP command? */
>      assert(state->new_bs == NULL);
> -    ret = bdrv_open(&state->new_bs, new_image_file, options,
> +    ret = bdrv_open(&state->new_bs, new_image_file, NULL, options,
>                      flags | BDRV_O_NO_BACKING, drv, &local_err);
>      /* We will manually add the backing_hd field to the bs later */
>      if (ret != 0) {
> @@ -1555,7 +1555,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
>      Error *local_err = NULL;
>      int ret;
>  
> -    ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
> +    ret = bdrv_open(&bs, filename, NULL, NULL, bdrv_flags, drv, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return;
> @@ -1992,7 +1992,7 @@ void qmp_drive_backup(const char *device, const char *target,
>      }
>  
>      target_bs = NULL;
> -    ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
> +    ret = bdrv_open(&target_bs, target, NULL, NULL, flags, drv, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return;
> @@ -2135,8 +2135,8 @@ void qmp_drive_mirror(const char *device, const char *target,
>       * file.
>       */
>      target_bs = NULL;
> -    ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
> -                    &local_err);
> +    ret = bdrv_open(&target_bs, target, NULL, NULL, flags | BDRV_O_NO_BACKING,
> +                    drv, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return;
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 14e6d0b..61e6ff3 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -813,8 +813,8 @@ static int blk_connect(struct XenDevice *xendev)
>              Error *local_err = NULL;
>              BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
>                                                             readonly);
> -            if (bdrv_open(&blkdev->bs,
> -                          blkdev->filename, NULL, qflags, drv, &local_err) != 0)
> +            if (bdrv_open(&blkdev->bs, blkdev->filename, NULL, NULL, qflags,
> +                          drv, &local_err) != 0)
>              {
>                  xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
>                                error_get_pretty(local_err));
> diff --git a/include/block/block.h b/include/block/block.h
> index 980869d..a421041 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -190,8 +190,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>                      QDict *options, const char *bdref_key, int flags,
>                      bool force_raw, bool allow_none, Error **errp);
>  int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
> -int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
> -              int flags, BlockDriver *drv, Error **errp);
> +int bdrv_open(BlockDriverState **pbs, const char *filename,
> +              const char *reference, QDict *options, int flags,
> +              BlockDriver *drv, Error **errp);
>  BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>                                      BlockDriverState *bs, int flags);
>  int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
> diff --git a/qemu-img.c b/qemu-img.c
> index 897aa56..b834d52 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
>          drv = NULL;
>      }
>  
> -    ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err);
> +    ret = bdrv_open(&bs, filename, NULL, NULL, flags, drv, &local_err);
>      if (ret < 0) {
>          error_report("Could not open '%s': %s", filename,
>                       error_get_pretty(local_err));
> @@ -2312,7 +2312,7 @@ static int img_rebase(int argc, char **argv)
>  
>          bs_old_backing = bdrv_new("old_backing");
>          bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
> -        ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
> +        ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, BDRV_O_FLAGS,
>                          old_backing_drv, &local_err);
>          if (ret) {
>              error_report("Could not open old backing file '%s': %s",
> @@ -2322,8 +2322,8 @@ static int img_rebase(int argc, char **argv)
>          }
>          if (out_baseimg[0]) {
>              bs_new_backing = bdrv_new("new_backing");
> -            ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
> -                        new_backing_drv, &local_err);
> +            ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL,
> +                            BDRV_O_FLAGS, new_backing_drv, &local_err);
>              if (ret) {
>                  error_report("Could not open new backing file '%s': %s",
>                               out_baseimg, error_get_pretty(local_err));
> diff --git a/qemu-io.c b/qemu-io.c
> index 8da8f6e..61d54c0 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -68,7 +68,9 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
>      } else {
>          qemuio_bs = bdrv_new("hda");
>  
> -        if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
> +        if (bdrv_open(&qemuio_bs, name, NULL, opts, flags, NULL, &local_err)
> +            < 0)
> +        {
>              fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
>                      error_get_pretty(local_err));
>              error_free(local_err);
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 0cf123c..711162c 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -597,7 +597,7 @@ int main(int argc, char **argv)
>  
>      bs = bdrv_new("hda");
>      srcpath = argv[optind];
> -    ret = bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err);
> +    ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err);
>      if (ret < 0) {
>          errno = -ret;
>          err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind],
> -- 
> 1.8.5.4
> 
> 

Re: [Qemu-devel] [PATCH v2 2/8] block: Add reference parameter to bdrv_open()

[Max Reitz]

On 10.02.2014 14:30, Benoît Canet wrote:
> Le Saturday 08 Feb 2014 à 18:39:13 (+0100), Max Reitz a écrit :
>> Allow bdrv_open() to handle references to existing block devices just as
>> bdrv_file_open() is already capable of.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c               | 41 ++++++++++++++++++++++++++++++++++-------
>>   block/qcow2.c         |  4 ++--
>>   block/vmdk.c          |  3 ++-
>>   block/vvfat.c         |  2 +-
>>   blockdev.c            | 12 ++++++------
>>   hw/block/xen_disk.c   |  4 ++--
>>   include/block/block.h |  5 +++--
>>   qemu-img.c            |  8 ++++----
>>   qemu-io.c             |  4 +++-
>>   qemu-nbd.c            |  2 +-
>>   10 files changed, 58 insertions(+), 27 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 40a585a..3a32c37 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1040,7 +1040,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>>       }
>>   
>>       if (!drv->bdrv_file_open) {
>> -        ret = bdrv_open(&bs, filename, options, flags, drv, &local_err);
>> +        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);
>> @@ -1119,7 +1119,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>>   
>>       assert(bs->backing_hd == NULL);
>>       ret = bdrv_open(&bs->backing_hd,
>> -                    *backing_filename ? backing_filename : NULL, options,
>> +                    *backing_filename ? backing_filename : NULL, NULL, options,
>>                       back_flags, back_drv, &local_err);
>>       if (ret < 0) {
>>           bs->backing_hd = NULL;
>> @@ -1199,7 +1199,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>>               goto done;
>>           }
>>   
>> -        ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
>> +        ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
>>       } else {
>>           ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
>>                                errp);
>> @@ -1221,8 +1221,9 @@ done:
>>    * If *pbs is NULL, a new BDS will be created with a pointer to it stored there.
>>    * If it is not NULL, the referenced BDS will be reused.
>>    */
>> -int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>> -              int flags, BlockDriver *drv, Error **errp)
>> +int bdrv_open(BlockDriverState **pbs, const char *filename,
>> +              const char *reference, QDict *options, int flags,
>> +              BlockDriver *drv, Error **errp)
> Maybe a little reference to the reference parameters in the comments would help
> ?

Yes, that would probably be helpful.

Max

> Aside from that:
>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
>
>>   {
>>       int ret;
>>       /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
>> @@ -1233,6 +1234,32 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>>   
>>       assert(pbs);
>>   
>> +    if (reference) {
>> +        bool options_non_empty = options ? qdict_size(options) : false;
>> +        QDECREF(options);
>> +
>> +        if (*pbs) {
>> +            error_setg(errp, "Cannot reuse an existing BDS when referencing "
>> +                       "another block device");
>> +            return -EINVAL;
>> +        }
>> +
>> +        if (filename || options_non_empty) {
>> +            error_setg(errp, "Cannot reference an existing block device with "
>> +                       "additional options or a new filename");
>> +            return -EINVAL;
>> +        }
>> +
>> +        bs = bdrv_find(reference);
>> +        if (!bs) {
>> +            error_setg(errp, "Cannot find block device '%s'", reference);
>> +            return -ENODEV;
>> +        }
>> +        bdrv_ref(bs);
>> +        *pbs = bs;
>> +        return 0;
>> +    }
>> +
>>       if (*pbs) {
>>           bs = *pbs;
>>       } else {
>> @@ -1260,7 +1287,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>>           /* Get the required size from the image */
>>           QINCREF(options);
>>           bs1 = NULL;
>> -        ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING,
>> +        ret = bdrv_open(&bs1, filename, NULL, options, BDRV_O_NO_BACKING,
>>                           drv, &local_err);
>>           if (ret < 0) {
>>               goto fail;
>> @@ -5305,7 +5332,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>>                   flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>>   
>>               bs = NULL;
>> -            ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags,
>> +            ret = bdrv_open(&bs, backing_file->value.s, NULL, NULL, back_flags,
>>                               backing_drv, &local_err);
>>               if (ret < 0) {
>>                   error_setg_errno(errp, -ret, "Could not open '%s': %s",
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index c8e8ba7..6996276 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1553,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>>        */
>>       BlockDriver* drv = bdrv_find_format("qcow2");
>>       assert(drv != NULL);
>> -    ret = bdrv_open(&bs, filename, NULL,
>> +    ret = bdrv_open(&bs, filename, NULL, NULL,
>>           BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err);
>>       if (ret < 0) {
>>           error_propagate(errp, local_err);
>> @@ -1604,7 +1604,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>>       bs = NULL;
>>   
>>       /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
>> -    ret = bdrv_open(&bs, filename, NULL,
>> +    ret = bdrv_open(&bs, filename, NULL, NULL,
>>                       BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
>>                       drv, &local_err);
>>       if (error_is_set(&local_err)) {
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 52cd12a..e007793 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -1756,7 +1756,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>>       }
>>       if (backing_file) {
>>           BlockDriverState *bs = NULL;
>> -        ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
>> +        ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_NO_BACKING, NULL,
>> +                        errp);
>>           if (ret != 0) {
>>               goto exit;
>>           }
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index ae7bc6f..d7a830e 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -2937,7 +2937,7 @@ static int enable_write_target(BDRVVVFATState *s)
>>       }
>>   
>>       s->qcow = NULL;
>> -    ret = bdrv_open(&s->qcow, s->qcow_filename, NULL,
>> +    ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, NULL,
>>               BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
>>               &local_err);
>>       if (ret < 0) {
>> diff --git a/blockdev.c b/blockdev.c
>> index 6dd351c..7b7e349 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -510,7 +510,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>>       bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>>   
>>       QINCREF(bs_opts);
>> -    ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
>> +    ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, &error);
>>   
>>       if (ret < 0) {
>>           error_setg(errp, "could not open disk image %s: %s",
>> @@ -1304,7 +1304,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>>       /* TODO Inherit bs->options or only take explicit options with an
>>        * extended QMP command? */
>>       assert(state->new_bs == NULL);
>> -    ret = bdrv_open(&state->new_bs, new_image_file, options,
>> +    ret = bdrv_open(&state->new_bs, new_image_file, NULL, options,
>>                       flags | BDRV_O_NO_BACKING, drv, &local_err);
>>       /* We will manually add the backing_hd field to the bs later */
>>       if (ret != 0) {
>> @@ -1555,7 +1555,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
>>       Error *local_err = NULL;
>>       int ret;
>>   
>> -    ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
>> +    ret = bdrv_open(&bs, filename, NULL, NULL, bdrv_flags, drv, &local_err);
>>       if (ret < 0) {
>>           error_propagate(errp, local_err);
>>           return;
>> @@ -1992,7 +1992,7 @@ void qmp_drive_backup(const char *device, const char *target,
>>       }
>>   
>>       target_bs = NULL;
>> -    ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
>> +    ret = bdrv_open(&target_bs, target, NULL, NULL, flags, drv, &local_err);
>>       if (ret < 0) {
>>           error_propagate(errp, local_err);
>>           return;
>> @@ -2135,8 +2135,8 @@ void qmp_drive_mirror(const char *device, const char *target,
>>        * file.
>>        */
>>       target_bs = NULL;
>> -    ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
>> -                    &local_err);
>> +    ret = bdrv_open(&target_bs, target, NULL, NULL, flags | BDRV_O_NO_BACKING,
>> +                    drv, &local_err);
>>       if (ret < 0) {
>>           error_propagate(errp, local_err);
>>           return;
>> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
>> index 14e6d0b..61e6ff3 100644
>> --- a/hw/block/xen_disk.c
>> +++ b/hw/block/xen_disk.c
>> @@ -813,8 +813,8 @@ static int blk_connect(struct XenDevice *xendev)
>>               Error *local_err = NULL;
>>               BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
>>                                                              readonly);
>> -            if (bdrv_open(&blkdev->bs,
>> -                          blkdev->filename, NULL, qflags, drv, &local_err) != 0)
>> +            if (bdrv_open(&blkdev->bs, blkdev->filename, NULL, NULL, qflags,
>> +                          drv, &local_err) != 0)
>>               {
>>                   xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
>>                                 error_get_pretty(local_err));
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 980869d..a421041 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -190,8 +190,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>>                       QDict *options, const char *bdref_key, int flags,
>>                       bool force_raw, bool allow_none, Error **errp);
>>   int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
>> -int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>> -              int flags, BlockDriver *drv, Error **errp);
>> +int bdrv_open(BlockDriverState **pbs, const char *filename,
>> +              const char *reference, QDict *options, int flags,
>> +              BlockDriver *drv, Error **errp);
>>   BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>>                                       BlockDriverState *bs, int flags);
>>   int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 897aa56..b834d52 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
>>           drv = NULL;
>>       }
>>   
>> -    ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err);
>> +    ret = bdrv_open(&bs, filename, NULL, NULL, flags, drv, &local_err);
>>       if (ret < 0) {
>>           error_report("Could not open '%s': %s", filename,
>>                        error_get_pretty(local_err));
>> @@ -2312,7 +2312,7 @@ static int img_rebase(int argc, char **argv)
>>   
>>           bs_old_backing = bdrv_new("old_backing");
>>           bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
>> -        ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
>> +        ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, BDRV_O_FLAGS,
>>                           old_backing_drv, &local_err);
>>           if (ret) {
>>               error_report("Could not open old backing file '%s': %s",
>> @@ -2322,8 +2322,8 @@ static int img_rebase(int argc, char **argv)
>>           }
>>           if (out_baseimg[0]) {
>>               bs_new_backing = bdrv_new("new_backing");
>> -            ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
>> -                        new_backing_drv, &local_err);
>> +            ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL,
>> +                            BDRV_O_FLAGS, new_backing_drv, &local_err);
>>               if (ret) {
>>                   error_report("Could not open new backing file '%s': %s",
>>                                out_baseimg, error_get_pretty(local_err));
>> diff --git a/qemu-io.c b/qemu-io.c
>> index 8da8f6e..61d54c0 100644
>> --- a/qemu-io.c
>> +++ b/qemu-io.c
>> @@ -68,7 +68,9 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
>>       } else {
>>           qemuio_bs = bdrv_new("hda");
>>   
>> -        if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
>> +        if (bdrv_open(&qemuio_bs, name, NULL, opts, flags, NULL, &local_err)
>> +            < 0)
>> +        {
>>               fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
>>                       error_get_pretty(local_err));
>>               error_free(local_err);
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 0cf123c..711162c 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -597,7 +597,7 @@ int main(int argc, char **argv)
>>   
>>       bs = bdrv_new("hda");
>>       srcpath = argv[optind];
>> -    ret = bdrv_open(&bs, srcpath, NULL, flags, drv, &local_err);
>> +    ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err);
>>       if (ret < 0) {
>>           errno = -ret;
>>           err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind],
>> -- 
>> 1.8.5.4
>>
>>

[Qemu-devel] [PATCH v2 3/8] block: Make bdrv_file_open() static

[Max Reitz]

Add the bdrv_open() option BDRV_O_PROTOCOL which results in passing the
call to bdrv_file_open(). Additionally, make bdrv_file_open() static and
therefore bdrv_open() the only way to call it.

Consequently, all existing calls to bdrv_file_open() have to be adjusted
to use bdrv_open() with the BDRV_O_PROTOCOL flag instead.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 17 ++++++++++++-----
 block/cow.c           |  6 +++---
 block/qcow.c          |  6 +++---
 block/qcow2.c         |  5 +++--
 block/qed.c           |  5 +++--
 block/sheepdog.c      |  8 +++++---
 block/vhdx.c          |  5 +++--
 block/vmdk.c          | 11 +++++++----
 include/block/block.h |  6 +++---
 qemu-io.c             |  4 +++-
 10 files changed, 45 insertions(+), 28 deletions(-)

diff --git a/block.c b/block.c
index 3a32c37..432d659 100644
--- a/block.c
+++ b/block.c
@@ -953,9 +953,9 @@ free_and_fail:
  * after the call (even on failure), so if the caller intends to reuse the
  * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
  */
-int bdrv_file_open(BlockDriverState **pbs, const char *filename,
-                   const char *reference, QDict *options, int flags,
-                   Error **errp)
+static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
+                          const char *reference, QDict *options, int flags,
+                          Error **errp)
 {
     BlockDriverState *bs = NULL;
     BlockDriver *drv;
@@ -1201,8 +1201,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
 
         ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
     } else {
-        ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
-                             errp);
+        *pbs = NULL;
+        ret = bdrv_open(pbs, filename, reference, image_options,
+                        flags | BDRV_O_PROTOCOL, NULL, errp);
     }
 
 done:
@@ -1234,6 +1235,12 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
 
     assert(pbs);
 
+    if (flags & BDRV_O_PROTOCOL) {
+        assert(!drv);
+        return bdrv_file_open(pbs, filename, reference, options,
+                              flags & ~BDRV_O_PROTOCOL, errp);
+    }
+
     if (reference) {
         bool options_non_empty = options ? qdict_size(options) : false;
         QDECREF(options);
diff --git a/block/cow.c b/block/cow.c
index 7fc0b12..f0748a2 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -332,7 +332,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
     const char *image_filename = NULL;
     Error *local_err = NULL;
     int ret;
-    BlockDriverState *cow_bs;
+    BlockDriverState *cow_bs = NULL;
 
     /* Read out options */
     while (options && options->name) {
@@ -351,8 +351,8 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
         return ret;
     }
 
-    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
-                         &local_err);
+    ret = bdrv_open(&cow_bs, filename, NULL, NULL,
+                    BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
     if (ret < 0) {
         qerror_report_err(local_err);
         error_free(local_err);
diff --git a/block/qcow.c b/block/qcow.c
index 948b0c5..4881d84 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -670,7 +670,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
     int flags = 0;
     Error *local_err = NULL;
     int ret;
-    BlockDriverState *qcow_bs;
+    BlockDriverState *qcow_bs = NULL;
 
     /* Read out options */
     while (options && options->name) {
@@ -691,8 +691,8 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
         return ret;
     }
 
-    ret = bdrv_file_open(&qcow_bs, filename, NULL, NULL, BDRV_O_RDWR,
-                         &local_err);
+    ret = bdrv_open(&qcow_bs, filename, NULL, NULL,
+                    BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
     if (ret < 0) {
         qerror_report_err(local_err);
         error_free(local_err);
diff --git a/block/qcow2.c b/block/qcow2.c
index 6996276..bd72172 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1481,7 +1481,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
      * 2 GB for 64k clusters, and we don't want to have a 2 GB initial file
      * size for any qcow2 image.
      */
-    BlockDriverState* bs;
+    BlockDriverState *bs = NULL;
     QCowHeader *header;
     uint8_t* refcount_table;
     Error *local_err = NULL;
@@ -1493,7 +1493,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         return ret;
     }
 
-    ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
+    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+                    NULL, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         return ret;
diff --git a/block/qed.c b/block/qed.c
index b9ca7ac..33fbce1 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -571,8 +571,9 @@ static int qed_create(const char *filename, uint32_t cluster_size,
         return ret;
     }
 
-    ret = bdrv_file_open(&bs, filename, NULL, NULL,
-                         BDRV_O_RDWR | BDRV_O_CACHE_WB, &local_err);
+    ret = bdrv_open(&bs, filename, NULL, NULL,
+                    BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL, NULL,
+                    &local_err);
     if (ret < 0) {
         qerror_report_err(local_err);
         error_free(local_err);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 672b9c9..6f2ec57 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1534,7 +1534,8 @@ static int sd_prealloc(const char *filename)
     Error *local_err = NULL;
     int ret;
 
-    ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
+    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+                    NULL, &local_err);
     if (ret < 0) {
         qerror_report_err(local_err);
         error_free(local_err);
@@ -1683,7 +1684,7 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
     }
 
     if (backing_file) {
-        BlockDriverState *bs;
+        BlockDriverState *bs = NULL;
         BDRVSheepdogState *base;
         BlockDriver *drv;
 
@@ -1695,7 +1696,8 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
             goto out;
         }
 
-        ret = bdrv_file_open(&bs, backing_file, NULL, NULL, 0, &local_err);
+        ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_PROTOCOL, NULL,
+                        &local_err);
         if (ret < 0) {
             qerror_report_err(local_err);
             error_free(local_err);
diff --git a/block/vhdx.c b/block/vhdx.c
index 55689cf..38d949c 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1724,7 +1724,7 @@ static int vhdx_create(const char *filename, QEMUOptionParameter *options,
 
     gunichar2 *creator = NULL;
     glong creator_items;
-    BlockDriverState *bs;
+    BlockDriverState *bs = NULL;
     const char *type = NULL;
     VHDXImageType image_type;
     Error *local_err = NULL;
@@ -1797,7 +1797,8 @@ static int vhdx_create(const char *filename, QEMUOptionParameter *options,
         goto exit;
     }
 
-    ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
+    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+                    NULL, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         goto exit;
diff --git a/block/vmdk.c b/block/vmdk.c
index e007793..70bf011 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -776,8 +776,9 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
 
         path_combine(extent_path, sizeof(extent_path),
                 desc_file_path, fname);
-        ret = bdrv_file_open(&extent_file, extent_path, NULL, NULL,
-                             bs->open_flags, errp);
+        extent_file = NULL;
+        ret = bdrv_open(&extent_file, extent_path, NULL, NULL,
+                        bs->open_flags | BDRV_O_PROTOCOL, NULL, errp);
         if (ret) {
             return ret;
         }
@@ -1493,7 +1494,8 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
         goto exit;
     }
 
-    ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
+    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+                    NULL, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         goto exit;
@@ -1831,7 +1833,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
             goto exit;
         }
     }
-    ret = bdrv_file_open(&new_bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
+    ret = bdrv_open(&new_bs, filename, NULL, NULL,
+                    BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not write description");
         goto exit;
diff --git a/include/block/block.h b/include/block/block.h
index a421041..bf78db5 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -102,6 +102,9 @@ typedef enum {
 #define BDRV_O_CHECK       0x1000  /* open solely for consistency check */
 #define BDRV_O_ALLOW_RDWR  0x2000  /* allow reopen to change from r/o to r/w */
 #define BDRV_O_UNMAP       0x4000  /* execute guest UNMAP/TRIM operations */
+#define BDRV_O_PROTOCOL    0x8000  /* if no block driver is explicitly given:
+                                      select an appropriate protocol driver,
+                                      ignoring the format layer */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
 
@@ -183,9 +186,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
 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,
-                   const char *reference, QDict *options, int flags,
-                   Error **errp);
 int bdrv_open_image(BlockDriverState **pbs, const char *filename,
                     QDict *options, const char *bdref_key, int flags,
                     bool force_raw, bool allow_none, Error **errp);
diff --git a/qemu-io.c b/qemu-io.c
index 61d54c0..71d7806 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -59,7 +59,9 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
     }
 
     if (growable) {
-        if (bdrv_file_open(&qemuio_bs, name, NULL, opts, flags, &local_err)) {
+        if (bdrv_open(&qemuio_bs, name, NULL, opts, flags | BDRV_O_PROTOCOL,
+                      NULL, &local_err))
+        {
             fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
                     error_get_pretty(local_err));
             error_free(local_err);
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH v2 3/8] block: Make bdrv_file_open() static

[Benoît Canet]

Le Saturday 08 Feb 2014 à 18:39:14 (+0100), Max Reitz a écrit :
> Add the bdrv_open() option BDRV_O_PROTOCOL which results in passing the
> call to bdrv_file_open(). Additionally, make bdrv_file_open() static and
> therefore bdrv_open() the only way to call it.
> 
> Consequently, all existing calls to bdrv_file_open() have to be adjusted
> to use bdrv_open() with the BDRV_O_PROTOCOL flag instead.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c               | 17 ++++++++++++-----
>  block/cow.c           |  6 +++---
>  block/qcow.c          |  6 +++---
>  block/qcow2.c         |  5 +++--
>  block/qed.c           |  5 +++--
>  block/sheepdog.c      |  8 +++++---
>  block/vhdx.c          |  5 +++--
>  block/vmdk.c          | 11 +++++++----
>  include/block/block.h |  6 +++---
>  qemu-io.c             |  4 +++-
>  10 files changed, 45 insertions(+), 28 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 3a32c37..432d659 100644
> --- a/block.c
> +++ b/block.c
> @@ -953,9 +953,9 @@ free_and_fail:
>   * after the call (even on failure), so if the caller intends to reuse the
>   * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
>   */
> -int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> -                   const char *reference, QDict *options, int flags,
> -                   Error **errp)
> +static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> +                          const char *reference, QDict *options, int flags,
> +                          Error **errp)
>  {
>      BlockDriverState *bs = NULL;
>      BlockDriver *drv;
> @@ -1201,8 +1201,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>  
>          ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
>      } else {
> -        ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
> -                             errp);
> +        *pbs = NULL;
> +        ret = bdrv_open(pbs, filename, reference, image_options,
> +                        flags | BDRV_O_PROTOCOL, NULL, errp);
>      }
>  
>  done:
> @@ -1234,6 +1235,12 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>  
>      assert(pbs);
>  
> +    if (flags & BDRV_O_PROTOCOL) {
> +        assert(!drv);
> +        return bdrv_file_open(pbs, filename, reference, options,
> +                              flags & ~BDRV_O_PROTOCOL, errp);
> +    }
> +
>      if (reference) {
>          bool options_non_empty = options ? qdict_size(options) : false;
>          QDECREF(options);
> diff --git a/block/cow.c b/block/cow.c
> index 7fc0b12..f0748a2 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -332,7 +332,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
>      const char *image_filename = NULL;
>      Error *local_err = NULL;
>      int ret;
> -    BlockDriverState *cow_bs;
> +    BlockDriverState *cow_bs = NULL;
>  
>      /* Read out options */
>      while (options && options->name) {
> @@ -351,8 +351,8 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
>          return ret;
>      }
>  
> -    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
> -                         &local_err);
> +    ret = bdrv_open(&cow_bs, filename, NULL, NULL,
> +                    BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
>      if (ret < 0) {
>          qerror_report_err(local_err);
>          error_free(local_err);
> diff --git a/block/qcow.c b/block/qcow.c
> index 948b0c5..4881d84 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -670,7 +670,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
>      int flags = 0;
>      Error *local_err = NULL;
>      int ret;
> -    BlockDriverState *qcow_bs;
> +    BlockDriverState *qcow_bs = NULL;
>  
>      /* Read out options */
>      while (options && options->name) {
> @@ -691,8 +691,8 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
>          return ret;
>      }
>  
> -    ret = bdrv_file_open(&qcow_bs, filename, NULL, NULL, BDRV_O_RDWR,
> -                         &local_err);
> +    ret = bdrv_open(&qcow_bs, filename, NULL, NULL,
> +                    BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
>      if (ret < 0) {
>          qerror_report_err(local_err);
>          error_free(local_err);
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6996276..bd72172 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1481,7 +1481,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>       * 2 GB for 64k clusters, and we don't want to have a 2 GB initial file
>       * size for any qcow2 image.
>       */
> -    BlockDriverState* bs;
> +    BlockDriverState *bs = NULL;
>      QCowHeader *header;
>      uint8_t* refcount_table;
>      Error *local_err = NULL;
> @@ -1493,7 +1493,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>          return ret;
>      }
>  
> -    ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
> +    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
> +                    NULL, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return ret;
> diff --git a/block/qed.c b/block/qed.c
> index b9ca7ac..33fbce1 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -571,8 +571,9 @@ static int qed_create(const char *filename, uint32_t cluster_size,
>          return ret;
>      }
>  
> -    ret = bdrv_file_open(&bs, filename, NULL, NULL,
> -                         BDRV_O_RDWR | BDRV_O_CACHE_WB, &local_err);
> +    ret = bdrv_open(&bs, filename, NULL, NULL,
> +                    BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL, NULL,
> +                    &local_err);
>      if (ret < 0) {
>          qerror_report_err(local_err);
>          error_free(local_err);
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 672b9c9..6f2ec57 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1534,7 +1534,8 @@ static int sd_prealloc(const char *filename)
>      Error *local_err = NULL;
>      int ret;
>  
> -    ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
> +    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
> +                    NULL, &local_err);
>      if (ret < 0) {
>          qerror_report_err(local_err);
>          error_free(local_err);
> @@ -1683,7 +1684,7 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
>      }
>  
>      if (backing_file) {
> -        BlockDriverState *bs;
> +        BlockDriverState *bs = NULL;
>          BDRVSheepdogState *base;
>          BlockDriver *drv;
>  
> @@ -1695,7 +1696,8 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
>              goto out;
>          }
>  
> -        ret = bdrv_file_open(&bs, backing_file, NULL, NULL, 0, &local_err);
> +        ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_PROTOCOL, NULL,
> +                        &local_err);
>          if (ret < 0) {
>              qerror_report_err(local_err);
>              error_free(local_err);
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 55689cf..38d949c 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1724,7 +1724,7 @@ static int vhdx_create(const char *filename, QEMUOptionParameter *options,
>  
>      gunichar2 *creator = NULL;
>      glong creator_items;
> -    BlockDriverState *bs;
> +    BlockDriverState *bs = NULL;
>      const char *type = NULL;
>      VHDXImageType image_type;
>      Error *local_err = NULL;
> @@ -1797,7 +1797,8 @@ static int vhdx_create(const char *filename, QEMUOptionParameter *options,
>          goto exit;
>      }
>  
> -    ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
> +    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
> +                    NULL, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          goto exit;
> diff --git a/block/vmdk.c b/block/vmdk.c
> index e007793..70bf011 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -776,8 +776,9 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>  
>          path_combine(extent_path, sizeof(extent_path),
>                  desc_file_path, fname);
> -        ret = bdrv_file_open(&extent_file, extent_path, NULL, NULL,
> -                             bs->open_flags, errp);
> +        extent_file = NULL;
> +        ret = bdrv_open(&extent_file, extent_path, NULL, NULL,
> +                        bs->open_flags | BDRV_O_PROTOCOL, NULL, errp);
>          if (ret) {
>              return ret;
>          }
> @@ -1493,7 +1494,8 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
>          goto exit;
>      }
>  
> -    ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
> +    ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
> +                    NULL, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          goto exit;
> @@ -1831,7 +1833,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>              goto exit;
>          }
>      }
> -    ret = bdrv_file_open(&new_bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
> +    ret = bdrv_open(&new_bs, filename, NULL, NULL,
> +                    BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, &local_err);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "Could not write description");
>          goto exit;
> diff --git a/include/block/block.h b/include/block/block.h
> index a421041..bf78db5 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -102,6 +102,9 @@ typedef enum {
>  #define BDRV_O_CHECK       0x1000  /* open solely for consistency check */
>  #define BDRV_O_ALLOW_RDWR  0x2000  /* allow reopen to change from r/o to r/w */
>  #define BDRV_O_UNMAP       0x4000  /* execute guest UNMAP/TRIM operations */
> +#define BDRV_O_PROTOCOL    0x8000  /* if no block driver is explicitly given:
> +                                      select an appropriate protocol driver,
> +                                      ignoring the format layer */
>  
>  #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
>  
> @@ -183,9 +186,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
>  void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
>  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,
> -                   const char *reference, QDict *options, int flags,
> -                   Error **errp);
>  int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>                      QDict *options, const char *bdref_key, int flags,
>                      bool force_raw, bool allow_none, Error **errp);
> diff --git a/qemu-io.c b/qemu-io.c
> index 61d54c0..71d7806 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -59,7 +59,9 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
>      }
>  
>      if (growable) {
> -        if (bdrv_file_open(&qemuio_bs, name, NULL, opts, flags, &local_err)) {
> +        if (bdrv_open(&qemuio_bs, name, NULL, opts, flags | BDRV_O_PROTOCOL,
> +                      NULL, &local_err))
> +        {
>              fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
>                      error_get_pretty(local_err));
>              error_free(local_err);
> -- 
> 1.8.5.4
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

[Qemu-devel] [PATCH v2 4/8] block: Reuse reference handling from bdrv_open()

[Max Reitz]

Remove the reference parameter and the related handling code from
bdrv_file_open(), since it exists in bdrv_open() now as well.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 block.c | 33 +++++++--------------------------
 1 file changed, 7 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index 432d659..4c105bb 100644
--- a/block.c
+++ b/block.c
@@ -954,8 +954,7 @@ free_and_fail:
  * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
  */
 static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
-                          const char *reference, QDict *options, int flags,
-                          Error **errp)
+                          QDict *options, int flags, Error **errp)
 {
     BlockDriverState *bs = NULL;
     BlockDriver *drv;
@@ -969,24 +968,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
         options = qdict_new();
     }
 
-    if (reference) {
-        if (filename || qdict_size(options)) {
-            error_setg(errp, "Cannot reference an existing block device with "
-                       "additional options or a new filename");
-            return -EINVAL;
-        }
-        QDECREF(options);
-
-        bs = bdrv_find(reference);
-        if (!bs) {
-            error_setg(errp, "Cannot find block device '%s'", reference);
-            return -ENODEV;
-        }
-        bdrv_ref(bs);
-        *pbs = bs;
-        return 0;
-    }
-
     bs = bdrv_new("");
     bs->options = options;
     options = qdict_clone_shallow(options);
@@ -1235,12 +1216,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
 
     assert(pbs);
 
-    if (flags & BDRV_O_PROTOCOL) {
-        assert(!drv);
-        return bdrv_file_open(pbs, filename, reference, options,
-                              flags & ~BDRV_O_PROTOCOL, errp);
-    }
-
     if (reference) {
         bool options_non_empty = options ? qdict_size(options) : false;
         QDECREF(options);
@@ -1267,6 +1242,12 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         return 0;
     }
 
+    if (flags & BDRV_O_PROTOCOL) {
+        assert(!drv);
+        return bdrv_file_open(pbs, filename, options, flags & ~BDRV_O_PROTOCOL,
+                              errp);
+    }
+
     if (*pbs) {
         bs = *pbs;
     } else {
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH v2 4/8] block: Reuse reference handling from bdrv_open()

[Benoît Canet]

Le Saturday 08 Feb 2014 à 18:39:15 (+0100), Max Reitz a écrit :
> Remove the reference parameter and the related handling code from
> bdrv_file_open(), since it exists in bdrv_open() now as well.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c | 33 +++++++--------------------------
>  1 file changed, 7 insertions(+), 26 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 432d659..4c105bb 100644
> --- a/block.c
> +++ b/block.c
> @@ -954,8 +954,7 @@ free_and_fail:
>   * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
>   */
>  static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> -                          const char *reference, QDict *options, int flags,
> -                          Error **errp)
> +                          QDict *options, int flags, Error **errp)
>  {
>      BlockDriverState *bs = NULL;
>      BlockDriver *drv;
> @@ -969,24 +968,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>          options = qdict_new();
>      }
>  
> -    if (reference) {
> -        if (filename || qdict_size(options)) {
> -            error_setg(errp, "Cannot reference an existing block device with "
> -                       "additional options or a new filename");
> -            return -EINVAL;
> -        }
> -        QDECREF(options);
> -
> -        bs = bdrv_find(reference);
> -        if (!bs) {
> -            error_setg(errp, "Cannot find block device '%s'", reference);
> -            return -ENODEV;
> -        }
> -        bdrv_ref(bs);
> -        *pbs = bs;
> -        return 0;
> -    }
> -
>      bs = bdrv_new("");
>      bs->options = options;
>      options = qdict_clone_shallow(options);
> @@ -1235,12 +1216,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>  
>      assert(pbs);
>  
> -    if (flags & BDRV_O_PROTOCOL) {
> -        assert(!drv);
> -        return bdrv_file_open(pbs, filename, reference, options,
> -                              flags & ~BDRV_O_PROTOCOL, errp);
> -    }
> -
>      if (reference) {
>          bool options_non_empty = options ? qdict_size(options) : false;
>          QDECREF(options);
> @@ -1267,6 +1242,12 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>          return 0;
>      }
>  
> +    if (flags & BDRV_O_PROTOCOL) {
> +        assert(!drv);
> +        return bdrv_file_open(pbs, filename, options, flags & ~BDRV_O_PROTOCOL,
> +                              errp);
> +    }
> +
>      if (*pbs) {
>          bs = *pbs;
>      } else {
> -- 
> 1.8.5.4
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

[Qemu-devel] [PATCH v2 5/8] block: Remove bdrv_new() from bdrv_file_open()

[Max Reitz]

Change bdrv_file_open() to take a simple pointer to an already existing
BDS instead of an indirect one. The BDS will be created in bdrv_open()
if necessary.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 4c105bb..c57ad2c 100644
--- a/block.c
+++ b/block.c
@@ -953,10 +953,9 @@ free_and_fail:
  * after the call (even on failure), so if the caller intends to reuse the
  * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
  */
-static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
+static int bdrv_file_open(BlockDriverState *bs, const char *filename,
                           QDict *options, int flags, Error **errp)
 {
-    BlockDriverState *bs = NULL;
     BlockDriver *drv;
     const char *drvname;
     bool allow_protocol_prefix = false;
@@ -968,7 +967,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
         options = qdict_new();
     }
 
-    bs = bdrv_new("");
     bs->options = options;
     options = qdict_clone_shallow(options);
 
@@ -1042,7 +1040,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
     QDECREF(options);
 
     bs->growable = 1;
-    *pbs = bs;
     return 0;
 
 fail:
@@ -1050,7 +1047,6 @@ fail:
     if (!bs->drv) {
         QDECREF(bs->options);
     }
-    bdrv_unref(bs);
     return ret;
 }
 
@@ -1242,18 +1238,24 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         return 0;
     }
 
-    if (flags & BDRV_O_PROTOCOL) {
-        assert(!drv);
-        return bdrv_file_open(pbs, filename, options, flags & ~BDRV_O_PROTOCOL,
-                              errp);
-    }
-
     if (*pbs) {
         bs = *pbs;
     } else {
         bs = bdrv_new("");
     }
 
+    if (flags & BDRV_O_PROTOCOL) {
+        assert(!drv);
+        ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
+                             errp);
+        if (ret && !*pbs) {
+            bdrv_unref(bs);
+        } else if (!ret) {
+            *pbs = bs;
+        }
+        return ret;
+    }
+
     /* NULL means an empty set of options */
     if (options == NULL) {
         options = qdict_new();
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH v2 5/8] block: Remove bdrv_new() from bdrv_file_open()

[Benoît Canet]

Le Saturday 08 Feb 2014 à 18:39:16 (+0100), Max Reitz a écrit :
> Change bdrv_file_open() to take a simple pointer to an already existing
> BDS instead of an indirect one. The BDS will be created in bdrv_open()
> if necessary.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 4c105bb..c57ad2c 100644
> --- a/block.c
> +++ b/block.c
> @@ -953,10 +953,9 @@ free_and_fail:
>   * after the call (even on failure), so if the caller intends to reuse the
>   * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
>   */
> -static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> +static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>                            QDict *options, int flags, Error **errp)
>  {
> -    BlockDriverState *bs = NULL;
>      BlockDriver *drv;
>      const char *drvname;
>      bool allow_protocol_prefix = false;
> @@ -968,7 +967,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>          options = qdict_new();
>      }
>  
> -    bs = bdrv_new("");
>      bs->options = options;
>      options = qdict_clone_shallow(options);
>  
> @@ -1042,7 +1040,6 @@ static int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>      QDECREF(options);
>  
>      bs->growable = 1;
> -    *pbs = bs;
>      return 0;
>  
>  fail:
> @@ -1050,7 +1047,6 @@ fail:
>      if (!bs->drv) {
>          QDECREF(bs->options);
>      }
> -    bdrv_unref(bs);
>      return ret;
>  }
>  
> @@ -1242,18 +1238,24 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>          return 0;
>      }
>  
> -    if (flags & BDRV_O_PROTOCOL) {
> -        assert(!drv);
> -        return bdrv_file_open(pbs, filename, options, flags & ~BDRV_O_PROTOCOL,
> -                              errp);
> -    }
> -
>      if (*pbs) {
>          bs = *pbs;
>      } else {
>          bs = bdrv_new("");
>      }
>  
> +    if (flags & BDRV_O_PROTOCOL) {
> +        assert(!drv);
> +        ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
> +                             errp);
> +        if (ret && !*pbs) {
> +            bdrv_unref(bs);
> +        } else if (!ret) {
> +            *pbs = bs;
> +        }
> +        return ret;
> +    }
> +
>      /* NULL means an empty set of options */
>      if (options == NULL) {
>          options = qdict_new();
> -- 
> 1.8.5.4
> 
    Reviewed-by: Benoit Canet <benoit@irqsave.net>

[Qemu-devel] [PATCH v2 6/8] block: Handle bs->options in bdrv_open() only

[Max Reitz]

The fail paths of bdrv_file_open() and bdrv_open() naturally exhibit
similarities, thus it is possible to reuse the one from bdrv_open() and
shorten the one in bdrv_file_open() accordingly.

Also, setting bs->options in bdrv_file_open() is not necessary if it is
already done in bdrv_open().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/block.c b/block.c
index c57ad2c..1fcd7e0 100644
--- a/block.c
+++ b/block.c
@@ -962,14 +962,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
     Error *local_err = NULL;
     int ret;
 
-    /* NULL means an empty set of options */
-    if (options == NULL) {
-        options = qdict_new();
-    }
-
-    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");
@@ -1044,9 +1036,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
 
 fail:
     QDECREF(options);
-    if (!bs->drv) {
-        QDECREF(bs->options);
-    }
     return ret;
 }
 
@@ -1244,18 +1233,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         bs = bdrv_new("");
     }
 
-    if (flags & BDRV_O_PROTOCOL) {
-        assert(!drv);
-        ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
-                             errp);
-        if (ret && !*pbs) {
-            bdrv_unref(bs);
-        } else if (!ret) {
-            *pbs = bs;
-        }
-        return ret;
-    }
-
     /* NULL means an empty set of options */
     if (options == NULL) {
         options = qdict_new();
@@ -1263,6 +1240,21 @@ 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, filename, options, flags & ~BDRV_O_PROTOCOL,
+                             &local_err);
+        options = NULL;
+        if (!ret) {
+            *pbs = bs;
+            return 0;
+        } else if (bs->drv) {
+            goto close_and_fail;
+        } else {
+            goto fail;
+        }
+    }
+
     /* For snapshot=on, create a temporary qcow2 overlay */
     if (flags & BDRV_O_SNAPSHOT) {
         BlockDriverState *bs1;
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH v2 6/8] block: Handle bs->options in bdrv_open() only

[Benoît Canet]

Le Saturday 08 Feb 2014 à 18:39:17 (+0100), Max Reitz a écrit :
> The fail paths of bdrv_file_open() and bdrv_open() naturally exhibit
> similarities, thus it is possible to reuse the one from bdrv_open() and
> shorten the one in bdrv_file_open() accordingly.
> 
> Also, setting bs->options in bdrv_file_open() is not necessary if it is
> already done in bdrv_open().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 38 +++++++++++++++-----------------------
>  1 file changed, 15 insertions(+), 23 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c57ad2c..1fcd7e0 100644
> --- a/block.c
> +++ b/block.c
> @@ -962,14 +962,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>      Error *local_err = NULL;
>      int ret;
>  
> -    /* NULL means an empty set of options */
> -    if (options == NULL) {
> -        options = qdict_new();
> -    }
> -
> -    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");
> @@ -1044,9 +1036,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>  
>  fail:
>      QDECREF(options);
> -    if (!bs->drv) {
> -        QDECREF(bs->options);
> -    }
>      return ret;
>  }
>  
> @@ -1244,18 +1233,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>          bs = bdrv_new("");
>      }
>  
> -    if (flags & BDRV_O_PROTOCOL) {
> -        assert(!drv);
> -        ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
> -                             errp);
> -        if (ret && !*pbs) {
> -            bdrv_unref(bs);
> -        } else if (!ret) {
> -            *pbs = bs;
> -        }
> -        return ret;
> -    }
> -
>      /* NULL means an empty set of options */
>      if (options == NULL) {
>          options = qdict_new();
> @@ -1263,6 +1240,21 @@ 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, filename, options, flags & ~BDRV_O_PROTOCOL,
> +                             &local_err);
> +        options = NULL;
> +        if (!ret) {
> +            *pbs = bs;
> +            return 0;
> +        } else if (bs->drv) {
> +            goto close_and_fail;
> +        } else {
> +            goto fail;
> +        }
> +    }
> +
>      /* For snapshot=on, create a temporary qcow2 overlay */
>      if (flags & BDRV_O_SNAPSHOT) {
>          BlockDriverState *bs1;
> -- 
> 1.8.5.4
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH v2 6/8] block: Handle bs->options in bdrv_open() only

[Benoît Canet]

Le Saturday 08 Feb 2014 à 18:39:17 (+0100), Max Reitz a écrit :
> The fail paths of bdrv_file_open() and bdrv_open() naturally exhibit
> similarities, thus it is possible to reuse the one from bdrv_open() and
> shorten the one in bdrv_file_open() accordingly.
> 
> Also, setting bs->options in bdrv_file_open() is not necessary if it is
> already done in bdrv_open().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 38 +++++++++++++++-----------------------
>  1 file changed, 15 insertions(+), 23 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c57ad2c..1fcd7e0 100644
> --- a/block.c
> +++ b/block.c
> @@ -962,14 +962,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>      Error *local_err = NULL;
>      int ret;
>  
> -    /* NULL means an empty set of options */
> -    if (options == NULL) {
> -        options = qdict_new();
> -    }
> -
> -    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");
> @@ -1044,9 +1036,6 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>  
>  fail:
>      QDECREF(options);
> -    if (!bs->drv) {
> -        QDECREF(bs->options);
> -    }
>      return ret;
>  }
>  
> @@ -1244,18 +1233,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>          bs = bdrv_new("");
>      }
>  
> -    if (flags & BDRV_O_PROTOCOL) {
> -        assert(!drv);
> -        ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
> -                             errp);
> -        if (ret && !*pbs) {
> -            bdrv_unref(bs);
> -        } else if (!ret) {
> -            *pbs = bs;
> -        }
> -        return ret;
> -    }
> -
>      /* NULL means an empty set of options */
>      if (options == NULL) {
>          options = qdict_new();
> @@ -1263,6 +1240,21 @@ 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, filename, options, flags & ~BDRV_O_PROTOCOL,
> +                             &local_err);
> +        options = NULL;
> +        if (!ret) {
> +            *pbs = bs;
> +            return 0;
> +        } else if (bs->drv) {
> +            goto close_and_fail;
> +        } else {
> +            goto fail;
> +        }
> +    }
> +
>      /* For snapshot=on, create a temporary qcow2 overlay */
>      if (flags & BDRV_O_SNAPSHOT) {
>          BlockDriverState *bs1;
> -- 
> 1.8.5.4
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

[Qemu-devel] [PATCH v2 7/8] block: Reuse success path from bdrv_open()

[Max Reitz]

The fail and success paths of bdrv_file_open() may be further shortened
by reusing code already existent in bdrv_open(). This includes
bdrv_file_open() not taking the reference to options which allows the
removal of QDECREF(options) in that function.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 68 ++++++++++++++++++++++++++++++++---------------------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/block.c b/block.c
index 1fcd7e0..22e5a44 100644
--- a/block.c
+++ b/block.c
@@ -948,13 +948,15 @@ free_and_fail:
 /*
  * Opens a file using a protocol (file, host_device, nbd, ...)
  *
- * 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 belongs to the block layer
- * after the call (even on failure), so if the caller intends to reuse the
- * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
+ * 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)
+                          QDict **options, int flags, Error **errp)
 {
     BlockDriver *drv;
     const char *drvname;
@@ -964,9 +966,9 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
 
     /* 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));
+        filename = qdict_get_try_str(*options, "filename");
+    } else if (filename && !qdict_haskey(*options, "filename")) {
+        qdict_put(*options, "filename", qstring_from_str(filename));
         allow_protocol_prefix = true;
     } else {
         error_setg(errp, "Can't specify 'file' and 'filename' options at the "
@@ -976,13 +978,13 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
     }
 
     /* Find the right block driver */
-    drvname = qdict_get_try_str(options, "driver");
+    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");
+        qdict_del(*options, "driver");
     } else if (filename) {
         drv = bdrv_find_protocol(filename, allow_protocol_prefix);
         if (!drv) {
@@ -1001,41 +1003,35 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
 
     /* Parse the filename and open it */
     if (drv->bdrv_parse_filename && filename) {
-        drv->bdrv_parse_filename(filename, options, &local_err);
+        drv->bdrv_parse_filename(filename, *options, &local_err);
         if (error_is_set(&local_err)) {
             error_propagate(errp, local_err);
             ret = -EINVAL;
             goto fail;
         }
-        qdict_del(options, "filename");
+        qdict_del(*options, "filename");
+    } else if (drv->bdrv_needs_filename && !filename) {
+        error_setg(errp, "The '%s' block driver requires a file name",
+                   drv->format_name);
+        ret = -EINVAL;
+        goto fail;
     }
 
     if (!drv->bdrv_file_open) {
-        ret = bdrv_open(&bs, filename, NULL, options, flags, drv, &local_err);
-        options = NULL;
+        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);
+        ret = bdrv_open_common(bs, NULL, *options, flags, drv, &local_err);
     }
     if (ret < 0) {
         error_propagate(errp, local_err);
         goto fail;
     }
 
-    /* Check if any unknown options were used */
-    if (options && (qdict_size(options) != 0)) {
-        const QDictEntry *entry = qdict_first(options);
-        error_setg(errp, "Block protocol '%s' doesn't support the option '%s'",
-                   drv->format_name, entry->key);
-        ret = -EINVAL;
-        goto fail;
-    }
-    QDECREF(options);
-
     bs->growable = 1;
     return 0;
 
 fail:
-    QDECREF(options);
     return ret;
 }
 
@@ -1242,12 +1238,10 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
 
     if (flags & BDRV_O_PROTOCOL) {
         assert(!drv);
-        ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
+        ret = bdrv_file_open(bs, filename, &options, flags & ~BDRV_O_PROTOCOL,
                              &local_err);
-        options = NULL;
         if (!ret) {
-            *pbs = bs;
-            return 0;
+            goto done;
         } else if (bs->drv) {
             goto close_and_fail;
         } else {
@@ -1384,12 +1378,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         }
     }
 
+done:
     /* Check if any unknown options were used */
-    if (qdict_size(options) != 0) {
+    if (options && (qdict_size(options) != 0)) {
         const QDictEntry *entry = qdict_first(options);
-        error_setg(errp, "Block format '%s' used by device '%s' doesn't "
-                   "support the option '%s'", drv->format_name, bs->device_name,
-                   entry->key);
+        if (flags & BDRV_O_PROTOCOL) {
+            error_setg(errp, "Block protocol '%s' doesn't support the option "
+                       "'%s'", drv->format_name, entry->key);
+        } else {
+            error_setg(errp, "Block format '%s' used by device '%s' doesn't "
+                       "support the option '%s'", drv->format_name,
+                       bs->device_name, entry->key);
+        }
 
         ret = -EINVAL;
         goto close_and_fail;
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH v2 7/8] block: Reuse success path from bdrv_open()

[Kevin Wolf]

Am 08.02.2014 um 18:39 hat Max Reitz geschrieben:
> The fail and success paths of bdrv_file_open() may be further shortened
> by reusing code already existent in bdrv_open(). This includes
> bdrv_file_open() not taking the reference to options which allows the
> removal of QDECREF(options) in that function.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

> @@ -1001,41 +1003,35 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>  
>      /* Parse the filename and open it */
>      if (drv->bdrv_parse_filename && filename) {
> -        drv->bdrv_parse_filename(filename, options, &local_err);
> +        drv->bdrv_parse_filename(filename, *options, &local_err);
>          if (error_is_set(&local_err)) {
>              error_propagate(errp, local_err);
>              ret = -EINVAL;
>              goto fail;
>          }
> -        qdict_del(options, "filename");
> +        qdict_del(*options, "filename");
> +    } else if (drv->bdrv_needs_filename && !filename) {
> +        error_setg(errp, "The '%s' block driver requires a file name",
> +                   drv->format_name);
> +        ret = -EINVAL;
> +        goto fail;
>      }

How did this part end up in this patch? It doesn't look wrong, though I
think bdrv_open_common() should already catch it. In any case it's an
addition that the commit message didn't mention.

Kevin

Re: [Qemu-devel] [PATCH v2 7/8] block: Reuse success path from bdrv_open()

[Max Reitz]

On 10.02.2014 15:56, Kevin Wolf wrote:
> Am 08.02.2014 um 18:39 hat Max Reitz geschrieben:
>> The fail and success paths of bdrv_file_open() may be further shortened
>> by reusing code already existent in bdrv_open(). This includes
>> bdrv_file_open() not taking the reference to options which allows the
>> removal of QDECREF(options) in that function.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> @@ -1001,41 +1003,35 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>>   
>>       /* Parse the filename and open it */
>>       if (drv->bdrv_parse_filename && filename) {
>> -        drv->bdrv_parse_filename(filename, options, &local_err);
>> +        drv->bdrv_parse_filename(filename, *options, &local_err);
>>           if (error_is_set(&local_err)) {
>>               error_propagate(errp, local_err);
>>               ret = -EINVAL;
>>               goto fail;
>>           }
>> -        qdict_del(options, "filename");
>> +        qdict_del(*options, "filename");
>> +    } else if (drv->bdrv_needs_filename && !filename) {
>> +        error_setg(errp, "The '%s' block driver requires a file name",
>> +                   drv->format_name);
>> +        ret = -EINVAL;
>> +        goto fail;
>>       }
> How did this part end up in this patch? It doesn't look wrong, though I
> think bdrv_open_common() should already catch it. In any case it's an
> addition that the commit message didn't mention.

I wonder. It definitely doesn't belong here, since as of your commit 
"block: Fail gracefully with missing filename" this check should be in 
bdrv_open_common() and not here. I guess, I somehow ended up reverting 
it to the old state here. I just hope there aren't any more such 
reverts; I'll take a look. I remember some rebase conflict here (for 
obvious reasons), so it's probably just in this case, though.

Max

Re: [Qemu-devel] [PATCH v2 7/8] block: Reuse success path from bdrv_open()

[Benoît Canet]

Le Saturday 08 Feb 2014 à 18:39:18 (+0100), Max Reitz a écrit :
> The fail and success paths of bdrv_file_open() may be further shortened
> by reusing code already existent in bdrv_open(). This includes
> bdrv_file_open() not taking the reference to options which allows the
> removal of QDECREF(options) in that function.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 68 ++++++++++++++++++++++++++++++++---------------------------------
>  1 file changed, 34 insertions(+), 34 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 1fcd7e0..22e5a44 100644
> --- a/block.c
> +++ b/block.c
> @@ -948,13 +948,15 @@ free_and_fail:
>  /*
>   * Opens a file using a protocol (file, host_device, nbd, ...)
>   *
> - * 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 belongs to the block layer
> - * after the call (even on failure), so if the caller intends to reuse the
> - * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
> + * 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)
> +                          QDict **options, int flags, Error **errp)
>  {
>      BlockDriver *drv;
>      const char *drvname;
> @@ -964,9 +966,9 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>  
>      /* 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));
> +        filename = qdict_get_try_str(*options, "filename");
> +    } else if (filename && !qdict_haskey(*options, "filename")) {
> +        qdict_put(*options, "filename", qstring_from_str(filename));
>          allow_protocol_prefix = true;
>      } else {
>          error_setg(errp, "Can't specify 'file' and 'filename' options at the "
> @@ -976,13 +978,13 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>      }
>  
>      /* Find the right block driver */
> -    drvname = qdict_get_try_str(options, "driver");
> +    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");
> +        qdict_del(*options, "driver");
>      } else if (filename) {
>          drv = bdrv_find_protocol(filename, allow_protocol_prefix);
>          if (!drv) {
> @@ -1001,41 +1003,35 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
>  
>      /* Parse the filename and open it */
>      if (drv->bdrv_parse_filename && filename) {
> -        drv->bdrv_parse_filename(filename, options, &local_err);
> +        drv->bdrv_parse_filename(filename, *options, &local_err);
>          if (error_is_set(&local_err)) {
>              error_propagate(errp, local_err);
>              ret = -EINVAL;
>              goto fail;
>          }
> -        qdict_del(options, "filename");
> +        qdict_del(*options, "filename");
> +    } else if (drv->bdrv_needs_filename && !filename) {
> +        error_setg(errp, "The '%s' block driver requires a file name",
> +                   drv->format_name);
> +        ret = -EINVAL;
> +        goto fail;
>      }
>  
>      if (!drv->bdrv_file_open) {
> -        ret = bdrv_open(&bs, filename, NULL, options, flags, drv, &local_err);
> -        options = NULL;
> +        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);
> +        ret = bdrv_open_common(bs, NULL, *options, flags, drv, &local_err);
>      }
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          goto fail;
>      }
>  
> -    /* Check if any unknown options were used */
> -    if (options && (qdict_size(options) != 0)) {
> -        const QDictEntry *entry = qdict_first(options);
> -        error_setg(errp, "Block protocol '%s' doesn't support the option '%s'",
> -                   drv->format_name, entry->key);
> -        ret = -EINVAL;
> -        goto fail;
> -    }
> -    QDECREF(options);
> -
>      bs->growable = 1;
>      return 0;
>  
>  fail:
> -    QDECREF(options);
>      return ret;
>  }
>  
> @@ -1242,12 +1238,10 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>  
>      if (flags & BDRV_O_PROTOCOL) {
>          assert(!drv);
> -        ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL,
> +        ret = bdrv_file_open(bs, filename, &options, flags & ~BDRV_O_PROTOCOL,
>                               &local_err);
> -        options = NULL;
>          if (!ret) {
> -            *pbs = bs;
> -            return 0;
> +            goto done;
>          } else if (bs->drv) {
>              goto close_and_fail;
>          } else {
> @@ -1384,12 +1378,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>          }
>      }
>  
> +done:
>      /* Check if any unknown options were used */
> -    if (qdict_size(options) != 0) {
> +    if (options && (qdict_size(options) != 0)) {
>          const QDictEntry *entry = qdict_first(options);
> -        error_setg(errp, "Block format '%s' used by device '%s' doesn't "
> -                   "support the option '%s'", drv->format_name, bs->device_name,
> -                   entry->key);
> +        if (flags & BDRV_O_PROTOCOL) {
> +            error_setg(errp, "Block protocol '%s' doesn't support the option "
> +                       "'%s'", drv->format_name, entry->key);
> +        } else {
> +            error_setg(errp, "Block format '%s' used by device '%s' doesn't "
> +                       "support the option '%s'", drv->format_name,
> +                       bs->device_name, entry->key);
> +        }
>  
>          ret = -EINVAL;
>          goto close_and_fail;
> -- 
> 1.8.5.4
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

[Qemu-devel] [PATCH v2 8/8] block: Remove bdrv_open_image()'s force_raw option

[Max Reitz]

This option is now unnecessary since specifying BDRV_O_PROTOCOL as flag
will do exactly the same.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 28 ++++------------------------
 block/blkdebug.c      |  2 +-
 block/blkverify.c     |  4 ++--
 include/block/block.h |  2 +-
 4 files changed, 8 insertions(+), 28 deletions(-)

diff --git a/block.c b/block.c
index 22e5a44..a1630e9 100644
--- a/block.c
+++ b/block.c
@@ -1107,10 +1107,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
  * Opens a disk image whose options are given as BlockdevRef in another block
  * device's options.
  *
- * If force_raw is true, bdrv_file_open() will be used, thereby preventing any
- * image format auto-detection. If it is false and a filename is given,
- * bdrv_open() will be used for auto-detection.
- *
  * If allow_none is true, no image will be opened if filename is false and no
  * BlockdevRef is given. *pbs will remain unchanged and 0 will be returned.
  *
@@ -1127,7 +1123,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
  */
 int bdrv_open_image(BlockDriverState **pbs, const char *filename,
                     QDict *options, const char *bdref_key, int flags,
-                    bool force_raw, bool allow_none, Error **errp)
+                    bool allow_none, Error **errp)
 {
     QDict *image_options;
     int ret;
@@ -1150,23 +1146,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
         goto done;
     }
 
-    if (filename && !force_raw) {
-        /* If a filename is given and the block driver should be detected
-           automatically (instead of using none), use bdrv_open() in order to do
-           that auto-detection. */
-        if (reference) {
-            error_setg(errp, "Cannot reference an existing block device while "
-                       "giving a filename");
-            ret = -EINVAL;
-            goto done;
-        }
-
-        ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
-    } else {
-        *pbs = NULL;
-        ret = bdrv_open(pbs, filename, reference, image_options,
-                        flags | BDRV_O_PROTOCOL, NULL, errp);
-    }
+    ret = bdrv_open(pbs, filename, reference, image_options, flags, NULL, errp);
 
 done:
     qdict_del(options, bdref_key);
@@ -1324,8 +1304,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
 
     assert(file == NULL);
     ret = bdrv_open_image(&file, filename, options, "file",
-                          bdrv_open_flags(bs, flags | BDRV_O_UNMAP), true, true,
-                          &local_err);
+                          bdrv_open_flags(bs, flags | BDRV_O_UNMAP) |
+                          BDRV_O_PROTOCOL, true, &local_err);
     if (ret < 0) {
         goto fail;
     }
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 053fa4c..6707f49 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -412,7 +412,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     /* Open the backing file */
     assert(bs->file == NULL);
     ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, "image",
-                          flags, true, false, &local_err);
+                          flags | BDRV_O_PROTOCOL, false, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         goto out;
diff --git a/block/blkverify.c b/block/blkverify.c
index 86585e7..b57da59 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -137,7 +137,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
     /* Open the raw file */
     assert(bs->file == NULL);
     ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-raw"), options,
-                          "raw", flags, true, false, &local_err);
+                          "raw", flags | BDRV_O_PROTOCOL, false, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         goto fail;
@@ -146,7 +146,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
     /* Open the test file */
     assert(s->test_file == NULL);
     ret = bdrv_open_image(&s->test_file, qemu_opt_get(opts, "x-image"), options,
-                          "test", flags, false, false, &local_err);
+                          "test", flags, false, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         s->test_file = NULL;
diff --git a/include/block/block.h b/include/block/block.h
index bf78db5..780f48b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -188,7 +188,7 @@ int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_parse_discard_flags(const char *mode, int *flags);
 int bdrv_open_image(BlockDriverState **pbs, const char *filename,
                     QDict *options, const char *bdref_key, int flags,
-                    bool force_raw, bool allow_none, Error **errp);
+                    bool allow_none, Error **errp);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
 int bdrv_open(BlockDriverState **pbs, const char *filename,
               const char *reference, QDict *options, int flags,
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH v2 8/8] block: Remove bdrv_open_image()'s force_raw option

[Benoît Canet]

Le Saturday 08 Feb 2014 à 18:39:19 (+0100), Max Reitz a écrit :
> This option is now unnecessary since specifying BDRV_O_PROTOCOL as flag
> will do exactly the same.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c               | 28 ++++------------------------
>  block/blkdebug.c      |  2 +-
>  block/blkverify.c     |  4 ++--
>  include/block/block.h |  2 +-
>  4 files changed, 8 insertions(+), 28 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 22e5a44..a1630e9 100644
> --- a/block.c
> +++ b/block.c
> @@ -1107,10 +1107,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>   * Opens a disk image whose options are given as BlockdevRef in another block
>   * device's options.
>   *
> - * If force_raw is true, bdrv_file_open() will be used, thereby preventing any
> - * image format auto-detection. If it is false and a filename is given,
> - * bdrv_open() will be used for auto-detection.
> - *
>   * If allow_none is true, no image will be opened if filename is false and no
>   * BlockdevRef is given. *pbs will remain unchanged and 0 will be returned.
>   *
> @@ -1127,7 +1123,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>   */
>  int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>                      QDict *options, const char *bdref_key, int flags,
> -                    bool force_raw, bool allow_none, Error **errp)
> +                    bool allow_none, Error **errp)
>  {
>      QDict *image_options;
>      int ret;
> @@ -1150,23 +1146,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>          goto done;
>      }
>  
> -    if (filename && !force_raw) {
> -        /* If a filename is given and the block driver should be detected
> -           automatically (instead of using none), use bdrv_open() in order to do
> -           that auto-detection. */
> -        if (reference) {
> -            error_setg(errp, "Cannot reference an existing block device while "
> -                       "giving a filename");
> -            ret = -EINVAL;
> -            goto done;
> -        }
> -
> -        ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
> -    } else {
> -        *pbs = NULL;
> -        ret = bdrv_open(pbs, filename, reference, image_options,
> -                        flags | BDRV_O_PROTOCOL, NULL, errp);
> -    }
> +    ret = bdrv_open(pbs, filename, reference, image_options, flags, NULL, errp);
>  
>  done:
>      qdict_del(options, bdref_key);
> @@ -1324,8 +1304,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>  
>      assert(file == NULL);
>      ret = bdrv_open_image(&file, filename, options, "file",
> -                          bdrv_open_flags(bs, flags | BDRV_O_UNMAP), true, true,
> -                          &local_err);
> +                          bdrv_open_flags(bs, flags | BDRV_O_UNMAP) |
> +                          BDRV_O_PROTOCOL, true, &local_err);
>      if (ret < 0) {
>          goto fail;
>      }
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 053fa4c..6707f49 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -412,7 +412,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>      /* Open the backing file */
>      assert(bs->file == NULL);
>      ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, "image",
> -                          flags, true, false, &local_err);
> +                          flags | BDRV_O_PROTOCOL, false, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          goto out;
> diff --git a/block/blkverify.c b/block/blkverify.c
> index 86585e7..b57da59 100644
> --- a/block/blkverify.c
> +++ b/block/blkverify.c
> @@ -137,7 +137,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
>      /* Open the raw file */
>      assert(bs->file == NULL);
>      ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-raw"), options,
> -                          "raw", flags, true, false, &local_err);
> +                          "raw", flags | BDRV_O_PROTOCOL, false, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          goto fail;
> @@ -146,7 +146,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
>      /* Open the test file */
>      assert(s->test_file == NULL);
>      ret = bdrv_open_image(&s->test_file, qemu_opt_get(opts, "x-image"), options,
> -                          "test", flags, false, false, &local_err);
> +                          "test", flags, false, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          s->test_file = NULL;
> diff --git a/include/block/block.h b/include/block/block.h
> index bf78db5..780f48b 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -188,7 +188,7 @@ int bdrv_parse_cache_flags(const char *mode, int *flags);
>  int bdrv_parse_discard_flags(const char *mode, int *flags);
>  int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>                      QDict *options, const char *bdref_key, int flags,
> -                    bool force_raw, bool allow_none, Error **errp);
> +                    bool allow_none, Error **errp);
>  int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
>  int bdrv_open(BlockDriverState **pbs, const char *filename,
>                const char *reference, QDict *options, int flags,
> -- 
> 1.8.5.4
> 
    Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH v2 0/8] block: Integrate bdrv_file_open() into bdrv_open()

[Kevin Wolf]

Am 08.02.2014 um 18:39 hat Max Reitz geschrieben:
> bdrv_file_open() is now nearly a subset of bdrv_open(), except for the
> fact that bdrv_file_open() is for protocols and bdrv_open() for block
> drivers. It is possible to use bdrv_file_open() with a block driver, but
> in that case that block driver must be explicitly specified.
> 
> Due to these great similarities, bdrv_file_open() can be integrated and
> made a special case of bdrv_open(). If the flag BDRV_O_PROTOCOL is
> specified, bdrv_open() will now do what bdrv_file_open() used to do:
> Auto-detecting a protocol instead of a block driver.
> 
> This series implements this and changes all calls to bdrv_file_open() to
> bdrv_open() calls with BDRV_O_PROTOCOL specified.
> 
> Note that this flag cannot be discerned automatically since it is
> impossible for bdrv_open() to know by itself whether a given file should
> be opened with or without the format layer involved: Both are valid
> alternatives. Therefore, it still has to be specified by the user.

I had two or three comments that can be addressed in a follow-up.

Series: Reviewed-by: Kevin Wolf <kwolf@redhat.com>