Re: [Qemu-devel] [PATCH 7/7] block: assign backing relationship in drive-backup

[Stefan Hajnoczi]

On Tue, Jul 02, 2013 at 01:59:49PM +0800, Fam Zheng wrote:
> Assign source image as the backing hd of target bs, so reading target bs
> gets the point-in-time copy of data from source image.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block/backup.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 4e9f927..2dd0540 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -294,6 +294,11 @@ static void coroutine_fn backup_run(void *opaque)
>      hbitmap_free(job->bitmap);
>  
>      bdrv_iostatus_disable(target);
> +
> +    bdrv_put_ref(target->backing_hd);
> +    target->backing_hd = NULL;
> +    target->backing_file[0] = '\0';
> +    target->backing_format[0] = '\0';
>      bdrv_put_ref(target);
>  
>      block_job_completed(&job->common, ret);
> @@ -332,7 +337,15 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
> *target,
>          return;
>      }
>  
> +    target->backing_hd = bs;
> +    pstrcpy(target->backing_file, sizeof(target->backing_file),
> +            bs->filename);
> +    pstrcpy(target->backing_format, sizeof(target->backing_format),
> +            bs->drv->format_name);
>      bdrv_get_ref(target);
> +    /* Get another ref to source for backing_hd relationship */
> +    bdrv_get_ref(bs);
> +
>      job->on_source_error = on_source_error;
>      job->on_target_error = on_target_error;
>      job->target = target;

This is a strange way of overriding the backing file.  The target exists
after drive-backup completes but now has a NULL backing_hd.

Also, we set backing_hd even for a raw image.

I thought this would be achieved as follows:

1. The management tool creates the qcow2 file.
2. drive-add if=none,id=target,file=backup.qcow2,backing_hd=drive0
3. drive-backup drive=drive0,target=target,mode=existing

Something along these lines.  The difference is that we do not force
override the backing_hd.  It is only done when the management tool/user
decides explicitly to use backing files.  Also, the target is left in a
usable state after the blockjob completes.

I know the drive-add approach has been shelved in favor of directly
using drive-backup, but this patch seems a little too hacky IMO.

Stefan