Re: [libvirt] [PATCH 04/15] conf: Introduce VIR_DEFINE_AUTOPTR_FUNC for virStoragePoolDef

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(a)redhat.com&gt; ---

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;

- 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;

@@ -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;

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;

cleanup: virObjectEventStateQueue(driver->storageEventState, event); - virStoragePoolDefFree(newDef);

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;

- 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);

+ 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; }

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;

+ VIR_AUTOPTR(virStoragePoolDef) def = NULL; virStoragePoolObjPtr obj = NULL; + virStoragePoolDefPtr objDef;

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);