8:52 a.m.
[...]
> @@ -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(a)redhat.com>