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