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>
---
src/conf/domain_conf.c | 3 +-
src/conf/storage_conf.c | 114 ++++++++++++++---------------
src/conf/storage_conf.h | 1 +
src/conf/virstorageobj.c | 27 +++----
src/phyp/phyp_driver.c | 3 +-
src/storage/storage_driver.c | 14 ++--
src/test/test_driver.c | 6 +-
tests/storagebackendsheepdogtest.c | 6 +-
tests/storagepoolxml2argvtest.c | 21 ++++--
tests/storagepoolxml2xmltest.c | 3 +-
tests/storagevolxml2argvtest.c | 16 ++--
tests/storagevolxml2xmltest.c | 3 +-
12 files changed, 103 insertions(+), 114 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5699a60549..ee0edff4b2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30996,7 +30996,7 @@ int
virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def)
{
virConnectPtr conn = NULL;
- virStoragePoolDefPtr pooldef = NULL;
+ VIR_AUTOPTR(virStoragePoolDef) pooldef = NULL;
virStoragePoolPtr pool = NULL;
virStorageVolPtr vol = NULL;
char *poolxml = NULL;
@@ -31160,7 +31160,6 @@ virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def)
virObjectUnref(pool);
virObjectUnref(vol);
VIR_FREE(poolxml);
- virStoragePoolDefFree(pooldef);
return ret;
}
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;
}
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index b8e73864c4..daf6f9b68c 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -461,6 +461,7 @@ VIR_ENUM_DECL(virStoragePartedFs);
VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE)
VIR_DEFINE_AUTOPTR_FUNC(virStoragePoolSource, virStoragePoolSourceFree);
+VIR_DEFINE_AUTOPTR_FUNC(virStoragePoolDef, virStoragePoolDefFree);
VIR_DEFINE_AUTOPTR_FUNC(virStorageVolDef, virStorageVolDefFree);
#endif /* LIBVIRT_STORAGE_CONF_H */
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index 7005de3c24..3a2f3fa77b 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -1579,7 +1579,7 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools,
const char *path,
const char *autostartLink)
{
- 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;
VIR_FREE(obj->configFile); /* for driver reload */
if (VIR_STRDUP(obj->configFile, path) < 0) {
@@ -1625,39 +1623,40 @@ virStoragePoolObjLoadState(virStoragePoolObjListPtr pools,
const char *name)
{
char *stateFile = NULL;
- virStoragePoolDefPtr def = NULL;
+ VIR_AUTOPTR(virStoragePoolDef) def = NULL;
virStoragePoolObjPtr obj = NULL;
xmlDocPtr xml = NULL;
xmlXPathContextPtr ctxt = NULL;
xmlNodePtr node = NULL;
if (!(stateFile = virFileBuildPath(stateDir, name, ".xml")))
- goto error;
+ return NULL;
if (!(xml = virXMLParseCtxt(stateFile, NULL, _("(pool state)"), &ctxt)))
- goto error;
+ goto cleanup;
if (!(node = virXPathNode("//pool", ctxt))) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Could not find any 'pool' element in state file"));
- goto error;
+ goto cleanup;
}
ctxt->node = node;
if (!(def = virStoragePoolDefParseXML(ctxt)))
- goto error;
+ goto cleanup;
if (STRNEQ(name, def->name)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Storage pool state file '%s' does not match "
"pool name '%s'"),
stateFile, def->name);
- goto error;
+ goto cleanup;
}
/* create the object */
if (!(obj = virStoragePoolObjAssignDef(pools, def, true)))
- goto error;
+ goto cleanup;
+ def = NULL;
/* XXX: future handling of some additional useful status data,
* for now, if a status file for a pool exists, the pool will be marked
@@ -1671,10 +1670,6 @@ virStoragePoolObjLoadState(virStoragePoolObjListPtr pools,
xmlFreeDoc(xml);
xmlXPathFreeContext(ctxt);
return obj;
-
- error:
- virStoragePoolDefFree(def);
- goto cleanup;
}
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index 966e0b3c0f..2ebf4f3cfd 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -1953,7 +1953,7 @@ phypStorageVolCreateXML(virStoragePoolPtr pool,
virCheckFlags(0, NULL);
VIR_AUTOPTR(virStorageVolDef) voldef = NULL;
- virStoragePoolDefPtr spdef = NULL;
+ VIR_AUTOPTR(virStoragePoolDef) spdef = NULL;
virStorageVolPtr vol = NULL;
virStorageVolPtr dup_vol = NULL;
char *key = NULL;
@@ -2036,7 +2036,6 @@ phypStorageVolCreateXML(virStoragePoolPtr pool,
err:
VIR_FREE(key);
- virStoragePoolDefFree(spdef);
virObjectUnref(vol);
return NULL;
}
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 48b4c5127f..dcdd52bbbf 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -690,7 +690,7 @@ storagePoolCreateXML(virConnectPtr conn,
const char *xml,
unsigned int flags)
{
- virStoragePoolDefPtr newDef;
+ VIR_AUTOPTR(virStoragePoolDef) newDef = NULL;
virStoragePoolObjPtr obj = NULL;
virStoragePoolDefPtr def;
virStoragePoolPtr pool = NULL;
@@ -762,7 +762,6 @@ storagePoolCreateXML(virConnectPtr conn,
cleanup:
VIR_FREE(stateFile);
- virStoragePoolDefFree(newDef);
virObjectEventStateQueue(driver->storageEventState, event);
virStoragePoolObjEndAPI(&obj);
return pool;
@@ -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);
virStoragePoolObjEndAPI(&obj);
return pool;
}
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 44ae711b5a..ff773f39b0 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4474,7 +4474,7 @@ testStoragePoolCreateXML(virConnectPtr conn,
unsigned int flags)
{
testDriverPtr privconn = conn->privateData;
- virStoragePoolDefPtr newDef;
+ VIR_AUTOPTR(virStoragePoolDef) newDef = NULL;
virStoragePoolObjPtr obj = NULL;
virStoragePoolDefPtr def;
virStoragePoolPtr pool = NULL;
@@ -4527,7 +4527,6 @@ testStoragePoolCreateXML(virConnectPtr conn,
pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
cleanup:
- virStoragePoolDefFree(newDef);
virObjectEventStateQueue(privconn->eventState, event);
virStoragePoolObjEndAPI(&obj);
virObjectUnlock(privconn);
@@ -4541,7 +4540,7 @@ testStoragePoolDefineXML(virConnectPtr conn,
unsigned int flags)
{
testDriverPtr privconn = conn->privateData;
- virStoragePoolDefPtr newDef;
+ VIR_AUTOPTR(virStoragePoolDef) newDef = NULL;
virStoragePoolObjPtr obj = NULL;
virStoragePoolDefPtr def;
virStoragePoolPtr pool = NULL;
@@ -4576,7 +4575,6 @@ testStoragePoolDefineXML(virConnectPtr conn,
pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
cleanup:
- virStoragePoolDefFree(newDef);
virObjectEventStateQueue(privconn->eventState, event);
virStoragePoolObjEndAPI(&obj);
virObjectUnlock(privconn);
diff --git a/tests/storagebackendsheepdogtest.c b/tests/storagebackendsheepdogtest.c
index 540f89c3ab..03ddf76d65 100644
--- a/tests/storagebackendsheepdogtest.c
+++ b/tests/storagebackendsheepdogtest.c
@@ -59,7 +59,7 @@ test_node_info_parser(const void *opaque)
collie_test test = data->data;
int ret = -1;
char *output = NULL;
- virStoragePoolDefPtr pool = NULL;
+ VIR_AUTOPTR(virStoragePoolDef) pool = NULL;
if (!(pool = virStoragePoolDefParseFile(data->poolxml)))
goto cleanup;
@@ -82,7 +82,6 @@ test_node_info_parser(const void *opaque)
cleanup:
VIR_FREE(output);
- virStoragePoolDefFree(pool);
return ret;
}
@@ -93,7 +92,7 @@ test_vdi_list_parser(const void *opaque)
collie_test test = data->data;
int ret = -1;
char *output = NULL;
- virStoragePoolDefPtr pool = NULL;
+ VIR_AUTOPTR(virStoragePoolDef) pool = NULL;
VIR_AUTOPTR(virStorageVolDef) vol = NULL;
if (!(pool = virStoragePoolDefParseFile(data->poolxml)))
@@ -120,7 +119,6 @@ test_vdi_list_parser(const void *opaque)
cleanup:
VIR_FREE(output);
- virStoragePoolDefFree(pool);
return ret;
}
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/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c
index acb15f3a2c..c8d5c41cd4 100644
--- a/tests/storagepoolxml2xmltest.c
+++ b/tests/storagepoolxml2xmltest.c
@@ -20,7 +20,7 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml)
{
char *actual = NULL;
int ret = -1;
- virStoragePoolDefPtr dev = NULL;
+ VIR_AUTOPTR(virStoragePoolDef) dev = NULL;
if (!(dev = virStoragePoolDefParseFile(inxml)))
goto fail;
@@ -35,7 +35,6 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml)
fail:
VIR_FREE(actual);
- virStoragePoolDefFree(dev);
return ret;
}
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);
virCommandFree(cmd);
VIR_FREE(actualCmdline);
virStoragePoolObjEndAPI(&obj);
diff --git a/tests/storagevolxml2xmltest.c b/tests/storagevolxml2xmltest.c
index 95e205a0ab..cb78bd5b28 100644
--- a/tests/storagevolxml2xmltest.c
+++ b/tests/storagevolxml2xmltest.c
@@ -19,7 +19,7 @@ testCompareXMLToXMLFiles(const char *poolxml, const char *inxml,
{
char *actual = NULL;
int ret = -1;
- virStoragePoolDefPtr pool = NULL;
+ VIR_AUTOPTR(virStoragePoolDef) pool = NULL;
VIR_AUTOPTR(virStorageVolDef) dev = NULL;
if (!(pool = virStoragePoolDefParseFile(poolxml)))
@@ -38,7 +38,6 @@ testCompareXMLToXMLFiles(const char *poolxml, const char *inxml,
fail:
VIR_FREE(actual);
- virStoragePoolDefFree(pool);
return ret;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[...]
>> @@ -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@redhat.com>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.