Re: [Qemu-devel] [PATCH v15 05/14] block: Add bdrv_set_backing_hd()

[Jeff Cody]

On Sun, Feb 23, 2014 at 09:54:46AM +0800, Fam Zheng wrote:
> This is the common but non-trivial steps to assign or change the
> backing_hd of BDS.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block.c               | 46 ++++++++++++++++++++++++++++++++++++++--------
>  include/block/block.h |  1 +
>  2 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 684b9d6..9caade9 100644
> --- a/block.c
> +++ b/block.c
> @@ -1041,6 +1041,32 @@ fail:
>      return ret;
>  }
>  
> +void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
> +{
> +    if (backing_hd) {
> +        /* Grab the reference before unref original backing_hd, so we are 
> safe
> +         * when rebasing in the backing chain.
> +         */
> +        bdrv_ref(backing_hd);
> +    }
> +
> +    if (bs->backing_hd) {
> +        bdrv_unref(bs->backing_hd);
> +    }
> +
> +    bs->backing_hd = backing_hd;
> +    if (!backing_hd) {
> +        bs->backing_file[0] = '\0';
> +        bs->backing_format[0] = '\0';
> +        goto out;
> +    }
> +    pstrcpy(bs->backing_file, sizeof(bs->backing_file), 
> backing_hd->filename);
> +    pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> +            backing_hd->drv ? backing_hd->drv->format_name : "");
> +out:
> +    bdrv_refresh_limits(bs);
> +}
> +
>  /*
>   * Opens the backing file for a BlockDriverState if not yet open
>   *
> @@ -1054,6 +1080,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
> *options, Error **errp)
>      char backing_filename[PATH_MAX];
>      int back_flags, ret;
>      BlockDriver *back_drv = NULL;
> +    BlockDriverState *backing_hd;
>      Error *local_err = NULL;
>  
>      if (bs->backing_hd != NULL) {
> @@ -1077,6 +1104,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
> *options, Error **errp)
>                                         sizeof(backing_filename));
>      }
>  
> +    backing_hd = bdrv_new("");
> +
>      if (bs->backing_format[0] != '\0') {
>          back_drv = bdrv_find_format(bs->backing_format);
>      }
> @@ -1085,23 +1114,24 @@ 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);
>  
> -    assert(bs->backing_hd == NULL);
> -    ret = bdrv_open(&bs->backing_hd,
> +    ret = bdrv_open(&backing_hd,
>                      *backing_filename ? backing_filename : NULL, NULL, 
> options,
>                      back_flags, back_drv, &local_err);
>      if (ret < 0) {
>          bs->backing_hd = NULL;

Assigning bs->backing_hd = NULL is no longer needed now, so this
should be removed.

> +        bdrv_unref(backing_hd);
> +        backing_hd = NULL;
>          bs->open_flags |= BDRV_O_NO_BACKING;
>          error_setg(errp, "Could not open backing file: %s",
>                     error_get_pretty(local_err));
>          error_free(local_err);
>          return ret;
>      }
> -
> -    if (bs->backing_hd->file) {
> -        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> -                bs->backing_hd->file->filename);
> -    }
> +    bdrv_set_backing_hd(bs, backing_hd);
> +    /* Now we have refcnt = 2 on backing_hd by bdrv_new and
> +     * bdrv_set_backing_hd, while we only need 1 */
> +    assert(backing_hd->refcnt == 2);
> +    bdrv_unref(backing_hd);

It is a bit worrisome to manually track refcnt like this, and I think
it might be red flag that bdrv_set_backing_hd() isn't entirely
correct.  It seems wrong (and a bit fragile) to have to compensate for
what is essentially a spurious bdrv_ref() call by another function.

Also, this relies on implicit knowledge that bdrv_set_backing_hd()
increases the refcnt, and that the refcnt is increased only once in
bdrv_set_backing_fd().


>  
>      /* Recalculate the BlockLimits with the backing file */
>      bdrv_refresh_limits(bs);
> @@ -1923,7 +1953,7 @@ void bdrv_append(BlockDriverState *bs_new, 
> BlockDriverState *bs_top)
>  
>      /* The contents of 'tmp' will become bs_top, as we are
>       * swapping bs_new and bs_top contents. */
> -    bs_top->backing_hd = bs_new;
> +    bdrv_set_backing_hd(bs_top, bs_new);

If the bs_top->backing_hd is NULL, this means the refcnt on bs_new is
off by one now, right?  And I think in the current usage,
bs_top->backing_hd will always be NULL here.

>      bs_top->open_flags &= ~BDRV_O_NO_BACKING;

Hmm, this should probably be in bdrv_set_backing_hd() for any non-NULL
backing_hd value.


>      pstrcpy(bs_top->backing_file, sizeof(bs_top->backing_file),
>              bs_new->filename);

This line, and the one below it (not in the diff context) are now
redundant, and should be removed.

> diff --git a/include/block/block.h b/include/block/block.h
> index a46f70a..ee1582d 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -208,6 +208,7 @@ int bdrv_parse_discard_flags(const char *mode, int 
> *flags);
>  int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>                      QDict *options, const char *bdref_key, int flags,
>                      bool allow_none, Error **errp);
> +void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
>  int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error 
> **errp);
>  int bdrv_open(BlockDriverState **pbs, const char *filename,
>                const char *reference, QDict *options, int flags,
> -- 
> 1.8.5.4
> 
>