[01/10] block: Change BDS parameter of bdrv_open() to **

Message ID	1390762963-25538-2-git-send-email-mreitz@redhat.com
State	New
Headers	show Return-Path: <qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org> From: Max Reitz <mreitz@redhat.com> To: qemu-devel@nongnu.org Date: Sun, 26 Jan 2014 20:02:34 +0100 Message-Id: <1390762963-25538-2-git-send-email-mreitz@redhat.com> In-Reply-To: <1390762963-25538-1-git-send-email-mreitz@redhat.com> References: <1390762963-25538-1-git-send-email-mreitz@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Max Reitz <mreitz@redhat.com> Subject: [Qemu-devel] [PATCH 01/10] block: Change BDS parameter of bdrv_open() to ** Precedence: list Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org

Max Reitz Jan. 26, 2014, 7:02 p.m. UTC

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(-)

Benoît Canet Jan. 27, 2014, 2:38 a.m. UTC | #1

Le Sunday 26 Jan 2014 à 20:02:34 (+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               | 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;
bdrv_open set *file = NULL before calling bdrv_open_image so this is redundant.
What would be the correct behavior if the caller was giving *pbs != NULL ?
Is loosing the reference to a previously existing bds right ?


> +        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;
>  
> @@ -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:
The goto chain is different on Kevin qemu.git block; there is an additional fail
label ?
On which tree and branch are these patches based ?

> @@ -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);
> +    }
>      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? */
The state has been g_malloc0 so ok.
> -    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);
It already open here so ok.
> +    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);

I wonder if the if(bs) test in the fail label is dead code.
The bdrv_unref in the test branch is not.
I don't see how bs could be NULL.
>      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
> 
>

Max Reitz Jan. 27, 2014, 6:51 p.m. UTC | #2

On 27.01.2014 03:38, Benoît Canet wrote:
> Le Sunday 26 Jan 2014 à 20:02:34 (+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               | 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;
> bdrv_open set *file = NULL before calling bdrv_open_image so this is redundant.
> What would be the correct behavior if the caller was giving *pbs != NULL ?
> Is loosing the reference to a previously existing bds right ?

I thought about that, too. But it would have to be a pretty broken 
caller to rely on this function to preserve *pbs if it fails and to 
overwrite it otherwise. But you're right, to conform with the behavior 
of bdrv_open/bdrv_file_open, it would probably make more sense not to 
overwrite *pbs.

>> +        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;
>>   
>> @@ -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:
> The goto chain is different on Kevin qemu.git block; there is an additional fail
> label ?
> On which tree and branch are these patches based ?

They are based on Kevin's (current) block branch (at least, that's what 
my git log says ;-)).

>
>> @@ -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);
>> +    }
>>       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? */
> The state has been g_malloc0 so ok.
>> -    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);
> It already open here so ok.
>> +    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);
> I wonder if the if(bs) test in the fail label is dead code.
> The bdrv_unref in the test branch is not.
> I don't see how bs could be NULL.

Oh, yes, you're right. I'll remove it.

>>       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
>>
>>

Thanks for reviewing!

Max

Jeff Cody Jan. 27, 2014, 7:31 p.m. UTC | #3

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
> 
>

Max Reitz Jan. 27, 2014, 7:35 p.m. UTC | #4

On 27.01.2014 20:31, Jeff Cody wrote:
> 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);

Sure, why not.

>> @@ -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.

Yes, I'll add one.

Max

>>       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
>>
>>

Kevin Wolf Jan. 29, 2014, 11:50 a.m. UTC | #5

Am 26.01.2014 um 20:02 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               | 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);

In fact, we could use bdrv_new() only in the bdrv_open_common() case now
and leave it to bdrv_open() in the other one. Not sure if it makes
sense, though, I'll have to read the other patches first.

> @@ -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",

This one made me think and I find it a bit worrying. I need to review
what the old value of bs->backing_hd is in order to find out if this was
a NULL argument that is already cleaned up or whether it needs to be
cleaned up here (this one is a NULL argument, so this code is fine).

In itself the behaviour is pretty consistent, you only have to free what
you explicity allocated previously. But I'm afraid that ownership of the
reference, especially in the failure case could confuse people.

Perhaps we should add an assert(bs->backing_hd == NULL) before such
bdrv_open() calls? It's not perfect, but maybe better than nothing.

> @@ -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;

Why do you initialise bs here? It's not supposed to be used before the
first assignment, so let the compiler warn against it.

>      const char *drvname;
>      Error *local_err = 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("");
> +    }

I think I'd prefer if you moved this above qdict_new(), so that all
option related code stays in one place.

>      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);
> +    }
>      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;
>      }

It might improve the readability if you leave the declaration without an
initialiser, but have an explicit target_bs = NULL immediately before
the bdrv_open.

> @@ -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;
>      }

Same here.

> 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],

So in summary, it looks technically correct, but I'm not sure about
maintainability. If the problems are only for this intermediate state,
I'm okay with leaving them as they are, but otherwise we should try and
figure out something.

Kevin

Max Reitz Jan. 31, 2014, 8:07 p.m. UTC | #6

On 29.01.2014 12:50, Kevin Wolf wrote:
> Am 26.01.2014 um 20:02 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               | 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);
> In fact, we could use bdrv_new() only in the bdrv_open_common() case now
> and leave it to bdrv_open() in the other one. Not sure if it makes
> sense, though, I'll have to read the other patches first.

There were some other cases such as blockdev_init() in which I didn't 
want to completely change the existing code.

>
>> @@ -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",
> This one made me think and I find it a bit worrying. I need to review
> what the old value of bs->backing_hd is in order to find out if this was
> a NULL argument that is already cleaned up or whether it needs to be
> cleaned up here (this one is a NULL argument, so this code is fine).
>
> In itself the behaviour is pretty consistent, you only have to free what
> you explicity allocated previously. But I'm afraid that ownership of the
> reference, especially in the failure case could confuse people.
>
> Perhaps we should add an assert(bs->backing_hd == NULL) before such
> bdrv_open() calls? It's not perfect, but maybe better than nothing.

This seems like a fair compromise to me.

>> @@ -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;
> Why do you initialise bs here? It's not supposed to be used before the
> first assignment, so let the compiler warn against it.

Good question, perhaps just a reflex induced by the "file = NULL" in 
front of it. ;-)

>>       const char *drvname;
>>       Error *local_err = 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("");
>> +    }
> I think I'd prefer if you moved this above qdict_new(), so that all
> option related code stays in one place.

Okay.

>>       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);
>> +    }
>>       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;
>>       }
> It might improve the readability if you leave the declaration without an
> initialiser, but have an explicit target_bs = NULL immediately before
> the bdrv_open.

Yes, especially with using assert() where such an initialization is not 
needed.

>> @@ -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;
>>       }
> Same here.
>
>> 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],
> So in summary, it looks technically correct, but I'm not sure about
> maintainability. If the problems are only for this intermediate state,
> I'm okay with leaving them as they are, but otherwise we should try and
> figure out something.

You're right in that bdrv_open should always create a new BDS and new 
BDS should always be created through bdrv_open; but right now, there are 
some places in the code (such as blockdev_init()) which I was just 
afraid of touching as they seemed to heavily rely on being able to 
separate those two steps.

Max

Kevin Wolf Feb. 3, 2014, 9:49 a.m. UTC | #7

Am 31.01.2014 um 21:07 hat Max Reitz geschrieben:
> >So in summary, it looks technically correct, but I'm not sure about
> >maintainability. If the problems are only for this intermediate state,
> >I'm okay with leaving them as they are, but otherwise we should try and
> >figure out something.
> 
> You're right in that bdrv_open should always create a new BDS and
> new BDS should always be created through bdrv_open; but right now,
> there are some places in the code (such as blockdev_init()) which I
> was just afraid of touching as they seemed to heavily rely on being
> able to separate those two steps.

Sure, there's a good reason why we work in incremental steps. This is a
huge conversion of infrastructure and trying to change it all at once
never worked out. We shouldn't worry too much about ugly intermediate
states as long as we're sure that they are really just intermediate.

Kevin

[01/10] block: Change BDS parameter of bdrv_open() to **

Commit Message

Comments

Patch