[...]
@@ -1590,14 +1590,12 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, _("Storage pool config filename '%s' does " "not match pool name '%s'"), path, def->name); - virStoragePoolDefFree(def); return NULL; }
- if (!(obj = virStoragePoolObjAssignDef(pools, def, false))) { - virStoragePoolDefFree(def); + if (!(obj = virStoragePoolObjAssignDef(pools, def, false))) return NULL; - } + def = NULL;
This feels odd, but I don't know how I'd do it better as I can't suggest putting it at the very end for obvious reasons, but it makes sense to convert so let's go with it. You could force a new cleanup label where you'd only do this NULL reset, but that doesn't feel right either.
[snip]
This is the "right" place as @def is consumed by AssignDef.
@@ -779,9 +778,10 @@ storagePoolDefineXML(virConnectPtr conn, const char *xml, unsigned int flags) { - virStoragePoolDefPtr newDef; + VIR_AUTOPTR(virStoragePoolDef) newDef = NULL; virStoragePoolObjPtr obj = NULL; virStoragePoolDefPtr def; + virStoragePoolDefPtr objNewDef;
I think it's sufficient if we stay with converting the above, dropping ^this one...
This processing was "confusing" and using objNewDef was my way around the confusion of using @newDef and @def w/r/t to virStoragePoolObjGetDef and virStoragePoolObjGetNewDef. If you jump into virStoragePoolObjAssignDef the @newDef could be either placed at obj->newDef or obj->def. In any case, I've removed the objNewDef
virStoragePoolPtr pool = NULL; virObjectEventPtr event = NULL;
@@ -801,14 +801,14 @@ storagePoolDefineXML(virConnectPtr conn,
if (!(obj = virStoragePoolObjAssignDef(driver->pools, newDef, false))) goto cleanup; - newDef = virStoragePoolObjGetNewDef(obj); + newDef = NULL; + objNewDef = virStoragePoolObjGetNewDef(obj); def = virStoragePoolObjGetDef(obj);
- if (virStoragePoolObjSaveDef(driver, obj, newDef ? newDef : def) < 0) { + if (virStoragePoolObjSaveDef(driver, obj, objNewDef ? objNewDef : def) < 0) { virStoragePoolObjRemove(driver->pools, obj); virObjectUnref(obj); obj = NULL; - newDef = NULL; goto cleanup; }
@@ -818,11 +818,9 @@ storagePoolDefineXML(virConnectPtr conn,
VIR_INFO("Defining storage pool '%s'", def->name); pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); - newDef = NULL;
... leaving out both preceding hunks...
cleanup: virObjectEventStateQueue(driver->storageEventState, event); - virStoragePoolDefFree(newDef);
...and then again going with ^this one
[snip]
diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c index e7f42dc0f8..2dd87ab731 100644 --- a/tests/storagepoolxml2argvtest.c +++ b/tests/storagepoolxml2argvtest.c @@ -24,30 +24,35 @@ testCompareXMLToArgvFiles(bool shouldFail, { VIR_AUTOFREE(char *) actualCmdline = NULL; VIR_AUTOFREE(char *) src = NULL; + VIR_AUTOPTR(virStoragePoolDef) def = NULL; + int defType; int ret = -1; virCommandPtr cmd = NULL; - virStoragePoolDefPtr def = NULL; virStoragePoolObjPtr pool = NULL; + virStoragePoolDefPtr objDef;
if (!(def = virStoragePoolDefParseFile(poolxml))) goto cleanup; + 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.
This one was a bit more of a pain though because of the usage of virStoragePoolObjSetDef which consumes @def... In any case, I'll just drop the usage of VIR_AUTOPTR for @def here, it's just not worth the effort. Although that then leaves the first two AUTOFREE's at the top and it also leaves @def being leaked when virStoragePoolObjSetDef is not called.
- switch ((virStoragePoolType)def->type) { + switch ((virStoragePoolType)defType) { case VIR_STORAGE_POOL_FS: case VIR_STORAGE_POOL_NETFS: + if (!(pool = virStoragePoolObjNew())) { - VIR_TEST_DEBUG("pool type %d alloc pool obj fails\n", def->type); - virStoragePoolDefFree(def);
Here too, I don't see much benefit in converting this function in order to get rid of a single line.
+ VIR_TEST_DEBUG("pool type %d alloc pool obj fails\n", defType); goto cleanup; } virStoragePoolObjSetDef(pool, def); + def = NULL; + objDef = virStoragePoolObjGetDef(pool);
if (!(src = virStorageBackendFileSystemGetPoolSource(pool))) { - VIR_TEST_DEBUG("pool type %d has no pool source\n", def->type); + VIR_TEST_DEBUG("pool type %d has no pool source\n", defType); goto cleanup; }
- cmd = virStorageBackendFileSystemMountCmd(MOUNT, def, src); + cmd = virStorageBackendFileSystemMountCmd(MOUNT, objDef, src); break;
case VIR_STORAGE_POOL_LOGICAL: @@ -67,12 +72,12 @@ testCompareXMLToArgvFiles(bool shouldFail, case VIR_STORAGE_POOL_VSTORAGE: case VIR_STORAGE_POOL_LAST: default: - VIR_TEST_DEBUG("pool type %d has no xml2argv test\n", def->type); + VIR_TEST_DEBUG("pool type %d has no xml2argv test\n", defType); goto cleanup; };
if (!(actualCmdline = virCommandToString(cmd, false))) { - VIR_TEST_DEBUG("pool type %d failed to get commandline\n", def->type); + VIR_TEST_DEBUG("pool type %d failed to get commandline\n", defType); goto cleanup; }
[snip]
diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index 8e19f10b73..2acbf567ca 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -50,18 +50,19 @@ testCompareXMLToArgvFiles(bool shouldFail,
VIR_AUTOPTR(virStorageVolDef) vol = NULL; VIR_AUTOPTR(virStorageVolDef) inputvol = NULL; - virStoragePoolDefPtr def = NULL; - virStoragePoolDefPtr inputpool = NULL; + VIR_AUTOPTR(virStoragePoolDef) inputpool = NULL;
Let's only convert @inputpool and not @def. The reason for that is that the logic is a bit unfortunate and I feel like we're stretching the whole VIR_AUTO idea, because..
+ VIR_AUTOPTR(virStoragePoolDef) def = NULL; virStoragePoolObjPtr obj = NULL; + virStoragePoolDefPtr objDef;
...you need another helper @var with kind of a confusing name in order to reset @def at the right time and then switch the usage to @objDef so that @def doesn't get autofreed in cases we don't want that => we should probably stay with the current code.
<sigh> Wish long ago I had stuck to original intent of when a function consumes an argument that the function should take the address of the argument forcing the caller to refetch. I'll remove this one though. BTW: Changes to me were less about getting rid of some number of lines. It wasn't the intent; however, it is the outcome. John
if (!(def = virStoragePoolDefParseFile(poolxml))) goto cleanup;
- if (!(obj = virStoragePoolObjNew())) { - virStoragePoolDefFree(def); + if (!(obj = virStoragePoolObjNew())) goto cleanup; - } virStoragePoolObjSetDef(obj, def); + def = NULL; + objDef = virStoragePoolObjGetDef(obj);
if (inputpoolxml) { if (!(inputpool = virStoragePoolDefParseFile(inputpoolxml))) @@ -71,14 +72,14 @@ testCompareXMLToArgvFiles(bool shouldFail, if (inputvolxml) parse_flags |= VIR_VOL_XML_PARSE_NO_CAPACITY;
- if (!(vol = virStorageVolDefParseFile(def, volxml, parse_flags))) + if (!(vol = virStorageVolDefParseFile(objDef, volxml, parse_flags))) goto cleanup;
if (inputvolxml && !(inputvol = virStorageVolDefParseFile(inputpool, inputvolxml, 0))) goto cleanup;
- testSetVolumeType(vol, def); + testSetVolumeType(vol, objDef); testSetVolumeType(inputvol, inputpool);
/* Using an input file for encryption requires a multi-step process @@ -139,7 +140,6 @@ testCompareXMLToArgvFiles(bool shouldFail, ret = 0;
cleanup: - virStoragePoolDefFree(inputpool);
Everything else is okay as is: Reviewed-by: Erik Skultety <eskultet@redhat.com>