Re: [Qemu-devel] [PULL 04/54] block: Add reference parameter to bdrv_open()

[Fam Zheng <famz@redhat.com>]

On Fri, 02/21 23:12, Kevin Wolf wrote:
> From: Max Reitz <mreitz@redhat.com>
> 
> Allow bdrv_open() to handle references to existing block devices just as
> bdrv_file_open() is already capable of.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c               | 44 +++++++++++++++++++++++++++++++++++++-------
>  block/qcow2.c         |  4 ++--
>  block/vmdk.c          |  3 ++-
>  block/vvfat.c         |  2 +-
>  blockdev.c            | 12 ++++++------
>  hw/block/xen_disk.c   |  4 ++--
>  include/block/block.h |  5 +++--
>  qemu-img.c            |  8 ++++----
>  qemu-io.c             |  4 +++-
>  qemu-nbd.c            |  2 +-
>  10 files changed, 61 insertions(+), 27 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c39bd3d..86a32e7 100644
> --- a/block.c
> +++ b/block.c
> @@ -1046,7 +1046,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>      }
>  
>      if (!drv->bdrv_file_open) {
> -        ret = bdrv_open(&bs, filename, options, flags, drv, &local_err);
> +        ret = bdrv_open(&bs, filename, NULL, options, flags, drv, &local_err);
>          options = NULL;
>      } else {
>          ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
> @@ -1125,7 +1125,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>  
>      assert(bs->backing_hd == NULL);
>      ret = bdrv_open(&bs->backing_hd,
> -                    *backing_filename ? backing_filename : NULL, options,
> +                    *backing_filename ? backing_filename : NULL, NULL, options,
>                      back_flags, back_drv, &local_err);
>      if (ret < 0) {
>          bs->backing_hd = NULL;
> @@ -1206,7 +1206,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>              goto done;
>          }
>  
> -        ret = bdrv_open(pbs, filename, image_options, flags, NULL, errp);
> +        ret = bdrv_open(pbs, filename, NULL, image_options, flags, NULL, errp);
>      } else {
>          ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
>                               errp);
> @@ -1227,9 +1227,14 @@ done:
>   *
>   * If *pbs is NULL, a new BDS will be created with a pointer to it stored there.
>   * If it is not NULL, the referenced BDS will be reused.
> + *
> + * The reference parameter may be used to specify an existing block device which
> + * should be opened. If specified, neither options nor a filename may be given,
> + * nor can an existing BDS be reused (that is, *pbs has to be NULL).
>   */
> -int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
> -              int flags, BlockDriver *drv, Error **errp)
> +int bdrv_open(BlockDriverState **pbs, const char *filename,
> +              const char *reference, QDict *options, int flags,
> +              BlockDriver *drv, Error **errp)
>  {
>      int ret;
>      /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> @@ -1240,6 +1245,31 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>  
>      assert(pbs);
>  
> +    if (reference) {
> +        bool options_non_empty = options ? qdict_size(options) : false;
> +        QDECREF(options);
> +
> +        if (*pbs) {
> +            error_setg(errp, "Cannot reuse an existing BDS when referencing "
> +                       "another block device");
> +            return -EINVAL;
> +        }
> +
> +        if (filename || options_non_empty) {
> +            error_setg(errp, "Cannot reference an existing block device with "
> +                       "additional options or a new filename");
> +            return -EINVAL;
> +        }
> +
> +        bs = bdrv_lookup_bs(reference, reference, errp);
> +        if (!bs) {
> +            return -ENODEV;
> +        }
> +        bdrv_ref(bs);
> +        *pbs = bs;
> +        return 0;
> +    }
> +
>      if (*pbs) {
>          bs = *pbs;
>      } else {
> @@ -1268,7 +1298,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
>          /* Get the required size from the image */
>          QINCREF(options);
>          bs1 = NULL;
> -        ret = bdrv_open(&bs1, filename, options, BDRV_O_NO_BACKING,
> +        ret = bdrv_open(&bs1, filename, NULL, options, BDRV_O_NO_BACKING,
>                          drv, &local_err);
>          if (ret < 0) {
>              goto fail;
> @@ -5309,7 +5339,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>                  flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>  
>              bs = NULL;
> -            ret = bdrv_open(&bs, backing_file->value.s, NULL, back_flags,
> +            ret = bdrv_open(&bs, backing_file->value.s, NULL, NULL, back_flags,
>                              backing_drv, &local_err);
>              if (ret < 0) {
>                  error_setg_errno(errp, -ret, "Could not open '%s': %s",
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 309ea12..e2aa5c1 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1553,7 +1553,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>       */
>      BlockDriver* drv = bdrv_find_format("qcow2");
>      assert(drv != NULL);
> -    ret = bdrv_open(&bs, filename, NULL,
> +    ret = bdrv_open(&bs, filename, NULL, NULL,
>          BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
> @@ -1604,7 +1604,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>      bs = NULL;
>  
>      /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
> -    ret = bdrv_open(&bs, filename, NULL,
> +    ret = bdrv_open(&bs, filename, NULL, NULL,
>                      BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
>                      drv, &local_err);
>      if (local_err) {
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 0622db5..5df486f 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1756,7 +1756,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>      }
>      if (backing_file) {
>          BlockDriverState *bs = NULL;
> -        ret = bdrv_open(&bs, backing_file, NULL, BDRV_O_NO_BACKING, NULL, errp);
> +        ret = bdrv_open(&bs, backing_file, NULL, NULL, BDRV_O_NO_BACKING, NULL,
> +                        errp);
>          if (ret != 0) {
>              goto exit;
>          }
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 0a9f886..4bde3bd 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2937,7 +2937,7 @@ static int enable_write_target(BDRVVVFATState *s)
>      }
>  
>      s->qcow = NULL;
> -    ret = bdrv_open(&s->qcow, s->qcow_filename, NULL,
> +    ret = bdrv_open(&s->qcow, s->qcow_filename, NULL, NULL,
>              BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
>              &local_err);
>      if (ret < 0) {
> diff --git a/blockdev.c b/blockdev.c
> index 056ec4c..357f760 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -504,7 +504,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>      bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>  
>      QINCREF(bs_opts);
> -    ret = bdrv_open(&dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
> +    ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, &error);
>  
>      if (ret < 0) {
>          error_setg(errp, "could not open disk image %s: %s",
> @@ -1333,7 +1333,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>      /* TODO Inherit bs->options or only take explicit options with an
>       * extended QMP command? */
>      assert(state->new_bs == NULL);
> -    ret = bdrv_open(&state->new_bs, new_image_file, options,
> +    ret = bdrv_open(&state->new_bs, new_image_file, NULL, options,
>                      flags | BDRV_O_NO_BACKING, drv, &local_err);
>      /* We will manually add the backing_hd field to the bs later */
>      if (ret != 0) {
> @@ -1582,7 +1582,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
>      Error *local_err = NULL;
>      int ret;
>  
> -    ret = bdrv_open(&bs, filename, NULL, bdrv_flags, drv, &local_err);
> +    ret = bdrv_open(&bs, filename, NULL, NULL, bdrv_flags, drv, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return;
> @@ -2019,7 +2019,7 @@ void qmp_drive_backup(const char *device, const char *target,
>      }
>  
>      target_bs = NULL;
> -    ret = bdrv_open(&target_bs, target, NULL, flags, drv, &local_err);
> +    ret = bdrv_open(&target_bs, target, NULL, NULL, flags, drv, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return;
> @@ -2162,8 +2162,8 @@ void qmp_drive_mirror(const char *device, const char *target,
>       * file.
>       */
>      target_bs = NULL;
> -    ret = bdrv_open(&target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
> -                    &local_err);
> +    ret = bdrv_open(&target_bs, target, NULL, NULL, flags | BDRV_O_NO_BACKING,
> +                    drv, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return;
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 14e6d0b..61e6ff3 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -813,8 +813,8 @@ static int blk_connect(struct XenDevice *xendev)
>              Error *local_err = NULL;
>              BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
>                                                             readonly);
> -            if (bdrv_open(&blkdev->bs,
> -                          blkdev->filename, NULL, qflags, drv, &local_err) != 0)
> +            if (bdrv_open(&blkdev->bs, blkdev->filename, NULL, NULL, qflags,
> +                          drv, &local_err) != 0)
>              {
>                  xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
>                                error_get_pretty(local_err));
> diff --git a/include/block/block.h b/include/block/block.h
> index 980869d..a421041 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -190,8 +190,9 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>                      QDict *options, const char *bdref_key, int flags,
>                      bool force_raw, bool allow_none, Error **errp);
>  int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
> -int bdrv_open(BlockDriverState **pbs, const char *filename, QDict *options,
> -              int flags, BlockDriver *drv, Error **errp);
> +int bdrv_open(BlockDriverState **pbs, const char *filename,
> +              const char *reference, QDict *options, int flags,
> +              BlockDriver *drv, Error **errp);
>  BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>                                      BlockDriverState *bs, int flags);
>  int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
> diff --git a/qemu-img.c b/qemu-img.c
> index 59b1013..79ab3e8 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -289,7 +289,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
>          drv = NULL;
>      }
>  
> -    ret = bdrv_open(&bs, filename, NULL, flags, drv, &local_err);
> +    ret = bdrv_open(&bs, filename, NULL, NULL, flags, drv, &local_err);
>      if (ret < 0) {
>          error_report("Could not open '%s': %s", filename,
>                       error_get_pretty(local_err));
> @@ -2312,7 +2312,7 @@ static int img_rebase(int argc, char **argv)
>  
>          bs_old_backing = bdrv_new("old_backing");
>          bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
> -        ret = bdrv_open(&bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
> +        ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, BDRV_O_FLAGS,
>                          old_backing_drv, &local_err);
>          if (ret) {
>              error_report("Could not open old backing file '%s': %s",
> @@ -2322,8 +2322,8 @@ static int img_rebase(int argc, char **argv)
>          }
>          if (out_baseimg[0]) {
>              bs_new_backing = bdrv_new("new_backing");
> -            ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
> -                        new_backing_drv, &local_err);
> +            ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL,
> +                            BDRV_O_FLAGS, new_backing_drv, &local_err);
>              if (ret) {
>                  error_report("Could not open new backing file '%s': %s",
>                               out_baseimg, error_get_pretty(local_err));
> diff --git a/qemu-io.c b/qemu-io.c
> index 8da8f6e..61d54c0 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -68,7 +68,9 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
>      } else {
>          qemuio_bs = bdrv_new("hda");
>  
> -        if (bdrv_open(&qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
> +        if (bdrv_open(&qemuio_bs, name, NULL, opts, flags, NULL, &local_err)
> +            < 0)
> +        {

checkpatch.pl is not happy with this open brace on new line.

Fam