[PATCH 0/2] qemu: blockjob: Fix ordering of operations when concluding a block copy
Modify the ordering so that the VM definition stays in a state allowing us to format a valid XML for the virt-aa-helper. Peter Krempa (2): qemu: blockjob: Update 'mirror' of a copy job before removing images qemu: blockjob: Clean out disk mirror data after concluding the job src/qemu/qemu_blockjob.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) -- 2.46.0
When concluding a job with a 'mirror' we first unplugged the appropriate no-longer used images from qemu and then updated the definition. Normally this wouldn't be a problem because for any other thread this is done under the VM lock thus atomic. Unfortunately though, the AppArmor security backend is using a VM XML to pass data to the helper process and the state of the definition at that point was unsuitable to format a valid XML thus making 'virt-aa-helper' report parsing failure. Since we're removing the images the proper state of the VM definition indeed should not include the mirror element any more at the point when the images are removed. Closes: https://gitlab.com/libvirt/libvirt/-/issues/601 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 42856df6d4..9c56d5cb95 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1218,6 +1218,8 @@ qemuBlockJobProcessEventConcludedCopyPivot(virQEMUDriver *driver, virDomainAsyncJob asyncJob) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virStorageSource) src = NULL; + VIR_DEBUG("copy job '%s' on VM '%s' pivoted", job->name, vm->def->name); /* mirror may be NULL for copy job corresponding to migration */ @@ -1241,9 +1243,11 @@ qemuBlockJobProcessEventConcludedCopyPivot(virQEMUDriver *driver, qemuBlockJobRewriteConfigDiskSource(vm, job->disk, job->disk->mirror); - qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, job->disk->src); - virObjectUnref(job->disk->src); + src = g_steal_pointer(&job->disk->src); + job->disk->src = g_steal_pointer(&job->disk->mirror); + + qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, src); } @@ -1254,6 +1258,7 @@ qemuBlockJobProcessEventConcludedCopyAbort(virQEMUDriver *driver, virDomainAsyncJob asyncJob) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virStorageSource) mirror = NULL; VIR_DEBUG("copy job '%s' on VM '%s' aborted", job->name, vm->def->name); @@ -1277,9 +1282,10 @@ qemuBlockJobProcessEventConcludedCopyAbort(virQEMUDriver *driver, g_clear_pointer(&job->disk->mirror->backingStore, virObjectUnref); } + mirror = g_steal_pointer(&job->disk->mirror); + /* activeWrite bitmap is removed automatically here */ - qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, job->disk->mirror); - g_clear_pointer(&job->disk->mirror, virObjectUnref); + qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, mirror); } -- 2.46.0
The 'disk->mirrorJob' and 'disk->mirrorState' fields need to be cleared after a blockjob, but should be kept around while 'disk->mirror' is still in place. As 'disk->mirror' is cleared only after conclusion of the job in 'qemuBlockJobEventProcessConcluded()' we should be resetting them only afterwards. Move the code later, but since the job is unregistered from the disk we need to store the pointer to the disk before concluding the job. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 9c56d5cb95..652b25540a 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1561,12 +1561,18 @@ qemuBlockJobEventProcess(virQEMUDriver *driver, case QEMU_BLOCKJOB_STATE_COMPLETED: case QEMU_BLOCKJOB_STATE_FAILED: case QEMU_BLOCKJOB_STATE_CANCELLED: - case QEMU_BLOCKJOB_STATE_CONCLUDED: - if (job->disk) { - job->disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; - job->disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; - } + case QEMU_BLOCKJOB_STATE_CONCLUDED: { + virDomainDiskDef *disk = job->disk; + qemuBlockJobEventProcessConcluded(job, driver, vm, asyncJob); + + /* Job was unregistered from the disk but we must ensure that the + * data is cleared */ + if (disk) { + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; + } + } break; case QEMU_BLOCKJOB_STATE_READY: -- 2.46.0
On Fri, Sep 27, 2024 at 09:21:22AM +0200, Peter Krempa wrote:
Modify the ordering so that the VM definition stays in a state allowing us to format a valid XML for the virt-aa-helper.
Peter Krempa (2): qemu: blockjob: Update 'mirror' of a copy job before removing images qemu: blockjob: Clean out disk mirror data after concluding the job
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (2)
-
Pavel Hrdina -
Peter Krempa