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 <pbonz...@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
