[libvirt] [PATCH 0/2] qemu: Fix blockjob assignment collision (blockdev-add saga)
We leaked a block job object in the tests because two jobs with distinct names referenced the same disk. Fix the test data and add code to prevent this issue. Peter Krempa (2): tests: qemustatusxml2xml: Fix disk target mess qemu: blockjob: Refuse to register blockjob if disk already has one src/qemu/qemu_blockjob.c | 6 ++++++ .../qemustatusxml2xmldata/blockjob-blockdev-in.xml | 14 +++++++------- 2 files changed, 13 insertions(+), 7 deletions(-) -- 2.21.0
There were accidentally two disks with 'vdc' target with corresponding blockjobs which made libvirt leak some references as there are not supposed to be two blockjobs for a single disk. Fix this mess by renaming some of the disks. In addition the block job names also didn't correspond to the naming convetion which also includes the disk target. Fix it as well. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../qemustatusxml2xmldata/blockjob-blockdev-in.xml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml index 67252a3729..e9031b7087 100644 --- a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml +++ b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml @@ -238,8 +238,8 @@ <disk dst='vdb'/> <base node='libvirt-14-format'/> </blockjob> - <blockjob name='pull-vdd-libvirt-17-format' type='active-commit' state='ready'> - <disk dst='vdd'/> + <blockjob name='commit-vde-libvirt-17-format' type='active-commit' state='ready'> + <disk dst='vde'/> <base node='libvirt-19-format'/> <top node='libvirt-17-format'/> </blockjob> @@ -260,10 +260,10 @@ </source> </src> </blockjob> - <blockjob name='pull-vdc-libvirt-4321-format' type='copy' state='ready' shallownew='yes'> - <disk dst='vdc'/> + <blockjob name='copy-vdd-libvirt-4321-format' type='copy' state='ready' shallownew='yes'> + <disk dst='vdd'/> </blockjob> - <blockjob name='pull-vdc-libvirt-9-format' type='commit' state='running'> + <blockjob name='commit-vdc-libvirt-9-format' type='commit' state='running'> <disk dst='vdc'/> <base node='libvirt-11-format'/> <top node='libvirt-9-format'/> @@ -585,7 +585,7 @@ </privateData> </source> </mirror> - <target dev='vdc' bus='virtio'/> + <target dev='vdd' bus='virtio'/> <alias name='virtio-disk3'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x0d' function='0x0'/> <privateData> @@ -655,7 +655,7 @@ <format type='qcow2'/> <source file='/tmp/activecommit2.qcow2'/> </mirror> - <target dev='vdd' bus='virtio'/> + <target dev='vde' bus='virtio'/> <alias name='virtio-disk3'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> <privateData> -- 2.21.0
On 9/14/19 3:44 AM, Peter Krempa wrote:
There were accidentally two disks with 'vdc' target with corresponding blockjobs which made libvirt leak some references as there are not supposed to be two blockjobs for a single disk. Fix this mess by renaming some of the disks.
In addition the block job names also didn't correspond to the naming convetion which also includes the disk target. Fix it as well.
convention
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../qemustatusxml2xmldata/blockjob-blockdev-in.xml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Most code paths prevent starting a blockjob if we already have one but the job registering function does not do this check. While this isn't a problem for regular cases we had a bad test case where we registered two jobs for a single disk which leaked one of the jobs. Prevent this in the registering function until we allow having multiple jobs per disk. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index a991309ee7..80d0269128 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -143,6 +143,12 @@ qemuBlockJobRegister(qemuBlockJobDataPtr job, { qemuDomainObjPrivatePtr priv = vm->privateData; + if (disk && QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("disk '%s' has a blockjob assigned"), disk->dst); + return -1; + } + if (virHashAddEntry(priv->blockjobs, job->name, virObjectRef(job)) < 0) { virObjectUnref(job); return -1; -- 2.21.0
On 9/14/19 3:44 AM, Peter Krempa wrote:
Most code paths prevent starting a blockjob if we already have one but the job registering function does not do this check. While this isn't a problem for regular cases we had a bad test case where we registered two jobs for a single disk which leaked one of the jobs. Prevent this in the registering function until we allow having multiple jobs per disk.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index a991309ee7..80d0269128 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -143,6 +143,12 @@ qemuBlockJobRegister(qemuBlockJobDataPtr job, { qemuDomainObjPrivatePtr priv = vm->privateData;
+ if (disk && QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("disk '%s' has a blockjob assigned"), disk->dst); + return -1; + } +
Can the user ever trigger this, in which case OPERATION_FAILED might be a nicer message? ACK series. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Sat, Sep 14, 2019 at 21:04:28 -0500, Eric Blake wrote:
On 9/14/19 3:44 AM, Peter Krempa wrote:
Most code paths prevent starting a blockjob if we already have one but the job registering function does not do this check. While this isn't a problem for regular cases we had a bad test case where we registered two jobs for a single disk which leaked one of the jobs. Prevent this in the registering function until we allow having multiple jobs per disk.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index a991309ee7..80d0269128 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -143,6 +143,12 @@ qemuBlockJobRegister(qemuBlockJobDataPtr job, { qemuDomainObjPrivatePtr priv = vm->privateData;
+ if (disk && QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("disk '%s' has a blockjob assigned"), disk->dst); + return -1; + } +
Can the user ever trigger this, in which case OPERATION_FAILED might be a nicer message?
All block job entrypoints should properly catch this error beforehand, so this is a programmer error check. Look for qemuDomainDiskBlockJobIsActive().
On Sat, Sep 14, 2019 at 10:43:59AM +0200, Peter Krempa wrote:
We leaked a block job object in the tests because two jobs with distinct names referenced the same disk. Fix the test data and add code to prevent this issue.
This makes way more sense than the patch form Michal, so I prefer this version. The fact that it gets triggered by an invalid test made me think that it is not possible to cause in a real life scenario, but I could not find a place where it would actually allow this to happen in a normal user workflow. But I'm not that familiar with that part of the codebase. Feel free to consider this also Reviewed-by: Martin Kletzander <mkletzan@redhat.com> With what Erik pointed out, but I would really, *really* like to know the answer to his question as I would like to know how wrong I was with my hypothesis ;-)
Peter Krempa (2): tests: qemustatusxml2xml: Fix disk target mess qemu: blockjob: Refuse to register blockjob if disk already has one
src/qemu/qemu_blockjob.c | 6 ++++++ .../qemustatusxml2xmldata/blockjob-blockdev-in.xml | 14 +++++++------- 2 files changed, 13 insertions(+), 7 deletions(-)
-- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Eric Blake -
Martin Kletzander -
Peter Krempa