1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/3] Some random fixes
  4. View patch

[libvirt] [PATCH 2/3] virpci: Fix memleak in virPCIDeviceIterDevices

Michal Privoznik posted 3 patches 5 years, 10 months ago
There is a newer version of this series
[libvirt] [PATCH 2/3] virpci: Fix memleak in virPCIDeviceIterDevices
Posted by Michal Privoznik 5 years, 10 months ago 
This partially reverts 00dc991ca167302c7a72f4fb16be061d05b12a32.

 2,030 (1,456 direct, 574 indirect) bytes in 14 blocks are definitely lost in loss record 77 of 80
    at 0x4C30E96: calloc (vg_replace_malloc.c:711)
    by 0x50F83AA: virAlloc (viralloc.c:143)
    by 0x5178DFA: virPCIDeviceNew (virpci.c:1753)
    by 0x51753E9: virPCIDeviceIterDevices (virpci.c:468)
    by 0x5175EB5: virPCIDeviceGetParent (virpci.c:759)
    by 0x517AB55: virPCIDeviceIsBehindSwitchLackingACS (virpci.c:2476)
    by 0x517AC24: virPCIDeviceIsAssignable (virpci.c:2494)
    by 0x10BF27: testVirPCIDeviceIsAssignable (virpcitest.c:229)
    by 0x10D14C: virTestRun (testutils.c:174)
    by 0x10C535: mymain (virpcitest.c:422)
    by 0x10F1B6: virTestMain (testutils.c:1112)
    by 0x10CF93: main (virpcitest.c:455)

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

diff --git a/src/util/virpci.c b/src/util/virpci.c
index ef578bf774..fb22460bf5 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -482,6 +482,8 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate,
             *matched = check;
             ret = 1;
             break;
+        } else {
+            virPCIDeviceFree(check);
         }
     }
     VIR_DIR_CLOSE(dir);
-- 
2.19.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] virpci: Fix memleak in virPCIDeviceIterDevices
Posted by Erik Skultety 5 years, 10 months ago 
On Wed, Jan 23, 2019 at 04:45:07PM +0100, Michal Privoznik wrote:
> This partially reverts 00dc991ca167302c7a72f4fb16be061d05b12a32.
>
>  2,030 (1,456 direct, 574 indirect) bytes in 14 blocks are definitely lost in loss record 77 of 80
>     at 0x4C30E96: calloc (vg_replace_malloc.c:711)
>     by 0x50F83AA: virAlloc (viralloc.c:143)
>     by 0x5178DFA: virPCIDeviceNew (virpci.c:1753)
>     by 0x51753E9: virPCIDeviceIterDevices (virpci.c:468)
>     by 0x5175EB5: virPCIDeviceGetParent (virpci.c:759)
>     by 0x517AB55: virPCIDeviceIsBehindSwitchLackingACS (virpci.c:2476)
>     by 0x517AC24: virPCIDeviceIsAssignable (virpci.c:2494)
>     by 0x10BF27: testVirPCIDeviceIsAssignable (virpcitest.c:229)
>     by 0x10D14C: virTestRun (testutils.c:174)
>     by 0x10C535: mymain (virpcitest.c:422)
>     by 0x10F1B6: virTestMain (testutils.c:1112)
>     by 0x10CF93: main (virpcitest.c:455)
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/util/virpci.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index ef578bf774..fb22460bf5 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -482,6 +482,8 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate,
>              *matched = check;

I think that instead of adding a new condition block, we could do VIR_STEAL_PTR
^here and then...

>              ret = 1;
>              break;
> +        } else {
> +            virPCIDeviceFree(check);
>          }

do virPCIDeviceFree(check) unconditionally. IMHO it would also look more clean.

Reviewed-by: Erik Skultety <eskultet@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] virpci: Fix memleak in virPCIDeviceIterDevices
Posted by Michal Privoznik 5 years, 10 months ago 
On 1/23/19 5:24 PM, Erik Skultety wrote:
> On Wed, Jan 23, 2019 at 04:45:07PM +0100, Michal Privoznik wrote:
>> This partially reverts 00dc991ca167302c7a72f4fb16be061d05b12a32.
>>
>>  2,030 (1,456 direct, 574 indirect) bytes in 14 blocks are definitely lost in loss record 77 of 80
>>     at 0x4C30E96: calloc (vg_replace_malloc.c:711)
>>     by 0x50F83AA: virAlloc (viralloc.c:143)
>>     by 0x5178DFA: virPCIDeviceNew (virpci.c:1753)
>>     by 0x51753E9: virPCIDeviceIterDevices (virpci.c:468)
>>     by 0x5175EB5: virPCIDeviceGetParent (virpci.c:759)
>>     by 0x517AB55: virPCIDeviceIsBehindSwitchLackingACS (virpci.c:2476)
>>     by 0x517AC24: virPCIDeviceIsAssignable (virpci.c:2494)
>>     by 0x10BF27: testVirPCIDeviceIsAssignable (virpcitest.c:229)
>>     by 0x10D14C: virTestRun (testutils.c:174)
>>     by 0x10C535: mymain (virpcitest.c:422)
>>     by 0x10F1B6: virTestMain (testutils.c:1112)
>>     by 0x10CF93: main (virpcitest.c:455)
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/util/virpci.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/src/util/virpci.c b/src/util/virpci.c
>> index ef578bf774..fb22460bf5 100644
>> --- a/src/util/virpci.c
>> +++ b/src/util/virpci.c
>> @@ -482,6 +482,8 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate,
>>              *matched = check;
> 
> I think that instead of adding a new condition block, we could do VIR_STEAL_PTR
> ^here and then...
> 
>>              ret = 1;
>>              break;
>> +        } else {
>> +            virPCIDeviceFree(check);
>>          }
> 
> do virPCIDeviceFree(check) unconditionally. IMHO it would also look more clean.

Or, even better, @check can be declared as VIR_AUTOPTR(virPCIDevice),
VIR_STREAL_PTR can be used to set @matched and then all
virPCIDeviceFree() calls can be removed. Does this work for you?

diff --git c/src/util/virpci.c w/src/util/virpci.c
index ef578bf774..017fe72f19 100644
--- c/src/util/virpci.c
+++ w/src/util/virpci.c
@@ -449,7 +449,7 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate,
 
     while ((ret = virDirRead(dir, &entry, PCI_SYSFS "devices")) > 0) {
         unsigned int domain, bus, slot, function;
-        virPCIDevicePtr check;
+        VIR_AUTOPTR(virPCIDevice) check = NULL;
         char *tmp;
 
         /* expected format: <domain>:<bus>:<slot>.<function> */
@@ -474,12 +474,11 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate,
         rc = predicate(dev, check, data);
         if (rc < 0) {
             /* the predicate returned an error, bail */
-            virPCIDeviceFree(check);
             ret = -1;
             break;
         } else if (rc == 1) {
             VIR_DEBUG("%s %s: iter matched on %s", dev->id, dev->name, check->name);
-            *matched = check;
+            VIR_STEAL_PTR(*matched, check);
             ret = 1;
             break;
         }

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] virpci: Fix memleak in virPCIDeviceIterDevices
Posted by Erik Skultety 5 years, 10 months ago 
On Thu, Jan 24, 2019 at 10:11:15AM +0100, Michal Privoznik wrote:
> On 1/23/19 5:24 PM, Erik Skultety wrote:
> > On Wed, Jan 23, 2019 at 04:45:07PM +0100, Michal Privoznik wrote:
> >> This partially reverts 00dc991ca167302c7a72f4fb16be061d05b12a32.
> >>
> >>  2,030 (1,456 direct, 574 indirect) bytes in 14 blocks are definitely lost in loss record 77 of 80
> >>     at 0x4C30E96: calloc (vg_replace_malloc.c:711)
> >>     by 0x50F83AA: virAlloc (viralloc.c:143)
> >>     by 0x5178DFA: virPCIDeviceNew (virpci.c:1753)
> >>     by 0x51753E9: virPCIDeviceIterDevices (virpci.c:468)
> >>     by 0x5175EB5: virPCIDeviceGetParent (virpci.c:759)
> >>     by 0x517AB55: virPCIDeviceIsBehindSwitchLackingACS (virpci.c:2476)
> >>     by 0x517AC24: virPCIDeviceIsAssignable (virpci.c:2494)
> >>     by 0x10BF27: testVirPCIDeviceIsAssignable (virpcitest.c:229)
> >>     by 0x10D14C: virTestRun (testutils.c:174)
> >>     by 0x10C535: mymain (virpcitest.c:422)
> >>     by 0x10F1B6: virTestMain (testutils.c:1112)
> >>     by 0x10CF93: main (virpcitest.c:455)
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  src/util/virpci.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/src/util/virpci.c b/src/util/virpci.c
> >> index ef578bf774..fb22460bf5 100644
> >> --- a/src/util/virpci.c
> >> +++ b/src/util/virpci.c
> >> @@ -482,6 +482,8 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate,
> >>              *matched = check;
> >
> > I think that instead of adding a new condition block, we could do VIR_STEAL_PTR
> > ^here and then...
> >
> >>              ret = 1;
> >>              break;
> >> +        } else {
> >> +            virPCIDeviceFree(check);
> >>          }
> >
> > do virPCIDeviceFree(check) unconditionally. IMHO it would also look more clean.
>
> Or, even better, @check can be declared as VIR_AUTOPTR(virPCIDevice),
> VIR_STREAL_PTR can be used to set @matched and then all
> virPCIDeviceFree() calls can be removed. Does this work for you?

Sure, go ahead, you already have my RB :).

Erik

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.