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@xxxxxxxxxx>
> ---
[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@xxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
