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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
>
>>
>> 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@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
