[libvirt] [PATCH] qemu_blockjob: Don't leak old job when registering a new one
If a disk that a block job is registered with already has a job (stored in private data) it needs to be unrefed to keep the refcounter in balance (and properly free the object): 2,577 (104 direct, 2,473 indirect) bytes in 1 blocks are definitely lost in loss record 168 of 174 at 0x4838B86: calloc (vg_replace_malloc.c:752) by 0x4A0AD42: virAllocVar (viralloc.c:346) by 0x4A84D12: virObjectNew (virobject.c:243) by 0x21EE13: qemuBlockJobDataNew (qemu_blockjob.c:111) by 0x184EBC: qemuDomainObjPrivateXMLParseBlockjobData (qemu_domain.c:3120) by 0x1854B5: qemuDomainObjPrivateXMLParseBlockjobs (qemu_domain.c:3191) by 0x186D7D: qemuDomainObjPrivateXMLParse (qemu_domain.c:3622) by 0x4B198CC: virDomainObjParseXML (domain_conf.c:21490) by 0x4B19DDF: virDomainObjParseNode (domain_conf.c:21630) by 0x4B19E7D: virDomainObjParseFile (domain_conf.c:21649) by 0x13FDD5: testCompareStatusXMLToXMLFiles (qemuxml2xmltest.c:61) by 0x16F7D2: virTestRun (testutils.c:115) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_blockjob.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index a991309ee7..67b1f677f8 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -151,6 +151,7 @@ qemuBlockJobRegister(qemuBlockJobDataPtr job, if (disk) { job->disk = disk; job->chain = virObjectRef(disk->src); + virObjectUnref(QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob); QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob = virObjectRef(job); } -- 2.21.0
On Fri, Sep 13, 2019 at 10:26:35PM +0200, Michal Privoznik wrote:
If a disk that a block job is registered with already has a job (stored in private data) it needs to be unrefed to keep the refcounter in balance (and properly free the object):
Please change the wording to something more readable, e.g.: "If a block job is registered with a disk that already has a job (stored in private data) ..." Am I correct (from reading the code) that this should only happen in tests? If yes, then: Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
2,577 (104 direct, 2,473 indirect) bytes in 1 blocks are definitely lost in loss record 168 of 174 at 0x4838B86: calloc (vg_replace_malloc.c:752) by 0x4A0AD42: virAllocVar (viralloc.c:346) by 0x4A84D12: virObjectNew (virobject.c:243) by 0x21EE13: qemuBlockJobDataNew (qemu_blockjob.c:111) by 0x184EBC: qemuDomainObjPrivateXMLParseBlockjobData (qemu_domain.c:3120) by 0x1854B5: qemuDomainObjPrivateXMLParseBlockjobs (qemu_domain.c:3191) by 0x186D7D: qemuDomainObjPrivateXMLParse (qemu_domain.c:3622) by 0x4B198CC: virDomainObjParseXML (domain_conf.c:21490) by 0x4B19DDF: virDomainObjParseNode (domain_conf.c:21630) by 0x4B19E7D: virDomainObjParseFile (domain_conf.c:21649) by 0x13FDD5: testCompareStatusXMLToXMLFiles (qemuxml2xmltest.c:61) by 0x16F7D2: virTestRun (testutils.c:115)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_blockjob.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index a991309ee7..67b1f677f8 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -151,6 +151,7 @@ qemuBlockJobRegister(qemuBlockJobDataPtr job, if (disk) { job->disk = disk; job->chain = virObjectRef(disk->src); + virObjectUnref(QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob); QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob = virObjectRef(job); }
-- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Sep 13, 2019 at 22:26:35 +0200, Michal Privoznik wrote:
If a disk that a block job is registered with already has a job (stored in private data) it needs to be unrefed to keep the refcounter in balance (and properly free the object):
2,577 (104 direct, 2,473 indirect) bytes in 1 blocks are definitely lost in loss record 168 of 174 at 0x4838B86: calloc (vg_replace_malloc.c:752) by 0x4A0AD42: virAllocVar (viralloc.c:346) by 0x4A84D12: virObjectNew (virobject.c:243) by 0x21EE13: qemuBlockJobDataNew (qemu_blockjob.c:111) by 0x184EBC: qemuDomainObjPrivateXMLParseBlockjobData (qemu_domain.c:3120) by 0x1854B5: qemuDomainObjPrivateXMLParseBlockjobs (qemu_domain.c:3191) by 0x186D7D: qemuDomainObjPrivateXMLParse (qemu_domain.c:3622) by 0x4B198CC: virDomainObjParseXML (domain_conf.c:21490) by 0x4B19DDF: virDomainObjParseNode (domain_conf.c:21630) by 0x4B19E7D: virDomainObjParseFile (domain_conf.c:21649) by 0x13FDD5: testCompareStatusXMLToXMLFiles (qemuxml2xmltest.c:61) by 0x16F7D2: virTestRun (testutils.c:115)
Is there some wrong test data perhaps? I suspect I made a mistake and there's two jobs referencing the same disk in the (fake) test data.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_blockjob.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index a991309ee7..67b1f677f8 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -151,6 +151,7 @@ qemuBlockJobRegister(qemuBlockJobDataPtr job, if (disk) { job->disk = disk; job->chain = virObjectRef(disk->src); + virObjectUnref(QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob);
This is not the right fix. Our block job code only ever allows one job per disk. The only thing we can do here is repeat the logic to refuse to register a job if there is one already registered prior to doing other things into the function but we should never just unregister random jobs from disks.
QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob = virObjectRef(job); }
-- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Martin Kletzander -
Michal Privoznik -
Peter Krempa