I got the idea, thanks, Max.
Sochin.Jiang
On 2016/12/19 23:31, Max Reitz wrote:
> 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 <sochin.ji...@huawei.com>
>> ---
>> 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
>
