Re: [Qemu-devel] [PATCH v4 5/7] block: Parse "backing" option to reference existing BDS

[Kevin Wolf]

Am 22.11.2013 um 06:24 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block.c                   | 37 ++++++++++++++++++++++++++++++++-----
>  include/block/block_int.h |  3 +++
>  2 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 3bf4c8a..a5da656 100644
> --- a/block.c
> +++ b/block.c
> @@ -1166,11 +1166,33 @@ int bdrv_open(BlockDriverState *bs, const char 
> *filename, QDict *options,
>      /* If there is a backing file, use it */
>      if ((flags & BDRV_O_NO_BACKING) == 0) {
>          QDict *backing_options;
> -
> -        qdict_extract_subqdict(options, &backing_options, "backing.");
> -        ret = bdrv_open_backing_file(bs, backing_options, &local_err);
> -        if (ret < 0) {
> -            goto close_and_fail;
> +        const char *backing_bs;
> +
> +        backing_bs = qdict_get_try_str(options, "backing");
> +        if (backing_bs) {
> +            bs->backing_hd = bdrv_find(backing_bs);
> +            qdict_del(options, "backing");
> +            if (bs->backing_hd) {
> +                bdrv_ref(bs->backing_hd);
> +                assert(!bs->backing_blocker);
> +                error_setg(&bs->backing_blocker,
> +                           "device is used as backing hd of '%s'",
> +                           bs->device_name);
> +                bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
> +                pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> +                        bs->backing_hd->filename);
> +                pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> +                        bs->backing_hd->drv->format_name);
> +            } else {
> +                error_setg(errp, "Backing device not found: %s", backing_bs);
> +                goto close_and_fail;
> +            }
> +        } else {
> +            qdict_extract_subqdict(options, &backing_options, "backing.");
> +            ret = bdrv_open_backing_file(bs, backing_options, &local_err);
> +            if (ret < 0) {
> +                goto close_and_fail;
> +            }
>          }
>      }

Can we move all of this code into bdrv_open_backing_file()?

I think the bdrv_op_block_all() part should apply to all kinds of
backing files. Currently you can't issue any monitor commands, so it
doesn't matter yet, but Benoît is working on adding per-node names and
then we might want this.

And consistency never hurts anyway.

> @@ -1461,6 +1483,11 @@ void bdrv_close(BlockDriverState *bs)
>  
>      if (bs->drv) {
>          if (bs->backing_hd) {
> +            if (error_is_set(&bs->backing_blocker)) {

Then this one should become unconditional as well, I think.

> +                bdrv_op_unblock_all(bs->backing_hd,
> +                                    bs->backing_blocker);
> +                error_free(bs->backing_blocker);
> +            }
>              bdrv_unref(bs->backing_hd);
>              bs->backing_hd = NULL;
>          }

Kevin