On Wed, Feb 06, 2019 at 08:41:36AM -0500, John Ferlan wrote:
Let's make use of the auto __cleanup capabilities cleaning up any now unnecessary goto paths. For virStoragePoolDefParseXML use the @def/@ret similarly as other methods. For storagePoolDefineXML make it clearer and use @objNewDef after adding @newDef to the object.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
[snip]
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 83ca379217..4fb5fb9f57 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -737,159 +737,157 @@ virStoragePoolDefPtr virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) { virStoragePoolOptionsPtr options; - virStoragePoolDefPtr ret; + VIR_AUTOPTR(virStoragePoolDef) def = NULL; + virStoragePoolDefPtr ret = NULL; xmlNodePtr source_node; char *type = NULL; char *uuid = NULL; char *target_path = NULL;
- if (VIR_ALLOC(ret) < 0) + if (VIR_ALLOC(def) < 0) return NULL;
type = virXPathString("string(./@type)", ctxt); if (type == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("storage pool missing type attribute")); - goto error; + goto cleanup; }
- if ((ret->type = virStoragePoolTypeFromString(type)) < 0) { + if ((def->type = virStoragePoolTypeFromString(type)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown storage pool type %s"), type); - goto error; + goto cleanup; }
- if ((options = virStoragePoolOptionsForPoolType(ret->type)) == NULL) - goto error; + if ((options = virStoragePoolOptionsForPoolType(def->type)) == NULL) + goto cleanup;
source_node = virXPathNode("./source", ctxt); if (source_node) { - if (virStoragePoolDefParseSource(ctxt, &ret->source, ret->type, + if (virStoragePoolDefParseSource(ctxt, &def->source, def->type, source_node) < 0) - goto error; + goto cleanup; } else { if (options->formatFromString) - ret->source.format = options->defaultFormat; + def->source.format = options->defaultFormat; }
- ret->name = virXPathString("string(./name)", ctxt); - if (ret->name == NULL && + def->name = virXPathString("string(./name)", ctxt); + if (def->name == NULL && options->flags & VIR_STORAGE_POOL_SOURCE_NAME && - VIR_STRDUP(ret->name, ret->source.name) < 0) - goto error; - if (ret->name == NULL) { + VIR_STRDUP(def->name, def->source.name) < 0) + goto cleanup; + if (def->name == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing pool source name element")); - goto error; + goto cleanup; }
- if (strchr(ret->name, '/')) { + if (strchr(def->name, '/')) { virReportError(VIR_ERR_XML_ERROR, - _("name %s cannot contain '/'"), ret->name); - goto error; + _("name %s cannot contain '/'"), def->name); + goto cleanup; }
uuid = virXPathString("string(./uuid)", ctxt); if (uuid == NULL) { - if (virUUIDGenerate(ret->uuid) < 0) { + if (virUUIDGenerate(def->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unable to generate uuid")); - goto error; + goto cleanup; } } else { - if (virUUIDParse(uuid, ret->uuid) < 0) { + if (virUUIDParse(uuid, def->uuid) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed uuid element")); - goto error; + goto cleanup; } }
if (options->flags & VIR_STORAGE_POOL_SOURCE_HOST) { - if (!ret->source.nhost) { + if (!def->source.nhost) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source host name")); - goto error; + goto cleanup; } }
if (options->flags & VIR_STORAGE_POOL_SOURCE_DIR) { - if (!ret->source.dir) { + if (!def->source.dir) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source path")); - goto error; + goto cleanup; } } if (options->flags & VIR_STORAGE_POOL_SOURCE_NAME) { - if (ret->source.name == NULL) { + if (def->source.name == NULL) { /* source name defaults to pool name */ - if (VIR_STRDUP(ret->source.name, ret->name) < 0) - goto error; + if (VIR_STRDUP(def->source.name, def->name) < 0) + goto cleanup; } }
if ((options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) && - (virStorageAdapterValidate(&ret->source.adapter)) < 0) - goto error; + (virStorageAdapterValidate(&def->source.adapter)) < 0) + goto cleanup;
/* If DEVICE is the only source type, then its required */ if (options->flags == VIR_STORAGE_POOL_SOURCE_DEVICE) { - if (!ret->source.ndevice) { + if (!def->source.ndevice) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source device name")); - goto error; + goto cleanup; } }
/* When we are working with a virtual disk we can skip the target * path and permissions */ if (!(options->flags & VIR_STORAGE_POOL_SOURCE_NETWORK)) { - if (ret->type == VIR_STORAGE_POOL_LOGICAL) { - if (virAsprintf(&target_path, "/dev/%s", ret->source.name) < 0) - goto error; - } else if (ret->type == VIR_STORAGE_POOL_ZFS) { - if (virAsprintf(&target_path, "/dev/zvol/%s", ret->source.name) < 0) - goto error; + if (def->type == VIR_STORAGE_POOL_LOGICAL) { + if (virAsprintf(&target_path, "/dev/%s", def->source.name) < 0) + goto cleanup; + } else if (def->type == VIR_STORAGE_POOL_ZFS) { + if (virAsprintf(&target_path, "/dev/zvol/%s", def->source.name) < 0) + goto cleanup; } else { target_path = virXPathString("string(./target/path)", ctxt); if (!target_path) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool target path")); - goto error; + goto cleanup; } } - ret->target.path = virFileSanitizePath(target_path); - if (!ret->target.path) - goto error; + def->target.path = virFileSanitizePath(target_path); + if (!def->target.path) + goto cleanup;
- if (virStorageDefParsePerms(ctxt, &ret->target.perms, + if (virStorageDefParsePerms(ctxt, &def->target.perms, "./target/permissions") < 0) - goto error; + goto cleanup; }
- if (ret->type == VIR_STORAGE_POOL_ISCSI_DIRECT && - !ret->source.initiator.iqn) { + if (def->type == VIR_STORAGE_POOL_ISCSI_DIRECT && + !def->source.initiator.iqn) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("missing initiator IQN")); - goto error; + goto cleanup; }
/* Make a copy of all the callback pointers here for easier use, * especially during the virStoragePoolSourceClear method */ - ret->ns = options->ns; - if (ret->ns.parse && - (ret->ns.parse)(ctxt, &ret->namespaceData) < 0) - goto error; + def->ns = options->ns; + if (def->ns.parse && + (def->ns.parse)(ctxt, &def->namespaceData) < 0) + goto cleanup; + + VIR_STEAL_PTR(ret, def);
cleanup: VIR_FREE(uuid); VIR_FREE(type); VIR_FREE(target_path); return ret; - - error: - virStoragePoolDefFree(ret); - ret = NULL; - goto cleanup;
Same argument as for the previous patch - I'm not convinced that the resulting diff is worth the removal of the last 5 lines. [snip]
- virStoragePoolDefPtr def; + VIR_AUTOPTR(virStoragePoolDef) def = NULL; virStoragePoolObjPtr obj;
if (!(def = virStoragePoolDefParseFile(path))) @@ -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]
@@ -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...
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.
- 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.
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>