Re: [Qemu-devel] [PATCH 21/47] block: add bdrv_ensure_backing_file

[Kevin Wolf <kwolf@redhat.com>]

Am 24.07.2012 13:03, schrieb Paolo Bonzini:
> Mirroring runs without the backing file so that it can be copied outside
> QEMU.  However, we need to add it at the time the job is completed and
> QEMU switches to the target.  Factor out the common bits of opening an
> image and completing a mirroring operation.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Once again, combining code motion and code changes in one patch makes it
harder to review.

bdrv_ensure_backing_file() isn't a good name, after reading only the
subject line I had no idea what this function might do. It's still not
entirely clear to me what the different to a bdrv_open_backing_file()
is, except that it doesn't do anything if a backing file is already
open. In which cases do we rely on this behaviour?

> ---
>  block.c |   69 ++++++++++++++++++++++++++++++++++++++++-----------------------
>  block.h |    1 +
>  2 files changed, 45 insertions(+), 25 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 19da114..002b442 100644
> --- a/block.c
> +++ b/block.c
> @@ -730,6 +730,48 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags)
>      return 0;
>  }
>  
> +int bdrv_ensure_backing_file(BlockDriverState *bs)
> +{
> +    char backing_filename[PATH_MAX];
> +    int back_flags, ret;
> +    BlockDriver *back_drv = NULL;
> +
> +    if (bs->backing_hd != NULL) {
> +        return 0;
> +    }
> +
> +    bs->open_flags &= ~BDRV_O_NO_BACKING;

This doesn't do anything in this patch because the function is never
called with BDRV_O_NO_BACKING set. Is it in preparation for a second user?

> +    if (bs->backing_file[0] == '\0') {
> +        return 0;
> +    }
> +
> +    bs->backing_hd = bdrv_new("");
> +    bdrv_get_full_backing_filename(bs, backing_filename,
> +                                   sizeof(backing_filename));
> +
> +    if (bs->backing_format[0] != '\0') {
> +        back_drv = bdrv_find_format(bs->backing_format);
> +    }
> +
> +    /* backing files always opened read-only */
> +    back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT);
> +
> +    ret = bdrv_open(bs->backing_hd, backing_filename, back_flags, back_drv);
> +    if (ret < 0) {
> +        bdrv_close(bs);

I don't like this because it makes the invalid assumption that the
caller has just opened bs and wants to close it if opening the backing
file fails. I think this is part of the error handling that belongs in
the caller: It opened bs, so it is responsible for closing it in error
cases.

> +        bdrv_delete(bs->backing_hd);

This is a bug fix of its own, should be a separate patch.

> +        bs->backing_hd = NULL;
> +        return ret;
> +    }
> +    if (bs->is_temporary) {
> +        bs->backing_hd->keep_read_only = !(bs->open_flags & BDRV_O_RDWR);
> +    } else {
> +        /* base images use the same setting as leaf */

Huh, "parent" turned into "leaf"?

> +        bs->backing_hd->keep_read_only = bs->keep_read_only;
> +    }
> +    return 0;
> +}

Kevin