Re: [libvirt] [PATCH v2 24/32] tests: Fix memory leak in testCompareXMLToArgvFiles
On Mon, Feb 11, 2019 at 08:18:13AM -0500, John Ferlan wrote:
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(a)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.
Absolutely, it's was only a nice-to-have suggestion, I didn't want to imply
anything.
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.
>
Embarrassing...sorry for not cross-checking with (my own) comments grom v1 :(
The "correct" solution is to not reference @def after the
As we agreed, not worth the effort...
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.
Sounds fine.
Erik