Re: [libvirt] [PATCH v2 24/32] tests: Fix memory leak in testCompareXMLToArgvFiles
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.
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(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list