If user starts a blockcommit without --pivot then we modify
access for qemu on both images and leave it like that until
pivot is executed. So far so good. Problem is, if user instead
of issuing pivot calls destroy on the domain. In this case we
don't ever clear the access we granted at the beginning.

https://bugzilla.redhat.com/show_bug.cgi?id=1741456#c19

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_process.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 8cb12ac9a6..fa9167441e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7617,6 +7617,15 @@ void qemuProcessStop(virQEMUDriverPtr driver,
     for (i = 0; i < vm->def->niothreadids; i++)
         vm->def->iothreadids[i]->thread_id = 0;
 
+    for (i = 0; i < def->ndisks; i++) {
+        virDomainDiskDefPtr disk = def->disks[i];
+
+        if (!disk->mirror)
+            continue;
+
+        qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->mirror);
+    }
+
     /* clear all private data entries which are no longer needed */
     qemuDomainObjPrivateDataClear(priv);
 
-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Nov 19, 2019 at 10:14:09 +0100, Michal Privoznik wrote:

In subject, please remove 'Simulate blockjob --abort' and replace it
e.g. with: 'remove image metadata for running mirror jobs' or something
similar, because this patch does not really do anything with the job
itself.

> If user starts a blockcommit without --pivot then we modify
> access for qemu on both images and leave it like that until
> pivot is executed. So far so good. Problem is, if user instead
> of issuing pivot calls destroy on the domain. In this case we
> don't ever clear the access we granted at the beginning.

This applies any time a job is still running. --pivot makes sure that it
finishes after qemu reaches the correct point, but the intermediate
state is still potentially long-running.

Also note that disk->mirror is only present for an active layer commit
job but not for regular commit jobs. Does this bug apply to those as
well?

It's harder to simulate that job though, but if qemu crashes
during the job exactly the same semantics as if --pivot was not used and
the VM was destroyed instead apply. We still relabelled some images.

Also disk->mirror is present also for virDomainBlockCopy which this
commit message omits completely.

You'll have to reformulate the commit message for this patch given that
you persuade me that checking disk->mirror is enough given the existence
of non-active jobs which don't populate disk->mirror.

> https://bugzilla.redhat.com/show_bug.cgi?id=1741456#c19
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_process.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 11/19/19 10:31 AM, Peter Krempa wrote:
> On Tue, Nov 19, 2019 at 10:14:09 +0100, Michal Privoznik wrote:
> 
> In subject, please remove 'Simulate blockjob --abort' and replace it
> e.g. with: 'remove image metadata for running mirror jobs' or something
> similar, because this patch does not really do anything with the job
> itself.
> 
>> If user starts a blockcommit without --pivot then we modify
>> access for qemu on both images and leave it like that until
>> pivot is executed. So far so good. Problem is, if user instead
>> of issuing pivot calls destroy on the domain. In this case we
>> don't ever clear the access we granted at the beginning.
> 
> This applies any time a job is still running. --pivot makes sure that it
> finishes after qemu reaches the correct point, but the intermediate
> state is still potentially long-running.

Right, point is, at the time we're doing qemuProcessStop(), qemu is no 
longer running -> we are restoring seclabels. No XATTR should be left 
behind.

> 
> Also note that disk->mirror is only present for an active layer commit
> job but not for regular commit jobs. Does this bug apply to those as
> well?

Interesting question. Let me test. (testing, testing, testing). Oh yeah 
it does. Looks like I'm going to need mimic the else branch from 
qemuBlockJobEventProcessLegacyCompleted(). That is, I'll need 
qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->src); too.

> 
> It's harder to simulate that job though, but if qemu crashes
> during the job exactly the same semantics as if --pivot was not used and
> the VM was destroyed instead apply. We still relabelled some images.

Oh right. But that's basically the same thing. The only difference is 
whether libvirt initiated qemu shutdown or not.

> 
> Also disk->mirror is present also for virDomainBlockCopy which this
> commit message omits completely.

Okay, will mention it in v2.

> 
> You'll have to reformulate the commit message for this patch given that
> you persuade me that checking disk->mirror is enough given the existence
> of non-active jobs which don't populate disk->mirror.
> 
>> https://bugzilla.redhat.com/show_bug.cgi?id=1741456#c19
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   src/qemu/qemu_process.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list