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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 835 bytes --]

On 12/19/2016 04:38 PM, 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.
> 

I think this is a premature policy decision.  Even though the user can
abuse it to lose data, I think there is still enough technical reason
for why a user might have a valid use case for doing this (perhaps for
creating incremental backups, where the user plans on re-chaining data
back together using 'qemu-img rebase -u'), so I don't think we should
forbid it at the qemu level.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2226 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

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

[sochin jiang]

 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.jiang@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))
+    {
+        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,
-- 
1.8.3.1

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

[sochin.jiang]

 Well, that makes sense, I lose my consideration, thanks.




Sochin Jiang

On 2016/12/19 23:31, Eric Blake wrote:
> On 12/19/2016 04:38 PM, 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.
>>
> 
> I think this is a premature policy decision.  Even though the user can
> abuse it to lose data, I think there is still enough technical reason
> for why a user might have a valid use case for doing this (perhaps for
> creating incremental backups, where the user plans on re-chaining data
> back together using 'qemu-img rebase -u'), so I don't think we should
> forbid it at the qemu level.
> 

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

[sochin.jiang]

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

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

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH] mirror: prevent 'top' mode mirroring when no backing file specified on the destination
Message-id: 1482187106-85065-1-git-send-email-sochin.jiang@huawei.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
05e3f52 mirror: prevent 'top' mode mirroring when no backing file specified on the destination

=== OUTPUT BEGIN ===
Checking PATCH 1/1: mirror: prevent 'top' mode mirroring when no backing file specified on the destination...
ERROR: that open brace { should be on the previous line
#27: FILE: block/mirror.c:1041:
+    if (mode == MIRROR_SYNC_MODE_TOP && !backing_bs(target))
+    {

total: 1 errors, 0 warnings, 12 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org