[7/7] block: assign backing relationship in drive-backup
diff mbox

Message ID 1372744789-997-8-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng July 2, 2013, 5:59 a.m. UTC
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 <famz@redhat.com>
---
 block/backup.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Stefan Hajnoczi July 4, 2013, 2:32 p.m. UTC | #1
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 <famz@redhat.com>
> ---
>  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

Patch
diff mbox

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;