[v8,08/12] block: Parse "backing" option to reference existing BDS

Now it's safe to allow reference for backing_hd in the interface.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

On Fri, Dec 13, 2013 at 03:35:16PM +0800, Fam Zheng wrote:
> diff --git a/block.c b/block.c
> index b3993d7..fba7148 100644
> --- a/block.c
> +++ b/block.c
> @@ -1191,11 +1191,25 @@ 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_name;
> +        BlockDriverState *backing_hd;
> +
> +        backing_name = qdict_get_try_str(options, "backing");
> +        qdict_del(options, "backing");

This causes a use-after-free since backing_name is a const char pointer
to the qdict element!

> +        if (backing_name) {
> +            backing_hd = bdrv_find(backing_name);
> +            if (!backing_hd) {
> +                error_set(&local_err, QERR_DEVICE_NOT_FOUND, backing_name);
> +                ret = -ENOENT;
> +                goto close_and_fail;
> +            }
> +            bdrv_set_backing_hd(bs, backing_hd);
> +        } 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;
> +            }
>          }

Seems like users can specify backing=foo backing.file=/tmp/a and we
ignore backing.file.  Is it useful to silently ignore the backing.
subdict?  The user may have given useless options by mistake.  An error
would help prevent weird options combinations.

>      }
>  
> @@ -1682,7 +1696,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
>      assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
>      assert(bs_new->job == NULL);
>      assert(bs_new->dev == NULL);
> -    assert(bdrv_op_blocker_is_empty(bs_new));
>      assert(bs_new->io_limits_enabled == false);
>      assert(!throttle_have_timer(&bs_new->throttle_state));
>  
> @@ -1701,7 +1714,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
>      /* Check a few fields that should remain attached to the device */
>      assert(bs_new->dev == NULL);
>      assert(bs_new->job == NULL);
> -    assert(bdrv_op_blocker_is_empty(bs_new));
>      assert(bs_new->io_limits_enabled == false);
>      assert(!throttle_have_timer(&bs_new->throttle_state));

Why are these hunks part of this patch?  I guess it makes sense *not* to
check for blockers in bdrv_swap().  Instead the high-level functions in
blockdev.c and elsewhere should check blockers.

On 2014年01月03日 17:19, Stefan Hajnoczi wrote:
>>       }
>>
>> @@ -1682,7 +1696,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
>>       assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
>>       assert(bs_new->job == NULL);
>>       assert(bs_new->dev == NULL);
>> -    assert(bdrv_op_blocker_is_empty(bs_new));
>>       assert(bs_new->io_limits_enabled == false);
>>       assert(!throttle_have_timer(&bs_new->throttle_state));
>>
>> @@ -1701,7 +1714,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
>>       /* Check a few fields that should remain attached to the device */
>>       assert(bs_new->dev == NULL);
>>       assert(bs_new->job == NULL);
>> -    assert(bdrv_op_blocker_is_empty(bs_new));
>>       assert(bs_new->io_limits_enabled == false);
>>       assert(!throttle_have_timer(&bs_new->throttle_state));
>
> Why are these hunks part of this patch?  I guess it makes sense *not* to
> check for blockers in bdrv_swap().  Instead the high-level functions in
> blockdev.c and elsewhere should check blockers.
>

The two checks are here because of the mechanical replace of in_use. 
They are removed because it is no longer true for some valid cases, e.g 
with "block-commit". So we need these hunks here, or do this as a 
preceding in the series.

Will update the commit message and keep it as is.

Fam