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