qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v2 1/8] block: Change BDS parameter of bdrv_open() to **
Date: Wed, 12 Feb 2014 01:15:38 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 
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 <address@hidden>
---
  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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]