[libvirt] [PATCH v2 24/32] tests: Fix memory leak in testCompareXMLToArgvFiles

[John Ferlan]

On 2/11/19 7:24 AM, Erik Skultety wrote:
> On Fri, Feb 08, 2019 at 01:37:18PM -0500, John Ferlan wrote:
>> Only one path will consume the @def; otherwise, we need to free it.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  tests/storagepoolxml2argvtest.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c
>> index 288b81af1d..f2a8af12b0 100644
>> --- a/tests/storagepoolxml2argvtest.c
>> +++ b/tests/storagepoolxml2argvtest.c
>> @@ -23,6 +23,7 @@ testCompareXMLToArgvFiles(bool shouldFail,
>>                            const char *cmdline)
>>  {
>>      int ret = -1;
>> +    bool consumeDef = false;
>>      virStoragePoolDefPtr def = NULL;
>>      virStoragePoolObjPtr pool = NULL;
>>      VIR_AUTOFREE(char *) actualCmdline = NULL;
>> @@ -41,6 +42,7 @@ testCompareXMLToArgvFiles(bool shouldFail,
>>              goto cleanup;
>>          }
>>          virStoragePoolObjSetDef(pool, def);
>> +        consumeDef = true;
> 
> Long term solution should probably be that these consumers make their own copy
> so that we don't need to differentiate. As for short term solution, I'd prefer
> if we freed def unconditionally and thus resetting @def to NULL before issuing
> break; in the _NETFS path, you'd need a def->type helper for that.

For this test, perhaps a waste to introduce a virStoragePoolDefCopy type
method to do a deep copy.

Ironically, from the review of v1:

>> +    defType = def->type;
>
> This is only 1 level of dereference, I don't see the point in shorting that. It
> also belongs to a separate trivial patch.
>

The "correct" solution is to not reference @def after the
virStoragePoolObjSetDef call since it "consumes" it; however, the
alternate solution to fetch objDef from @pool wasn't accepted so
this was essentially the way around that.

I can restore defType here or I could add a :

const char *defTypeStr = virStoragePoolTypeToString(def->type) and
use that as '%s' instead of the %d def->type in the debug message.

Then remove the consumeDef and use def = NULL before getting to cleanup
after the virStoragePoolObjSetDef call.

John

> 
> Reviewed-by: Erik Skultety <eskultet at redhat.com>
> 
>>
>>          if (!(src = virStorageBackendFileSystemGetPoolSource(pool))) {
>>              VIR_TEST_DEBUG("pool type %d has no pool source\n", def->type);
>> @@ -83,6 +85,8 @@ testCompareXMLToArgvFiles(bool shouldFail,
>>      ret = 0;
>>
>>   cleanup:
>> +    if (!consumeDef)
>> +        virStoragePoolDefFree(def);
>>      virStoragePoolObjEndAPI(&pool);
>>      if (shouldFail) {
>>          virResetLastError();
>> --
>> 2.20.1
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list