[...]
>> @@ -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@xxxxxxxxxx>
>
