1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] qemu_blockjob: Don't leak old job when registering a new one

Michal Privoznik posted 1 patch 4 years, 7 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/85b9b0b1679c7f24a9d0ef58e905522fdc6d0bc3.1568406395.git.mprivozn@redhat.com
src/qemu/qemu_blockjob.c | 1 +
1 file changed, 1 insertion(+)
[libvirt] [PATCH] qemu_blockjob: Don't leak old job when registering a new one
Posted by Michal Privoznik 4 years, 7 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_blockjob: Don't leak old job when registering a new one
Posted by Martin Kletzander 4 years, 7 months ago 
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu_blockjob: Don't leak old job when registering a new one
Posted by Peter Krempa 4 years, 7 months ago 
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.