Even if an op blocker is present for BLOCK_OP_TYPE_MIRROR_SOURCE,
it is checked a bit late and the result is that the target is
created even if drive-mirror subsequently fails.  Add an early
check to avoid this.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 blockdev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 8e977eef11..c7e2e0a00e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3565,6 +3565,11 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
         return;
     }
 
+    /* Early check to avoid creating target */
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
+        return;
+    }
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
-- 
2.14.3

On Wed, 02/07 17:29, Paolo Bonzini wrote:
> Even if an op blocker is present for BLOCK_OP_TYPE_MIRROR_SOURCE,
> it is checked a bit late and the result is that the target is
> created even if drive-mirror subsequently fails.  Add an early
> check to avoid this.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  blockdev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 8e977eef11..c7e2e0a00e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3565,6 +3565,11 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>          return;
>      }
>  
> +    /* Early check to avoid creating target */
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
> +        return;
> +    }
> +
>      aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);
>  
> -- 
> 2.14.3
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

On Wed 07 Feb 2018 05:29:20 PM CET, Paolo Bonzini wrote:
> Even if an op blocker is present for BLOCK_OP_TYPE_MIRROR_SOURCE,
> it is checked a bit late and the result is that the target is
> created even if drive-mirror subsequently fails.  Add an early
> check to avoid this.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  blockdev.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index 8e977eef11..c7e2e0a00e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3565,6 +3565,11 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>          return;
>      }
>  
> +    /* Early check to avoid creating target */
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
> +        return;
> +    }
> +
>      aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);

Do we need to hold the AioContext in order to check for op blockers?

Berto

On 08/02/2018 11:10, Alberto Garcia wrote:
> On Wed 07 Feb 2018 05:29:20 PM CET, Paolo Bonzini wrote:
>> Even if an op blocker is present for BLOCK_OP_TYPE_MIRROR_SOURCE,
>> it is checked a bit late and the result is that the target is
>> created even if drive-mirror subsequently fails.  Add an early
>> check to avoid this.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  blockdev.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 8e977eef11..c7e2e0a00e 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3565,6 +3565,11 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>>          return;
>>      }
>>  
>> +    /* Early check to avoid creating target */
>> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
>> +        return;
>> +    }
>> +
>>      aio_context = bdrv_get_aio_context(bs);
>>      aio_context_acquire(aio_context);
> 
> Do we need to hold the AioContext in order to check for op blockers?

In include/block/block_int.h, they are not in the "Protected by
AioContext lock" section.

Paolo

On Wed 07 Feb 2018 05:29:20 PM CET, Paolo Bonzini wrote:
> Even if an op blocker is present for BLOCK_OP_TYPE_MIRROR_SOURCE,
> it is checked a bit late and the result is that the target is
> created even if drive-mirror subsequently fails.  Add an early
> check to avoid this.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Am 07.02.2018 um 17:29 hat Paolo Bonzini geschrieben:
> Even if an op blocker is present for BLOCK_OP_TYPE_MIRROR_SOURCE,
> it is checked a bit late and the result is that the target is
> created even if drive-mirror subsequently fails.  Add an early
> check to avoid this.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks, applied to the block branch.

Kevin

On 02/07/2018 11:29 AM, Paolo Bonzini wrote:
> Even if an op blocker is present for BLOCK_OP_TYPE_MIRROR_SOURCE,
> it is checked a bit late and the result is that the target is
> created even if drive-mirror subsequently fails.  Add an early
> check to avoid this.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  blockdev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 8e977eef11..c7e2e0a00e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3565,6 +3565,11 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>          return;
>      }
>  
> +    /* Early check to avoid creating target */
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
> +        return;
> +    }
> +
>      aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);
>  
> 

What's the implication of the temporarily-extant target node that it
needs to be avoided so strictly?

On 10/02/2018 00:07, John Snow wrote:
>> +    /* Early check to avoid creating target */
>> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
>> +        return;
>> +    }
>> +
>>      aio_context = bdrv_get_aio_context(bs);
>>      aio_context_acquire(aio_context);
>>  
>>
> What's the implication of the temporarily-extant target node that it
> needs to be avoided so strictly?
> 

Creating a file on disk, that no one will ever remvoe. :)

Paolo

On Mon, Feb 12, 2018 at 10:58:31AM +0100, Paolo Bonzini wrote:
> On 10/02/2018 00:07, John Snow wrote:
> >> +    /* Early check to avoid creating target */
> >> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
> >> +        return;
> >> +    }
> >> +
> >>      aio_context = bdrv_get_aio_context(bs);
> >>      aio_context_acquire(aio_context);
> >>  
> >>
> > What's the implication of the temporarily-extant target node that it
> > needs to be avoided so strictly?
> > 
> 
> Creating a file on disk, that no one will ever remvoe. :)

Fortunately libvirt's SELinux policy will probably prevent QEMU creating
it in the first place :-)

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Am 12.02.2018 um 11:02 hat Daniel P. Berrangé geschrieben:
> On Mon, Feb 12, 2018 at 10:58:31AM +0100, Paolo Bonzini wrote:
> > On 10/02/2018 00:07, John Snow wrote:
> > >> +    /* Early check to avoid creating target */
> > >> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
> > >> +        return;
> > >> +    }
> > >> +
> > >>      aio_context = bdrv_get_aio_context(bs);
> > >>      aio_context_acquire(aio_context);
> > >>  
> > >>
> > > What's the implication of the temporarily-extant target node that it
> > > needs to be avoided so strictly?
> > > 
> > 
> > Creating a file on disk, that no one will ever remvoe. :)
> 
> Fortunately libvirt's SELinux policy will probably prevent QEMU creating
> it in the first place :-)

Well, calling drive-mirror without allowing QEMU to create the target
image would be a bit pointless, so I think we can assume that libvirt
did set up the file permission so that QEMU can create it. (Unless
mode=existing is used, but I understand that libvirt doesn't want to
create images with qemu-img, so that doesn't seem to be the case...)

I don't know if libvirt takes care to remove a potentially already
created file if the command then fails, but hopefully it does and the
patch is not actually needed with libvirt.

Kevin

On Mon, Feb 12, 2018 at 01:42:11PM +0100, Kevin Wolf wrote:
> Am 12.02.2018 um 11:02 hat Daniel P. Berrangé geschrieben:
> > On Mon, Feb 12, 2018 at 10:58:31AM +0100, Paolo Bonzini wrote:
> > > On 10/02/2018 00:07, John Snow wrote:
> > > >> +    /* Early check to avoid creating target */
> > > >> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
> > > >> +        return;
> > > >> +    }
> > > >> +
> > > >>      aio_context = bdrv_get_aio_context(bs);
> > > >>      aio_context_acquire(aio_context);
> > > >>  
> > > >>
> > > > What's the implication of the temporarily-extant target node that it
> > > > needs to be avoided so strictly?
> > > > 
> > > 
> > > Creating a file on disk, that no one will ever remvoe. :)
> > 
> > Fortunately libvirt's SELinux policy will probably prevent QEMU creating
> > it in the first place :-)
> 
> Well, calling drive-mirror without allowing QEMU to create the target
> image would be a bit pointless, so I think we can assume that libvirt
> did set up the file permission so that QEMU can create it. (Unless
> mode=existing is used, but I understand that libvirt doesn't want to
> create images with qemu-img, so that doesn't seem to be the case...)

We use either mode=existing or mode=absolute-paths depending on what
the mgmt app asked for in the API call to libvirt. I'm still kind of
suprised if mode=absolute-paths will work because we ought to be
blocking the creation of the file AFAIK and we can't pre-label a file
that doesn't exist yet.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|