Re: [Qemu-devel] [PATCH 01/10] block: Change BDS parameter of bdrv_open() to **

[Jeff Cody <jcody@redhat.com>]

On Sun, Jan 26, 2014 at 08:02:34PM +0100, Max Reitz wrote:
> 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               | 53 ++++++++++++++++++++++++++-------------------------
>  block/qcow2.c         | 14 +++++++++-----
>  block/vmdk.c          |  5 ++---
>  block/vvfat.c         |  6 ++----
>  blockdev.c            | 21 ++++++++------------
>  hw/block/xen_disk.c   |  2 +-
>  include/block/block.h |  2 +-
>  qemu-img.c            |  6 +++---
>  qemu-io.c             |  2 +-
>  qemu-nbd.c            |  2 +-
>  10 files changed, 55 insertions(+), 58 deletions(-)
> 
> diff --git a/block.c b/block.c
> index cb21a5f..c660609 100644
> --- a/block.c
> +++ b/block.c
> @@ -1039,7 +1039,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);
> @@ -1108,8 +1108,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);
>      }
> @@ -1118,11 +1116,10 @@ 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,
> +    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",
> @@ -1189,8 +1186,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");
> @@ -1198,13 +1193,8 @@ 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;
> -        }
> +        *pbs = NULL;
> +        ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
>      } else {
>          ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
>                               errp);
> @@ -1222,14 +1212,17 @@ 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 = NULL;
>      const char *drvname;
>      Error *local_err = NULL;
>  

Since it is valid to pass *pbs = NULL for new allocation, perhaps we
should add an assert here to help thwart easy mistakes, and make sure
that **pbs != NULL:

 assert(pbs != NULL);


> @@ -1238,12 +1231,17 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>          options = qdict_new();
>      }
>  
> +    if (*pbs) {
> +        bs = *pbs;
> +    } else {
> +        bs = bdrv_new("");
> +    }
>      bs->options = options;
>      options = qdict_clone_shallow(options);
>  
>      /* For snapshot=on, create a temporary qcow2 overlay */
>      if (flags & BDRV_O_SNAPSHOT) {
> -        BlockDriverState *bs1;
> +        BlockDriverState *bs1 = NULL;
>          int64_t total_size;
>          BlockDriver *bdrv_qcow2;
>          QEMUOptionParameter *create_options;
> @@ -1253,12 +1251,10 @@ 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,
> +        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;
> @@ -1386,6 +1382,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:
> @@ -1399,13 +1396,20 @@ fail:
>      QDECREF(bs->options);
>      QDECREF(options);
>      bs->options = NULL;
> +    if (!*pbs) {
> +        bdrv_unref(bs);
> +    }
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
>      }
>      return ret;
>  
>  close_and_fail:
> -    bdrv_close(bs);
> +    if (*pbs) {
> +        bdrv_close(bs);
> +    } else {
> +        bdrv_unref(bs);
> +    }

If *pbs is != NULL, and we reach an error case, we have to clean up
the new BDS we created.  This makes sense, but it may not be
immediately obvious that *pbs = NULL means that *bs was allocated in
this function.  It may be worth a comment in the fail labels.

>      QDECREF(options);
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
> @@ -5276,7 +5280,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>      size = get_option_parameter(param, BLOCK_OPT_SIZE);
>      if (size && size->value.n == -1) {
>          if (backing_file && backing_file->value.s) {
> -            BlockDriverState *bs;
> +            BlockDriverState *bs = NULL;
>              uint64_t size;
>              char buf[32];
>              int back_flags;
> @@ -5285,9 +5289,7 @@ 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,
> +            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",
> @@ -5295,7 +5297,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/qcow2.c b/block/qcow2.c
> index 2da62b8..9a25fbd 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1541,7 +1541,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
> @@ -1550,7 +1551,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);
> @@ -1597,10 +1598,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)) {
> @@ -1610,7 +1612,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 99ca60f..0fbf230 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..42163f8 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,11 @@ 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,
> +    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 +1554,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;
> @@ -1906,7 +1905,7 @@ void qmp_drive_backup(const char *device, const char *target,
>                        Error **errp)
>  {
>      BlockDriverState *bs;
> -    BlockDriverState *target_bs;
> +    BlockDriverState *target_bs = NULL;
>      BlockDriverState *source = NULL;
>      BlockDriver *drv = NULL;
>      Error *local_err = NULL;
> @@ -1991,10 +1990,8 @@ 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);
> +    ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
>      if (ret < 0) {
> -        bdrv_unref(target_bs);
>          error_propagate(errp, local_err);
>          return;
>      }
> @@ -2027,7 +2024,7 @@ void qmp_drive_mirror(const char *device, const char *target,
>                        Error **errp)
>  {
>      BlockDriverState *bs;
> -    BlockDriverState *source, *target_bs;
> +    BlockDriverState *source, *target_bs = NULL;
>      BlockDriver *drv = NULL;
>      Error *local_err = NULL;
>      int flags;
> @@ -2135,11 +2132,9 @@ 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,
> +    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..c39d486 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));
> @@ -2314,7 +2314,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 +2324,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 d669028..2f06dc6 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.3
> 
>