Re: [Qemu-devel] [PATCH] mirror: prevent 'top' mode mirroring when no backing file specified on the destination

[Max Reitz]

On 19.12.2016 23:38, sochin jiang wrote:
>  Mirroring using 'top' mode without backing file specified on the target can 
> be success,
>  but end with a disaster.
> 
>  For example:
>    Migration can be success in this situation while the virtual machine on 
> the destination
>  is no longer usable because of backing lost.
> 
>  Remind the user earlier and return error in case of misoperation.
> 
> Signed-off-by: sochin jiang <address@hidden>
> ---
>  block/mirror.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 301ba92..3476696 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1038,6 +1038,12 @@ void mirror_start(const char *job_id, BlockDriverState 
> *bs,
>          error_setg(errp, "Sync mode 'incremental' not supported");
>          return;
>      }
> +    if (mode == MIRROR_SYNC_MODE_TOP && !backing_bs(target))
> +    {

Syntactic issue: The opening brace should be on the same line as the "if".

> +        error_setg(errp, "Target Backing required using Sync mode 'top'");
> +        return;
> +    }
> +
>      is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
>      base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
>      mirror_start_job(job_id, bs, BLOCK_JOB_DEFAULT, target, replaces,

General issue: For blockdev-mirror, I think this is a legitimate use
case. As far as I'm aware, libvirt uses the mirror block job for all
backups -- they do so by cancelling the block job after the
BLOCK_JOB_READY event instead of letting it complete. So a user might
want to mirror some drive somewhere else (in sync=top mode, with the new
location not yet having assigned a backing file to it), then cancel the
block job after BLOCK_JOB_READY and only assign the backing file at some
later point.

An even greater issue is that qmp_drive_mirror() opens the target BDS
with BDRV_O_NO_BACKING. Therefore, this will always error out with
drive-mirror and sync=top (unless the source image does not have a
backing file, in which case the sync=top will silently be converted to
sync=full).

For drive-mirror, the target's backing chain will not be set up until
mirror_complete().

Max

signature.asc
Description: OpenPGP digital signature