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

[Erik Skultety]

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 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.

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