Let's make use of the auto __cleanup capabilities cleaning up any now unnecessary goto paths. For virStorageAuthDefCopy use authdef and ret as consistently as similar other code. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/domain_conf.c | 29 +++++++++++------------------ src/conf/storage_conf.c | 6 ++---- src/qemu/qemu_parse_command.c | 6 ++---- src/util/virstoragefile.c | 33 ++++++++++++++------------------- src/util/virstoragefile.h | 1 + 5 files changed, 30 insertions(+), 45 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1fc4c8a35a..5699a60549 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7578,10 +7578,9 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode, virDomainHostdevSubsysSCSIPtr def, xmlXPathContextPtr ctxt) { - int ret = -1; int auth_secret_usage = -1; xmlNodePtr cur; - virStorageAuthDefPtr authdef = NULL; + VIR_AUTOPTR(virStorageAuthDef) authdef = NULL; virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &def->u.iscsi; /* For the purposes of command line creation, this needs to look @@ -7594,23 +7593,23 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode, if (!(iscsisrc->src->path = virXMLPropString(sourcenode, "name"))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing iSCSI hostdev source path name")); - goto cleanup; + return -1; } if (virDomainStorageNetworkParseHosts(sourcenode, &iscsisrc->src->hosts, &iscsisrc->src->nhosts) < 0) - goto cleanup; + return -1; if (iscsisrc->src->nhosts < 1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing the host address for the iSCSI hostdev")); - goto cleanup; + return -1; } if (iscsisrc->src->nhosts > 1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("only one source host address may be specified " "for the iSCSI hostdev")); - goto cleanup; + return -1; } cur = sourcenode->children; @@ -7618,30 +7617,25 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode, if (cur->type == XML_ELEMENT_NODE && virXMLNodeNameEqual(cur, "auth")) { if (!(authdef = virStorageAuthDefParse(cur, ctxt))) - goto cleanup; + return -1; if ((auth_secret_usage = virSecretUsageTypeFromString(authdef->secrettype)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("invalid secret type %s"), authdef->secrettype); - goto cleanup; + return -1; } if (auth_secret_usage != VIR_SECRET_USAGE_TYPE_ISCSI) { virReportError(VIR_ERR_INTERNAL_ERROR, _("hostdev invalid secret type '%s'"), authdef->secrettype); - goto cleanup; + return -1; } - iscsisrc->src->auth = authdef; - authdef = NULL; + VIR_STEAL_PTR(iscsisrc->src->auth, authdef); } cur = cur->next; } - ret = 0; - - cleanup: - virStorageAuthDefFree(authdef); - return ret; + return 0; } static int @@ -9684,7 +9678,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, virStorageEncryptionPtr encryption = NULL; char *serial = NULL; char *startupPolicy = NULL; - virStorageAuthDefPtr authdef = NULL; + VIR_AUTOPTR(virStorageAuthDef) authdef = NULL; char *tray = NULL; char *removable = NULL; char *logical_block_size = NULL; @@ -10094,7 +10088,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(target); VIR_FREE(tray); VIR_FREE(removable); - virStorageAuthDefFree(authdef); VIR_FREE(devaddr); VIR_FREE(serial); virStorageEncryptionFree(encryption); diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 1ee31ca676..9cd3d836a3 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -458,7 +458,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, int nsource; size_t i; virStoragePoolOptionsPtr options; - virStorageAuthDefPtr authdef = NULL; + VIR_AUTOPTR(virStorageAuthDef) authdef = NULL; char *name = NULL; char *port = NULL; char *ver = NULL; @@ -584,8 +584,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup; } - source->auth = authdef; - authdef = NULL; + VIR_STEAL_PTR(source->auth, authdef); } /* Option protocol version string (NFSvN) */ @@ -615,7 +614,6 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, VIR_FREE(port); VIR_FREE(nodeset); - virStorageAuthDefFree(authdef); return ret; } diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c index c4650f01e0..81691cb85e 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -59,7 +59,7 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, char *sock = NULL; char *volimg = NULL; char *secret = NULL; - virStorageAuthDefPtr authdef = NULL; + VIR_AUTOPTR(virStorageAuthDef) authdef = NULL; if (VIR_ALLOC(def->src->hosts) < 0) goto error; @@ -133,8 +133,7 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, if (VIR_STRDUP(authdef->secrettype, secrettype) < 0) goto error; } - def->src->auth = authdef; - authdef = NULL; + VIR_STEAL_PTR(def->src->auth, authdef); /* Cannot formulate a secretType (eg, usage or uuid) given * what is provided. @@ -152,7 +151,6 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, error: virStorageNetHostDefClear(def->src->hosts); VIR_FREE(def->src->hosts); - virStorageAuthDefFree(authdef); goto cleanup; } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 8319ba9c8c..b3f5e0204d 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1879,26 +1879,24 @@ virStorageAuthDefFree(virStorageAuthDefPtr authdef) virStorageAuthDefPtr virStorageAuthDefCopy(const virStorageAuthDef *src) { - virStorageAuthDefPtr ret; + VIR_AUTOPTR(virStorageAuthDef) authdef = NULL; + virStorageAuthDefPtr ret = NULL; - if (VIR_ALLOC(ret) < 0) + if (VIR_ALLOC(authdef) < 0) return NULL; - if (VIR_STRDUP(ret->username, src->username) < 0) - goto error; + if (VIR_STRDUP(authdef->username, src->username) < 0) + return NULL; /* Not present for storage pool, but used for disk source */ - if (VIR_STRDUP(ret->secrettype, src->secrettype) < 0) - goto error; - ret->authType = src->authType; + if (VIR_STRDUP(authdef->secrettype, src->secrettype) < 0) + return NULL; + authdef->authType = src->authType; - if (virSecretLookupDefCopy(&ret->seclookupdef, &src->seclookupdef) < 0) - goto error; + if (virSecretLookupDefCopy(&authdef->seclookupdef, &src->seclookupdef) < 0) + return NULL; + VIR_STEAL_PTR(ret, authdef); return ret; - - error: - virStorageAuthDefFree(ret); - return NULL; } @@ -1907,7 +1905,7 @@ virStorageAuthDefParse(xmlNodePtr node, xmlXPathContextPtr ctxt) { xmlNodePtr saveNode = ctxt->node; - virStorageAuthDefPtr authdef = NULL; + VIR_AUTOPTR(virStorageAuthDef) authdef = NULL; virStorageAuthDefPtr ret = NULL; xmlNodePtr secretnode = NULL; char *authtype = NULL; @@ -1958,7 +1956,6 @@ virStorageAuthDefParse(xmlNodePtr node, cleanup: VIR_FREE(authtype); - virStorageAuthDefFree(authdef); ctxt->node = saveNode; return ret; @@ -2832,7 +2829,7 @@ virStorageSourceParseRBDColonString(const char *rbdstr, { char *options = NULL; char *p, *e, *next; - virStorageAuthDefPtr authdef = NULL; + VIR_AUTOPTR(virStorageAuthDef) authdef = NULL; /* optionally skip the "rbd:" prefix if provided */ if (STRPREFIX(rbdstr, "rbd:")) @@ -2895,9 +2892,8 @@ virStorageSourceParseRBDColonString(const char *rbdstr, if (VIR_STRDUP(authdef->secrettype, virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_CEPH)) < 0) goto error; - src->auth = authdef; + VIR_STEAL_PTR(src->auth, authdef); src->authInherited = true; - authdef = NULL; /* Cannot formulate a secretType (eg, usage or uuid) given * what is provided. @@ -2936,7 +2932,6 @@ virStorageSourceParseRBDColonString(const char *rbdstr, error: VIR_FREE(options); - virStorageAuthDefFree(authdef); return -1; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index a98d5103fa..f1164dde9b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -543,4 +543,5 @@ void virStorageFileReportBrokenChain(int errcode, virStorageSourcePtr src, virStorageSourcePtr parent); +VIR_DEFINE_AUTOPTR_FUNC(virStorageAuthDef, virStorageAuthDefFree); #endif /* LIBVIRT_VIRSTORAGEFILE_H */ -- 2.20.1

On 2/7/19 4:10 AM, Erik Skultety wrote: I don't mind moving them. I generally just try to keep the usages together. You'll find this sprinkled throughout - generating separate patches could explode the series into perhaps 30+ patches which I doubt anyone would jump at the chance to review. I can separate before pushing and I assume by extracting it/them I can just add your R-by too... Perhaps because I had recently reviewed Cole's series about removing the semicolon from VIR_ENUM_DECL, VIR_ENUM_IMPL, VIR_LOG_INIT, and VIR_ONCE_GLOBAL_INIT - it was just "fresh" in my mind that macros should have semicolons (moreso than actually looking at that definition). I can change them all. Tks - John

Let's make use of the auto __cleanup capabilities cleaning up any now unnecessary goto paths. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/storage_conf.c | 4 ++-- src/conf/storage_conf.h | 2 ++ src/storage/storage_backend_fs.c | 3 +-- src/storage/storage_backend_gluster.c | 3 +-- src/storage/storage_backend_iscsi.c | 3 +-- src/storage/storage_backend_iscsi_direct.c | 3 +-- src/test/test_driver.c | 14 +++++++------- 7 files changed, 15 insertions(+), 17 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 9cd3d836a3..6099c64b26 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -625,7 +625,8 @@ virStoragePoolDefParseSourceString(const char *srcSpec, xmlDocPtr doc = NULL; xmlNodePtr node = NULL; xmlXPathContextPtr xpath_ctxt = NULL; - virStoragePoolSourcePtr def = NULL, ret = NULL; + VIR_AUTOPTR(virStoragePoolSource) def = NULL; + virStoragePoolSourcePtr ret = NULL; if (!(doc = virXMLParseStringCtxt(srcSpec, _("(storage_source_specification)"), @@ -647,7 +648,6 @@ virStoragePoolDefParseSourceString(const char *srcSpec, VIR_STEAL_PTR(ret, def); cleanup: - virStoragePoolSourceFree(def); xmlFreeDoc(doc); xmlXPathFreeContext(xpath_ctxt); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 1ba4b80616..3fa97bba76 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -460,4 +460,6 @@ VIR_ENUM_DECL(virStoragePartedFs); VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_AUTOSTART | \ VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE) +VIR_DEFINE_AUTOPTR_FUNC(virStoragePoolSource, virStoragePoolSourceFree); + #endif /* LIBVIRT_STORAGE_CONF_H */ diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index ddb422d874..06c76fde4f 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -145,7 +145,7 @@ virStorageBackendFileSystemNetFindPoolSources(const char *srcSpec, .sources = NULL } }; - virStoragePoolSourcePtr source = NULL; + VIR_AUTOPTR(virStoragePoolSource) source = NULL; char *ret = NULL; size_t i; int retNFS = -1; @@ -196,7 +196,6 @@ virStorageBackendFileSystemNetFindPoolSources(const char *srcSpec, virStoragePoolSourceClear(&state.list.sources[i]); VIR_FREE(state.list.sources); - virStoragePoolSourceFree(source); return ret; } diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index e09bc54196..8c6205b081 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -489,7 +489,7 @@ virStorageBackendGlusterFindPoolSources(const char *srcSpec, .nsources = 0, .sources = NULL }; - virStoragePoolSourcePtr source = NULL; + VIR_AUTOPTR(virStoragePoolSource) source = NULL; char *ret = NULL; int rc; size_t i; @@ -532,7 +532,6 @@ virStorageBackendGlusterFindPoolSources(const char *srcSpec, virStoragePoolSourceClear(&list.sources[i]); VIR_FREE(list.sources); - virStoragePoolSourceFree(source); return ret; } diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 4792bd70b8..483ba15102 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -158,7 +158,7 @@ static char * virStorageBackendISCSIFindPoolSources(const char *srcSpec, unsigned int flags) { - virStoragePoolSourcePtr source = NULL; + VIR_AUTOPTR(virStoragePoolSource) source = NULL; size_t ntargets = 0; char **targets = NULL; char *ret = NULL; @@ -227,7 +227,6 @@ virStorageBackendISCSIFindPoolSources(const char *srcSpec, VIR_FREE(targets[i]); VIR_FREE(targets); VIR_FREE(portal); - virStoragePoolSourceFree(source); return ret; } diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c index 0babe26550..ddc150f03d 100644 --- a/src/storage/storage_backend_iscsi_direct.c +++ b/src/storage/storage_backend_iscsi_direct.c @@ -486,7 +486,7 @@ static char * virStorageBackendISCSIDirectFindPoolSources(const char *srcSpec, unsigned int flags) { - virStoragePoolSourcePtr source = NULL; + VIR_AUTOPTR(virStoragePoolSource) source = NULL; size_t ntargets = 0; char **targets = NULL; char *ret = NULL; @@ -557,7 +557,6 @@ virStorageBackendISCSIDirectFindPoolSources(const char *srcSpec, VIR_FREE(targets[i]); VIR_FREE(targets); VIR_FREE(portal); - virStoragePoolSourceFree(source); return ret; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index b06567855e..743d03ae8b 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4394,7 +4394,7 @@ testConnectFindStoragePoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, const char *srcSpec, unsigned int flags) { - virStoragePoolSourcePtr source = NULL; + VIR_AUTOPTR(virStoragePoolSource) source = NULL; int pool_type; char *ret = NULL; @@ -4404,30 +4404,32 @@ testConnectFindStoragePoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, if (!pool_type) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown storage pool type %s"), type); - goto cleanup; + return NULL; } if (srcSpec) { source = virStoragePoolDefParseSourceString(srcSpec, pool_type); if (!source) - goto cleanup; + return NULL; } switch (pool_type) { case VIR_STORAGE_POOL_LOGICAL: ignore_value(VIR_STRDUP(ret, defaultPoolSourcesLogicalXML)); + return ret; break; case VIR_STORAGE_POOL_NETFS: if (!source || !source->hosts[0].name) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("hostname must be specified for netfs sources")); - goto cleanup; + return NULL; } ignore_value(virAsprintf(&ret, defaultPoolSourcesNetFSXML, source->hosts[0].name)); + return ret; break; default: @@ -4435,9 +4437,7 @@ testConnectFindStoragePoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, _("pool type '%s' does not support source discovery"), type); } - cleanup: - virStoragePoolSourceFree(source); - return ret; + return NULL; } -- 2.20.1

Let's make use of the auto __cleanup capabilities cleaning up any now unnecessary goto paths. For virStorageVolDefParseXML use the @def/@ret similarly as other methods. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/storage_conf.c | 112 ++++++++++----------- src/conf/storage_conf.h | 1 + src/esx/esx_storage_backend_vmfs.c | 6 +- src/phyp/phyp_driver.c | 3 +- src/storage/storage_backend_gluster.c | 9 +- src/storage/storage_backend_iscsi_direct.c | 19 ++-- src/storage/storage_backend_mpath.c | 24 ++--- src/storage/storage_backend_rbd.c | 9 +- src/storage/storage_backend_sheepdog.c | 15 ++- src/storage/storage_driver.c | 6 +- src/storage/storage_util.c | 6 +- src/test/test_driver.c | 10 +- src/vbox/vbox_storage.c | 3 +- tests/storagebackendsheepdogtest.c | 3 +- tests/storagevolxml2argvtest.c | 5 +- tests/storagevolxml2xmltest.c | 3 +- 16 files changed, 100 insertions(+), 134 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 6099c64b26..83ca379217 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1168,7 +1168,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, xmlXPathContextPtr ctxt, unsigned int flags) { - virStorageVolDefPtr ret; + VIR_AUTOPTR(virStorageVolDef) def = NULL; + virStorageVolDefPtr ret = NULL; virStorageVolOptionsPtr options; char *type = NULL; char *allocation = NULL; @@ -1187,132 +1188,132 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (options == NULL) return NULL; - if (VIR_ALLOC(ret) < 0) + if (VIR_ALLOC(def) < 0) return NULL; - ret->target.type = VIR_STORAGE_TYPE_FILE; + def->target.type = VIR_STORAGE_TYPE_FILE; - ret->name = virXPathString("string(./name)", ctxt); - if (ret->name == NULL) { + def->name = virXPathString("string(./name)", ctxt); + if (def->name == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing volume name element")); - goto error; + goto cleanup; } /* Normally generated by pool refresh, but useful for unit tests */ - ret->key = virXPathString("string(./key)", ctxt); + def->key = virXPathString("string(./key)", ctxt); /* Technically overridden by pool refresh, but useful for unit tests */ type = virXPathString("string(./@type)", ctxt); if (type) { - if ((ret->type = virStorageVolTypeFromString(type)) < 0) { + if ((def->type = virStorageVolTypeFromString(type)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown volume type '%s'"), type); - goto error; + goto cleanup; } } if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) { - if (VIR_ALLOC(ret->target.backingStore) < 0) - goto error; + if (VIR_ALLOC(def->target.backingStore) < 0) + goto cleanup; - ret->target.backingStore->type = VIR_STORAGE_TYPE_FILE; + def->target.backingStore->type = VIR_STORAGE_TYPE_FILE; - ret->target.backingStore->path = backingStore; + def->target.backingStore->path = backingStore; backingStore = NULL; if (options->formatFromString) { char *format = virXPathString("string(./backingStore/format/@type)", ctxt); if (format == NULL) - ret->target.backingStore->format = options->defaultFormat; + def->target.backingStore->format = options->defaultFormat; else - ret->target.backingStore->format = (options->formatFromString)(format); + def->target.backingStore->format = (options->formatFromString)(format); - if (ret->target.backingStore->format < 0) { + if (def->target.backingStore->format < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown volume format type %s"), format); VIR_FREE(format); - goto error; + goto cleanup; } VIR_FREE(format); } - if (VIR_ALLOC(ret->target.backingStore->perms) < 0) - goto error; - if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms, + if (VIR_ALLOC(def->target.backingStore->perms) < 0) + goto cleanup; + if (virStorageDefParsePerms(ctxt, def->target.backingStore->perms, "./backingStore/permissions") < 0) - goto error; + goto cleanup; } capacity = virXPathString("string(./capacity)", ctxt); unit = virXPathString("string(./capacity/@unit)", ctxt); if (capacity) { - if (virStorageSize(unit, capacity, &ret->target.capacity) < 0) - goto error; + if (virStorageSize(unit, capacity, &def->target.capacity) < 0) + goto cleanup; } else if (!(flags & VIR_VOL_XML_PARSE_NO_CAPACITY) && !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) && - virStorageSourceHasBacking(&ret->target))) { + virStorageSourceHasBacking(&def->target))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing capacity element")); - goto error; + goto cleanup; } VIR_FREE(unit); allocation = virXPathString("string(./allocation)", ctxt); if (allocation) { unit = virXPathString("string(./allocation/@unit)", ctxt); - if (virStorageSize(unit, allocation, &ret->target.allocation) < 0) - goto error; - ret->target.has_allocation = true; + if (virStorageSize(unit, allocation, &def->target.allocation) < 0) + goto cleanup; + def->target.has_allocation = true; } else { - ret->target.allocation = ret->target.capacity; + def->target.allocation = def->target.capacity; } - ret->target.path = virXPathString("string(./target/path)", ctxt); + def->target.path = virXPathString("string(./target/path)", ctxt); if (options->formatFromString) { char *format = virXPathString("string(./target/format/@type)", ctxt); if (format == NULL) - ret->target.format = options->defaultFormat; + def->target.format = options->defaultFormat; else - ret->target.format = (options->formatFromString)(format); + def->target.format = (options->formatFromString)(format); - if (ret->target.format < 0) { + if (def->target.format < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown volume format type %s"), format); VIR_FREE(format); - goto error; + goto cleanup; } VIR_FREE(format); } - if (VIR_ALLOC(ret->target.perms) < 0) - goto error; - if (virStorageDefParsePerms(ctxt, ret->target.perms, + if (VIR_ALLOC(def->target.perms) < 0) + goto cleanup; + if (virStorageDefParsePerms(ctxt, def->target.perms, "./target/permissions") < 0) - goto error; + goto cleanup; node = virXPathNode("./target/encryption", ctxt); if (node != NULL) { - ret->target.encryption = virStorageEncryptionParseNode(node, ctxt); - if (ret->target.encryption == NULL) - goto error; + def->target.encryption = virStorageEncryptionParseNode(node, ctxt); + if (def->target.encryption == NULL) + goto cleanup; } - ret->target.compat = virXPathString("string(./target/compat)", ctxt); - if (virStorageFileCheckCompat(ret->target.compat) < 0) - goto error; + def->target.compat = virXPathString("string(./target/compat)", ctxt); + if (virStorageFileCheckCompat(def->target.compat) < 0) + goto cleanup; if (virXPathNode("./target/nocow", ctxt)) - ret->target.nocow = true; + def->target.nocow = true; if (virXPathNode("./target/features", ctxt)) { if ((n = virXPathNodeSet("./target/features/*", ctxt, &nodes)) < 0) - goto error; + goto cleanup; - if (!ret->target.compat && VIR_STRDUP(ret->target.compat, "1.1") < 0) - goto error; + if (!def->target.compat && VIR_STRDUP(def->target.compat, "1.1") < 0) + goto cleanup; - if (!(ret->target.features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST))) - goto error; + if (!(def->target.features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST))) + goto cleanup; for (i = 0; i < n; i++) { int f = virStorageFileFeatureTypeFromString((const char*)nodes[i]->name); @@ -1320,13 +1321,15 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (f < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported feature %s"), (const char*)nodes[i]->name); - goto error; + goto cleanup; } - ignore_value(virBitmapSetBit(ret->target.features, f)); + ignore_value(virBitmapSetBit(def->target.features, f)); } VIR_FREE(nodes); } + VIR_STEAL_PTR(ret, def); + cleanup: VIR_FREE(nodes); VIR_FREE(allocation); @@ -1335,11 +1338,6 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, VIR_FREE(type); VIR_FREE(backingStore); return ret; - - error: - virStorageVolDefFree(ret); - ret = NULL; - goto cleanup; } diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 3fa97bba76..b8e73864c4 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -461,5 +461,6 @@ VIR_ENUM_DECL(virStoragePartedFs); VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE) VIR_DEFINE_AUTOPTR_FUNC(virStoragePoolSource, virStoragePoolSourceFree); +VIR_DEFINE_AUTOPTR_FUNC(virStorageVolDef, virStorageVolDefFree); #endif /* LIBVIRT_STORAGE_CONF_H */ diff --git a/src/esx/esx_storage_backend_vmfs.c b/src/esx/esx_storage_backend_vmfs.c index 8458d4e95c..6e1c3c0285 100644 --- a/src/esx/esx_storage_backend_vmfs.c +++ b/src/esx/esx_storage_backend_vmfs.c @@ -836,7 +836,7 @@ esxStorageVolCreateXML(virStoragePoolPtr pool, virStorageVolPtr volume = NULL; esxPrivate *priv = pool->conn->privateData; virStoragePoolDef poolDef; - virStorageVolDefPtr def = NULL; + VIR_AUTOPTR(virStorageVolDef) def = NULL; char *tmp; char *unescapedDatastorePath = NULL; char *unescapedDirectoryName = NULL; @@ -1024,7 +1024,6 @@ esxStorageVolCreateXML(virStoragePoolPtr pool, virtualDiskSpec->adapterType = NULL; } - virStorageVolDefFree(def); VIR_FREE(unescapedDatastorePath); VIR_FREE(unescapedDirectoryName); VIR_FREE(unescapedDirectoryAndFileName); @@ -1054,7 +1053,7 @@ esxStorageVolCreateXMLFrom(virStoragePoolPtr pool, esxPrivate *priv = pool->conn->privateData; virStoragePoolDef poolDef; char *sourceDatastorePath = NULL; - virStorageVolDefPtr def = NULL; + VIR_AUTOPTR(virStorageVolDef) def = NULL; char *tmp; char *unescapedDatastorePath = NULL; char *unescapedDirectoryName = NULL; @@ -1207,7 +1206,6 @@ esxStorageVolCreateXMLFrom(virStoragePoolPtr pool, cleanup: VIR_FREE(sourceDatastorePath); - virStorageVolDefFree(def); VIR_FREE(unescapedDatastorePath); VIR_FREE(unescapedDirectoryName); VIR_FREE(unescapedDirectoryAndFileName); diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 4ffa08ff43..966e0b3c0f 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1952,7 +1952,7 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, { virCheckFlags(0, NULL); - virStorageVolDefPtr voldef = NULL; + VIR_AUTOPTR(virStorageVolDef) voldef = NULL; virStoragePoolDefPtr spdef = NULL; virStorageVolPtr vol = NULL; virStorageVolPtr dup_vol = NULL; @@ -2036,7 +2036,6 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, err: VIR_FREE(key); - virStorageVolDefFree(voldef); virStoragePoolDefFree(spdef); virObjectUnref(vol); return NULL; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 8c6205b081..cb9f7e4735 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -242,7 +242,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, virStorageVolDefPtr *volptr) { int ret = -1; - virStorageVolDefPtr vol = NULL; + VIR_AUTOPTR(virStorageVolDef) vol = NULL; glfs_fd_t *fd = NULL; virStorageSourcePtr meta = NULL; char *header = NULL; @@ -278,8 +278,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, if (S_ISDIR(st->st_mode)) { vol->type = VIR_STORAGE_VOL_NETDIR; vol->target.format = VIR_STORAGE_FILE_DIR; - *volptr = vol; - vol = NULL; + VIR_STEAL_PTR(*volptr, vol); ret = 0; goto cleanup; } @@ -328,12 +327,10 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, vol->target.compat = meta->compat; meta->compat = NULL; - *volptr = vol; - vol = NULL; + VIR_STEAL_PTR(*volptr, vol); ret = 0; cleanup: virStorageSourceFree(meta); - virStorageVolDefFree(vol); if (fd) glfs_close(fd); VIR_FREE(header); diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c index ddc150f03d..f287fbf010 100644 --- a/src/storage/storage_backend_iscsi_direct.c +++ b/src/storage/storage_backend_iscsi_direct.c @@ -305,22 +305,21 @@ virISCSIDirectRefreshVol(virStoragePoolObjPtr pool, char *portal) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virStorageVolDefPtr vol = NULL; + VIR_AUTOPTR(virStorageVolDef) vol = NULL; uint32_t block_size; uint32_t nb_block; - int ret = -1; virStoragePoolObjClearVols(pool); if (virISCSIDirectTestUnitReady(iscsi, lun) < 0) - goto cleanup; + return -1; if (VIR_ALLOC(vol) < 0) - goto cleanup; + return -1; vol->type = VIR_STORAGE_VOL_NETWORK; if (virISCSIDirectGetVolumeCapacity(iscsi, lun, &block_size, &nb_block) < 0) - goto cleanup; + return -1; vol->target.capacity = block_size * nb_block; vol->target.allocation = block_size * nb_block; @@ -328,17 +327,13 @@ virISCSIDirectRefreshVol(virStoragePoolObjPtr pool, def->allocation += vol->target.allocation; if (virISCSIDirectSetVolumeAttributes(pool, vol, lun, portal) < 0) - goto cleanup; + return -1; if (virStoragePoolObjAddVol(pool, vol) < 0) - goto cleanup; - + return -1; vol = NULL; - ret = 0; - cleanup: - virStorageVolDefFree(vol); - return ret; + return 0; } static int diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index b3a49ee1b2..423f945fbc 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -46,42 +46,36 @@ virStorageBackendMpathNewVol(virStoragePoolObjPtr pool, const char *dev) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virStorageVolDefPtr vol; - int ret = -1; + VIR_AUTOPTR(virStorageVolDef) vol = NULL; if (VIR_ALLOC(vol) < 0) - goto cleanup; + return -1; vol->type = VIR_STORAGE_VOL_BLOCK; if (virAsprintf(&(vol->name), "dm-%u", devnum) < 0) - goto cleanup; + return -1; if (virAsprintf(&vol->target.path, "/dev/%s", dev) < 0) - goto cleanup; + return -1; if (virStorageBackendUpdateVolInfo(vol, true, VIR_STORAGE_VOL_OPEN_DEFAULT, 0) < 0) { - goto cleanup; + return -1; } /* XXX should use logical unit's UUID instead */ if (VIR_STRDUP(vol->key, vol->target.path) < 0) - goto cleanup; + return -1; if (virStoragePoolObjAddVol(pool, vol) < 0) - goto cleanup; + return -1; def->capacity += vol->target.capacity; def->allocation += vol->target.allocation; - ret = 0; - - cleanup: + vol = NULL; - if (ret != 0) - virStorageVolDefFree(vol); - - return ret; + return 0; } diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index cfbce1ad19..ece04f0f2d 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -618,7 +618,7 @@ virStorageBackendRBDRefreshPool(virStoragePoolObjPtr pool) } for (name = names; name < names + max_size;) { - virStorageVolDefPtr vol; + VIR_AUTOPTR(virStorageVolDef) vol = NULL; if (STREQ(name, "")) break; @@ -626,10 +626,8 @@ virStorageBackendRBDRefreshPool(virStoragePoolObjPtr pool) if (VIR_ALLOC(vol) < 0) goto cleanup; - if (VIR_STRDUP(vol->name, name) < 0) { - VIR_FREE(vol); + if (VIR_STRDUP(vol->name, name) < 0) goto cleanup; - } name += strlen(name) + 1; @@ -648,15 +646,14 @@ virStorageBackendRBDRefreshPool(virStoragePoolObjPtr pool) if (r == -ENOENT || r == -ETIMEDOUT) continue; - virStorageVolDefFree(vol); goto cleanup; } if (virStoragePoolObjAddVol(pool, vol) < 0) { - virStorageVolDefFree(vol); virStoragePoolObjClearVols(pool); goto cleanup; } + vol = NULL; } VIR_DEBUG("Found %zu images in RBD pool %s", diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index b6f424bea1..7cba6e02aa 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -110,30 +110,27 @@ virStorageBackendSheepdogAddHostArg(virCommandPtr cmd, static int virStorageBackendSheepdogAddVolume(virStoragePoolObjPtr pool, const char *diskInfo) { - virStorageVolDefPtr vol = NULL; + VIR_AUTOPTR(virStorageVolDef) vol = NULL; if (diskInfo == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing disk info when adding volume")); - goto error; + return -1; } if (VIR_ALLOC(vol) < 0 || VIR_STRDUP(vol->name, diskInfo) < 0) - goto error; + return -1; vol->type = VIR_STORAGE_VOL_NETWORK; if (virStorageBackendSheepdogRefreshVol(pool, vol) < 0) - goto error; + return -1; if (virStoragePoolObjAddVol(pool, vol) < 0) - goto error; + return -1; + vol = NULL; return 0; - - error: - virStorageVolDefFree(vol); - return -1; } static int diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 878a40cac5..48b4c5127f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1820,7 +1820,7 @@ storageVolCreateXML(virStoragePoolPtr pool, virStoragePoolObjPtr obj; virStoragePoolDefPtr def; virStorageBackendPtr backend; - virStorageVolDefPtr voldef = NULL; + VIR_AUTOPTR(virStorageVolDef) voldef = NULL; virStorageVolPtr vol = NULL, newvol = NULL; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); @@ -1941,7 +1941,6 @@ storageVolCreateXML(virStoragePoolPtr pool, cleanup: virObjectUnref(newvol); - virStorageVolDefFree(voldef); virStoragePoolObjEndAPI(&obj); return vol; } @@ -1957,7 +1956,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr pool, virStoragePoolObjPtr objsrc = NULL; virStorageBackendPtr backend; virStorageVolDefPtr voldefsrc = NULL; - virStorageVolDefPtr voldef = NULL; + VIR_AUTOPTR(virStorageVolDef) voldef = NULL; virStorageVolDefPtr shadowvol = NULL; virStorageVolPtr newvol = NULL; virStorageVolPtr vol = NULL; @@ -2134,7 +2133,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr pool, cleanup: virObjectUnref(newvol); - virStorageVolDefFree(voldef); VIR_FREE(shadowvol); virStoragePoolObjEndAPI(&obj); virStoragePoolObjEndAPI(&objsrc); diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 95e05d950b..158cf3adb6 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3672,7 +3672,7 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) struct dirent *ent; struct statvfs sb; struct stat statbuf; - virStorageVolDefPtr vol = NULL; + VIR_AUTOPTR(virStorageVolDef) vol = NULL; virStorageSourcePtr target = NULL; int direrr; int fd = -1, ret = -1; @@ -3767,7 +3767,6 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) cleanup: VIR_DIR_CLOSE(dir); VIR_FORCE_CLOSE(fd); - virStorageVolDefFree(vol); virStorageSourceFree(target); if (ret < 0) virStoragePoolObjClearVols(pool); @@ -3816,7 +3815,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, const char *dev) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virStorageVolDefPtr vol = NULL; + VIR_AUTOPTR(virStorageVolDef) vol = NULL; char *devpath = NULL; int retval = -1; @@ -3898,7 +3897,6 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, retval = 0; cleanup: - virStorageVolDefFree(vol); VIR_FREE(devpath); return retval; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 743d03ae8b..44ae711b5a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1025,7 +1025,7 @@ testOpenVolumesForPool(const char *file, size_t i; int num, ret = -1; xmlNodePtr *nodes = NULL; - virStorageVolDefPtr volDef = NULL; + VIR_AUTOPTR(virStorageVolDef) volDef = NULL; /* Find storage volumes */ if (virAsprintf(&vol_xpath, "/node/pool[%d]/volume", objidx) < 0) @@ -1064,7 +1064,6 @@ testOpenVolumesForPool(const char *file, ret = 0; error: - virStorageVolDefFree(volDef); VIR_FREE(nodes); return ret; } @@ -5053,7 +5052,7 @@ testStorageVolCreateXML(virStoragePoolPtr pool, testDriverPtr privconn = pool->conn->privateData; virStoragePoolObjPtr obj; virStoragePoolDefPtr def; - virStorageVolDefPtr privvol = NULL; + VIR_AUTOPTR(virStorageVolDef) privvol = NULL; virStorageVolPtr ret = NULL; virCheckFlags(0, NULL); @@ -5098,7 +5097,6 @@ testStorageVolCreateXML(virStoragePoolPtr pool, privvol = NULL; cleanup: - virStorageVolDefFree(privvol); virStoragePoolObjEndAPI(&obj); return ret; } @@ -5113,7 +5111,8 @@ testStorageVolCreateXMLFrom(virStoragePoolPtr pool, testDriverPtr privconn = pool->conn->privateData; virStoragePoolObjPtr obj; virStoragePoolDefPtr def; - virStorageVolDefPtr privvol = NULL, origvol = NULL; + VIR_AUTOPTR(virStorageVolDef) privvol = NULL; + virStorageVolDefPtr origvol = NULL; virStorageVolPtr ret = NULL; virCheckFlags(0, NULL); @@ -5166,7 +5165,6 @@ testStorageVolCreateXMLFrom(virStoragePoolPtr pool, privvol = NULL; cleanup: - virStorageVolDefFree(privvol); virStoragePoolObjEndAPI(&obj); return ret; } diff --git a/src/vbox/vbox_storage.c b/src/vbox/vbox_storage.c index 7047e54084..4ca342a449 100644 --- a/src/vbox/vbox_storage.c +++ b/src/vbox/vbox_storage.c @@ -401,7 +401,7 @@ vboxStorageVolCreateXML(virStoragePoolPtr pool, const char *xml, unsigned int flags) { vboxDriverPtr data = pool->conn->privateData; - virStorageVolDefPtr def = NULL; + VIR_AUTOPTR(virStorageVolDef) def = NULL; PRUnichar *hddFormatUtf16 = NULL; PRUnichar *hddNameUtf16 = NULL; virStoragePoolDef poolDef; @@ -501,7 +501,6 @@ vboxStorageVolCreateXML(virStoragePoolPtr pool, VBOX_RELEASE(progress); VBOX_UTF16_FREE(hddFormatUtf16); VBOX_UTF16_FREE(hddNameUtf16); - virStorageVolDefFree(def); return ret; } diff --git a/tests/storagebackendsheepdogtest.c b/tests/storagebackendsheepdogtest.c index 616af22d73..540f89c3ab 100644 --- a/tests/storagebackendsheepdogtest.c +++ b/tests/storagebackendsheepdogtest.c @@ -94,7 +94,7 @@ test_vdi_list_parser(const void *opaque) int ret = -1; char *output = NULL; virStoragePoolDefPtr pool = NULL; - virStorageVolDefPtr vol = NULL; + VIR_AUTOPTR(virStorageVolDef) vol = NULL; if (!(pool = virStoragePoolDefParseFile(data->poolxml))) goto cleanup; @@ -121,7 +121,6 @@ test_vdi_list_parser(const void *opaque) cleanup: VIR_FREE(output); virStoragePoolDefFree(pool); - virStorageVolDefFree(vol); return ret; } diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index bc2da37410..8e19f10b73 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -48,7 +48,8 @@ testCompareXMLToArgvFiles(bool shouldFail, virCommandPtr cmd = NULL; - virStorageVolDefPtr vol = NULL, inputvol = NULL; + VIR_AUTOPTR(virStorageVolDef) vol = NULL; + VIR_AUTOPTR(virStorageVolDef) inputvol = NULL; virStoragePoolDefPtr def = NULL; virStoragePoolDefPtr inputpool = NULL; virStoragePoolObjPtr obj = NULL; @@ -139,8 +140,6 @@ testCompareXMLToArgvFiles(bool shouldFail, cleanup: virStoragePoolDefFree(inputpool); - virStorageVolDefFree(vol); - virStorageVolDefFree(inputvol); virCommandFree(cmd); VIR_FREE(actualCmdline); virStoragePoolObjEndAPI(&obj); diff --git a/tests/storagevolxml2xmltest.c b/tests/storagevolxml2xmltest.c index 040bbc7585..95e205a0ab 100644 --- a/tests/storagevolxml2xmltest.c +++ b/tests/storagevolxml2xmltest.c @@ -20,7 +20,7 @@ testCompareXMLToXMLFiles(const char *poolxml, const char *inxml, char *actual = NULL; int ret = -1; virStoragePoolDefPtr pool = NULL; - virStorageVolDefPtr dev = NULL; + VIR_AUTOPTR(virStorageVolDef) dev = NULL; if (!(pool = virStoragePoolDefParseFile(poolxml))) goto fail; @@ -39,7 +39,6 @@ testCompareXMLToXMLFiles(const char *poolxml, const char *inxml, fail: VIR_FREE(actual); virStoragePoolDefFree(pool); - virStorageVolDefFree(dev); return ret; } -- 2.20.1

On 2/7/19 8:06 AM, Erik Skultety wrote: So the point wasn't to get rid of the label, the point was to be consistent. A "side reference" to this is the LGTM static analysis website Daniel pointed out last week in some patches. One of the "alerts" they post is use of "goto backwards". It was in a slightly different usage model, but multiple labels in the code has gotten us in trouble before especially when code is refactored or a couple lines moved where a goto cleanup should now be a goto error or goto endjob (more likely). I obviously know this amount of diff would generate a noteworthy comment and it's fine. Still I considered how I've seen this type of processing done for other modules and I liked the format better where the @def was being used to fill things in and the @ret was stolen to be returned. To me that just looks cleaner. I can like in other instances create a pre-patch that will only make the def/ret type change and then the magic of VIR_AUTOPTR in the subsequent patch. But like VIR_STEAL_PTR in general while generating the patches it felt like I'd end up with a very large series. I was between a rock and a hard place, so I took the easy way out. Tough to undo years of goto's and error path additions. While doing this I started to realize these types of changes probably don't fall into a first time patch for new users! There needs to be a fair amount of "finesse" to properly order things. Tks - John

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

[...] This is the "right" place as @def is consumed by AssignDef. 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 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. <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

Let's make use of the auto __cleanup capabilities cleaning up any now unnecessary goto paths. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_disk.c | 85 +++++++++------------ src/storage/storage_backend_fs.c | 39 +++------- src/storage/storage_backend_logical.c | 101 +++++++------------------ src/storage/storage_backend_sheepdog.c | 50 +++++------- src/storage/storage_backend_vstorage.c | 14 +--- src/storage/storage_backend_zfs.c | 47 +++--------- src/storage/storage_driver.c | 3 +- src/storage/storage_util.c | 34 +++------ src/util/virstoragefile.c | 63 +++++++-------- tests/storagepoolxml2argvtest.c | 3 +- tests/storagevolxml2argvtest.c | 6 +- tests/virstoragetest.c | 6 +- 12 files changed, 148 insertions(+), 303 deletions(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 061c494b7d..230cf44b97 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -356,7 +356,7 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool, virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); char *parthelper_path; - virCommandPtr cmd; + VIR_AUTOPTR(virCommand) cmd = NULL; struct virStorageBackendDiskPoolVolData cbdata = { .pool = pool, .vol = vol, @@ -392,7 +392,6 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool, 6, virStorageBackendDiskMakeVol, &cbdata); - virCommandFree(cmd); VIR_FREE(parthelper_path); return ret; } @@ -421,7 +420,7 @@ virStorageBackendDiskReadGeometry(virStoragePoolObjPtr pool) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); char *parthelper_path; - virCommandPtr cmd; + VIR_AUTOPTR(virCommand) cmd = NULL; int ret; if (!(parthelper_path = virFileFindResource("libvirt_parthelper", @@ -438,7 +437,6 @@ virStorageBackendDiskReadGeometry(virStoragePoolObjPtr pool) 3, virStorageBackendDiskMakePoolGeometry, pool); - virCommandFree(cmd); VIR_FREE(parthelper_path); return ret; } @@ -502,51 +500,40 @@ virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool, virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); int format = def->source.format; const char *fmt; - bool ok_to_mklabel = false; - int ret = -1; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE | - VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret); + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, -1); - VIR_EXCLUSIVE_FLAGS_GOTO(VIR_STORAGE_POOL_BUILD_OVERWRITE, - VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, - error); + VIR_EXCLUSIVE_FLAGS_RET(VIR_STORAGE_POOL_BUILD_OVERWRITE, + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, + -1); fmt = virStoragePoolFormatDiskTypeToString(format); - if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) { - ok_to_mklabel = true; - } else { - if (virStorageBackendDeviceIsEmpty(def->source.devices[0].path, - fmt, true)) - ok_to_mklabel = true; - } + if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) && + !(virStorageBackendDeviceIsEmpty(def->source.devices[0].path, + fmt, true))) + return -1; - if (ok_to_mklabel) { - if (virStorageBackendZeroPartitionTable(def->source.devices[0].path, - 1024 * 1024) < 0) - goto error; + if (virStorageBackendZeroPartitionTable(def->source.devices[0].path, + 1024 * 1024) < 0) + return -1; - /* eg parted /dev/sda mklabel --script msdos */ - if (format == VIR_STORAGE_POOL_DISK_UNKNOWN) - format = def->source.format = VIR_STORAGE_POOL_DISK_DOS; - if (format == VIR_STORAGE_POOL_DISK_DOS) - fmt = "msdos"; - else - fmt = virStoragePoolFormatDiskTypeToString(format); - - cmd = virCommandNewArgList(PARTED, - def->source.devices[0].path, - "mklabel", - "--script", - fmt, - NULL); - ret = virCommandRun(cmd, NULL); - } + /* eg parted /dev/sda mklabel --script msdos */ + if (format == VIR_STORAGE_POOL_DISK_UNKNOWN) + format = def->source.format = VIR_STORAGE_POOL_DISK_DOS; + if (format == VIR_STORAGE_POOL_DISK_DOS) + fmt = "msdos"; + else + fmt = virStoragePoolFormatDiskTypeToString(format); - error: - virCommandFree(cmd); - return ret; + cmd = virCommandNewArgList(PARTED, + def->source.devices[0].path, + "mklabel", + "--script", + fmt, + NULL); + return virCommandRun(cmd, NULL); } @@ -787,7 +774,7 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool, virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); char *src_path = def->source.devices[0].path; char *srcname = last_component(src_path); - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; bool isDevMapperDevice; int rc = -1; @@ -862,7 +849,6 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool, rc = 0; cleanup: VIR_FREE(devpath); - virCommandFree(cmd); return rc; } @@ -876,11 +862,13 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool, unsigned long long startOffset = 0, endOffset = 0; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); virErrorPtr save_err; - virCommandPtr cmd = virCommandNewArgList(PARTED, - def->source.devices[0].path, - "mkpart", - "--script", - NULL); + VIR_AUTOPTR(virCommand) cmd = NULL; + + cmd = virCommandNewArgList(PARTED, + def->source.devices[0].path, + "mkpart", + "--script", + NULL); if (vol->target.encryption && vol->target.encryption->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { @@ -934,7 +922,6 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool, cleanup: VIR_FREE(partFormat); - virCommandFree(cmd); return res; error: diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 06c76fde4f..7d05ceeeb8 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -95,8 +95,6 @@ virStorageBackendFileSystemNetFindPoolSourcesFunc(char **const groups, static int virStorageBackendFileSystemNetFindNFSPoolSources(virNetfsDiscoverState *state) { - int ret = -1; - /* * # showmount --no-headers -e HOSTNAME * /tmp * @@ -112,7 +110,7 @@ virStorageBackendFileSystemNetFindNFSPoolSources(virNetfsDiscoverState *state) 1 }; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; cmd = virCommandNewArgList(SHOWMOUNT, "--no-headers", @@ -120,16 +118,9 @@ virStorageBackendFileSystemNetFindNFSPoolSources(virNetfsDiscoverState *state) state->host, NULL); - if (virCommandRunRegex(cmd, 1, regexes, vars, - virStorageBackendFileSystemNetFindPoolSourcesFunc, - state, NULL, NULL) < 0) - goto cleanup; - - ret = 0; - - cleanup: - virCommandFree(cmd); - return ret; + return virCommandRunRegex(cmd, 1, regexes, vars, + virStorageBackendFileSystemNetFindPoolSourcesFunc, + state, NULL, NULL); } @@ -309,7 +300,7 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); char *src = NULL; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; int ret = -1; int rc; @@ -334,7 +325,6 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) ret = 0; cleanup: - virCommandFree(cmd); VIR_FREE(src); return ret; } @@ -376,8 +366,7 @@ static int virStorageBackendFileSystemStop(virStoragePoolObjPtr pool) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr cmd = NULL; - int ret = -1; + VIR_AUTOPTR(virCommand) cmd = NULL; int rc; if (virStorageBackendFileSystemIsValid(pool) < 0) @@ -388,13 +377,7 @@ virStorageBackendFileSystemStop(virStoragePoolObjPtr pool) return rc; cmd = virCommandNewArgList(UMOUNT, def->target.path, NULL); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - - ret = 0; - cleanup: - virCommandFree(cmd); - return ret; + return virCommandRun(cmd, NULL); } #endif /* WITH_STORAGE_FS */ @@ -432,8 +415,7 @@ static int virStorageBackendExecuteMKFS(const char *device, const char *format) { - int ret = 0; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; cmd = virCommandNewArgList(MKFS, "-t", format, NULL); @@ -456,11 +438,10 @@ virStorageBackendExecuteMKFS(const char *device, _("Failed to make filesystem of " "type '%s' on device '%s'"), format, device); - ret = -1; + return -1; } - virCommandFree(cmd); - return ret; + return 0; } #else /* #ifdef MKFS */ static int diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index fd95bd0d48..e0ddc962b1 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -48,13 +48,11 @@ static int virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, bool on) { - int ret; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr cmd = virStorageBackendLogicalChangeCmd(VGCHANGE, def, on); + VIR_AUTOPTR(virCommand) cmd = NULL; - ret = virCommandRun(cmd, NULL); - virCommandFree(cmd); - return ret; + cmd = virStorageBackendLogicalChangeCmd(VGCHANGE, def, on); + return virCommandRun(cmd, NULL); } @@ -67,11 +65,11 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, static void virStorageBackendLogicalRemoveDevice(const char *path) { - virCommandPtr cmd = virCommandNewArgList(PVREMOVE, path, NULL); + VIR_AUTOPTR(virCommand) cmd = NULL; + cmd = virCommandNewArgList(PVREMOVE, path, NULL); if (virCommandRun(cmd, NULL) < 0) VIR_INFO("Failed to pvremove logical device '%s'", path); - virCommandFree(cmd); } @@ -85,8 +83,7 @@ virStorageBackendLogicalRemoveDevice(const char *path) static int virStorageBackendLogicalInitializeDevice(const char *path) { - int ret = -1; - virCommandPtr pvcmd = NULL; + VIR_AUTOPTR(virCommand) pvcmd = NULL; /* * LVM requires that the first sector is blanked if using @@ -101,15 +98,7 @@ virStorageBackendLogicalInitializeDevice(const char *path) * clever enough todo this for us :-( */ pvcmd = virCommandNewArgList(PVCREATE, path, NULL); - if (virCommandRun(pvcmd, NULL) < 0) - goto cleanup; - - ret = 0; - - cleanup: - virCommandFree(pvcmd); - - return ret; + return virCommandRun(pvcmd, NULL); } @@ -426,8 +415,7 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, int vars[] = { VIR_STORAGE_VOL_LOGICAL_REGEX_COUNT }; - int ret = -1; - virCommandPtr cmd; + VIR_AUTOPTR(virCommand) cmd = NULL; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); struct virStorageBackendLogicalPoolVolData cbdata = { .pool = pool, @@ -444,20 +432,9 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, "lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size,size,lv_attr", def->source.name, NULL); - if (virCommandRunRegex(cmd, - 1, - regexes, - vars, - virStorageBackendLogicalMakeVol, - &cbdata, - "lvs", - NULL) < 0) - goto cleanup; - - ret = 0; - cleanup: - virCommandFree(cmd); - return ret; + return virCommandRunRegex(cmd, 1, regexes, vars, + virStorageBackendLogicalMakeVol, + &cbdata, "lvs", NULL); } static int @@ -550,8 +527,7 @@ virStorageBackendLogicalGetPoolSources(virStoragePoolSourceListPtr sourceList) int vars[] = { 2 }; - virCommandPtr cmd; - int ret = -1; + VIR_AUTOPTR(virCommand) cmd = NULL; /* * NOTE: ignoring errors here; this is just to "touch" any logical volumes @@ -567,15 +543,9 @@ virStorageBackendLogicalGetPoolSources(virStoragePoolSourceListPtr sourceList) "--noheadings", "-o", "pv_name,vg_name", NULL, NULL); - if (virCommandRunRegex(cmd, 1, regexes, vars, - virStorageBackendLogicalFindPoolSourcesFunc, - sourceList, "pvs", NULL) < 0) - goto cleanup; - ret = 0; - - cleanup: - virCommandFree(cmd); - return ret; + return virCommandRunRegex(cmd, 1, regexes, vars, + virStorageBackendLogicalFindPoolSourcesFunc, + sourceList, "pvs", NULL); } @@ -737,7 +707,7 @@ virStorageBackendLogicalBuildPool(virStoragePoolObjPtr pool, unsigned int flags) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr vgcmd = NULL; + VIR_AUTOPTR(virCommand) vgcmd = NULL; int ret = -1; size_t i = 0; @@ -773,8 +743,6 @@ virStorageBackendLogicalBuildPool(virStoragePoolObjPtr pool, ret = 0; cleanup: - virCommandFree(vgcmd); - /* On any failure, run through the devices that had pvcreate run in * in order to run pvremove on the device; otherwise, subsequent build * will fail if a pvcreate had been run already. */ @@ -805,7 +773,7 @@ virStorageBackendLogicalRefreshPool(virStoragePoolObjPtr pool) 2 }; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; int ret = -1; virWaitForDevices(); @@ -838,7 +806,6 @@ virStorageBackendLogicalRefreshPool(virStoragePoolObjPtr pool) ret = 0; cleanup: - virCommandFree(cmd); if (ret < 0) virStoragePoolObjClearVols(pool); return ret; @@ -863,9 +830,8 @@ virStorageBackendLogicalDeletePool(virStoragePoolObjPtr pool, unsigned int flags) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; size_t i; - int ret = -1; virCheckFlags(0, -1); @@ -874,17 +840,13 @@ virStorageBackendLogicalDeletePool(virStoragePoolObjPtr pool, "-f", def->source.name, NULL); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return -1; /* now remove the pv devices and clear them out */ for (i = 0; i < def->source.ndevice; i++) virStorageBackendLogicalRemoveDevice(def->source.devices[i].path); - ret = 0; - - cleanup: - virCommandFree(cmd); - return ret; + return 0; } @@ -893,10 +855,8 @@ virStorageBackendLogicalDeleteVol(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, virStorageVolDefPtr vol, unsigned int flags) { - int ret = -1; - - virCommandPtr lvchange_cmd = NULL; - virCommandPtr lvremove_cmd = NULL; + VIR_AUTOPTR(virCommand) lvchange_cmd = NULL; + VIR_AUTOPTR(virCommand) lvremove_cmd = NULL; virCheckFlags(0, -1); @@ -907,18 +867,14 @@ virStorageBackendLogicalDeleteVol(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, if (virCommandRun(lvremove_cmd, NULL) < 0) { if (virCommandRun(lvchange_cmd, NULL) < 0) { - goto cleanup; + return -1; } else { if (virCommandRun(lvremove_cmd, NULL) < 0) - goto cleanup; + return -1; } } - ret = 0; - cleanup: - virCommandFree(lvchange_cmd); - virCommandFree(lvremove_cmd); - return ret; + return 0; } @@ -926,9 +882,8 @@ static int virStorageBackendLogicalLVCreate(virStorageVolDefPtr vol, virStoragePoolDefPtr def) { - int ret; unsigned long long capacity = vol->target.capacity; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; if (vol->target.encryption && vol->target.encryption->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { @@ -961,9 +916,7 @@ virStorageBackendLogicalLVCreate(virStorageVolDefPtr vol, else virCommandAddArg(cmd, def->source.name); - ret = virCommandRun(cmd, NULL); - virCommandFree(cmd); - return ret; + return virCommandRun(cmd, NULL); } diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index 458dbca58b..d9de74f52e 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -140,9 +140,10 @@ virStorageBackendSheepdogRefreshAllVol(virStoragePoolObjPtr pool) char *output = NULL; VIR_AUTOPTR(virString) lines = NULL; VIR_AUTOPTR(virString) cells = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; size_t i; - virCommandPtr cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", "-r", NULL); + cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", "-r", NULL); virStorageBackendSheepdogAddHostArg(cmd, pool); virCommandSetOutputBuffer(cmd, &output); if (virCommandRun(cmd, NULL) < 0) @@ -172,7 +173,6 @@ virStorageBackendSheepdogRefreshAllVol(virStoragePoolObjPtr pool) ret = 0; cleanup: - virCommandFree(cmd); VIR_FREE(output); return ret; } @@ -183,7 +183,7 @@ virStorageBackendSheepdogRefreshPool(virStoragePoolObjPtr pool) { int ret = -1; char *output = NULL; - virCommandPtr cmd; + VIR_AUTOPTR(virCommand) cmd = NULL; cmd = virCommandNewArgList(SHEEPDOGCLI, "node", "info", "-r", NULL); virStorageBackendSheepdogAddHostArg(cmd, pool); @@ -197,7 +197,6 @@ virStorageBackendSheepdogRefreshPool(virStoragePoolObjPtr pool) ret = virStorageBackendSheepdogRefreshAllVol(pool); cleanup: - virCommandFree(cmd); VIR_FREE(output); return ret; } @@ -208,14 +207,12 @@ virStorageBackendSheepdogDeleteVol(virStoragePoolObjPtr pool, virStorageVolDefPtr vol, unsigned int flags) { + VIR_AUTOPTR(virCommand) cmd = NULL; virCheckFlags(0, -1); - virCommandPtr cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "delete", vol->name, NULL); + cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "delete", vol->name, NULL); virStorageBackendSheepdogAddHostArg(cmd, pool); - int ret = virCommandRun(cmd, NULL); - - virCommandFree(cmd); - return ret; + return virCommandRun(cmd, NULL); } @@ -252,27 +249,20 @@ virStorageBackendSheepdogBuildVol(virStoragePoolObjPtr pool, virStorageVolDefPtr vol, unsigned int flags) { - int ret = -1; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; virCheckFlags(0, -1); if (!vol->target.capacity) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("volume capacity required for this pool")); - goto cleanup; + return -1; } cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "create", vol->name, NULL); virCommandAddArgFormat(cmd, "%llu", vol->target.capacity); virStorageBackendSheepdogAddHostArg(cmd, pool); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - - ret = 0; - cleanup: - virCommandFree(cmd); - return ret; + return virCommandRun(cmd, NULL); } @@ -344,30 +334,30 @@ virStorageBackendSheepdogRefreshVol(virStoragePoolObjPtr pool, int ret; char *output = NULL; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", vol->name, "-r", NULL); + VIR_AUTOPTR(virCommand) cmd = NULL; + cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", vol->name, "-r", NULL); virStorageBackendSheepdogAddHostArg(cmd, pool); virCommandSetOutputBuffer(cmd, &output); ret = virCommandRun(cmd, NULL); if (ret < 0) - goto cleanup; + return ret; if ((ret = virStorageBackendSheepdogParseVdiList(vol, output)) < 0) - goto cleanup; + return ret; vol->type = VIR_STORAGE_VOL_NETWORK; VIR_FREE(vol->key); if (virAsprintf(&vol->key, "%s/%s", def->source.name, vol->name) < 0) - goto cleanup; + return -1; VIR_FREE(vol->target.path); ignore_value(VIR_STRDUP(vol->target.path, vol->name)); - cleanup: - virCommandFree(cmd); - return ret; + + return 0; } @@ -377,15 +367,13 @@ virStorageBackendSheepdogResizeVol(virStoragePoolObjPtr pool, unsigned long long capacity, unsigned int flags) { + VIR_AUTOPTR(virCommand) cmd = NULL; virCheckFlags(0, -1); - virCommandPtr cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "resize", vol->name, NULL); + cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "resize", vol->name, NULL); virCommandAddArgFormat(cmd, "%llu", capacity); virStorageBackendSheepdogAddHostArg(cmd, pool); - int ret = virCommandRun(cmd, NULL); - - virCommandFree(cmd); - return ret; + return virCommandRun(cmd, NULL); } diff --git a/src/storage/storage_backend_vstorage.c b/src/storage/storage_backend_vstorage.c index 0a9abd446b..e62234fd70 100644 --- a/src/storage/storage_backend_vstorage.c +++ b/src/storage/storage_backend_vstorage.c @@ -39,7 +39,7 @@ virStorageBackendVzPoolStart(virStoragePoolObjPtr pool) { int ret = -1; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; char *grp_name = NULL; char *usr_name = NULL; char *mode = NULL; @@ -75,7 +75,6 @@ virStorageBackendVzPoolStart(virStoragePoolObjPtr pool) ret = 0; cleanup: - virCommandFree(cmd); VIR_FREE(mode); VIR_FREE(grp_name); VIR_FREE(usr_name); @@ -125,8 +124,7 @@ static int virStorageBackendVzPoolStop(virStoragePoolObjPtr pool) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr cmd = NULL; - int ret = -1; + VIR_AUTOPTR(virCommand) cmd = NULL; int rc; /* Short-circuit if already unmounted */ @@ -134,13 +132,7 @@ virStorageBackendVzPoolStop(virStoragePoolObjPtr pool) return rc; cmd = virCommandNewArgList(UMOUNT, def->target.path, NULL); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - - ret = 0; - cleanup: - virCommandFree(cmd); - return ret; + return virCommandRun(cmd, NULL); } diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c index f728477c2a..55f8a138cf 100644 --- a/src/storage/storage_backend_zfs.c +++ b/src/storage/storage_backend_zfs.c @@ -51,7 +51,7 @@ VIR_LOG_INIT("storage.storage_backend_zfs"); static int virStorageBackendZFSVolModeNeeded(void) { - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; int ret = -1, exit_code = -1; char *error = NULL; @@ -77,7 +77,6 @@ virStorageBackendZFSVolModeNeeded(void) ret = 0; cleanup: - virCommandFree(cmd); VIR_FREE(error); return ret; } @@ -179,7 +178,7 @@ virStorageBackendZFSFindVols(virStoragePoolObjPtr pool, virStorageVolDefPtr vol) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; char *volumes_list = NULL; VIR_AUTOPTR(virString) lines = NULL; size_t i; @@ -218,7 +217,6 @@ virStorageBackendZFSFindVols(virStoragePoolObjPtr pool, } cleanup: - virCommandFree(cmd); VIR_FREE(volumes_list); return 0; @@ -228,8 +226,8 @@ static int virStorageBackendZFSRefreshPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr cmd = NULL; char *zpool_props = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; VIR_AUTOPTR(virString) lines = NULL; VIR_AUTOPTR(virString) tokens = NULL; size_t i; @@ -292,7 +290,6 @@ virStorageBackendZFSRefreshPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED) goto cleanup; cleanup: - virCommandFree(cmd); VIR_FREE(zpool_props); return 0; @@ -303,7 +300,7 @@ virStorageBackendZFSCreateVol(virStoragePoolObjPtr pool, virStorageVolDefPtr vol) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; int ret = -1; int volmode_needed = -1; @@ -364,7 +361,6 @@ virStorageBackendZFSCreateVol(virStoragePoolObjPtr pool, ret = 0; cleanup: - virCommandFree(cmd); return ret; } @@ -374,9 +370,8 @@ virStorageBackendZFSDeleteVol(virStoragePoolObjPtr pool, virStorageVolDefPtr vol, unsigned int flags) { - int ret = -1; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr destroy_cmd = NULL; + VIR_AUTOPTR(virCommand) destroy_cmd = NULL; virCheckFlags(0, -1); @@ -385,13 +380,7 @@ virStorageBackendZFSDeleteVol(virStoragePoolObjPtr pool, virCommandAddArgFormat(destroy_cmd, "%s/%s", def->source.name, vol->name); - if (virCommandRun(destroy_cmd, NULL) < 0) - goto cleanup; - - ret = 0; - cleanup: - virCommandFree(destroy_cmd); - return ret; + return virCommandRun(destroy_cmd, NULL); } static int @@ -399,9 +388,8 @@ virStorageBackendZFSBuildPool(virStoragePoolObjPtr pool, unsigned int flags) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; size_t i; - int ret = -1; virCheckFlags(0, -1); @@ -417,14 +405,7 @@ virStorageBackendZFSBuildPool(virStoragePoolObjPtr pool, for (i = 0; i < def->source.ndevice; i++) virCommandAddArg(cmd, def->source.devices[i].path); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - - ret = 0; - - cleanup: - virCommandFree(cmd); - return ret; + return virCommandRun(cmd, NULL); } static int @@ -432,22 +413,14 @@ virStorageBackendZFSDeletePool(virStoragePoolObjPtr pool, unsigned int flags) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr cmd = NULL; - int ret = -1; + VIR_AUTOPTR(virCommand) cmd = NULL; virCheckFlags(0, -1); cmd = virCommandNewArgList(ZPOOL, "destroy", def->source.name, NULL); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - - ret = 0; - - cleanup: - virCommandFree(cmd); - return ret; + return virCommandRun(cmd, NULL); } virStorageBackend virStorageBackendZFS = { diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index dcdd52bbbf..7c377439dc 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2207,7 +2207,7 @@ static int virStorageBackendPloopRestoreDesc(char *path) { int ret = -1; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; char *refresh_tool = NULL; char *desc = NULL; @@ -2238,7 +2238,6 @@ virStorageBackendPloopRestoreDesc(char *path) cleanup: VIR_FREE(refresh_tool); - virCommandFree(cmd); VIR_FREE(desc); return ret; } diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 158cf3adb6..b9dbd3048a 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -617,7 +617,7 @@ storageBackendCreatePloop(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, unsigned int flags) { int ret = -1; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; char *create_tool = NULL; bool created = false; @@ -677,7 +677,6 @@ storageBackendCreatePloop(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, created = true; ret = virCommandRun(cmd, NULL); cleanup: - virCommandFree(cmd); VIR_FREE(create_tool); if (ret < 0 && created) virFileDeleteTree(vol->target.path); @@ -690,7 +689,7 @@ storagePloopResize(virStorageVolDefPtr vol, unsigned long long capacity) { int ret = -1; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; char *resize_tool = NULL; resize_tool = virFindFileInPath("ploop"); @@ -705,7 +704,6 @@ storagePloopResize(virStorageVolDefPtr vol, virCommandAddArgFormat(cmd, "%s/DiskDescriptor.xml", vol->target.path); ret = virCommandRun(cmd, NULL); - virCommandFree(cmd); VIR_FREE(resize_tool); return ret; } @@ -1319,8 +1317,7 @@ storageBackendDoCreateQemuImg(virStoragePoolObjPtr pool, const char *inputSecretPath, virStorageVolEncryptConvertStep convertStep) { - int ret; - virCommandPtr cmd; + VIR_AUTOPTR(virCommand) cmd = NULL; cmd = virStorageBackendCreateQemuImgCmdFromVol(pool, vol, inputvol, flags, create_tool, @@ -1329,11 +1326,7 @@ storageBackendDoCreateQemuImg(virStoragePoolObjPtr pool, if (!cmd) return -1; - ret = virStorageBackendCreateExecCommand(pool, vol, cmd); - - virCommandFree(cmd); - - return ret; + return virStorageBackendCreateExecCommand(pool, vol, cmd); } @@ -2324,7 +2317,7 @@ storageBackendResizeQemuImg(virStoragePoolObjPtr pool, { int ret = -1; char *img_tool = NULL; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; const char *type; char *secretPath = NULL; char *secretAlias = NULL; @@ -2395,7 +2388,6 @@ storageBackendResizeQemuImg(virStoragePoolObjPtr pool, VIR_FREE(secretPath); } VIR_FREE(secretAlias); - virCommandFree(cmd); return ret; } @@ -2449,7 +2441,7 @@ virStorageBackendVolResizeLocal(virStoragePoolObjPtr pool, static int storageBackendPloopHasSnapshots(char *path) { - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; char *output = NULL; char *snap_tool = NULL; int ret = -1; @@ -2477,7 +2469,6 @@ storageBackendPloopHasSnapshots(char *path) cleanup: VIR_FREE(output); - virCommandFree(cmd); return ret; } @@ -2685,7 +2676,7 @@ storageBackendVolWipeLocalFile(const char *path, int ret = -1, fd = -1; const char *alg_char = NULL; struct stat st; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; fd = open(path, O_RDWR); if (fd == -1) { @@ -2763,7 +2754,6 @@ storageBackendVolWipeLocalFile(const char *path, } cleanup: - virCommandFree(cmd); VIR_FORCE_CLOSE(fd); return ret; } @@ -2773,7 +2763,7 @@ static int storageBackendVolWipePloop(virStorageVolDefPtr vol, unsigned int algorithm) { - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; char *target_path = NULL; char *disk_desc = NULL; char *create_tool = NULL; @@ -2820,7 +2810,6 @@ storageBackendVolWipePloop(virStorageVolDefPtr vol, VIR_FREE(disk_desc); VIR_FREE(target_path); VIR_FREE(create_tool); - virCommandFree(cmd); return ret; } @@ -3034,7 +3023,7 @@ virStorageBackendFindGlusterPoolSources(const char *host, { char *glusterpath = NULL; char *outbuf = NULL; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; int rc; int ret = -1; @@ -3069,7 +3058,6 @@ virStorageBackendFindGlusterPoolSources(const char *host, cleanup: VIR_FREE(outbuf); - virCommandFree(cmd); VIR_FREE(glusterpath); return ret; } @@ -3309,12 +3297,13 @@ virStorageBackendPARTEDFindLabel(const char *device, const char *const args[] = { device, "print", "--script", NULL, }; - virCommandPtr cmd = virCommandNew(PARTED); + VIR_AUTOPTR(virCommand) cmd = NULL; char *output = NULL; char *error = NULL; char *start, *end; int ret = VIR_STORAGE_PARTED_ERROR; + cmd = virCommandNew(PARTED); virCommandAddArgSet(cmd, args); virCommandAddEnvString(cmd, "LC_ALL=C"); virCommandSetOutputBuffer(cmd, &output); @@ -3359,7 +3348,6 @@ virStorageBackendPARTEDFindLabel(const char *device, ret = VIR_STORAGE_PARTED_DIFFERENT; cleanup: - virCommandFree(cmd); VIR_FREE(output); VIR_FREE(error); return ret; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cd2437a03e..2e5c64e034 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1376,13 +1376,14 @@ int virStorageFileGetLVMKey(const char *path, * 06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky */ int status; - virCommandPtr cmd = virCommandNewArgList(LVS, "--noheadings", - "--unbuffered", "--nosuffix", - "--options", "uuid", path, - NULL - ); + VIR_AUTOPTR(virCommand) cmd = NULL; int ret = -1; + cmd = virCommandNewArgList(LVS, "--noheadings", + "--unbuffered", "--nosuffix", + "--options", "uuid", path, + NULL + ); *key = NULL; /* Run the program and capture its output */ @@ -1417,8 +1418,6 @@ int virStorageFileGetLVMKey(const char *path, if (*key && STREQ(*key, "")) VIR_FREE(*key); - virCommandFree(cmd); - return ret; } #else @@ -1451,20 +1450,20 @@ virStorageFileGetSCSIKey(const char *path, bool ignoreError ATTRIBUTE_UNUSED) { int status; - virCommandPtr cmd = virCommandNewArgList("/lib/udev/scsi_id", - "--replace-whitespace", - "--whitelisted", - "--device", path, - NULL - ); - int ret = -2; - + VIR_AUTOPTR(virCommand) cmd = NULL; + + cmd = virCommandNewArgList("/lib/udev/scsi_id", + "--replace-whitespace", + "--whitelisted", + "--device", path, + NULL + ); *key = NULL; /* Run the program and capture its output */ virCommandSetOutputBuffer(cmd, key); if (virCommandRun(cmd, &status) < 0) - goto cleanup; + return -2; /* Explicitly check status == 0, rather than passing NULL * to virCommandRun because we don't want to raise an actual @@ -1476,15 +1475,10 @@ virStorageFileGetSCSIKey(const char *path, *nl = '\0'; } - ret = 0; - - cleanup: if (*key && STREQ(*key, "")) VIR_FREE(*key); - virCommandFree(cmd); - - return ret; + return 0; } #else int virStorageFileGetSCSIKey(const char *path, @@ -1522,23 +1516,23 @@ virStorageFileGetNPIVKey(const char *path, { int status; VIR_AUTOFREE(char *) outbuf = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; const char *serial; const char *port; - virCommandPtr cmd = virCommandNewArgList("/lib/udev/scsi_id", - "--replace-whitespace", - "--whitelisted", - "--export", - "--device", path, - NULL - ); - int ret = -2; + cmd = virCommandNewArgList("/lib/udev/scsi_id", + "--replace-whitespace", + "--whitelisted", + "--export", + "--device", path, + NULL + ); *key = NULL; /* Run the program and capture its output */ virCommandSetOutputBuffer(cmd, &outbuf); if (virCommandRun(cmd, &status) < 0) - goto cleanup; + return -2; /* Explicitly check status == 0, rather than passing NULL * to virCommandRun because we don't want to raise an actual @@ -1562,12 +1556,7 @@ virStorageFileGetNPIVKey(const char *path, ignore_value(virAsprintf(key, "%s_PORT%s", serial, port)); } - ret = 0; - - cleanup: - virCommandFree(cmd); - - return ret; + return 0; } #else int virStorageFileGetNPIVKey(const char *path ATTRIBUTE_UNUSED, diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c index 2dd87ab731..116a75c3ea 100644 --- a/tests/storagepoolxml2argvtest.c +++ b/tests/storagepoolxml2argvtest.c @@ -25,9 +25,9 @@ testCompareXMLToArgvFiles(bool shouldFail, VIR_AUTOFREE(char *) actualCmdline = NULL; VIR_AUTOFREE(char *) src = NULL; VIR_AUTOPTR(virStoragePoolDef) def = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; int defType; int ret = -1; - virCommandPtr cmd = NULL; virStoragePoolObjPtr pool = NULL; virStoragePoolDefPtr objDef; @@ -88,7 +88,6 @@ testCompareXMLToArgvFiles(bool shouldFail, ret = 0; cleanup: - virCommandFree(cmd); VIR_FREE(actualCmdline); virStoragePoolObjEndAPI(&pool); if (shouldFail) { diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index 2acbf567ca..38bb2ae004 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -45,9 +45,7 @@ testCompareXMLToArgvFiles(bool shouldFail, char *actualCmdline = NULL; virStorageVolEncryptConvertStep convertStep = VIR_STORAGE_VOL_ENCRYPT_NONE; int ret = -1; - - virCommandPtr cmd = NULL; - + VIR_AUTOPTR(virCommand) cmd = NULL; VIR_AUTOPTR(virStorageVolDef) vol = NULL; VIR_AUTOPTR(virStorageVolDef) inputvol = NULL; VIR_AUTOPTR(virStoragePoolDef) inputpool = NULL; @@ -91,6 +89,7 @@ testCompareXMLToArgvFiles(bool shouldFail, convertStep = VIR_STORAGE_VOL_ENCRYPT_CREATE; do { + virCommandFree(cmd); cmd = virStorageBackendCreateQemuImgCmdFromVol(obj, vol, inputvol, flags, create_tool, @@ -140,7 +139,6 @@ testCompareXMLToArgvFiles(bool shouldFail, ret = 0; cleanup: - virCommandFree(cmd); VIR_FREE(actualCmdline); virStoragePoolObjEndAPI(&obj); return ret; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index c7c40b16f8..c448d8b733 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -128,7 +128,7 @@ static int testPrepImages(void) { int ret = EXIT_FAILURE; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; char *buf = NULL; bool compat = false; @@ -246,7 +246,6 @@ testPrepImages(void) ret = 0; cleanup: VIR_FREE(buf); - virCommandFree(cmd); if (ret) testCleanupImages(); return ret; @@ -713,7 +712,7 @@ static int mymain(void) { int ret; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; struct testChainData data; struct testLookupData data2; struct testPathCanonicalizeData data3; @@ -1604,7 +1603,6 @@ mymain(void) /* Final cleanup */ virStorageSourceFree(chain); testCleanupImages(); - virCommandFree(cmd); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.20.1

Let's make use of the auto __cleanup capabilities. This also allows for the cleanup of some goto paths. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend.c | 9 +-- src/storage/storage_backend_disk.c | 62 +++++++----------- src/storage/storage_backend_fs.c | 17 ++--- src/storage/storage_backend_gluster.c | 30 ++++----- src/storage/storage_backend_iscsi.c | 73 +++++++--------------- src/storage/storage_backend_iscsi_direct.c | 37 ++++------- src/storage/storage_backend_logical.c | 35 ++++------- src/storage/storage_backend_mpath.c | 18 +++--- src/storage/storage_backend_rbd.c | 35 ++++------- src/storage/storage_backend_scsi.c | 63 +++++++------------ src/storage/storage_backend_sheepdog.c | 27 +++----- src/storage/storage_backend_vstorage.c | 25 +++----- src/storage/storage_backend_zfs.c | 15 ++--- src/storage/storage_file_gluster.c | 16 ++--- 14 files changed, 153 insertions(+), 309 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a54c338cf0..5c8275e978 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -87,8 +87,7 @@ virStorageDriverLoadBackendModule(const char *name, const char *regfunc, bool forceload) { - char *modfile = NULL; - int ret; + VIR_AUTOFREE(char *) modfile = NULL; if (!(modfile = virFileFindResourceFull(name, "libvirt_storage_backend_", @@ -98,11 +97,7 @@ virStorageDriverLoadBackendModule(const char *name, "LIBVIRT_STORAGE_BACKEND_DIR"))) return -1; - ret = virModuleLoad(modfile, regfunc, forceload); - - VIR_FREE(modfile); - - return ret; + return virModuleLoad(modfile, regfunc, forceload); } diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 230cf44b97..f2f56ee3de 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -56,7 +56,8 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, virStorageVolDefPtr vol) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - char *tmp, *devpath, *partname; + char *tmp, *partname; + VIR_AUTOFREE(char *) devpath = NULL; bool addVol = false; /* Prepended path will be same for all partitions, so we can @@ -89,7 +90,6 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, * way of doing this... */ vol->target.path = virStorageBackendStablePath(pool, devpath, true); - VIR_FREE(devpath); if (vol->target.path == NULL) goto error; } @@ -355,13 +355,12 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool, */ virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - char *parthelper_path; + VIR_AUTOFREE(char *) parthelper_path = NULL; VIR_AUTOPTR(virCommand) cmd = NULL; struct virStorageBackendDiskPoolVolData cbdata = { .pool = pool, .vol = vol, }; - int ret; if (!(parthelper_path = virFileFindResource("libvirt_parthelper", abs_topbuilddir "/src", @@ -388,12 +387,7 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool, def->allocation = 0; def->capacity = def->available = 0; - ret = virCommandRunNul(cmd, - 6, - virStorageBackendDiskMakeVol, - &cbdata); - VIR_FREE(parthelper_path); - return ret; + return virCommandRunNul(cmd, 6, virStorageBackendDiskMakeVol, &cbdata); } static int @@ -419,9 +413,8 @@ static int virStorageBackendDiskReadGeometry(virStoragePoolObjPtr pool) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - char *parthelper_path; + VIR_AUTOFREE(char *) parthelper_path = NULL; VIR_AUTOPTR(virCommand) cmd = NULL; - int ret; if (!(parthelper_path = virFileFindResource("libvirt_parthelper", abs_topbuilddir "/src", @@ -433,12 +426,8 @@ virStorageBackendDiskReadGeometry(virStoragePoolObjPtr pool) "-g", NULL); - ret = virCommandRunNul(cmd, - 3, - virStorageBackendDiskMakePoolGeometry, - pool); - VIR_FREE(parthelper_path); - return ret; + return virCommandRunNul(cmd, 3, virStorageBackendDiskMakePoolGeometry, + pool); } static int @@ -769,14 +758,13 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool, unsigned int flags) { char *part_num = NULL; - char *devpath = NULL; char *dev_name; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); char *src_path = def->source.devices[0].path; char *srcname = last_component(src_path); + VIR_AUTOFREE(char *) devpath = NULL; VIR_AUTOPTR(virCommand) cmd = NULL; bool isDevMapperDevice; - int rc = -1; virCheckFlags(0, -1); @@ -799,7 +787,7 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool, virReportSystemError(errno, _("Couldn't read volume target path '%s'"), vol->target.path); - goto cleanup; + return -1; } dev_name = last_component(devpath); } @@ -810,7 +798,7 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool, virReportError(VIR_ERR_INTERNAL_ERROR, _("Volume path '%s' did not start with parent " "pool source device name."), dev_name); - goto cleanup; + return -1; } part_num = dev_name + strlen(srcname); @@ -824,7 +812,7 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool, virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse partition number from target " "'%s'"), dev_name); - goto cleanup; + return -1; } /* eg parted /dev/sda rm 2 or /dev/mapper/mpathc rm 2 */ @@ -835,7 +823,7 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool, part_num, NULL); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return -1; /* Refreshing the pool is the easiest option as LOGICAL and EXTENDED * partition allocation/capacity management is handled within @@ -844,12 +832,9 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool, */ virStoragePoolObjClearVols(pool); if (virStorageBackendDiskRefreshPool(pool) < 0) - goto cleanup; + return -1; - rc = 0; - cleanup: - VIR_FREE(devpath); - return rc; + return 0; } @@ -857,11 +842,10 @@ static int virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool, virStorageVolDefPtr vol) { - int res = -1; - char *partFormat = NULL; unsigned long long startOffset = 0, endOffset = 0; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); virErrorPtr save_err; + VIR_AUTOFREE(char *)partFormat = NULL; VIR_AUTOPTR(virCommand) cmd = NULL; cmd = virCommandNewArgList(PARTED, @@ -874,11 +858,11 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool, vol->target.encryption->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("storage pool only supports LUKS encrypted volumes")); - goto cleanup; + return -1; } if (virStorageBackendDiskPartFormat(pool, vol, &partFormat) != 0) - goto cleanup; + return -1; virCommandAddArg(cmd, partFormat); /* If we're going to encrypt using LUKS, then we could need up to @@ -888,13 +872,13 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool, if (virStorageBackendDiskPartBoundaries(pool, &startOffset, &endOffset, vol->target.capacity) < 0) - goto cleanup; + return -1; virCommandAddArgFormat(cmd, "%lluB", startOffset); virCommandAddArgFormat(cmd, "%lluB", endOffset); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return -1; /* wait for device node to show up */ virWaitForDevices(); @@ -918,11 +902,7 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool, goto error; } - res = 0; - - cleanup: - VIR_FREE(partFormat); - return res; + return 0; error: /* Best effort to remove the partition. Ignore any errors @@ -932,7 +912,7 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool, ignore_value(virStorageBackendDiskDeleteVol(pool, vol, 0)); virSetError(save_err); virFreeError(save_err); - goto cleanup; + return -1; } diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 7d05ceeeb8..41d010dea0 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -246,7 +246,7 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) { int ret = -1; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - char *src = NULL; + VIR_AUTOFREE(char *) src = NULL; FILE *mtab; struct mntent ent; char buf[1024]; @@ -282,7 +282,6 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) cleanup: VIR_FORCE_FCLOSE(mtab); - VIR_FREE(src); return ret; } @@ -299,9 +298,8 @@ static int virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - char *src = NULL; + VIR_AUTOFREE(char *) src = NULL; VIR_AUTOPTR(virCommand) cmd = NULL; - int ret = -1; int rc; if (virStorageBackendFileSystemIsValid(pool) < 0) @@ -320,13 +318,7 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) return -1; cmd = virStorageBackendFileSystemMountCmd(MOUNT, def, src); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - - ret = 0; - cleanup: - VIR_FREE(src); - return ret; + return virCommandRun(cmd, NULL); } @@ -579,7 +571,7 @@ virStoragePoolDefFSNamespaceParse(xmlXPathContextPtr ctxt, void **data) { virStoragePoolFSMountOptionsDefPtr cmdopts = NULL; - xmlNodePtr *nodes = NULL; + VIR_AUTOFREE(xmlNodePtr *)nodes = NULL; int nnodes; size_t i; int ret = -1; @@ -617,7 +609,6 @@ virStoragePoolDefFSNamespaceParse(xmlXPathContextPtr ctxt, ret = 0; cleanup: - VIR_FREE(nodes); virStoragePoolDefFSNamespaceFree(cmdopts); return ret; } diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index cb9f7e4735..0fe548f7e0 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -127,11 +127,9 @@ virStorageBackendGlusterOpen(virStoragePoolObjPtr pool) if (glfs_set_volfile_server(ret->vol, "tcp", ret->uri->server, ret->uri->port) < 0 || glfs_init(ret->vol) < 0) { - char *uri = virURIFormat(ret->uri); - - virReportSystemError(errno, _("failed to connect to %s"), - NULLSTR(uri)); - VIR_FREE(uri); + VIR_AUTOFREE(char *) uri = NULL; + uri = virURIFormat(ret->uri); + virReportSystemError(errno, _("failed to connect to %s"), NULLSTR(uri)); goto error; } @@ -187,8 +185,7 @@ virStorageBackendGlusterSetMetadata(virStorageBackendGlusterStatePtr state, virStorageVolDefPtr vol, const char *name) { - int ret = -1; - char *path = NULL; + VIR_AUTOFREE(char *) path = NULL; char *tmp; VIR_FREE(vol->key); @@ -200,35 +197,31 @@ virStorageBackendGlusterSetMetadata(virStorageBackendGlusterStatePtr state, if (name) { VIR_FREE(vol->name); if (VIR_STRDUP(vol->name, name) < 0) - goto cleanup; + return -1; } if (virAsprintf(&path, "%s%s%s", state->volname, state->dir, vol->name) < 0) - goto cleanup; + return -1; tmp = state->uri->path; if (virAsprintf(&state->uri->path, "/%s", path) < 0) { state->uri->path = tmp; - goto cleanup; + return -1; } if (!(vol->target.path = virURIFormat(state->uri))) { VIR_FREE(state->uri->path); state->uri->path = tmp; - goto cleanup; + return -1; } VIR_FREE(state->uri->path); state->uri->path = tmp; /* the path is unique enough to serve as a volume key */ if (VIR_STRDUP(vol->key, vol->target.path) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - VIR_FREE(path); - return ret; + return 0; } @@ -243,9 +236,9 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, { int ret = -1; VIR_AUTOPTR(virStorageVolDef) vol = NULL; + VIR_AUTOFREE(char *) header = NULL; glfs_fd_t *fd = NULL; virStorageSourcePtr meta = NULL; - char *header = NULL; ssize_t len; int backingFormat; @@ -333,7 +326,6 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, virStorageSourceFree(meta); if (fd) glfs_close(fd); - VIR_FREE(header); return ret; } diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 483ba15102..41fa5e128d 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -130,27 +130,20 @@ static int virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool, const char *session) { - char *sysfs_path; - int retval = -1; + VIR_AUTOFREE(char *) sysfs_path = NULL; uint32_t host; if (virAsprintf(&sysfs_path, "/sys/class/iscsi_session/session%s/device", session) < 0) - goto cleanup; + return -1; if (virStorageBackendISCSIGetHostNumber(sysfs_path, &host) < 0) - goto cleanup; + return -1; if (virStorageBackendSCSIFindLUs(pool, host) < 0) - goto cleanup; - - retval = 0; - - cleanup: - - VIR_FREE(sysfs_path); + return -1; - return retval; + return 0; } @@ -159,6 +152,7 @@ virStorageBackendISCSIFindPoolSources(const char *srcSpec, unsigned int flags) { VIR_AUTOPTR(virStoragePoolSource) source = NULL; + VIR_AUTOFREE(char *) portal = NULL; size_t ntargets = 0; char **targets = NULL; char *ret = NULL; @@ -168,7 +162,6 @@ virStorageBackendISCSIFindPoolSources(const char *srcSpec, .nsources = 0, .sources = NULL }; - char *portal = NULL; virCheckFlags(0, NULL); @@ -223,10 +216,10 @@ virStorageBackendISCSIFindPoolSources(const char *srcSpec, } VIR_FREE(list.sources); } + /* NB: Not virString -like, managed be VIR_APPEND_ELEMENT */ for (i = 0; i < ntargets; i++) VIR_FREE(targets[i]); VIR_FREE(targets); - VIR_FREE(portal); return ret; } @@ -235,7 +228,7 @@ virStorageBackendISCSICheckPool(virStoragePoolObjPtr pool, bool *isActive) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - char *session = NULL; + VIR_AUTOFREE(char *) session = NULL; int ret = -1; *isActive = false; @@ -259,10 +252,8 @@ virStorageBackendISCSICheckPool(virStoragePoolObjPtr pool, return -1; } - if ((session = virStorageBackendISCSISession(pool, true)) != NULL) { + if ((session = virStorageBackendISCSISession(pool, true))) *isActive = true; - VIR_FREE(session); - } ret = 0; return ret; @@ -330,9 +321,8 @@ static int virStorageBackendISCSIStartPool(virStoragePoolObjPtr pool) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - char *portal = NULL; - char *session = NULL; - int ret = -1; + VIR_AUTOFREE(char *) portal = NULL; + VIR_AUTOFREE(char *) session = NULL; if (def->source.nhost != 1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -355,50 +345,40 @@ virStorageBackendISCSIStartPool(virStoragePoolObjPtr pool) if ((session = virStorageBackendISCSISession(pool, true)) == NULL) { if ((portal = virStorageBackendISCSIPortal(&def->source)) == NULL) - goto cleanup; + return -1; /* Create a static node record for the IQN target. Must be done * in order for login to the target */ if (virISCSINodeNew(portal, def->source.devices[0].path) < 0) - goto cleanup; + return -1; if (virStorageBackendISCSISetAuth(portal, &def->source) < 0) - goto cleanup; + return -1; if (virISCSIConnectionLogin(portal, def->source.initiator.iqn, def->source.devices[0].path) < 0) - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FREE(portal); - VIR_FREE(session); - return ret; + return 0; } static int virStorageBackendISCSIRefreshPool(virStoragePoolObjPtr pool) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - char *session = NULL; + VIR_AUTOFREE(char *) session = NULL; def->allocation = def->capacity = def->available = 0; if ((session = virStorageBackendISCSISession(pool, false)) == NULL) - goto cleanup; + return -1; if (virISCSIRescanLUNs(session) < 0) - goto cleanup; + return -1; if (virStorageBackendISCSIFindLUs(pool, session) < 0) - goto cleanup; - VIR_FREE(session); + return -1; return 0; - - cleanup: - VIR_FREE(session); - return -1; } @@ -406,13 +386,11 @@ static int virStorageBackendISCSIStopPool(virStoragePoolObjPtr pool) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - char *portal; - char *session; - int ret = -1; + VIR_AUTOFREE(char *) portal = NULL; + VIR_AUTOFREE(char *) session = NULL; if ((session = virStorageBackendISCSISession(pool, true)) == NULL) return 0; - VIR_FREE(session); if ((portal = virStorageBackendISCSIPortal(&def->source)) == NULL) return -1; @@ -420,12 +398,9 @@ virStorageBackendISCSIStopPool(virStoragePoolObjPtr pool) if (virISCSIConnectionLogout(portal, def->source.initiator.iqn, def->source.devices[0].path) < 0) - goto cleanup; - ret = 0; + return -1; - cleanup: - VIR_FREE(portal); - return ret; + return 0; } virStorageBackend virStorageBackendISCSI = { diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c index 82fa4d7a25..6458b0f835 100644 --- a/src/storage/storage_backend_iscsi_direct.c +++ b/src/storage/storage_backend_iscsi_direct.c @@ -421,15 +421,14 @@ virISCSIDirectUpdateTargets(struct iscsi_context *iscsi, } for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next) { - char *target = NULL; + VIR_AUTOFREE(char *) target = NULL; if (VIR_STRDUP(target, tmp_addr->target_name) < 0) goto cleanup; - if (VIR_APPEND_ELEMENT(tmp_targets, tmp_ntargets, target) < 0) { - VIR_FREE(target); + if (VIR_APPEND_ELEMENT(tmp_targets, tmp_ntargets, target) < 0) goto cleanup; - } + target = NULL; } VIR_STEAL_PTR(*targets, tmp_targets); @@ -481,6 +480,7 @@ virStorageBackendISCSIDirectFindPoolSources(const char *srcSpec, unsigned int flags) { VIR_AUTOPTR(virStoragePoolSource) source = NULL; + VIR_AUTOFREE(char *) portal = NULL; size_t ntargets = 0; char **targets = NULL; char *ret = NULL; @@ -490,7 +490,6 @@ virStorageBackendISCSIDirectFindPoolSources(const char *srcSpec, .nsources = 0, .sources = NULL }; - char *portal = NULL; virCheckFlags(0, NULL); @@ -550,7 +549,6 @@ virStorageBackendISCSIDirectFindPoolSources(const char *srcSpec, for (i = 0; i < ntargets; i++) VIR_FREE(targets[i]); VIR_FREE(targets); - VIR_FREE(portal); return ret; } @@ -560,7 +558,7 @@ virStorageBackendISCSIDirectSetConnection(virStoragePoolObjPtr pool, { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); struct iscsi_context *iscsi = NULL; - char *portal = NULL; + VIR_AUTOFREE(char *) portal = NULL; if (!(iscsi = virISCSIDirectCreateContext(def->source.initiator.iqn))) goto error; @@ -577,7 +575,6 @@ virStorageBackendISCSIDirectSetConnection(virStoragePoolObjPtr pool, VIR_STEAL_PTR(*portalRet, portal); cleanup: - VIR_FREE(portal); return iscsi; error: @@ -590,19 +587,14 @@ static int virStorageBackendISCSIDirectRefreshPool(virStoragePoolObjPtr pool) { struct iscsi_context *iscsi = NULL; - char *portal = NULL; + VIR_AUTOFREE(char *) portal = NULL; int ret = -1; - if (!(iscsi = virStorageBackendISCSIDirectSetConnection(pool, &portal))) - goto cleanup; - if (virISCSIDirectReportLuns(pool, iscsi, portal) < 0) - goto disconect; - ret = 0; - disconect: + if (!(iscsi = virStorageBackendISCSIDirectSetConnection(pool, &portal))) + return -1; + ret = virISCSIDirectReportLuns(pool, iscsi, portal); virISCSIDirectDisconnect(iscsi); iscsi_destroy_context(iscsi); - cleanup: - VIR_FREE(portal); return ret; } @@ -638,7 +630,7 @@ virStorageBackendISCSIDirectVolWipeZero(virStorageVolDefPtr vol, struct scsi_task *task = NULL; int lun = 0; int ret = -1; - unsigned char *data; + VIR_AUTOFREE(unsigned char *) data = NULL; if (virStorageBackendISCSIDirectGetLun(vol, &lun) < 0) return ret; @@ -655,22 +647,19 @@ virStorageBackendISCSIDirectVolWipeZero(virStorageVolDefPtr vol, if (!(task = iscsi_write10_sync(iscsi, lun, lba, data, block_size * BLOCK_PER_PACKET, block_size, 0, 0, 0, 0, 0))) - goto cleanup; + return -1; scsi_free_scsi_task(task); lba += BLOCK_PER_PACKET; } else { if (!(task = iscsi_write10_sync(iscsi, lun, lba, data, block_size, block_size, 0, 0, 0, 0, 0))) - goto cleanup; + return -1; scsi_free_scsi_task(task); lba++; } } - ret = 0; - cleanup: - VIR_FREE(data); - return ret; + return 0; } static int diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index e0ddc962b1..2a205a4e95 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -117,9 +117,9 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, { int nextents, ret = -1; const char *regex_unit = "(\\S+)\\((\\S+)\\)"; - char *regex = NULL; - regex_t *reg = NULL; - regmatch_t *vars = NULL; + VIR_AUTOFREE(char *) regex = NULL; + VIR_AUTOFREE(regex_t *) reg = NULL; + VIR_AUTOFREE(regmatch_t *) vars = NULL; char *p = NULL; size_t i; int err, nvars; @@ -202,7 +202,7 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, for (i = 0; i < nextents; i++) { size_t j; int len; - char *offset_str = NULL; + VIR_AUTOFREE(char *) offset_str = NULL; j = (i * 2) + 1; len = vars[j].rm_eo - vars[j].rm_so; @@ -219,10 +219,8 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, if (virStrToLong_ull(offset_str, NULL, 10, &offset) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume extent offset value")); - VIR_FREE(offset_str); goto cleanup; } - VIR_FREE(offset_str); extent.start = offset * size; extent.end = (offset * size) + length; @@ -234,9 +232,6 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, ret = 0; cleanup: - VIR_FREE(regex); - VIR_FREE(reg); - VIR_FREE(vars); VIR_FREE(extent.path); return ret; } @@ -459,15 +454,15 @@ virStorageBackendLogicalFindPoolSourcesFunc(char **const groups, void *data) { virStoragePoolSourceListPtr sourceList = data; - char *pvname = NULL; - char *vgname = NULL; + VIR_AUTOFREE(char *) pvname = NULL; + VIR_AUTOFREE(char *) vgname = NULL; size_t i; virStoragePoolSourceDevicePtr dev; virStoragePoolSource *thisSource; if (VIR_STRDUP(pvname, groups[0]) < 0 || VIR_STRDUP(vgname, groups[1]) < 0) - goto error; + return -1; thisSource = NULL; for (i = 0; i < sourceList->nsources; i++) { @@ -479,30 +474,22 @@ virStorageBackendLogicalFindPoolSourcesFunc(char **const groups, if (thisSource == NULL) { if (!(thisSource = virStoragePoolSourceListNewSource(sourceList))) - goto error; + return -1; - thisSource->name = vgname; + VIR_STEAL_PTR(thisSource->name, vgname); } - else - VIR_FREE(vgname); if (VIR_REALLOC_N(thisSource->devices, thisSource->ndevice + 1) != 0) - goto error; + return -1; dev = &thisSource->devices[thisSource->ndevice]; thisSource->ndevice++; thisSource->format = VIR_STORAGE_POOL_LOGICAL_LVM2; memset(dev, 0, sizeof(*dev)); - dev->path = pvname; + VIR_STEAL_PTR(dev->path, pvname); return 0; - - error: - VIR_FREE(pvname); - VIR_FREE(vgname); - - return -1; } /* diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 423f945fbc..07b670f2f1 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -153,8 +153,8 @@ static int virStorageBackendCreateVols(virStoragePoolObjPtr pool, struct dm_names *names) { - int retval = -1, is_mpath = 0; - char *map_device = NULL; + int is_mpath = 0; + VIR_AUTOFREE(char *) map_device = NULL; uint32_t minor = -1; uint32_t next; @@ -162,24 +162,23 @@ virStorageBackendCreateVols(virStoragePoolObjPtr pool, is_mpath = virStorageBackendIsMultipath(names->name); if (is_mpath < 0) - goto out; + return -1; if (is_mpath == 1) { if (virAsprintf(&map_device, "mapper/%s", names->name) < 0) - goto out; + return -1; if (virStorageBackendGetMinorNumber(names->name, &minor) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to get %s minor number"), names->name); - goto out; + return -1; } if (virStorageBackendMpathNewVol(pool, minor, map_device) < 0) - goto out; + return -1; - VIR_FREE(map_device); } /* Given the way libdevmapper returns its data, I don't see @@ -191,10 +190,7 @@ virStorageBackendCreateVols(virStoragePoolObjPtr pool, } while (next); - retval = 0; - out: - VIR_FREE(map_device); - return retval; + return 0; } diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index ece04f0f2d..99ddb2c922 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -86,7 +86,7 @@ virStoragePoolDefRBDNamespaceParse(xmlXPathContextPtr ctxt, void **data) { virStoragePoolRBDConfigOptionsDefPtr cmdopts = NULL; - xmlNodePtr *nodes = NULL; + VIR_AUTOFREE(xmlNodePtr *)nodes = NULL; int nnodes; size_t i; int ret = -1; @@ -145,7 +145,6 @@ virStoragePoolDefRBDNamespaceParse(xmlXPathContextPtr ctxt, ret = 0; cleanup: - VIR_FREE(nodes); virStoragePoolDefRBDNamespaceFree(cmdopts); return ret; } @@ -213,7 +212,7 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, char *rados_key = NULL; virBuffer mon_host = VIR_BUFFER_INITIALIZER; size_t i; - char *mon_buff = NULL; + VIR_AUTOFREE(char *) mon_buff = NULL; const char *client_mount_timeout = "30"; const char *mon_op_timeout = "30"; const char *osd_op_timeout = "30"; @@ -348,7 +347,6 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, virObjectUnref(conn); virBufferFreeAndReset(&mon_host); - VIR_FREE(mon_buff); return ret; } @@ -574,7 +572,8 @@ virStorageBackendRBDRefreshPool(virStoragePoolObjPtr pool) int ret = -1; int len = -1; int r = 0; - char *name, *names = NULL; + char *name; + VIR_AUTOFREE(char *) names = NULL; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); virStorageBackendRBDStatePtr ptr = NULL; struct rados_cluster_stat_t clusterstat; @@ -662,7 +661,6 @@ virStorageBackendRBDRefreshPool(virStoragePoolObjPtr pool) ret = 0; cleanup: - VIR_FREE(names); virStorageBackendRBDFreeState(&ptr); return ret; } @@ -677,7 +675,7 @@ virStorageBackendRBDCleanupSnapshots(rados_ioctx_t ioctx, int max_snaps = 128; int snap_count, protected; size_t i; - rbd_snap_info_t *snaps = NULL; + VIR_AUTOFREE(rbd_snap_info_t *) snaps = NULL; rbd_image_t image = NULL; if ((r = rbd_open(ioctx, vol->name, &image, NULL)) < 0) { @@ -737,8 +735,6 @@ virStorageBackendRBDCleanupSnapshots(rados_ioctx_t ioctx, if (snaps) rbd_snap_list_end(snaps); - VIR_FREE(snaps); - if (image) rbd_close(image); @@ -949,7 +945,7 @@ virStorageBackendRBDSnapshotFindNoDiff(rbd_image_t image, int max_snaps = 128; size_t i; int diff; - rbd_snap_info_t *snaps = NULL; + VIR_AUTOFREE(rbd_snap_info_t *) snaps = NULL; rbd_image_info_t info; if ((r = rbd_stat(image, &info, sizeof(info))) < 0) { @@ -1023,8 +1019,6 @@ virStorageBackendRBDSnapshotFindNoDiff(rbd_image_t image, if (snaps) rbd_snap_list_end(snaps); - VIR_FREE(snaps); - return ret; } @@ -1098,7 +1092,7 @@ virStorageBackendRBDCloneImage(rados_ioctx_t io, uint64_t stripe_count; uint64_t stripe_unit; virBuffer snapname = VIR_BUFFER_INITIALIZER; - char *snapname_buff = NULL; + VIR_AUTOFREE(char *) snapname_buff = NULL; rbd_image_t image = NULL; if ((r = rbd_open(io, origvol, &image, NULL)) < 0) { @@ -1170,7 +1164,6 @@ virStorageBackendRBDCloneImage(rados_ioctx_t io, cleanup: virBufferFreeAndReset(&snapname); - VIR_FREE(snapname_buff); if (image) rbd_close(image); @@ -1271,13 +1264,12 @@ virStorageBackendRBDVolWipeZero(rbd_image_t image, uint64_t stripe_count) { int r = -1; - int ret = -1; unsigned long long offset = 0; unsigned long long length; - char *writebuf; + VIR_AUTOFREE(char *) writebuf = NULL; if (VIR_ALLOC_N(writebuf, info->obj_size * stripe_count) < 0) - goto cleanup; + return -1; while (offset < info->size) { length = MIN((info->size - offset), (info->obj_size * stripe_count)); @@ -1286,7 +1278,7 @@ virStorageBackendRBDVolWipeZero(rbd_image_t image, virReportSystemError(-r, _("writing %llu bytes failed on " "RBD image %s at offset %llu"), length, imgname, offset); - goto cleanup; + return -1; } VIR_DEBUG("Wrote %llu bytes to RBD image %s at offset %llu", @@ -1295,12 +1287,7 @@ virStorageBackendRBDVolWipeZero(rbd_image_t image, offset += length; } - ret = 0; - - cleanup: - VIR_FREE(writebuf); - - return ret; + return 0; } static int diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 14f01f9ec0..1a10e43647 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -56,16 +56,14 @@ static int virStorageBackendSCSITriggerRescan(uint32_t host) { int fd = -1; - int retval = 0; - char *path; + int retval = -1; + VIR_AUTOFREE(char *) path = NULL; VIR_DEBUG("Triggering rescan of host %d", host); if (virAsprintf(&path, "%s/host%u/scan", - LINUX_SYSFS_SCSI_HOST_PREFIX, host) < 0) { - retval = -1; - goto out; - } + LINUX_SYSFS_SCSI_HOST_PREFIX, host) < 0) + return -1; VIR_DEBUG("Scan trigger path is '%s'", path); @@ -75,8 +73,7 @@ virStorageBackendSCSITriggerRescan(uint32_t host) virReportSystemError(errno, _("Could not open '%s' to trigger host scan"), path); - retval = -1; - goto free_path; + goto cleanup; } if (safewrite(fd, @@ -86,13 +83,12 @@ virStorageBackendSCSITriggerRescan(uint32_t host) virReportSystemError(errno, _("Write to '%s' to trigger host scan failed"), path); - retval = -1; } + retval = 0; + + cleanup: VIR_FORCE_CLOSE(fd); - free_path: - VIR_FREE(path); - out: VIR_DEBUG("Rescan of host %d complete", host); return retval; } @@ -178,7 +174,6 @@ static char * getAdapterName(virStorageAdapterPtr adapter) { char *name = NULL; - char *parentaddr = NULL; if (adapter->type == VIR_STORAGE_ADAPTER_TYPE_SCSI_HOST) { virStorageAdapterSCSIHostPtr scsi_host = &adapter->data.scsi_host; @@ -192,7 +187,7 @@ getAdapterName(virStorageAdapterPtr adapter) addr->slot, addr->function, unique_id))) - goto cleanup; + return NULL; } else { ignore_value(VIR_STRDUP(name, scsi_host->name)); } @@ -206,8 +201,6 @@ getAdapterName(virStorageAdapterPtr adapter) } } - cleanup: - VIR_FREE(parentaddr); return name; } @@ -248,8 +241,8 @@ checkParent(const char *name, const char *parent_name) { unsigned int host_num; - char *scsi_host_name = NULL; - char *vhba_parent = NULL; + VIR_AUTOFREE(char *) scsi_host_name = NULL; + VIR_AUTOFREE(char *) vhba_parent = NULL; bool retval = false; virConnectPtr conn = NULL; @@ -291,8 +284,6 @@ checkParent(const char *name, cleanup: virObjectUnref(conn); - VIR_FREE(vhba_parent); - VIR_FREE(scsi_host_name); return retval; } @@ -302,7 +293,7 @@ createVport(virStoragePoolDefPtr def, const char *configFile, virStorageAdapterFCHostPtr fchost) { - char *name = NULL; + VIR_AUTOFREE(char *) name = NULL; virStoragePoolFCRefreshInfoPtr cbdata = NULL; virThread thread; int ret = -1; @@ -363,7 +354,6 @@ createVport(virStoragePoolDefPtr def, ret = 0; cleanup: - VIR_FREE(name); return ret; } @@ -373,10 +363,9 @@ virStorageBackendSCSICheckPool(virStoragePoolObjPtr pool, bool *isActive) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - char *path = NULL; - char *name = NULL; + VIR_AUTOFREE(char *) path = NULL; + VIR_AUTOFREE(char *) name = NULL; unsigned int host; - int ret = -1; *isActive = false; @@ -394,28 +383,23 @@ virStorageBackendSCSICheckPool(virStoragePoolObjPtr pool, } if (virSCSIHostGetNumber(name, &host) < 0) - goto cleanup; + return -1; if (virAsprintf(&path, "%s/host%d", LINUX_SYSFS_SCSI_HOST_PREFIX, host) < 0) - goto cleanup; + return -1; *isActive = virFileExists(path); - ret = 0; - cleanup: - VIR_FREE(path); - VIR_FREE(name); - return ret; + return 0; } static int virStorageBackendSCSIRefreshPool(virStoragePoolObjPtr pool) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - char *name = NULL; + VIR_AUTOFREE(char *) name = NULL; unsigned int host; - int ret = -1; def->allocation = def->capacity = def->available = 0; @@ -423,20 +407,17 @@ virStorageBackendSCSIRefreshPool(virStoragePoolObjPtr pool) return -1; if (virSCSIHostGetNumber(name, &host) < 0) - goto out; + return -1; VIR_DEBUG("Scanning host%u", host); if (virStorageBackendSCSITriggerRescan(host) < 0) - goto out; + return -1; if (virStorageBackendSCSIFindLUs(pool, host) < 0) - goto out; + return -1; - ret = 0; - out: - VIR_FREE(name); - return ret; + return 0; } diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index d9de74f52e..1215b36b61 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -136,8 +136,7 @@ virStorageBackendSheepdogAddVolume(virStoragePoolObjPtr pool, const char *diskIn static int virStorageBackendSheepdogRefreshAllVol(virStoragePoolObjPtr pool) { - int ret = -1; - char *output = NULL; + VIR_AUTOFREE(char *) output = NULL; VIR_AUTOPTR(virString) lines = NULL; VIR_AUTOPTR(virString) cells = NULL; VIR_AUTOPTR(virCommand) cmd = NULL; @@ -147,11 +146,11 @@ virStorageBackendSheepdogRefreshAllVol(virStoragePoolObjPtr pool) virStorageBackendSheepdogAddHostArg(cmd, pool); virCommandSetOutputBuffer(cmd, &output); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return -1; lines = virStringSplit(output, "\n", 0); if (lines == NULL) - goto cleanup; + return -1; for (i = 0; lines[i]; i++) { const char *line = lines[i]; @@ -163,42 +162,34 @@ virStorageBackendSheepdogRefreshAllVol(virStoragePoolObjPtr pool) if (cells != NULL && virStringListLength((const char * const *)cells) > 2) { if (virStorageBackendSheepdogAddVolume(pool, cells[1]) < 0) - goto cleanup; + return -1; } virStringListFree(cells); cells = NULL; } - ret = 0; - - cleanup: - VIR_FREE(output); - return ret; + return 0; } static int virStorageBackendSheepdogRefreshPool(virStoragePoolObjPtr pool) { - int ret = -1; - char *output = NULL; + VIR_AUTOFREE(char *) output = NULL; VIR_AUTOPTR(virCommand) cmd = NULL; cmd = virCommandNewArgList(SHEEPDOGCLI, "node", "info", "-r", NULL); virStorageBackendSheepdogAddHostArg(cmd, pool); virCommandSetOutputBuffer(cmd, &output); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return -1; if (virStorageBackendSheepdogParseNodeInfo(virStoragePoolObjGetDef(pool), output) < 0) - goto cleanup; + return -1; - ret = virStorageBackendSheepdogRefreshAllVol(pool); - cleanup: - VIR_FREE(output); - return ret; + return virStorageBackendSheepdogRefreshAllVol(pool); } diff --git a/src/storage/storage_backend_vstorage.c b/src/storage/storage_backend_vstorage.c index e62234fd70..6979fac3c6 100644 --- a/src/storage/storage_backend_vstorage.c +++ b/src/storage/storage_backend_vstorage.c @@ -40,9 +40,9 @@ virStorageBackendVzPoolStart(virStoragePoolObjPtr pool) int ret = -1; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); VIR_AUTOPTR(virCommand) cmd = NULL; - char *grp_name = NULL; - char *usr_name = NULL; - char *mode = NULL; + VIR_AUTOFREE(char *) grp_name = NULL; + VIR_AUTOFREE(char *) usr_name = NULL; + VIR_AUTOFREE(char *) mode = NULL; /* Check the permissions */ if (def->target.perms.mode == (mode_t)-1) @@ -55,13 +55,13 @@ virStorageBackendVzPoolStart(virStoragePoolObjPtr pool) /* Convert ids to names because vstorage uses names */ if (!(grp_name = virGetGroupName(def->target.perms.gid))) - goto cleanup; + return -1; if (!(usr_name = virGetUserName(def->target.perms.uid))) - goto cleanup; + return -1; if (virAsprintf(&mode, "%o", def->target.perms.mode) < 0) - goto cleanup; + return -1; cmd = virCommandNewArgList(VSTORAGE_MOUNT, "-c", def->source.name, @@ -70,15 +70,7 @@ virStorageBackendVzPoolStart(virStoragePoolObjPtr pool) "-g", grp_name, "-u", usr_name, NULL); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - ret = 0; - - cleanup: - VIR_FREE(mode); - VIR_FREE(grp_name); - VIR_FREE(usr_name); - return ret; + return virCommandRun(cmd, NULL); } @@ -90,7 +82,7 @@ virStorageBackendVzIsMounted(virStoragePoolObjPtr pool) FILE *mtab; struct mntent ent; char buf[1024]; - char *cluster = NULL; + VIR_AUTOFREE(char *) cluster = NULL; if (virAsprintf(&cluster, "vstorage://%s", def->source.name) < 0) return -1; @@ -115,7 +107,6 @@ virStorageBackendVzIsMounted(virStoragePoolObjPtr pool) cleanup: VIR_FORCE_FCLOSE(mtab); - VIR_FREE(cluster); return ret; } diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c index 55f8a138cf..8bbc4e01ed 100644 --- a/src/storage/storage_backend_zfs.c +++ b/src/storage/storage_backend_zfs.c @@ -53,7 +53,7 @@ virStorageBackendZFSVolModeNeeded(void) { VIR_AUTOPTR(virCommand) cmd = NULL; int ret = -1, exit_code = -1; - char *error = NULL; + VIR_AUTOFREE(char *) error = NULL; /* 'zfs get' without arguments prints out * usage information to stderr, including @@ -77,7 +77,6 @@ virStorageBackendZFSVolModeNeeded(void) ret = 0; cleanup: - VIR_FREE(error); return ret; } @@ -86,13 +85,12 @@ virStorageBackendZFSCheckPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, bool *isActive) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - char *devpath; + VIR_AUTOFREE(char *) devpath = NULL; if (virAsprintf(&devpath, "/dev/zvol/%s", def->source.name) < 0) return -1; *isActive = virFileIsDir(devpath); - VIR_FREE(devpath); return 0; } @@ -179,7 +177,7 @@ virStorageBackendZFSFindVols(virStoragePoolObjPtr pool, { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); VIR_AUTOPTR(virCommand) cmd = NULL; - char *volumes_list = NULL; + VIR_AUTOFREE(char *) volumes_list = NULL; VIR_AUTOPTR(virString) lines = NULL; size_t i; @@ -203,10 +201,10 @@ virStorageBackendZFSFindVols(virStoragePoolObjPtr pool, NULL); virCommandSetOutputBuffer(cmd, &volumes_list); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return -1; if (!(lines = virStringSplit(volumes_list, "\n", 0))) - goto cleanup; + return -1; for (i = 0; lines[i]; i++) { if (STREQ(lines[i], "")) @@ -216,9 +214,6 @@ virStorageBackendZFSFindVols(virStoragePoolObjPtr pool, continue; } - cleanup: - VIR_FREE(volumes_list); - return 0; } diff --git a/src/storage/storage_file_gluster.c b/src/storage/storage_file_gluster.c index f8bbde8cfe..6f87c77f1f 100644 --- a/src/storage/storage_file_gluster.c +++ b/src/storage/storage_file_gluster.c @@ -258,7 +258,7 @@ virStorageFileBackendGlusterReadlinkCallback(const char *path, void *data) { virStorageFileBackendGlusterPrivPtr priv = data; - char *buf = NULL; + VIR_AUTOFREE(char *) buf = NULL; size_t bufsiz = 0; ssize_t ret; struct stat st; @@ -277,13 +277,13 @@ virStorageFileBackendGlusterReadlinkCallback(const char *path, realloc: if (VIR_EXPAND_N(buf, bufsiz, 256) < 0) - goto error; + return -1; if ((ret = glfs_readlink(priv->vol, path, buf, bufsiz)) < 0) { virReportSystemError(errno, _("failed to read link of gluster file '%s'"), path); - goto error; + return -1; } if (ret == bufsiz) @@ -291,13 +291,9 @@ virStorageFileBackendGlusterReadlinkCallback(const char *path, buf[ret] = '\0'; - *linkpath = buf; + VIR_STEAL_PTR(*linkpath, buf); return 0; - - error: - VIR_FREE(buf); - return -1; } @@ -305,7 +301,7 @@ static const char * virStorageFileBackendGlusterGetUniqueIdentifier(virStorageSourcePtr src) { virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; - char *filePath = NULL; + VIR_AUTOFREE(char *) filePath = NULL; if (priv->canonpath) return priv->canonpath; @@ -321,8 +317,6 @@ virStorageFileBackendGlusterGetUniqueIdentifier(virStorageSourcePtr src) src->volume, filePath)); - VIR_FREE(filePath); - return priv->canonpath; } -- 2.20.1

[...] Right - just rote habit I guess... For this one I disagree... similar to the @vgname, previously we "failed" through the error: label and returned -1, but with the AUTO stuff added, we now cannot leave @vgname and @pvname as NULL. This one needs to stay. John

Let's make use of the auto __cleanup capabilities cleaning up any now unnecessary goto paths. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_driver.c | 70 ++++++++++++------------------------ 1 file changed, 23 insertions(+), 47 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7c377439dc..61e0756e3e 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -122,7 +122,7 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj, virStoragePoolDefPtr def = virStoragePoolObjGetDef(obj); bool active = false; virStorageBackendPtr backend; - char *stateFile; + VIR_AUTOFREE(char *) stateFile = NULL; if ((backend = virStorageBackendForType(def->type)) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -164,8 +164,6 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj, if (!virStoragePoolObjIsActive(obj)) virStoragePoolUpdateInactive(&obj); - VIR_FREE(stateFile); - return; } @@ -203,7 +201,7 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj, } if (started) { - char *stateFile; + VIR_AUTOFREE(char *) stateFile = NULL; virStoragePoolObjClearVols(obj); stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); @@ -217,7 +215,6 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj, } else { virStoragePoolObjSetActive(obj, true); } - VIR_FREE(stateFile); } } @@ -240,16 +237,15 @@ storageStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - int ret = -1; - char *configdir = NULL; - char *rundir = NULL; + VIR_AUTOFREE(char *) configdir = NULL; + VIR_AUTOFREE(char *) rundir = NULL; if (VIR_ALLOC(driver) < 0) - return ret; + return -1; if (virMutexInit(&driver->lock) < 0) { VIR_FREE(driver); - return ret; + return -1; } storageDriverLock(); @@ -302,16 +298,12 @@ storageStateInitialize(bool privileged, storageDriverUnlock(); - ret = 0; - cleanup: - VIR_FREE(configdir); - VIR_FREE(rundir); - return ret; + return 0; error: storageDriverUnlock(); storageStateCleanup(); - goto cleanup; + return -1; } /** @@ -691,12 +683,12 @@ storagePoolCreateXML(virConnectPtr conn, unsigned int flags) { VIR_AUTOPTR(virStoragePoolDef) newDef = NULL; + VIR_AUTOFREE(char *) stateFile = NULL; virStoragePoolObjPtr obj = NULL; virStoragePoolDefPtr def; virStoragePoolPtr pool = NULL; virStorageBackendPtr backend; virObjectEventPtr event = NULL; - char *stateFile = NULL; unsigned int build_flags = 0; virCheckFlags(VIR_STORAGE_POOL_CREATE_WITH_BUILD | @@ -761,7 +753,6 @@ storagePoolCreateXML(virConnectPtr conn, pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); cleanup: - VIR_FREE(stateFile); virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolObjEndAPI(&obj); return pool; @@ -892,7 +883,7 @@ storagePoolCreate(virStoragePoolPtr pool, virStorageBackendPtr backend; virObjectEventPtr event = NULL; int ret = -1; - char *stateFile = NULL; + VIR_AUTOFREE(char *) stateFile = NULL; unsigned int build_flags = 0; virCheckFlags(VIR_STORAGE_POOL_CREATE_WITH_BUILD | @@ -955,7 +946,6 @@ storagePoolCreate(virStoragePoolPtr pool, ret = 0; cleanup: - VIR_FREE(stateFile); virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolObjEndAPI(&obj); return ret; @@ -1013,7 +1003,7 @@ storagePoolDestroy(virStoragePoolPtr pool) virStoragePoolDefPtr def; virStorageBackendPtr backend; virObjectEventPtr event = NULL; - char *stateFile = NULL; + VIR_AUTOFREE(char *) stateFile = NULL; int ret = -1; if (!(obj = storagePoolObjFindByUUID(pool->uuid, pool->name))) @@ -1045,7 +1035,6 @@ storagePoolDestroy(virStoragePoolPtr pool) goto cleanup; unlink(stateFile); - VIR_FREE(stateFile); if (backend->stopPool && backend->stopPool(obj) < 0) @@ -1078,7 +1067,7 @@ storagePoolDelete(virStoragePoolPtr pool, virStoragePoolDefPtr def; virStorageBackendPtr backend; virObjectEventPtr event = NULL; - char *stateFile = NULL; + VIR_AUTOFREE(char *) stateFile = NULL; int ret = -1; if (!(obj = virStoragePoolObjFromStoragePool(pool))) @@ -1111,7 +1100,6 @@ storagePoolDelete(virStoragePoolPtr pool, goto cleanup; unlink(stateFile); - VIR_FREE(stateFile); if (!backend->deletePool) { virReportError(VIR_ERR_NO_SUPPORT, @@ -1172,10 +1160,10 @@ storagePoolRefresh(virStoragePoolPtr pool, virStoragePoolObjClearVols(obj); if (backend->refreshPool(obj) < 0) { - char *stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); + VIR_AUTOFREE(char *) stateFile = NULL; + stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); storagePoolRefreshFailCleanup(backend, obj, stateFile); - VIR_FREE(stateFile); event = virStoragePoolEventLifecycleNew(def->name, def->uuid, @@ -1540,7 +1528,7 @@ storageVolLookupByPathCallback(virStoragePoolObjPtr obj, { struct storageVolLookupData *data = (struct storageVolLookupData *)opaque; virStoragePoolDefPtr def; - char *stable_path = NULL; + VIR_AUTOFREE(char *) stable_path = NULL; if (!virStoragePoolObjIsActive(obj)) return false; @@ -1579,7 +1567,6 @@ storageVolLookupByPathCallback(virStoragePoolObjPtr obj, } data->voldef = virStorageVolDefFindByPath(obj, stable_path); - VIR_FREE(stable_path); return !!data->voldef; } @@ -1649,7 +1636,7 @@ storagePoolLookupByTargetPath(virConnectPtr conn, virStoragePoolObjPtr obj; virStoragePoolDefPtr def; virStoragePoolPtr pool = NULL; - char *cleanpath; + VIR_AUTOFREE(char *) cleanpath = NULL; cleanpath = virFileSanitizePath(path); if (!cleanpath) @@ -1660,7 +1647,7 @@ storagePoolLookupByTargetPath(virConnectPtr conn, cleanpath))) { def = virStoragePoolObjGetDef(obj); if (virStoragePoolLookupByTargetPathEnsureACL(conn, def) < 0) - goto cleanup; + return NULL; pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); virStoragePoolObjEndAPI(&obj); @@ -1678,8 +1665,6 @@ storagePoolLookupByTargetPath(virConnectPtr conn, } } - cleanup: - VIR_FREE(cleanpath); return pool; } @@ -2206,40 +2191,31 @@ virStorageVolPoolRefreshDataFree(void *opaque) static int virStorageBackendPloopRestoreDesc(char *path) { - int ret = -1; VIR_AUTOPTR(virCommand) cmd = NULL; - char *refresh_tool = NULL; - char *desc = NULL; + VIR_AUTOFREE(char *) refresh_tool = NULL; + VIR_AUTOFREE(char *) desc = NULL; if (virAsprintf(&desc, "%s/DiskDescriptor.xml", path) < 0) - return ret; + return -1; if (virFileRemove(desc, 0, 0) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("refresh ploop failed:" " unable to delete DiskDescriptor.xml")); - goto cleanup; + return -1; } refresh_tool = virFindFileInPath("ploop"); if (!refresh_tool) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unable to find ploop, please install ploop tools")); - goto cleanup; + return -1; } cmd = virCommandNewArgList(refresh_tool, "restore-descriptor", path, NULL); virCommandAddArgFormat(cmd, "%s/root.hds", path); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - - ret = 0; - - cleanup: - VIR_FREE(refresh_tool); - VIR_FREE(desc); - return ret; + return virCommandRun(cmd, NULL); } -- 2.20.1

On Wed, Feb 06, 2019 at 08:41:41AM -0500, John Ferlan wrote: [snip] We're changing the logic ^here - previously if virStorageBackendUpdateVolInfo succeeded but virStorageBackendSCSISerial returned NULL, we'd still return retval which would have been equal to 0. To me, your change feels right, but I want to make sure no caller was relying on 0 even though virStorageBackendSCSISerial might have failed. Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt;

Let's make use of the auto __cleanup capabilities cleaning up any now unnecessary goto paths. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/storage_conf.c | 179 +++++++++++++++++----------------------- 1 file changed, 74 insertions(+), 105 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 4fb5fb9f57..5f1122422a 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -453,15 +453,15 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, xmlNodePtr node) { int ret = -1; - xmlNodePtr relnode, authnode, *nodeset = NULL; + xmlNodePtr relnode, authnode; xmlNodePtr adapternode; int nsource; size_t i; virStoragePoolOptionsPtr options; VIR_AUTOPTR(virStorageAuthDef) authdef = NULL; - char *name = NULL; - char *port = NULL; - char *ver = NULL; + VIR_AUTOFREE(char *) port = NULL; + VIR_AUTOFREE(char *) ver = NULL; + VIR_AUTOFREE(xmlNodePtr *) nodeset = NULL; int n; relnode = ctxt->node; @@ -478,7 +478,9 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, } if (options->formatFromString) { - char *format = virXPathString("string(./format/@type)", ctxt); + VIR_AUTOFREE(char *) format = NULL; + + format = virXPathString("string(./format/@type)", ctxt); if (format == NULL) source->format = options->defaultFormat; else @@ -487,10 +489,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, if (source->format < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown pool format type %s"), format); - VIR_FREE(format); goto cleanup; } - VIR_FREE(format); } if ((n = virXPathNodeSet("./host", ctxt, &nodeset)) < 0) @@ -502,13 +502,12 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, source->nhost = n; for (i = 0; i < source->nhost; i++) { - name = virXMLPropString(nodeset[i], "name"); - if (name == NULL) { + source->hosts[i].name = virXMLPropString(nodeset[i], "name"); + if (!source->hosts[i].name) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool host name")); goto cleanup; } - source->hosts[i].name = name; port = virXMLPropString(nodeset[i], "port"); if (port) { @@ -518,7 +517,6 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, port); goto cleanup; } - VIR_FREE(port); } } } @@ -532,7 +530,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup; for (i = 0; i < nsource; i++) { - char *partsep; + VIR_AUTOFREE(char *) partsep = NULL; virStoragePoolSourceDevice dev = { .path = NULL }; dev.path = virXMLPropString(nodeset[i], "path"); @@ -550,10 +548,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, _("invalid part_separator setting '%s'"), partsep); virStoragePoolSourceDeviceClear(&dev); - VIR_FREE(partsep); goto cleanup; } - VIR_FREE(partsep); } if (VIR_APPEND_ELEMENT(source->devices, source->ndevice, dev) < 0) { @@ -612,8 +608,6 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, cleanup: ctxt->node = relnode; - VIR_FREE(port); - VIR_FREE(nodeset); return ret; } @@ -660,7 +654,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, virStoragePermsPtr perms, const char *permxpath) { - char *mode; + VIR_AUTOFREE(char *) mode = NULL; long long val; int ret = -1; xmlNodePtr relnode; @@ -683,13 +677,11 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, int tmp; if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) { - VIR_FREE(mode); virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed octal mode")); goto error; } perms->mode = tmp; - VIR_FREE(mode); } else { perms->mode = (mode_t) -1; } @@ -740,9 +732,9 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) VIR_AUTOPTR(virStoragePoolDef) def = NULL; virStoragePoolDefPtr ret = NULL; xmlNodePtr source_node; - char *type = NULL; - char *uuid = NULL; - char *target_path = NULL; + VIR_AUTOFREE(char *) type = NULL; + VIR_AUTOFREE(char *) uuid = NULL; + VIR_AUTOFREE(char *) target_path = NULL; if (VIR_ALLOC(def) < 0) return NULL; @@ -751,23 +743,23 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if (type == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("storage pool missing type attribute")); - goto cleanup; + return NULL; } if ((def->type = virStoragePoolTypeFromString(type)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown storage pool type %s"), type); - goto cleanup; + return NULL; } if ((options = virStoragePoolOptionsForPoolType(def->type)) == NULL) - goto cleanup; + return NULL; source_node = virXPathNode("./source", ctxt); if (source_node) { if (virStoragePoolDefParseSource(ctxt, &def->source, def->type, source_node) < 0) - goto cleanup; + return NULL; } else { if (options->formatFromString) def->source.format = options->defaultFormat; @@ -777,17 +769,18 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if (def->name == NULL && options->flags & VIR_STORAGE_POOL_SOURCE_NAME && VIR_STRDUP(def->name, def->source.name) < 0) - goto cleanup; + return NULL; + if (def->name == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing pool source name element")); - goto cleanup; + return NULL; } if (strchr(def->name, '/')) { virReportError(VIR_ERR_XML_ERROR, _("name %s cannot contain '/'"), def->name); - goto cleanup; + return NULL; } uuid = virXPathString("string(./uuid)", ctxt); @@ -795,13 +788,13 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if (virUUIDGenerate(def->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unable to generate uuid")); - goto cleanup; + return NULL; } } else { if (virUUIDParse(uuid, def->uuid) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed uuid element")); - goto cleanup; + return NULL; } } @@ -809,7 +802,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if (!def->source.nhost) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source host name")); - goto cleanup; + return NULL; } } @@ -817,27 +810,27 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if (!def->source.dir) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source path")); - goto cleanup; + return NULL; } } if (options->flags & VIR_STORAGE_POOL_SOURCE_NAME) { if (def->source.name == NULL) { /* source name defaults to pool name */ if (VIR_STRDUP(def->source.name, def->name) < 0) - goto cleanup; + return NULL; } } if ((options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) && (virStorageAdapterValidate(&def->source.adapter)) < 0) - goto cleanup; + return NULL; /* If DEVICE is the only source type, then its required */ if (options->flags == VIR_STORAGE_POOL_SOURCE_DEVICE) { if (!def->source.ndevice) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source device name")); - goto cleanup; + return NULL; } } @@ -846,32 +839,32 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) if (!(options->flags & VIR_STORAGE_POOL_SOURCE_NETWORK)) { if (def->type == VIR_STORAGE_POOL_LOGICAL) { if (virAsprintf(&target_path, "/dev/%s", def->source.name) < 0) - goto cleanup; + return NULL; } else if (def->type == VIR_STORAGE_POOL_ZFS) { if (virAsprintf(&target_path, "/dev/zvol/%s", def->source.name) < 0) - goto cleanup; + return NULL; } else { target_path = virXPathString("string(./target/path)", ctxt); if (!target_path) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool target path")); - goto cleanup; + return NULL; } } def->target.path = virFileSanitizePath(target_path); if (!def->target.path) - goto cleanup; + return NULL; if (virStorageDefParsePerms(ctxt, &def->target.perms, "./target/permissions") < 0) - goto cleanup; + return NULL; } if (def->type == VIR_STORAGE_POOL_ISCSI_DIRECT && !def->source.initiator.iqn) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("missing initiator IQN")); - goto cleanup; + return NULL; } /* Make a copy of all the callback pointers here for easier use, @@ -879,14 +872,10 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) def->ns = options->ns; if (def->ns.parse && (def->ns.parse)(ctxt, &def->namespaceData) < 0) - goto cleanup; + return NULL; VIR_STEAL_PTR(ret, def); - cleanup: - VIR_FREE(uuid); - VIR_FREE(type); - VIR_FREE(target_path); return ret; } @@ -1169,13 +1158,13 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, VIR_AUTOPTR(virStorageVolDef) def = NULL; virStorageVolDefPtr ret = NULL; virStorageVolOptionsPtr options; - char *type = NULL; - char *allocation = NULL; - char *capacity = NULL; - char *unit = NULL; - char *backingStore = NULL; + VIR_AUTOFREE(char *) type = NULL; + VIR_AUTOFREE(char *) allocation = NULL; + VIR_AUTOFREE(char *) capacity = NULL; + VIR_AUTOFREE(char *) unit = NULL; + VIR_AUTOFREE(char *) backingStore = NULL; + VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; xmlNodePtr node; - xmlNodePtr *nodes = NULL; size_t i; int n; @@ -1195,7 +1184,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (def->name == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing volume name element")); - goto cleanup; + return NULL; } /* Normally generated by pool refresh, but useful for unit tests */ @@ -1207,13 +1196,13 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if ((def->type = virStorageVolTypeFromString(type)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown volume type '%s'"), type); - goto cleanup; + return NULL; } } if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) { if (VIR_ALLOC(def->target.backingStore) < 0) - goto cleanup; + return NULL; def->target.backingStore->type = VIR_STORAGE_TYPE_FILE; @@ -1221,7 +1210,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, backingStore = NULL; if (options->formatFromString) { - char *format = virXPathString("string(./backingStore/format/@type)", ctxt); + VIR_AUTOFREE(char *) format = NULL; + + format = virXPathString("string(./backingStore/format/@type)", ctxt); if (format == NULL) def->target.backingStore->format = options->defaultFormat; else @@ -1230,29 +1221,27 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (def->target.backingStore->format < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown volume format type %s"), format); - VIR_FREE(format); - goto cleanup; + return NULL; } - VIR_FREE(format); } if (VIR_ALLOC(def->target.backingStore->perms) < 0) - goto cleanup; + return NULL; if (virStorageDefParsePerms(ctxt, def->target.backingStore->perms, "./backingStore/permissions") < 0) - goto cleanup; + return NULL; } capacity = virXPathString("string(./capacity)", ctxt); unit = virXPathString("string(./capacity/@unit)", ctxt); if (capacity) { if (virStorageSize(unit, capacity, &def->target.capacity) < 0) - goto cleanup; + return NULL; } else if (!(flags & VIR_VOL_XML_PARSE_NO_CAPACITY) && !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) && virStorageSourceHasBacking(&def->target))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing capacity element")); - goto cleanup; + return NULL; } VIR_FREE(unit); @@ -1260,7 +1249,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (allocation) { unit = virXPathString("string(./allocation/@unit)", ctxt); if (virStorageSize(unit, allocation, &def->target.allocation) < 0) - goto cleanup; + return NULL; def->target.has_allocation = true; } else { def->target.allocation = def->target.capacity; @@ -1268,7 +1257,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, def->target.path = virXPathString("string(./target/path)", ctxt); if (options->formatFromString) { - char *format = virXPathString("string(./target/format/@type)", ctxt); + VIR_AUTOFREE(char *) format = NULL; + + format = virXPathString("string(./target/format/@type)", ctxt); if (format == NULL) def->target.format = options->defaultFormat; else @@ -1277,41 +1268,39 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (def->target.format < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown volume format type %s"), format); - VIR_FREE(format); - goto cleanup; + return NULL; } - VIR_FREE(format); } if (VIR_ALLOC(def->target.perms) < 0) - goto cleanup; + return NULL; if (virStorageDefParsePerms(ctxt, def->target.perms, "./target/permissions") < 0) - goto cleanup; + return NULL; node = virXPathNode("./target/encryption", ctxt); if (node != NULL) { def->target.encryption = virStorageEncryptionParseNode(node, ctxt); if (def->target.encryption == NULL) - goto cleanup; + return NULL; } def->target.compat = virXPathString("string(./target/compat)", ctxt); if (virStorageFileCheckCompat(def->target.compat) < 0) - goto cleanup; + return NULL; if (virXPathNode("./target/nocow", ctxt)) def->target.nocow = true; if (virXPathNode("./target/features", ctxt)) { if ((n = virXPathNodeSet("./target/features/*", ctxt, &nodes)) < 0) - goto cleanup; + return NULL; if (!def->target.compat && VIR_STRDUP(def->target.compat, "1.1") < 0) - goto cleanup; + return NULL; if (!(def->target.features = virBitmapNew(VIR_STORAGE_FILE_FEATURE_LAST))) - goto cleanup; + return NULL; for (i = 0; i < n; i++) { int f = virStorageFileFeatureTypeFromString((const char*)nodes[i]->name); @@ -1319,7 +1308,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (f < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported feature %s"), (const char*)nodes[i]->name); - goto cleanup; + return NULL; } ignore_value(virBitmapSetBit(def->target.features, f)); } @@ -1327,14 +1316,6 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, } VIR_STEAL_PTR(ret, def); - - cleanup: - VIR_FREE(nodes); - VIR_FREE(allocation); - VIR_FREE(capacity); - VIR_FREE(unit); - VIR_FREE(type); - VIR_FREE(backingStore); return ret; } @@ -1616,32 +1597,27 @@ virStoragePoolSaveState(const char *stateFile, virStoragePoolDefPtr def) { virBuffer buf = VIR_BUFFER_INITIALIZER; - int ret = -1; - char *xml; + VIR_AUTOFREE(char *) xml = NULL; virBufferAddLit(&buf, "<poolstate>\n"); virBufferAdjustIndent(&buf, 2); if (virStoragePoolDefFormatBuf(&buf, def) < 0) - goto error; + return -1; virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</poolstate>\n"); if (virBufferCheckError(&buf) < 0) - goto error; + return -1; if (!(xml = virBufferContentAndReset(&buf))) - goto error; + return -1; if (virStoragePoolSaveXML(stateFile, def, xml)) - goto error; - - ret = 0; + return -1; - error: - VIR_FREE(xml); - return ret; + return 0; } @@ -1649,8 +1625,7 @@ int virStoragePoolSaveConfig(const char *configFile, virStoragePoolDefPtr def) { - char *xml; - int ret = -1; + VIR_AUTOFREE(char *) xml = NULL; if (!(xml = virStoragePoolDefFormat(def))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1658,13 +1633,7 @@ virStoragePoolSaveConfig(const char *configFile, return -1; } - if (virStoragePoolSaveXML(configFile, def, xml)) - goto cleanup; - - ret = 0; - cleanup: - VIR_FREE(xml); - return ret; + return virStoragePoolSaveXML(configFile, def, xml); } -- 2.20.1

Let's make use of the auto __cleanup capabilities cleaning up any now unnecessary goto paths. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/util/virstoragefile.c | 130 ++++++++++++++------------------------ 1 file changed, 47 insertions(+), 83 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2e5c64e034..c2f044d2db 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -595,7 +595,8 @@ vmdk4GetBackingStore(char **res, size_t buf_size) { static const char prefix[] = "parentFileNameHint=\""; - char *desc, *start, *end; + VIR_AUTOFREE(char *) desc = NULL; + char *start, *end; size_t len; int ret = BACKING_STORE_ERROR; @@ -645,7 +646,6 @@ vmdk4GetBackingStore(char **res, ret = BACKING_STORE_OK; cleanup: - VIR_FREE(desc); return ret; } @@ -1084,7 +1084,7 @@ virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid) int ret = -1; struct stat sb; ssize_t len = VIR_STORAGE_MAX_HEADER; - char *header = NULL; + VIR_AUTOFREE(char *) header = NULL; if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) { virReportSystemError(-fd, _("Failed to open file '%s'"), path); @@ -1115,7 +1115,6 @@ virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid) ret = virStorageFileProbeFormatFromBuf(path, header, len); cleanup: - VIR_FREE(header); VIR_FORCE_CLOSE(fd); return ret; @@ -1215,7 +1214,7 @@ virStorageFileGetMetadataFromFD(const char *path, { virStorageSourcePtr ret = NULL; virStorageSourcePtr meta = NULL; - char *buf = NULL; + VIR_AUTOFREE(char *) buf = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; struct stat sb; int dummy; @@ -1265,7 +1264,6 @@ virStorageFileGetMetadataFromFD(const char *path, cleanup: virStorageSourceFree(meta); - VIR_FREE(buf); return ret; } @@ -1614,8 +1612,7 @@ virStorageFileParseChainIndex(const char *diskTarget, unsigned int *chainIndex) { unsigned int idx = 0; - char *target = NULL; - int ret = 0; + VIR_AUTOFREE(char *) target = NULL; *chainIndex = 0; @@ -1626,21 +1623,18 @@ virStorageFileParseChainIndex(const char *diskTarget, return 0; if (idx == 0) - goto cleanup; + return 0; if (STRNEQ(diskTarget, target)) { virReportError(VIR_ERR_INVALID_ARG, _("requested target '%s' does not match target '%s'"), target, diskTarget); - ret = -1; - goto cleanup; + return -1; } *chainIndex = idx; - cleanup: - VIR_FREE(target); - return ret; + return 0; } @@ -1890,9 +1884,9 @@ virStorageAuthDefParse(xmlNodePtr node, { xmlNodePtr saveNode = ctxt->node; VIR_AUTOPTR(virStorageAuthDef) authdef = NULL; + VIR_AUTOFREE(char *) authtype = NULL; virStorageAuthDefPtr ret = NULL; xmlNodePtr secretnode = NULL; - char *authtype = NULL; ctxt->node = node; @@ -1939,7 +1933,6 @@ virStorageAuthDefParse(xmlNodePtr node, VIR_STEAL_PTR(ret, authdef); cleanup: - VIR_FREE(authtype); ctxt->node = saveNode; return ret; @@ -1983,10 +1976,10 @@ virStoragePRDefParseXML(xmlXPathContextPtr ctxt) { virStoragePRDefPtr prd; virStoragePRDefPtr ret = NULL; - char *managed = NULL; - char *type = NULL; - char *path = NULL; - char *mode = NULL; + VIR_AUTOFREE(char *) managed = NULL; + VIR_AUTOFREE(char *) type = NULL; + VIR_AUTOFREE(char *) path = NULL; + VIR_AUTOFREE(char *) mode = NULL; if (VIR_ALLOC(prd) < 0) return NULL; @@ -2045,10 +2038,6 @@ virStoragePRDefParseXML(xmlXPathContextPtr ctxt) VIR_STEAL_PTR(ret, prd); cleanup: - VIR_FREE(mode); - VIR_FREE(path); - VIR_FREE(type); - VIR_FREE(managed); virStoragePRDefFree(prd); return ret; } @@ -2601,7 +2590,7 @@ static virStorageSourcePtr virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, const char *rel) { - char *dirname = NULL; + VIR_AUTOFREE(char *) dirname = NULL; virStorageSourcePtr ret; if (VIR_ALLOC(ret) < 0) @@ -2645,7 +2634,6 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, } cleanup: - VIR_FREE(dirname); return ret; error: @@ -2809,8 +2797,8 @@ int virStorageSourceParseRBDColonString(const char *rbdstr, virStorageSourcePtr src) { - char *options = NULL; char *p, *e, *next; + VIR_AUTOFREE(char *) options = NULL; VIR_AUTOPTR(virStorageAuthDef) authdef = NULL; /* optionally skip the "rbd:" prefix if provided */ @@ -2818,19 +2806,19 @@ virStorageSourceParseRBDColonString(const char *rbdstr, rbdstr += strlen("rbd:"); if (VIR_STRDUP(src->path, rbdstr) < 0) - goto error; + return -1; p = strchr(src->path, ':'); if (p) { if (VIR_STRDUP(options, p + 1) < 0) - goto error; + return -1; *p = '\0'; } /* snapshot name */ if ((p = strchr(src->path, '@'))) { if (VIR_STRDUP(src->snapshot, p + 1) < 0) - goto error; + return -1; *p = '\0'; } @@ -2838,7 +2826,7 @@ virStorageSourceParseRBDColonString(const char *rbdstr, if ((p = strchr(src->path, '/'))) { VIR_STEAL_PTR(src->volume, src->path); if (VIR_STRDUP(src->path, p + 1) < 0) - goto error; + return -1; *p = '\0'; } @@ -2866,14 +2854,14 @@ virStorageSourceParseRBDColonString(const char *rbdstr, if (STRPREFIX(p, "id=")) { /* formulate authdef for src->auth */ if (VIR_ALLOC(authdef) < 0) - goto error; + return -1; if (VIR_STRDUP(authdef->username, p + strlen("id=")) < 0) - goto error; + return -1; if (VIR_STRDUP(authdef->secrettype, virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_CEPH)) < 0) - goto error; + return -1; VIR_STEAL_PTR(src->auth, authdef); src->authInherited = true; @@ -2897,7 +2885,7 @@ virStorageSourceParseRBDColonString(const char *rbdstr, } if (virStorageSourceRBDAddHost(src, h) < 0) - goto error; + return -1; h = sep; } @@ -2905,16 +2893,11 @@ virStorageSourceParseRBDColonString(const char *rbdstr, if (STRPREFIX(p, "conf=") && VIR_STRDUP(src->configFile, p + strlen("conf=")) < 0) - goto error; + return -1; p = next; } - VIR_FREE(options); return 0; - - error: - VIR_FREE(options); - return -1; } @@ -2983,36 +2966,35 @@ static int virStorageSourceParseBackingColon(virStorageSourcePtr src, const char *path) { - char *protocol = NULL; + VIR_AUTOFREE(char *) protocol = NULL; const char *p; - int ret = -1; if (!(p = strchr(path, ':'))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid backing protocol string '%s'"), path); - goto cleanup; + return -1; } if (VIR_STRNDUP(protocol, path, p - path) < 0) - goto cleanup; + return -1; if ((src->protocol = virStorageNetProtocolTypeFromString(protocol)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid backing protocol '%s'"), protocol); - goto cleanup; + return -1; } switch ((virStorageNetProtocol) src->protocol) { case VIR_STORAGE_NET_PROTOCOL_NBD: if (virStorageSourceParseNBDColonString(path, src) < 0) - goto cleanup; + return -1; break; case VIR_STORAGE_NET_PROTOCOL_RBD: if (virStorageSourceParseRBDColonString(path, src) < 0) - goto cleanup; + return -1; break; case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: @@ -3021,7 +3003,7 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src, virReportError(VIR_ERR_INTERNAL_ERROR, _("backing store parser is not implemented for protocol %s"), protocol); - goto cleanup; + return -1; case VIR_STORAGE_NET_PROTOCOL_HTTP: case VIR_STORAGE_NET_PROTOCOL_HTTPS: @@ -3035,14 +3017,10 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src, virReportError(VIR_ERR_INTERNAL_ERROR, _("malformed backing store path for protocol %s"), protocol); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FREE(protocol); - return ret; + return 0; } @@ -3593,7 +3571,7 @@ virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, virJSONValuePtr deflattened = NULL; virJSONValuePtr file; const char *drvname; - char *str = NULL; + VIR_AUTOFREE(char *) str = NULL; size_t i; int ret = -1; @@ -3628,7 +3606,6 @@ virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, "driver '%s'"), drvname); cleanup: - VIR_FREE(str); virJSONValueFree(deflattened); return ret; } @@ -3997,8 +3974,8 @@ virStorageFileCanonicalizePath(const char *path, bool beginDoubleSlash = false; char **components = NULL; size_t ncomponents = 0; - char *linkpath = NULL; - char *currentpath = NULL; + VIR_AUTOFREE(char *) linkpath = NULL; + VIR_AUTOFREE(char *) currentpath = NULL; size_t i = 0; size_t j = 0; int rc; @@ -4131,8 +4108,6 @@ virStorageFileCanonicalizePath(const char *path, cleanup: virHashFree(cycle); virStringListFreeCount(components, ncomponents); - VIR_FREE(linkpath); - VIR_FREE(currentpath); return ret; } @@ -4175,25 +4150,22 @@ virStorageFileGetRelativeBackingPath(virStorageSourcePtr top, char **relpath) { virStorageSourcePtr next; - char *tmp = NULL; - char *path = NULL; - char ret = -1; + VIR_AUTOFREE(char *) tmp = NULL; + VIR_AUTOFREE(char *) path = NULL; *relpath = NULL; for (next = top; virStorageSourceIsBacking(next); next = next->backingStore) { - if (!next->relPath) { - ret = 1; - goto cleanup; - } + if (!next->relPath) + return 1; if (!(tmp = virStorageFileRemoveLastPathComponent(path))) - goto cleanup; + return -1; VIR_FREE(path); if (virAsprintf(&path, "%s%s", tmp, next->relPath) < 0) - goto cleanup; + return -1; VIR_FREE(tmp); @@ -4205,18 +4177,12 @@ virStorageFileGetRelativeBackingPath(virStorageSourcePtr top, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to resolve relative backing name: " "base image is not in backing chain")); - goto cleanup; + return -1; } - *relpath = path; - path = NULL; - - ret = 0; + VIR_STEAL_PTR(*relpath, path); - cleanup: - VIR_FREE(path); - VIR_FREE(tmp); - return ret; + return 0; } @@ -4867,7 +4833,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, { int ret = -1; const char *uniqueName; - char *buf = NULL; + VIR_AUTOFREE(char *) buf = NULL; ssize_t headerLen; virStorageSourcePtr backingStore = NULL; int backingFormat; @@ -4954,7 +4920,6 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, cleanup: if (virStorageSourceHasBacking(src)) src->backingStore->id = depth; - VIR_FREE(buf); virStorageFileDeinit(src); virStorageSourceFree(backingStore); return ret; @@ -5026,7 +4991,7 @@ virStorageFileGetBackingStoreStr(virStorageSourcePtr src, char **backing) { virStorageSourcePtr tmp = NULL; - char *buf = NULL; + VIR_AUTOFREE(char *) buf = NULL; ssize_t headerLen; int ret = -1; int rv; @@ -5063,7 +5028,6 @@ virStorageFileGetBackingStoreStr(virStorageSourcePtr src, ret = 0; cleanup: - VIR_FREE(buf); virStorageSourceFree(tmp); return ret; -- 2.20.1

Let's make use of the auto __cleanup capabilities cleaning up any now unnecessary goto paths. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/test/test_driver.c | 179 ++++++++++++++++------------------------- 1 file changed, 69 insertions(+), 110 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ff773f39b0..dc2b72d31a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -200,7 +200,7 @@ testDomainDefNamespaceParse(xmlDocPtr xml ATTRIBUTE_UNUSED, void **data) { testDomainNamespaceDefPtr nsdata = NULL; - xmlNodePtr *nodes = NULL; + VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; int tmp, n; size_t i; unsigned int tmpuint; @@ -233,7 +233,6 @@ testDomainDefNamespaceParse(xmlDocPtr xml ATTRIBUTE_UNUSED, nsdata->snap_nodes[nsdata->num_snap_nodes] = newnode; nsdata->num_snap_nodes++; } - VIR_FREE(nodes); tmp = virXPathBoolean("boolean(./test:transient)", ctxt); if (tmp == -1) { @@ -280,7 +279,6 @@ testDomainDefNamespaceParse(xmlDocPtr xml ATTRIBUTE_UNUSED, return 0; error: - VIR_FREE(nodes); testDomainDefNamespaceFree(nsdata); return -1; } @@ -695,11 +693,10 @@ testParseXMLDocFromFile(xmlNodePtr node, const char *file, const char *type) xmlNodePtr ret = NULL; xmlDocPtr doc = NULL; char *absFile = NULL; - char *relFile = virXMLPropString(node, "file"); + VIR_AUTOFREE(char *) relFile = NULL; - if (relFile != NULL) { + if ((relFile = virXMLPropString(node, "file"))) { absFile = testBuildFilename(file, relFile); - VIR_FREE(relFile); if (!absFile) { virReportError(VIR_ERR_INTERNAL_ERROR, _("resolving %s filename"), type); @@ -722,14 +719,13 @@ testParseXMLDocFromFile(xmlNodePtr node, const char *file, const char *type) error: xmlFreeDoc(doc); - VIR_FREE(absFile); return ret; } static int testParseNodeInfo(virNodeInfoPtr nodeInfo, xmlXPathContextPtr ctxt) { - char *str; + VIR_AUTOFREE(char *) str = NULL; long l; int ret; @@ -794,10 +790,8 @@ testParseNodeInfo(virNodeInfoPtr nodeInfo, xmlXPathContextPtr ctxt) if (virStrcpyStatic(nodeInfo->model, str) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Model %s too big for destination"), str); - VIR_FREE(str); goto error; } - VIR_FREE(str); } ret = virXPathLong("string(/node/memory[1])", ctxt, &l); @@ -877,12 +871,12 @@ testParseDomains(testDriverPtr privconn, { int num, ret = -1; size_t i; - xmlNodePtr *nodes = NULL; + VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; virDomainObjPtr obj = NULL; num = virXPathNodeSet("/node/domain", ctxt, &nodes); if (num < 0) - goto error; + return -1; for (i = 0; i < num; i++) { virDomainDefPtr def; @@ -928,7 +922,6 @@ testParseDomains(testDriverPtr privconn, ret = 0; error: virDomainObjEndAPI(&obj); - VIR_FREE(nodes); return ret; } @@ -938,38 +931,35 @@ testParseNetworks(testDriverPtr privconn, const char *file, xmlXPathContextPtr ctxt) { - int num, ret = -1; + int num; size_t i; - xmlNodePtr *nodes = NULL; + VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; virNetworkObjPtr obj; num = virXPathNodeSet("/node/network", ctxt, &nodes); if (num < 0) - goto error; + return -1; for (i = 0; i < num; i++) { virNetworkDefPtr def; xmlNodePtr node = testParseXMLDocFromFile(nodes[i], file, "network"); if (!node) - goto error; + return -1; def = virNetworkDefParseNode(ctxt->doc, node); if (!def) - goto error; + return -1; if (!(obj = virNetworkObjAssignDef(privconn->networks, def, 0))) { virNetworkDefFree(def); - goto error; + return -1; } virNetworkObjSetActive(obj, true); virNetworkObjEndAPI(&obj); } - ret = 0; - error: - VIR_FREE(nodes); - return ret; + return 0; } @@ -978,39 +968,36 @@ testParseInterfaces(testDriverPtr privconn, const char *file, xmlXPathContextPtr ctxt) { - int num, ret = -1; + int num; size_t i; - xmlNodePtr *nodes = NULL; + VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; virInterfaceObjPtr obj; num = virXPathNodeSet("/node/interface", ctxt, &nodes); if (num < 0) - goto error; + return -1; for (i = 0; i < num; i++) { virInterfaceDefPtr def; xmlNodePtr node = testParseXMLDocFromFile(nodes[i], file, "interface"); if (!node) - goto error; + return -1; def = virInterfaceDefParseNode(ctxt->doc, node); if (!def) - goto error; + return -1; if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, def))) { virInterfaceDefFree(def); - goto error; + return -1; } virInterfaceObjSetActive(obj, true); virInterfaceObjEndAPI(&obj); } - ret = 0; - error: - VIR_FREE(nodes); - return ret; + return 0; } @@ -1021,51 +1008,47 @@ testOpenVolumesForPool(const char *file, int objidx) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(obj); - char *vol_xpath; size_t i; - int num, ret = -1; - xmlNodePtr *nodes = NULL; + int num; + VIR_AUTOFREE(char *) vol_xpath = NULL; + VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; VIR_AUTOPTR(virStorageVolDef) volDef = NULL; /* Find storage volumes */ if (virAsprintf(&vol_xpath, "/node/pool[%d]/volume", objidx) < 0) - goto error; + return -1; num = virXPathNodeSet(vol_xpath, ctxt, &nodes); - VIR_FREE(vol_xpath); if (num < 0) - goto error; + return -1; for (i = 0; i < num; i++) { xmlNodePtr node = testParseXMLDocFromFile(nodes[i], file, "volume"); if (!node) - goto error; + return -1; if (!(volDef = virStorageVolDefParseNode(def, ctxt->doc, node, 0))) - goto error; + return -1; if (!volDef->target.path) { if (virAsprintf(&volDef->target.path, "%s/%s", def->target.path, volDef->name) < 0) - goto error; + return -1; } if (!volDef->key && VIR_STRDUP(volDef->key, volDef->target.path) < 0) - goto error; + return -1; if (virStoragePoolObjAddVol(obj, volDef) < 0) - goto error; + return -1; def->allocation += volDef->target.allocation; def->available = (def->capacity - def->allocation); volDef = NULL; } - ret = 0; - error: - VIR_FREE(nodes); - return ret; + return 0; } @@ -1074,50 +1057,47 @@ testParseStorage(testDriverPtr privconn, const char *file, xmlXPathContextPtr ctxt) { - int num, ret = -1; + int num; size_t i; - xmlNodePtr *nodes = NULL; + VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; virStoragePoolObjPtr obj; num = virXPathNodeSet("/node/pool", ctxt, &nodes); if (num < 0) - goto error; + return -1; for (i = 0; i < num; i++) { virStoragePoolDefPtr def; xmlNodePtr node = testParseXMLDocFromFile(nodes[i], file, "pool"); if (!node) - goto error; + return -1; def = virStoragePoolDefParseNode(ctxt->doc, node); if (!def) - goto error; + return -1; if (!(obj = virStoragePoolObjAssignDef(privconn->pools, def, false))) { virStoragePoolDefFree(def); - goto error; + return -1; } if (testStoragePoolObjSetDefaults(obj) == -1) { virStoragePoolObjEndAPI(&obj); - goto error; + return -1; } virStoragePoolObjSetActive(obj, true); /* Find storage volumes */ if (testOpenVolumesForPool(file, ctxt, obj, i+1) < 0) { virStoragePoolObjEndAPI(&obj); - goto error; + return -1; } virStoragePoolObjEndAPI(&obj); } - ret = 0; - error: - VIR_FREE(nodes); - return ret; + return 0; } @@ -1126,79 +1106,70 @@ testParseNodedevs(testDriverPtr privconn, const char *file, xmlXPathContextPtr ctxt) { - int num, ret = -1; + int num; size_t i; - xmlNodePtr *nodes = NULL; + VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; virNodeDeviceObjPtr obj; num = virXPathNodeSet("/node/device", ctxt, &nodes); if (num < 0) - goto error; + return -1; for (i = 0; i < num; i++) { virNodeDeviceDefPtr def; xmlNodePtr node = testParseXMLDocFromFile(nodes[i], file, "nodedev"); if (!node) - goto error; + return -1; def = virNodeDeviceDefParseNode(ctxt->doc, node, 0, NULL); if (!def) - goto error; + return -1; if (!(obj = virNodeDeviceObjListAssignDef(privconn->devs, def))) { virNodeDeviceDefFree(def); - goto error; + return -1; } virNodeDeviceObjSetSkipUpdateCaps(obj, true); virNodeDeviceObjEndAPI(&obj); } - ret = 0; - error: - VIR_FREE(nodes); - return ret; + return 0; } static int testParseAuthUsers(testDriverPtr privconn, xmlXPathContextPtr ctxt) { - int num, ret = -1; + int num; size_t i; - xmlNodePtr *nodes = NULL; + VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; num = virXPathNodeSet("/node/auth/user", ctxt, &nodes); if (num < 0) - goto error; + return -1; privconn->numAuths = num; if (num && VIR_ALLOC_N(privconn->auths, num) < 0) - goto error; + return -1; for (i = 0; i < num; i++) { - char *username, *password; + VIR_AUTOFREE(char *) username = NULL; ctxt->node = nodes[i]; username = virXPathString("string(.)", ctxt); if (!username || STREQ(username, "")) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing username in /node/auth/user field")); - VIR_FREE(username); - goto error; + return -1; } /* This field is optional. */ - password = virXMLPropString(nodes[i], "password"); - - privconn->auths[i].username = username; - privconn->auths[i].password = password; + privconn->auths[i].password = virXMLPropString(nodes[i], "password"); + VIR_STEAL_PTR(privconn->auths[i].username, username); } - ret = 0; - error: - VIR_FREE(nodes); - return ret; + return 0; } static int @@ -1348,7 +1319,8 @@ testConnectAuthenticate(virConnectPtr conn, testDriverPtr privconn = conn->privateData; int ret = -1; ssize_t i; - char *username = NULL, *password = NULL; + VIR_AUTOFREE(char *) username = NULL; + VIR_AUTOFREE(char *) password = NULL; virObjectLock(privconn); if (privconn->numAuths == 0) { @@ -1388,8 +1360,6 @@ testConnectAuthenticate(virConnectPtr conn, ret = 0; cleanup: virObjectUnlock(privconn); - VIR_FREE(username); - VIR_FREE(password); return ret; } @@ -1977,7 +1947,7 @@ testDomainSaveFlags(virDomainPtr domain, const char *path, const char *dxml, unsigned int flags) { testDriverPtr privconn = domain->conn->privateData; - char *xml = NULL; + VIR_AUTOFREE(char *) xml = NULL; int fd = -1; int len; virDomainObjPtr privdom; @@ -2052,8 +2022,6 @@ testDomainSaveFlags(virDomainPtr domain, const char *path, ret = 0; cleanup: - VIR_FREE(xml); - /* Don't report failure in close or unlink, because * in either case we're already in a failure scenario * and have reported an earlier error */ @@ -2080,7 +2048,7 @@ testDomainRestoreFlags(virConnectPtr conn, unsigned int flags) { testDriverPtr privconn = conn->privateData; - char *xml = NULL; + VIR_AUTOFREE(char *) xml = NULL; char magic[15]; int fd = -1; int len; @@ -2162,7 +2130,6 @@ testDomainRestoreFlags(virConnectPtr conn, cleanup: virDomainDefFree(def); - VIR_FREE(xml); VIR_FORCE_CLOSE(fd); virDomainObjEndAPI(&dom); virObjectEventStateQueue(privconn->eventState, event); @@ -2578,8 +2545,7 @@ testDomainRenameCallback(virDomainObjPtr privdom, virObjectEventPtr event_new = NULL; virObjectEventPtr event_old = NULL; int ret = -1; - char *new_dom_name = NULL; - char *old_dom_name = NULL; + VIR_AUTOFREE(char *) new_dom_name = NULL; virCheckFlags(0, -1); @@ -2597,9 +2563,8 @@ testDomainRenameCallback(virDomainObjPtr privdom, VIR_DOMAIN_EVENT_UNDEFINED_RENAMED); /* Switch name in domain definition. */ - old_dom_name = privdom->def->name; - privdom->def->name = new_dom_name; - new_dom_name = NULL; + VIR_FREE(privdom->def->name); + VIR_STEAL_PTR(privdom->def->name, new_dom_name); event_new = virDomainEventLifecycleNewFromObj(privdom, VIR_DOMAIN_EVENT_DEFINED, @@ -2607,8 +2572,6 @@ testDomainRenameCallback(virDomainObjPtr privdom, ret = 0; cleanup: - VIR_FREE(old_dom_name); - VIR_FREE(new_dom_name); virObjectEventStateQueue(driver->eventState, event_old); virObjectEventStateQueue(driver->eventState, event_new); return ret; @@ -5471,7 +5434,7 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, const char *wwnn, const char *wwpn) { - char *xml = NULL; + VIR_AUTOFREE(char *) xml = NULL; virNodeDeviceDefPtr def = NULL; virNodeDevCapsDefPtr caps; virNodeDeviceObjPtr obj = NULL, objcopy = NULL; @@ -5540,7 +5503,6 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, virObjectEventStateQueue(driver->eventState, event); cleanup: - VIR_FREE(xml); virNodeDeviceDefFree(def); return obj; } @@ -5553,7 +5515,8 @@ testNodeDeviceCreateXML(virConnectPtr conn, { testDriverPtr driver = conn->privateData; virNodeDeviceDefPtr def = NULL; - char *wwnn = NULL, *wwpn = NULL; + VIR_AUTOFREE(char *) wwnn = NULL; + VIR_AUTOFREE(char *) wwpn = NULL; virNodeDevicePtr dev = NULL, ret = NULL; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr objdef; @@ -5597,8 +5560,6 @@ testNodeDeviceCreateXML(virConnectPtr conn, virNodeDeviceObjEndAPI(&obj); virNodeDeviceDefFree(def); virObjectUnref(dev); - VIR_FREE(wwnn); - VIR_FREE(wwpn); return ret; } @@ -5610,7 +5571,8 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) virNodeDeviceObjPtr obj = NULL; virNodeDeviceObjPtr parentobj = NULL; virNodeDeviceDefPtr def; - char *wwnn = NULL, *wwpn = NULL; + VIR_AUTOFREE(char *) wwnn = NULL; + VIR_AUTOFREE(char *) wwpn = NULL; virObjectEventPtr event = NULL; if (!(obj = testNodeDeviceObjFindByName(driver, dev->name))) @@ -5649,8 +5611,6 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) cleanup: virNodeDeviceObjEndAPI(&obj); virObjectEventStateQueue(driver->eventState, event); - VIR_FREE(wwnn); - VIR_FREE(wwpn); return ret; } @@ -6342,7 +6302,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain, virDomainSnapshotObjPtr snap = NULL; virDomainSnapshotPtr snapshot = NULL; virObjectEventPtr event = NULL; - char *xml = NULL; + VIR_AUTOFREE(char *) xml = NULL; bool update_current = true; bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE; unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; @@ -6427,7 +6387,6 @@ testDomainSnapshotCreateXML(virDomainPtr domain, snapshot = virGetDomainSnapshot(domain, snap->def->name); cleanup: - VIR_FREE(xml); if (vm) { if (snapshot) { virDomainSnapshotObjPtr other; -- 2.20.1

Let's make use of the auto __cleanup capabilities cleaning up any now unnecessary goto paths. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tests/storagebackendsheepdogtest.c | 50 ++++++++++++------------------ tests/storagepoolxml2argvtest.c | 18 +++-------- tests/storagepoolxml2xmltest.c | 33 ++++++-------------- tests/storagevolxml2argvtest.c | 42 +++++++++---------------- tests/storagevolxml2xmltest.c | 38 +++++++---------------- tests/virstoragetest.c | 45 +++++++-------------------- tests/virstorageutiltest.c | 7 ++--- 7 files changed, 75 insertions(+), 158 deletions(-) diff --git a/tests/storagebackendsheepdogtest.c b/tests/storagebackendsheepdogtest.c index 03ddf76d65..1806f9725f 100644 --- a/tests/storagebackendsheepdogtest.c +++ b/tests/storagebackendsheepdogtest.c @@ -57,32 +57,27 @@ test_node_info_parser(const void *opaque) { const struct testNodeInfoParserData *data = opaque; collie_test test = data->data; - int ret = -1; - char *output = NULL; + VIR_AUTOFREE(char *) output = NULL; VIR_AUTOPTR(virStoragePoolDef) pool = NULL; if (!(pool = virStoragePoolDefParseFile(data->poolxml))) - goto cleanup; + return -1; if (VIR_STRDUP(output, test.output) < 0) - goto cleanup; + return -1; if (virStorageBackendSheepdogParseNodeInfo(pool, output) != test.expected_return) - goto cleanup; + return -1; - if (test.expected_return) { - ret = 0; - goto cleanup; - } + if (test.expected_return) + return 0; if (pool->capacity == test.expected_capacity && pool->allocation == test.expected_allocation) - ret = 0; + return 0; - cleanup: - VIR_FREE(output); - return ret; + return -1; } static int @@ -90,36 +85,31 @@ test_vdi_list_parser(const void *opaque) { const struct testVDIListParserData *data = opaque; collie_test test = data->data; - int ret = -1; - char *output = NULL; + VIR_AUTOFREE(char *) output = NULL; VIR_AUTOPTR(virStoragePoolDef) pool = NULL; VIR_AUTOPTR(virStorageVolDef) vol = NULL; if (!(pool = virStoragePoolDefParseFile(data->poolxml))) - goto cleanup; + return -1; if (!(vol = virStorageVolDefParseFile(pool, data->volxml, 0))) - goto cleanup; + return -1; if (VIR_STRDUP(output, test.output) < 0) - goto cleanup; + return -1; if (virStorageBackendSheepdogParseVdiList(vol, output) != test.expected_return) - goto cleanup; + return -1; - if (test.expected_return) { - ret = 0; - goto cleanup; - } + if (test.expected_return) + return 0; if (vol->target.capacity == test.expected_capacity && vol->target.allocation == test.expected_allocation) - ret = 0; + return 0; - cleanup: - VIR_FREE(output); - return ret; + return -1; } @@ -127,8 +117,8 @@ static int mymain(void) { int ret = 0; - char *poolxml = NULL; - char *volxml = NULL; + VIR_AUTOFREE(char *) poolxml = NULL; + VIR_AUTOFREE(char *) volxml = NULL; collie_test node_info_tests[] = { {"", -1, 0, 0}, @@ -215,8 +205,6 @@ mymain(void) } cleanup: - VIR_FREE(poolxml); - VIR_FREE(volxml); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c index 116a75c3ea..e76e86527f 100644 --- a/tests/storagepoolxml2argvtest.c +++ b/tests/storagepoolxml2argvtest.c @@ -88,7 +88,6 @@ testCompareXMLToArgvFiles(bool shouldFail, ret = 0; cleanup: - VIR_FREE(actualCmdline); virStoragePoolObjEndAPI(&pool); if (shouldFail) { virResetLastError(); @@ -106,27 +105,20 @@ struct testInfo { static int testCompareXMLToArgvHelper(const void *data) { - int result = -1; const struct testInfo *info = data; - char *poolxml = NULL; - char *cmdline = NULL; + VIR_AUTOFREE(char *) poolxml = NULL; + VIR_AUTOFREE(char *) cmdline = NULL; if (virAsprintf(&poolxml, "%s/storagepoolxml2xmlin/%s.xml", abs_srcdir, info->pool) < 0) - goto cleanup; + return -1; if (virAsprintf(&cmdline, "%s/storagepoolxml2argvdata/%s%s.argv", abs_srcdir, info->pool, info->platformSuffix) < 0 && !info->shouldFail) - goto cleanup; - - result = testCompareXMLToArgvFiles(info->shouldFail, poolxml, cmdline); - - cleanup: - VIR_FREE(poolxml); - VIR_FREE(cmdline); + return -1; - return result; + return testCompareXMLToArgvFiles(info->shouldFail, poolxml, cmdline); } diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index c8d5c41cd4..bd3408e8b8 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -18,47 +18,34 @@ static int testCompareXMLToXMLFiles(const char *inxml, const char *outxml) { - char *actual = NULL; - int ret = -1; + VIR_AUTOFREE(char *) actual = NULL; VIR_AUTOPTR(virStoragePoolDef) dev = NULL; if (!(dev = virStoragePoolDefParseFile(inxml))) - goto fail; + return -1; if (!(actual = virStoragePoolDefFormat(dev))) - goto fail; + return -1; if (virTestCompareToFile(actual, outxml) < 0) - goto fail; + return -1; - ret = 0; - - fail: - VIR_FREE(actual); - return ret; + return 0; } static int testCompareXMLToXMLHelper(const void *data) { - int result = -1; - char *inxml = NULL; - char *outxml = NULL; + VIR_AUTOFREE(char *) inxml = NULL; + VIR_AUTOFREE(char *) outxml = NULL; if (virAsprintf(&inxml, "%s/storagepoolxml2xmlin/%s.xml", abs_srcdir, (const char*)data) < 0 || virAsprintf(&outxml, "%s/storagepoolxml2xmlout/%s.xml", - abs_srcdir, (const char*)data) < 0) { - goto cleanup; - } - - result = testCompareXMLToXMLFiles(inxml, outxml); - - cleanup: - VIR_FREE(inxml); - VIR_FREE(outxml); + abs_srcdir, (const char*)data) < 0) + return -1; - return result; + return testCompareXMLToXMLFiles(inxml, outxml); } static int diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index 38bb2ae004..3a4c020f68 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -42,9 +42,9 @@ testCompareXMLToArgvFiles(bool shouldFail, unsigned int flags, unsigned long parse_flags) { - char *actualCmdline = NULL; virStorageVolEncryptConvertStep convertStep = VIR_STORAGE_VOL_ENCRYPT_NONE; int ret = -1; + VIR_AUTOFREE(char *) actualCmdline = NULL; VIR_AUTOPTR(virCommand) cmd = NULL; VIR_AUTOPTR(virStorageVolDef) vol = NULL; VIR_AUTOPTR(virStorageVolDef) inputvol = NULL; @@ -109,7 +109,7 @@ testCompareXMLToArgvFiles(bool shouldFail, goto cleanup; } else { char *createCmdline = actualCmdline; - char *cvtCmdline; + VIR_AUTOFREE(char *) cvtCmdline = NULL; int rc; if (!(cvtCmdline = virCommandToString(cmd, false))) @@ -119,7 +119,6 @@ testCompareXMLToArgvFiles(bool shouldFail, createCmdline, cvtCmdline); VIR_FREE(createCmdline); - VIR_FREE(cvtCmdline); if (rc < 0) goto cleanup; } @@ -139,7 +138,6 @@ testCompareXMLToArgvFiles(bool shouldFail, ret = 0; cleanup: - VIR_FREE(actualCmdline); virStoragePoolObjEndAPI(&obj); return ret; } @@ -158,45 +156,35 @@ struct testInfo { static int testCompareXMLToArgvHelper(const void *data) { - int result = -1; const struct testInfo *info = data; - char *poolxml = NULL; - char *inputpoolxml = NULL; - char *volxml = NULL; - char *inputvolxml = NULL; - char *cmdline = NULL; + VIR_AUTOFREE(char *) poolxml = NULL; + VIR_AUTOFREE(char *) inputpoolxml = NULL; + VIR_AUTOFREE(char *) volxml = NULL; + VIR_AUTOFREE(char *) inputvolxml = NULL; + VIR_AUTOFREE(char *) cmdline = NULL; if (info->inputvol && virAsprintf(&inputvolxml, "%s/storagevolxml2xmlin/%s.xml", abs_srcdir, info->inputvol) < 0) - goto cleanup; + return -1; if (info->inputpool && virAsprintf(&inputpoolxml, "%s/storagepoolxml2xmlin/%s.xml", abs_srcdir, info->inputpool) < 0) - goto cleanup; + return -1; if (virAsprintf(&poolxml, "%s/storagepoolxml2xmlin/%s.xml", abs_srcdir, info->pool) < 0 || virAsprintf(&volxml, "%s/storagevolxml2xmlin/%s.xml", abs_srcdir, info->vol) < 0) { - goto cleanup; + return -1; } if (virAsprintf(&cmdline, "%s/storagevolxml2argvdata/%s.argv", abs_srcdir, info->cmdline) < 0 && !info->shouldFail) - goto cleanup; - - result = testCompareXMLToArgvFiles(info->shouldFail, poolxml, volxml, - inputpoolxml, inputvolxml, - cmdline, info->flags, - info->parseflags); - - cleanup: - VIR_FREE(poolxml); - VIR_FREE(volxml); - VIR_FREE(inputvolxml); - VIR_FREE(inputpoolxml); - VIR_FREE(cmdline); + return -1; - return result; + return testCompareXMLToArgvFiles(info->shouldFail, poolxml, volxml, + inputpoolxml, inputvolxml, + cmdline, info->flags, + info->parseflags); } diff --git a/tests/storagevolxml2xmltest.c b/tests/storagevolxml2xmltest.c index cb78bd5b28..7c5d8e7e38 100644 --- a/tests/storagevolxml2xmltest.c +++ b/tests/storagevolxml2xmltest.c @@ -17,28 +17,23 @@ static int testCompareXMLToXMLFiles(const char *poolxml, const char *inxml, const char *outxml, unsigned int flags) { - char *actual = NULL; - int ret = -1; + VIR_AUTOFREE(char *) actual = NULL; VIR_AUTOPTR(virStoragePoolDef) pool = NULL; VIR_AUTOPTR(virStorageVolDef) dev = NULL; if (!(pool = virStoragePoolDefParseFile(poolxml))) - goto fail; + return -1; if (!(dev = virStorageVolDefParseFile(pool, inxml, flags))) - goto fail; + return -1; if (!(actual = virStorageVolDefFormat(pool, dev))) - goto fail; + return -1; if (virTestCompareToFile(actual, outxml) < 0) - goto fail; + return -1; - ret = 0; - - fail: - VIR_FREE(actual); - return ret; + return 0; } struct testInfo { @@ -50,29 +45,20 @@ struct testInfo { static int testCompareXMLToXMLHelper(const void *data) { - int result = -1; const struct testInfo *info = data; - char *poolxml = NULL; - char *inxml = NULL; - char *outxml = NULL; + VIR_AUTOFREE(char *) poolxml = NULL; + VIR_AUTOFREE(char *) inxml = NULL; + VIR_AUTOFREE(char *) outxml = NULL; if (virAsprintf(&poolxml, "%s/storagepoolxml2xmlin/%s.xml", abs_srcdir, info->pool) < 0 || virAsprintf(&inxml, "%s/storagevolxml2xmlin/%s.xml", abs_srcdir, info->name) < 0 || virAsprintf(&outxml, "%s/storagevolxml2xmlout/%s.xml", - abs_srcdir, info->name) < 0) { - goto cleanup; - } - - result = testCompareXMLToXMLFiles(poolxml, inxml, outxml, info->flags); - - cleanup: - VIR_FREE(poolxml); - VIR_FREE(inxml); - VIR_FREE(outxml); + abs_srcdir, info->name) < 0) + return -1; - return result; + return testCompareXMLToXMLFiles(poolxml, inxml, outxml, info->flags); } diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index c448d8b733..71c371891f 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -129,7 +129,7 @@ testPrepImages(void) { int ret = EXIT_FAILURE; VIR_AUTOPTR(virCommand) cmd = NULL; - char *buf = NULL; + VIR_AUTOFREE(char *) buf = NULL; bool compat = false; qemuimg = virFindFileInPath("qemu-img"); @@ -245,7 +245,6 @@ testPrepImages(void) ret = 0; cleanup: - VIR_FREE(buf); if (ret) testCleanupImages(); return ret; @@ -313,7 +312,7 @@ testStorageChain(const void *args) virStorageSourcePtr meta; virStorageSourcePtr elt; size_t i = 0; - char *broken = NULL; + VIR_AUTOFREE(char *) broken = NULL; meta = testStorageFileGetMetadata(data->start, data->format, -1, -1); if (!meta) { @@ -349,8 +348,8 @@ testStorageChain(const void *args) elt = meta; while (virStorageSourceIsBacking(elt)) { - char *expect = NULL; - char *actual = NULL; + VIR_AUTOFREE(char *) expect = NULL; + VIR_AUTOFREE(char *) actual = NULL; if (i == data->nfiles) { fprintf(stderr, "probed chain was too long\n"); @@ -379,18 +378,12 @@ testStorageChain(const void *args) elt->format, virStorageNetProtocolTypeToString(elt->protocol), NULLSTR(elt->nhosts ? elt->hosts[0].name : NULL)) < 0) { - VIR_FREE(expect); - VIR_FREE(actual); goto cleanup; } if (STRNEQ(expect, actual)) { virTestDifference(stderr, expect, actual); - VIR_FREE(expect); - VIR_FREE(actual); goto cleanup; } - VIR_FREE(expect); - VIR_FREE(actual); elt = elt->backingStore; i++; } @@ -401,7 +394,6 @@ testStorageChain(const void *args) ret = 0; cleanup: - VIR_FREE(broken); virStorageSourceFree(meta); return ret; } @@ -539,8 +531,7 @@ static int testPathCanonicalize(const void *args) { const struct testPathCanonicalizeData *data = args; - char *canon = NULL; - int ret = -1; + VIR_AUTOFREE(char *) canon = NULL; canon = virStorageFileCanonicalizePath(data->path, testPathCanonicalizeReadlink, @@ -551,15 +542,10 @@ testPathCanonicalize(const void *args) "path canonicalization of '%s' failed: expected '%s' got '%s'\n", data->path, NULLSTR(data->expect), NULLSTR(canon)); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FREE(canon); - - return ret; + return 0; } static virStorageSource backingchain[12]; @@ -629,14 +615,13 @@ static int testPathRelative(const void *args) { const struct testPathRelativeBacking *data = args; - char *actual = NULL; - int ret = -1; + VIR_AUTOFREE(char *) actual = NULL; if (virStorageFileGetRelativeBackingPath(data->top, data->base, &actual) < 0) { fprintf(stderr, "relative backing path resolution failed\n"); - goto cleanup; + return -1; } if (STRNEQ_NULLABLE(data->expect, actual)) { @@ -644,15 +629,10 @@ testPathRelative(const void *args) "expected '%s', got '%s'\n", data->top->path, data->base->path, NULLSTR(data->expect), NULLSTR(actual)); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FREE(actual); - - return ret; + return 0; } @@ -667,7 +647,7 @@ testBackingParse(const void *args) const struct testBackingParseData *data = args; virBuffer buf = VIR_BUFFER_INITIALIZER; virStorageSourcePtr src = NULL; - char *xml = NULL; + VIR_AUTOFREE(char *) xml = NULL; int ret = -1; if (!(src = virStorageSourceNewFromBackingAbsolute(data->backing))) { @@ -702,7 +682,6 @@ testBackingParse(const void *args) cleanup: virStorageSourceFree(src); virBufferFreeAndReset(&buf); - VIR_FREE(xml); return ret; } diff --git a/tests/virstorageutiltest.c b/tests/virstorageutiltest.c index d91c5d4d6f..766a910975 100644 --- a/tests/virstorageutiltest.c +++ b/tests/virstorageutiltest.c @@ -45,8 +45,8 @@ testGlusterExtractPoolSources(const void *opaque) .sources = NULL }; size_t i; - char *srcxmldata = NULL; - char *actual = NULL; + VIR_AUTOFREE(char *) srcxmldata = NULL; + VIR_AUTOFREE(char *) actual = NULL; int ret = -1; if (virTestLoadFile(data->srcxml, &srcxmldata) < 0) @@ -62,9 +62,6 @@ testGlusterExtractPoolSources(const void *opaque) ret = virTestCompareToFile(actual, data->dstxml); cleanup: - VIR_FREE(srcxmldata); - VIR_FREE(actual); - for (i = 0; i < list.nsources; i++) virStoragePoolSourceClear(&list.sources[i]); VIR_FREE(list.sources); -- 2.20.1

Modify code to use the VIR_AUTOCLOSE logic cleaning up any now unnecessary goto paths. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_logical.c | 3 +- src/storage/storage_backend_scsi.c | 12 +-- src/storage/storage_file_fs.c | 15 +-- src/storage/storage_util.c | 150 ++++++++++---------------- src/util/virstoragefile.c | 39 +++---- 5 files changed, 77 insertions(+), 142 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 2a205a4e95..8ae866f23e 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -911,7 +911,7 @@ static int virStorageBackendLogicalCreateVol(virStoragePoolObjPtr pool, virStorageVolDefPtr vol) { - int fd = -1; + VIR_AUTOCLOSE fd = -1; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); virErrorPtr err; struct stat sb; @@ -971,7 +971,6 @@ virStorageBackendLogicalCreateVol(virStoragePoolObjPtr pool, error: err = virSaveLastError(); - VIR_FORCE_CLOSE(fd); virStorageBackendLogicalDeleteVol(pool, vol, 0); virSetError(err); virFreeError(err); diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 1a10e43647..2ae55af6c9 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -55,8 +55,7 @@ struct _virStoragePoolFCRefreshInfo { static int virStorageBackendSCSITriggerRescan(uint32_t host) { - int fd = -1; - int retval = -1; + VIR_AUTOCLOSE fd = -1; VIR_AUTOFREE(char *) path = NULL; VIR_DEBUG("Triggering rescan of host %d", host); @@ -73,24 +72,19 @@ virStorageBackendSCSITriggerRescan(uint32_t host) virReportSystemError(errno, _("Could not open '%s' to trigger host scan"), path); - goto cleanup; + return -1; } if (safewrite(fd, LINUX_SYSFS_SCSI_HOST_SCAN_STRING, sizeof(LINUX_SYSFS_SCSI_HOST_SCAN_STRING)) < 0) { - VIR_FORCE_CLOSE(fd); virReportSystemError(errno, _("Write to '%s' to trigger host scan failed"), path); } - retval = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); VIR_DEBUG("Rescan of host %d complete", host); - return retval; + return 0; } /** diff --git a/src/storage/storage_file_fs.c b/src/storage/storage_file_fs.c index 3b6ed6e34d..0028389e60 100644 --- a/src/storage/storage_file_fs.c +++ b/src/storage/storage_file_fs.c @@ -83,7 +83,7 @@ virStorageFileBackendFileInit(virStorageSourcePtr src) static int virStorageFileBackendFileCreate(virStorageSourcePtr src) { - int fd = -1; + VIR_AUTOCLOSE fd = -1; mode_t mode = S_IRUSR; if (!src->readonly) @@ -95,7 +95,6 @@ virStorageFileBackendFileCreate(virStorageSourcePtr src) return -1; } - VIR_FORCE_CLOSE(fd); return 0; } @@ -121,7 +120,7 @@ virStorageFileBackendFileRead(virStorageSourcePtr src, size_t len, char **buf) { - int fd = -1; + VIR_AUTOCLOSE fd = -1; ssize_t ret = -1; if ((fd = virFileOpenAs(src->path, O_RDONLY, 0, @@ -134,19 +133,15 @@ virStorageFileBackendFileRead(virStorageSourcePtr src, if (offset > 0) { if (lseek(fd, offset, SEEK_SET) == (off_t) -1) { virReportSystemError(errno, _("cannot seek into '%s'"), src->path); - goto cleanup; + return -1; } } if ((ret = virFileReadHeaderFD(fd, len, buf)) < 0) { - virReportSystemError(errno, - _("cannot read header '%s'"), src->path); - goto cleanup; + virReportSystemError(errno, _("cannot read header '%s'"), src->path); + return -1; } - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; } diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index d87d2eca1e..345566a3af 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -130,7 +130,6 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, bool want_sparse, bool reflink_copy) { - int inputfd = -1; int amtread = -1; int ret = 0; size_t rbytes = READ_BLOCK_SIZE_DEFAULT; @@ -138,6 +137,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, int interval; VIR_AUTOFREE(char *) zerobuf = NULL; VIR_AUTOFREE(char *) buf = NULL; + VIR_AUTOCLOSE inputfd = -1; struct stat st; if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { @@ -145,7 +145,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, virReportSystemError(errno, _("could not open input path '%s'"), inputvol->target.path); - goto cleanup; + return ret; } #ifdef __linux__ @@ -157,15 +157,11 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, if (wbytes < WRITE_BLOCK_SIZE_DEFAULT) wbytes = WRITE_BLOCK_SIZE_DEFAULT; - if (VIR_ALLOC_N(zerobuf, wbytes) < 0) { - ret = -errno; - goto cleanup; - } + if (VIR_ALLOC_N(zerobuf, wbytes) < 0) + return -errno; - if (VIR_ALLOC_N(buf, rbytes) < 0) { - ret = -errno; - goto cleanup; - } + if (VIR_ALLOC_N(buf, rbytes) < 0) + return -errno; if (reflink_copy) { if (reflinkCloneFile(fd, inputfd) < 0) { @@ -173,10 +169,10 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, virReportSystemError(errno, _("failed to clone files from '%s'"), inputvol->target.path); - goto cleanup; + return ret; } else { VIR_DEBUG("btrfs clone finished."); - goto cleanup; + return 0; } } @@ -191,7 +187,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, virReportSystemError(errno, _("failed reading from file '%s'"), inputvol->target.path); - goto cleanup; + return ret; } *total -= amtread; @@ -208,14 +204,14 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, virReportSystemError(errno, _("cannot extend file '%s'"), vol->target.path); - goto cleanup; + return ret; } } else if (safewrite(fd, buf+offset, interval) < 0) { ret = -errno; virReportSystemError(errno, _("failed writing to file '%s'"), vol->target.path); - goto cleanup; + return ret; } } while ((amtleft -= interval) > 0); @@ -225,23 +221,18 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, ret = -errno; virReportSystemError(errno, _("cannot sync data to file '%s'"), vol->target.path); - goto cleanup; + return ret; } - if (VIR_CLOSE(inputfd) < 0) { ret = -errno; virReportSystemError(errno, _("cannot close file '%s'"), inputvol->target.path); - goto cleanup; + return ret; } - inputfd = -1; - - cleanup: - VIR_FORCE_CLOSE(inputfd); - return ret; + return 0; } static int @@ -250,8 +241,7 @@ storageBackendCreateBlockFrom(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, virStorageVolDefPtr inputvol, unsigned int flags) { - int fd = -1; - int ret = -1; + VIR_AUTOCLOSE fd = -1; unsigned long long remain; struct stat st; gid_t gid; @@ -267,7 +257,7 @@ storageBackendCreateBlockFrom(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("metadata preallocation is not supported for block " "volumes")); - goto cleanup; + return -1; } if (flags & VIR_STORAGE_VOL_CREATE_REFLINK) @@ -277,7 +267,7 @@ storageBackendCreateBlockFrom(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, virReportSystemError(errno, _("cannot create path '%s'"), vol->target.path); - goto cleanup; + return -1; } remain = vol->target.capacity; @@ -285,13 +275,13 @@ storageBackendCreateBlockFrom(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, if (inputvol) { if (virStorageBackendCopyToFD(vol, inputvol, fd, &remain, false, reflink_copy) < 0) - goto cleanup; + return -1; } if (fstat(fd, &st) == -1) { virReportSystemError(errno, _("stat of '%s' failed"), vol->target.path); - goto cleanup; + return -1; } uid = (vol->target.perms->uid != st.st_uid) ? vol->target.perms->uid : (uid_t)-1; @@ -303,7 +293,7 @@ storageBackendCreateBlockFrom(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, _("cannot chown '%s' to (%u, %u)"), vol->target.path, (unsigned int)uid, (unsigned int)gid); - goto cleanup; + return -1; } mode = (vol->target.perms->mode == (mode_t)-1 ? @@ -312,21 +302,16 @@ storageBackendCreateBlockFrom(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), vol->target.path, mode); - goto cleanup; + return -1; } if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("cannot close file '%s'"), vol->target.path); - goto cleanup; + return -1; } - fd = -1; - ret = 0; - cleanup: - VIR_FORCE_CLOSE(fd); - - return ret; + return 0; } static int @@ -419,7 +404,7 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); int ret = -1; - int fd = -1; + VIR_AUTOCLOSE fd = -1; int operation_flags; bool reflink_copy = false; mode_t open_mode = VIR_STORAGE_DEFAULT_VOL_PERM_MODE; @@ -501,7 +486,6 @@ storageBackendCreateRaw(virStoragePoolObjPtr pool, ignore_value(virFileRemove(vol->target.path, vol->target.perms->uid, vol->target.perms->gid)); - VIR_FORCE_CLOSE(fd); return ret; } @@ -542,7 +526,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, * re-open the file and attempt to force the mode change. */ if (mode != (st.st_mode & S_IRWXUGO)) { - int fd = -1; + VIR_AUTOCLOSE fd = -1; int flags = VIR_FILE_OPEN_FORK | VIR_FILE_OPEN_FORCE_MODE; if ((fd = virFileOpenAs(vol->target.path, O_RDWR, mode, @@ -550,7 +534,6 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, vol->target.perms->gid, flags)) >= 0) { /* Success - means we're good */ - VIR_FORCE_CLOSE(fd); ret = 0; goto cleanup; } @@ -1227,7 +1210,7 @@ storageBackendCreateQemuImgSecretPath(virStoragePoolObjPtr pool, { virStorageEncryptionPtr enc = vol->target.encryption; char *secretPath = NULL; - int fd = -1; + VIR_AUTOCLOSE fd = -1; uint8_t *secret = NULL; size_t secretlen = 0; virConnectPtr conn = NULL; @@ -1268,7 +1251,6 @@ storageBackendCreateQemuImgSecretPath(virStoragePoolObjPtr pool, _("failed to write secret file")); goto error; } - VIR_FORCE_CLOSE(fd); if ((vol->target.perms->uid != (uid_t)-1) && (vol->target.perms->gid != (gid_t)-1)) { @@ -1283,7 +1265,6 @@ storageBackendCreateQemuImgSecretPath(virStoragePoolObjPtr pool, cleanup: virObjectUnref(conn); VIR_DISPOSE_N(secret, secretlen); - VIR_FORCE_CLOSE(fd); return secretPath; @@ -1751,17 +1732,17 @@ storageBackendUpdateVolTargetInfo(virStorageVolType voltype, unsigned int openflags, unsigned int readflags) { - int ret, fd = -1; + int ret; struct stat sb; + VIR_AUTOCLOSE fd = -1; VIR_AUTOFREE(char *) buf = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; - if ((ret = virStorageBackendVolOpen(target->path, &sb, openflags)) < 0) - goto cleanup; - fd = ret; + if ((fd = virStorageBackendVolOpen(target->path, &sb, openflags)) < 0) + return -1; if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb)) < 0) - goto cleanup; + return ret; if ((voltype == VIR_STORAGE_VOL_FILE || voltype == VIR_STORAGE_VOL_BLOCK) && target->format != VIR_STORAGE_FILE_NONE) { @@ -1769,49 +1750,39 @@ storageBackendUpdateVolTargetInfo(virStorageVolType voltype, if (storageBackendIsPloopDir(target->path)) { if ((ret = storageBackendRedoPloopUpdate(target, &sb, &fd, openflags)) < 0) - goto cleanup; + return ret; target->format = VIR_STORAGE_FILE_PLOOP; } else { - ret = 0; - goto cleanup; + return 0; } } if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { virReportSystemError(errno, _("cannot seek to start of '%s'"), target->path); - ret = -1; - goto cleanup; + return -1; } if ((len = virFileReadHeaderFD(fd, len, &buf)) < 0) { if (readflags & VIR_STORAGE_VOL_READ_NOERROR) { VIR_WARN("ignoring failed header read for '%s'", target->path); - ret = -2; + return -2; } else { virReportSystemError(errno, _("cannot read header '%s'"), target->path); - ret = -1; + return -1; } - goto cleanup; } - if (virStorageSourceUpdateCapacity(target, buf, len, false) < 0) { - ret = -1; - goto cleanup; - } + if (virStorageSourceUpdateCapacity(target, buf, len, false) < 0) + return -1; } - if (withBlockVolFormat) { - if ((ret = virStorageBackendDetectBlockVolFormatFD(target, fd, - readflags)) < 0) - goto cleanup; - } + if (withBlockVolFormat) + return virStorageBackendDetectBlockVolFormatFD(target, fd, readflags); - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } /* @@ -2617,9 +2588,9 @@ storageBackendVolWipeLocalFile(const char *path, unsigned long long allocation, bool zero_end) { - int ret = -1, fd = -1; const char *alg_char = NULL; struct stat st; + VIR_AUTOCLOSE fd = -1; VIR_AUTOPTR(virCommand) cmd = NULL; fd = open(path, O_RDWR); @@ -2627,14 +2598,14 @@ storageBackendVolWipeLocalFile(const char *path, virReportSystemError(errno, _("Failed to open storage volume with path '%s'"), path); - goto cleanup; + return -1; } if (fstat(fd, &st) == -1) { virReportSystemError(errno, _("Failed to stat storage volume with path '%s'"), path); - goto cleanup; + return -1; } switch ((virStorageVolWipeAlgorithm) algorithm) { @@ -2668,12 +2639,12 @@ storageBackendVolWipeLocalFile(const char *path, case VIR_STORAGE_VOL_WIPE_ALG_TRIM: virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("'trim' algorithm not supported")); - goto cleanup; + return -1; case VIR_STORAGE_VOL_WIPE_ALG_LAST: virReportError(VIR_ERR_INVALID_ARG, _("unsupported algorithm %d"), algorithm); - goto cleanup; + return -1; } VIR_DEBUG("Wiping file '%s' with algorithm '%s'", path, alg_char); @@ -2682,24 +2653,14 @@ storageBackendVolWipeLocalFile(const char *path, cmd = virCommandNew(SCRUB); virCommandAddArgList(cmd, "-f", "-p", alg_char, path, NULL); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - - ret = 0; - } else { - if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) { - ret = storageBackendVolZeroSparseFileLocal(path, st.st_size, fd); - } else { - ret = storageBackendWipeLocal(path, fd, allocation, st.st_blksize, - zero_end); - } - if (ret < 0) - goto cleanup; + return virCommandRun(cmd, NULL); } - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) + return storageBackendVolZeroSparseFileLocal(path, st.st_size, fd); + + return storageBackendWipeLocal(path, fd, allocation, st.st_blksize, + zero_end); } @@ -3392,7 +3353,7 @@ storageBackendProbeTarget(virStorageSourcePtr target, virStorageEncryptionPtr *encryption) { int backingStoreFormat; - int fd = -1; + VIR_AUTOCLOSE fd = -1; int ret = -1; int rc; virStorageSourcePtr meta = NULL; @@ -3499,7 +3460,6 @@ storageBackendProbeTarget(virStorageSourcePtr target, } cleanup: - VIR_FORCE_CLOSE(fd); virStorageSourceFree(meta); return ret; } @@ -3570,9 +3530,10 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) struct statvfs sb; struct stat statbuf; VIR_AUTOPTR(virStorageVolDef) vol = NULL; + VIR_AUTOCLOSE fd = -1; virStorageSourcePtr target = NULL; int direrr; - int fd = -1, ret = -1; + int ret = -1; if (virDirOpen(&dir, def->target.path) < 0) goto cleanup; @@ -3663,7 +3624,6 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) ret = 0; cleanup: VIR_DIR_CLOSE(dir); - VIR_FORCE_CLOSE(fd); virStorageSourceFree(target); if (ret < 0) virStoragePoolObjClearVols(pool); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index c2f044d2db..1d77f6ac4c 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1080,10 +1080,9 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid) { - int fd; - int ret = -1; struct stat sb; ssize_t len = VIR_STORAGE_MAX_HEADER; + VIR_AUTOCLOSE fd = -1; VIR_AUTOFREE(char *) header = NULL; if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) { @@ -1093,31 +1092,24 @@ virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid) if (fstat(fd, &sb) < 0) { virReportSystemError(errno, _("cannot stat file '%s'"), path); - goto cleanup; + return -1; } /* No header to probe for directories */ - if (S_ISDIR(sb.st_mode)) { - ret = VIR_STORAGE_FILE_DIR; - goto cleanup; - } + if (S_ISDIR(sb.st_mode)) + return VIR_STORAGE_FILE_DIR; if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { virReportSystemError(errno, _("cannot set to start of '%s'"), path); - goto cleanup; + return -1; } if ((len = virFileReadHeaderFD(fd, len, &header)) < 0) { virReportSystemError(errno, _("cannot read header '%s'"), path); - goto cleanup; + return -1; } - ret = virStorageFileProbeFormatFromBuf(path, header, len); - - cleanup: - VIR_FORCE_CLOSE(fd); - - return ret; + return virStorageFileProbeFormatFromBuf(path, header, len); } @@ -1312,13 +1304,12 @@ virStorageFileResize(const char *path, unsigned long long capacity, bool pre_allocate) { - int fd = -1; - int ret = -1; + VIR_AUTOCLOSE fd = -1; int rc; if ((fd = open(path, O_RDWR)) < 0) { virReportSystemError(errno, _("Unable to open '%s'"), path); - goto cleanup; + return -1; } if (pre_allocate) { @@ -1331,26 +1322,22 @@ virStorageFileResize(const char *path, _("Failed to pre-allocate space for " "file '%s'"), path); } - goto cleanup; + return -1; } } if (ftruncate(fd, capacity) < 0) { virReportSystemError(errno, _("Failed to truncate file '%s'"), path); - goto cleanup; + return -1; } if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("Unable to save '%s'"), path); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } -- 2.20.1

Let's make use of the auto __cleanup capabilities cleaning up any now unnecessary goto paths. A few methods were modified to use a more common methodology of defining/using @def and then stealing the pointer to @ret to return allowing @def to be autofree'd if the swap didn't occur on various return NULL; error paths. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/domain_conf.c | 3 +- src/qemu/qemu_domain.c | 3 +- src/qemu/qemu_driver.c | 9 +- src/qemu/qemu_migration.c | 3 +- src/storage/storage_backend_gluster.c | 3 +- src/storage/storage_util.c | 31 +-- src/util/virstoragefile.c | 292 ++++++++++++-------------- src/util/virstoragefile.h | 1 + tests/qemublocktest.c | 6 +- tests/virstoragetest.c | 60 +++--- 10 files changed, 179 insertions(+), 232 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ee0edff4b2..3af900da7f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9059,7 +9059,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, unsigned int flags, virDomainXMLOptionPtr xmlopt) { - virStorageSourcePtr backingStore = NULL; + VIR_AUTOPTR(virStorageSource) backingStore = NULL; xmlNodePtr save_ctxt = ctxt->node; xmlNodePtr source; char *type = NULL; @@ -9126,7 +9126,6 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, ret = 0; cleanup: - virStorageSourceFree(backingStore); VIR_FREE(type); VIR_FREE(format); VIR_FREE(idx); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b6c1a0e4e5..b4acf72bf2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2730,7 +2730,7 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, { xmlNodePtr savedNode = ctxt->node; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - virStorageSourcePtr migrSource = NULL; + VIR_AUTOPTR(virStorageSource) migrSource = NULL; char *format = NULL; char *type = NULL; int ret = -1; @@ -2781,7 +2781,6 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node, ret = 0; cleanup: - virStorageSourceFree(migrSource); VIR_FREE(format); VIR_FREE(type); ctxt->node = savedNode; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 427c1d02a8..88dd7846dc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -274,7 +274,7 @@ qemuSecurityChownCallback(const virStorageSource *src, uid_t uid, gid_t gid) { - virStorageSourcePtr cpy = NULL; + VIR_AUTOPTR(virStorageSource) cpy = NULL; struct stat sb; int save_errno = 0; int ret = -1; @@ -319,7 +319,6 @@ qemuSecurityChownCallback(const virStorageSource *src, cleanup: save_errno = errno; virStorageFileDeinit(cpy); - virStorageSourceFree(cpy); errno = save_errno; return ret; @@ -17888,7 +17887,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, virDomainObjPtr vm; int ret = -1; unsigned long long speed = bandwidth; - virStorageSourcePtr dest = NULL; + VIR_AUTOPTR(virStorageSource) dest = NULL; virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | @@ -17950,7 +17949,6 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, cleanup: virDomainObjEndAPI(&vm); - virStorageSourceFree(dest); return ret; } @@ -18080,7 +18078,7 @@ qemuDomainBlockCommit(virDomainPtr dom, char *topPath = NULL; char *basePath = NULL; char *backingPath = NULL; - virStorageSourcePtr mirror = NULL; + VIR_AUTOPTR(virStorageSource) mirror = NULL; unsigned long long speed = bandwidth; qemuBlockJobDataPtr job = NULL; qemuBlockJobType jobtype = QEMU_BLOCKJOB_TYPE_COMMIT; @@ -18282,7 +18280,6 @@ qemuDomainBlockCommit(virDomainPtr dom, virFreeError(orig_err); } } - virStorageSourceFree(mirror); qemuBlockJobStartupFinalize(job); qemuDomainObjEndJob(driver, vm); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 1433b2c2f3..6a680ba2e8 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -788,7 +788,7 @@ qemuMigrationSrcNBDStorageCopyBlockdev(virQEMUDriverPtr driver, { qemuBlockStorageSourceAttachDataPtr data = NULL; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - virStorageSourcePtr copysrc = NULL; + VIR_AUTOPTR(virStorageSource) copysrc = NULL; int mon_ret = 0; int ret = -1; @@ -849,7 +849,6 @@ qemuMigrationSrcNBDStorageCopyBlockdev(virQEMUDriverPtr driver, cleanup: qemuBlockStorageSourceAttachDataFree(data); - virStorageSourceFree(copysrc); return ret; } diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 0fe548f7e0..cb86db22a8 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -237,8 +237,8 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, int ret = -1; VIR_AUTOPTR(virStorageVolDef) vol = NULL; VIR_AUTOFREE(char *) header = NULL; + VIR_AUTOPTR(virStorageSource) meta = NULL; glfs_fd_t *fd = NULL; - virStorageSourcePtr meta = NULL; ssize_t len; int backingFormat; @@ -323,7 +323,6 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, VIR_STEAL_PTR(*volptr, vol); ret = 0; cleanup: - virStorageSourceFree(meta); if (fd) glfs_close(fd); return ret; diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 345566a3af..d138379a81 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3354,9 +3354,8 @@ storageBackendProbeTarget(virStorageSourcePtr target, { int backingStoreFormat; VIR_AUTOCLOSE fd = -1; - int ret = -1; int rc; - virStorageSourcePtr meta = NULL; + VIR_AUTOPTR(virStorageSource) meta = NULL; struct stat sb; if (encryption) @@ -3368,17 +3367,16 @@ storageBackendProbeTarget(virStorageSourcePtr target, fd = rc; if (virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb) < 0) - goto cleanup; + return -1; if (S_ISDIR(sb.st_mode)) { if (storageBackendIsPloopDir(target->path)) { if (storageBackendRedoPloopUpdate(target, &sb, &fd, VIR_STORAGE_VOL_FS_PROBE_FLAGS) < 0) - goto cleanup; + return -1; } else { target->format = VIR_STORAGE_FILE_DIR; - ret = 0; - goto cleanup; + return 0; } } @@ -3386,11 +3384,11 @@ storageBackendProbeTarget(virStorageSourcePtr target, fd, VIR_STORAGE_FILE_AUTO, &backingStoreFormat))) - goto cleanup; + return -1; if (meta->backingStoreRaw) { if (!(target->backingStore = virStorageSourceNewFromBacking(meta))) - goto cleanup; + return -1; target->backingStore->format = backingStoreFormat; @@ -3401,7 +3399,7 @@ storageBackendProbeTarget(virStorageSourcePtr target, virStorageSourceFree(target->backingStore); if (VIR_ALLOC(target->backingStore) < 0) - goto cleanup; + return -1; target->backingStore->type = VIR_STORAGE_TYPE_NETWORK; target->backingStore->path = meta->backingStoreRaw; @@ -3430,8 +3428,6 @@ storageBackendProbeTarget(virStorageSourcePtr target, target->format = meta->format; /* Default to success below this point */ - ret = 0; - if (meta->capacity) target->capacity = meta->capacity; @@ -3450,18 +3446,14 @@ storageBackendProbeTarget(virStorageSourcePtr target, } virBitmapFree(target->features); - target->features = meta->features; - meta->features = NULL; + VIR_STEAL_PTR(target->features, meta->features); if (meta->compat) { VIR_FREE(target->compat); - target->compat = meta->compat; - meta->compat = NULL; + VIR_STEAL_PTR(target->compat, meta->compat); } - cleanup: - virStorageSourceFree(meta); - return ret; + return 0; } @@ -3531,7 +3523,7 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) struct stat statbuf; VIR_AUTOPTR(virStorageVolDef) vol = NULL; VIR_AUTOCLOSE fd = -1; - virStorageSourcePtr target = NULL; + VIR_AUTOPTR(virStorageSource) target = NULL; int direrr; int ret = -1; @@ -3624,7 +3616,6 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) ret = 0; cleanup: VIR_DIR_CLOSE(dir); - virStorageSourceFree(target); if (ret < 0) virStoragePoolObjClearVols(pool); return ret; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 1d77f6ac4c..5a4b63acc4 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1117,22 +1117,20 @@ static virStorageSourcePtr virStorageFileMetadataNew(const char *path, int format) { + VIR_AUTOPTR(virStorageSource) def = NULL; virStorageSourcePtr ret = NULL; - if (VIR_ALLOC(ret) < 0) + if (VIR_ALLOC(def) < 0) return NULL; - ret->format = format; - ret->type = VIR_STORAGE_TYPE_FILE; + def->format = format; + def->type = VIR_STORAGE_TYPE_FILE; - if (VIR_STRDUP(ret->path, path) < 0) - goto error; + if (VIR_STRDUP(def->path, path) < 0) + return NULL; + VIR_STEAL_PTR(ret, def); return ret; - - error: - virStorageSourceFree(ret); - return NULL; } @@ -1205,7 +1203,7 @@ virStorageFileGetMetadataFromFD(const char *path, { virStorageSourcePtr ret = NULL; - virStorageSourcePtr meta = NULL; + VIR_AUTOPTR(virStorageSource) meta = NULL; VIR_AUTOFREE(char *) buf = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; struct stat sb; @@ -1231,21 +1229,21 @@ virStorageFileGetMetadataFromFD(const char *path, meta->type = VIR_STORAGE_TYPE_DIR; meta->format = VIR_STORAGE_FILE_DIR; VIR_STEAL_PTR(ret, meta); - goto cleanup; + return ret; } if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { virReportSystemError(errno, _("cannot seek to start of '%s'"), meta->path); - goto cleanup; + return NULL; } if ((len = virFileReadHeaderFD(fd, len, &buf)) < 0) { virReportSystemError(errno, _("cannot read header '%s'"), meta->path); - goto cleanup; + return NULL; } if (virStorageFileGetMetadataInternal(meta, buf, len, backingFormat) < 0) - goto cleanup; + return NULL; if (S_ISREG(sb.st_mode)) meta->type = VIR_STORAGE_TYPE_FILE; @@ -1253,9 +1251,6 @@ virStorageFileGetMetadataFromFD(const char *path, meta->type = VIR_STORAGE_TYPE_BLOCK; VIR_STEAL_PTR(ret, meta); - - cleanup: - virStorageSourceFree(meta); return ret; } @@ -2243,99 +2238,97 @@ virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src, bool backingChain) { + VIR_AUTOPTR(virStorageSource) def = NULL; virStorageSourcePtr ret = NULL; - if (VIR_ALLOC(ret) < 0) + if (VIR_ALLOC(def) < 0) return NULL; - ret->id = src->id; - ret->type = src->type; - ret->protocol = src->protocol; - ret->format = src->format; - ret->capacity = src->capacity; - ret->allocation = src->allocation; - ret->has_allocation = src->has_allocation; - ret->physical = src->physical; - ret->readonly = src->readonly; - ret->shared = src->shared; - ret->haveTLS = src->haveTLS; - ret->tlsFromConfig = src->tlsFromConfig; - ret->detected = src->detected; - ret->debugLevel = src->debugLevel; - ret->debug = src->debug; - ret->iomode = src->iomode; - ret->cachemode = src->cachemode; - ret->discard = src->discard; - ret->detect_zeroes = src->detect_zeroes; + def->id = src->id; + def->type = src->type; + def->protocol = src->protocol; + def->format = src->format; + def->capacity = src->capacity; + def->allocation = src->allocation; + def->has_allocation = src->has_allocation; + def->physical = src->physical; + def->readonly = src->readonly; + def->shared = src->shared; + def->haveTLS = src->haveTLS; + def->tlsFromConfig = src->tlsFromConfig; + def->detected = src->detected; + def->debugLevel = src->debugLevel; + def->debug = src->debug; + def->iomode = src->iomode; + def->cachemode = src->cachemode; + def->discard = src->discard; + def->detect_zeroes = src->detect_zeroes; /* storage driver metadata are not copied */ - ret->drv = NULL; - - if (VIR_STRDUP(ret->path, src->path) < 0 || - VIR_STRDUP(ret->volume, src->volume) < 0 || - VIR_STRDUP(ret->relPath, src->relPath) < 0 || - VIR_STRDUP(ret->backingStoreRaw, src->backingStoreRaw) < 0 || - VIR_STRDUP(ret->snapshot, src->snapshot) < 0 || - VIR_STRDUP(ret->configFile, src->configFile) < 0 || - VIR_STRDUP(ret->nodeformat, src->nodeformat) < 0 || - VIR_STRDUP(ret->nodestorage, src->nodestorage) < 0 || - VIR_STRDUP(ret->compat, src->compat) < 0 || - VIR_STRDUP(ret->tlsAlias, src->tlsAlias) < 0 || - VIR_STRDUP(ret->tlsCertdir, src->tlsCertdir) < 0) - goto error; + def->drv = NULL; + + if (VIR_STRDUP(def->path, src->path) < 0 || + VIR_STRDUP(def->volume, src->volume) < 0 || + VIR_STRDUP(def->relPath, src->relPath) < 0 || + VIR_STRDUP(def->backingStoreRaw, src->backingStoreRaw) < 0 || + VIR_STRDUP(def->snapshot, src->snapshot) < 0 || + VIR_STRDUP(def->configFile, src->configFile) < 0 || + VIR_STRDUP(def->nodeformat, src->nodeformat) < 0 || + VIR_STRDUP(def->nodestorage, src->nodestorage) < 0 || + VIR_STRDUP(def->compat, src->compat) < 0 || + VIR_STRDUP(def->tlsAlias, src->tlsAlias) < 0 || + VIR_STRDUP(def->tlsCertdir, src->tlsCertdir) < 0) + return NULL; if (src->nhosts) { - if (!(ret->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts))) - goto error; + if (!(def->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts))) + return NULL; - ret->nhosts = src->nhosts; + def->nhosts = src->nhosts; } if (src->srcpool && - !(ret->srcpool = virStorageSourcePoolDefCopy(src->srcpool))) - goto error; + !(def->srcpool = virStorageSourcePoolDefCopy(src->srcpool))) + return NULL; if (src->features && - !(ret->features = virBitmapNewCopy(src->features))) - goto error; + !(def->features = virBitmapNewCopy(src->features))) + return NULL; if (src->encryption && - !(ret->encryption = virStorageEncryptionCopy(src->encryption))) - goto error; + !(def->encryption = virStorageEncryptionCopy(src->encryption))) + return NULL; if (src->perms && - !(ret->perms = virStoragePermsCopy(src->perms))) - goto error; + !(def->perms = virStoragePermsCopy(src->perms))) + return NULL; if (src->timestamps && - !(ret->timestamps = virStorageTimestampsCopy(src->timestamps))) - goto error; + !(def->timestamps = virStorageTimestampsCopy(src->timestamps))) + return NULL; - if (virStorageSourceSeclabelsCopy(ret, src) < 0) - goto error; + if (virStorageSourceSeclabelsCopy(def, src) < 0) + return NULL; if (src->auth && - !(ret->auth = virStorageAuthDefCopy(src->auth))) - goto error; + !(def->auth = virStorageAuthDefCopy(src->auth))) + return NULL; if (src->pr && - !(ret->pr = virStoragePRDefCopy(src->pr))) - goto error; + !(def->pr = virStoragePRDefCopy(src->pr))) + return NULL; - if (virStorageSourceInitiatorCopy(&ret->initiator, &src->initiator)) - goto error; + if (virStorageSourceInitiatorCopy(&def->initiator, &src->initiator)) + return NULL; if (backingChain && src->backingStore) { - if (!(ret->backingStore = virStorageSourceCopy(src->backingStore, + if (!(def->backingStore = virStorageSourceCopy(src->backingStore, true))) - goto error; + return NULL; } + VIR_STEAL_PTR(ret, def); return ret; - - error: - virStorageSourceFree(ret); - return NULL; } @@ -2578,55 +2571,51 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, const char *rel) { VIR_AUTOFREE(char *) dirname = NULL; - virStorageSourcePtr ret; + VIR_AUTOPTR(virStorageSource) def = NULL; + virStorageSourcePtr ret = NULL; - if (VIR_ALLOC(ret) < 0) + if (VIR_ALLOC(def) < 0) return NULL; /* store relative name */ - if (VIR_STRDUP(ret->relPath, parent->backingStoreRaw) < 0) - goto error; + if (VIR_STRDUP(def->relPath, parent->backingStoreRaw) < 0) + return NULL; if (!(dirname = mdir_name(parent->path))) { virReportOOMError(); - goto error; + return NULL; } if (STRNEQ(dirname, "/")) { - if (virAsprintf(&ret->path, "%s/%s", dirname, rel) < 0) - goto error; + if (virAsprintf(&def->path, "%s/%s", dirname, rel) < 0) + return NULL; } else { - if (virAsprintf(&ret->path, "/%s", rel) < 0) - goto error; + if (virAsprintf(&def->path, "/%s", rel) < 0) + return NULL; } if (virStorageSourceGetActualType(parent) == VIR_STORAGE_TYPE_NETWORK) { - ret->type = VIR_STORAGE_TYPE_NETWORK; + def->type = VIR_STORAGE_TYPE_NETWORK; /* copy the host network part */ - ret->protocol = parent->protocol; + def->protocol = parent->protocol; if (parent->nhosts) { - if (!(ret->hosts = virStorageNetHostDefCopy(parent->nhosts, + if (!(def->hosts = virStorageNetHostDefCopy(parent->nhosts, parent->hosts))) - goto error; + return NULL; - ret->nhosts = parent->nhosts; + def->nhosts = parent->nhosts; } - if (VIR_STRDUP(ret->volume, parent->volume) < 0) - goto error; + if (VIR_STRDUP(def->volume, parent->volume) < 0) + return NULL; } else { /* set the type to _FILE, the caller shall update it to the actual type */ - ret->type = VIR_STORAGE_TYPE_FILE; + def->type = VIR_STORAGE_TYPE_FILE; } - cleanup: + VIR_STEAL_PTR(ret, def); return ret; - - error: - virStorageSourceFree(ret); - ret = NULL; - goto cleanup; } @@ -3619,49 +3608,47 @@ virStorageSourcePtr virStorageSourceNewFromBackingAbsolute(const char *path) { const char *json; - virStorageSourcePtr ret; + VIR_AUTOPTR(virStorageSource) def = NULL; + virStorageSourcePtr ret = NULL; int rc; - if (VIR_ALLOC(ret) < 0) + if (VIR_ALLOC(def) < 0) return NULL; if (virStorageIsFile(path)) { - ret->type = VIR_STORAGE_TYPE_FILE; + def->type = VIR_STORAGE_TYPE_FILE; - if (VIR_STRDUP(ret->path, path) < 0) - goto error; + if (VIR_STRDUP(def->path, path) < 0) + return NULL; } else { - ret->type = VIR_STORAGE_TYPE_NETWORK; + def->type = VIR_STORAGE_TYPE_NETWORK; VIR_DEBUG("parsing backing store string: '%s'", path); /* handle URI formatted backing stores */ if ((json = STRSKIP(path, "json:"))) - rc = virStorageSourceParseBackingJSON(ret, json); + rc = virStorageSourceParseBackingJSON(def, json); else if (strstr(path, "://")) - rc = virStorageSourceParseBackingURI(ret, path); + rc = virStorageSourceParseBackingURI(def, path); else - rc = virStorageSourceParseBackingColon(ret, path); + rc = virStorageSourceParseBackingColon(def, path); if (rc < 0) - goto error; + return NULL; - virStorageSourceNetworkAssignDefaultPorts(ret); + virStorageSourceNetworkAssignDefaultPorts(def); /* Some of the legacy parsers parse authentication data since they are * also used in other places. For backing store detection the * authentication data would be invalid anyways, so we clear it */ - if (ret->auth) { - virStorageAuthDefFree(ret->auth); - ret->auth = NULL; + if (def->auth) { + virStorageAuthDefFree(def->auth); + def->auth = NULL; } } + VIR_STEAL_PTR(ret, def); return ret; - - error: - virStorageSourceFree(ret); - return NULL; } @@ -3669,40 +3656,38 @@ virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent) { struct stat st; - virStorageSourcePtr ret; + VIR_AUTOPTR(virStorageSource) def = NULL; + virStorageSourcePtr ret = NULL; if (virStorageIsRelative(parent->backingStoreRaw)) - ret = virStorageSourceNewFromBackingRelative(parent, + def = virStorageSourceNewFromBackingRelative(parent, parent->backingStoreRaw); else - ret = virStorageSourceNewFromBackingAbsolute(parent->backingStoreRaw); + def = virStorageSourceNewFromBackingAbsolute(parent->backingStoreRaw); - if (ret) { + if (def) { /* possibly update local type */ - if (ret->type == VIR_STORAGE_TYPE_FILE) { - if (stat(ret->path, &st) == 0) { + if (def->type == VIR_STORAGE_TYPE_FILE) { + if (stat(def->path, &st) == 0) { if (S_ISDIR(st.st_mode)) { - ret->type = VIR_STORAGE_TYPE_DIR; - ret->format = VIR_STORAGE_FILE_DIR; + def->type = VIR_STORAGE_TYPE_DIR; + def->format = VIR_STORAGE_FILE_DIR; } else if (S_ISBLK(st.st_mode)) { - ret->type = VIR_STORAGE_TYPE_BLOCK; + def->type = VIR_STORAGE_TYPE_BLOCK; } } } /* copy parent's labelling and other top level stuff */ - if (virStorageSourceInitChainElement(ret, parent, true) < 0) - goto error; + if (virStorageSourceInitChainElement(def, parent, true) < 0) + return NULL; - ret->readonly = true; - ret->detected = true; + def->readonly = true; + def->detected = true; } + VIR_STEAL_PTR(ret, def); return ret; - - error: - virStorageSourceFree(ret); - return NULL; } @@ -3843,8 +3828,7 @@ virStorageSourceUpdateCapacity(virStorageSourcePtr src, ssize_t len, bool probe) { - int ret = -1; - virStorageSourcePtr meta = NULL; + VIR_AUTOPTR(virStorageSource) meta = NULL; int format = src->format; /* Raw files: capacity is physical size. For all other files: if @@ -3855,12 +3839,12 @@ virStorageSourceUpdateCapacity(virStorageSourcePtr src, virReportError(VIR_ERR_INTERNAL_ERROR, _("no disk format for %s and probing is disabled"), src->path); - goto cleanup; + return -1; } if ((format = virStorageFileProbeFormatFromBuf(src->path, buf, len)) < 0) - goto cleanup; + return -1; src->format = format; } @@ -3873,17 +3857,13 @@ virStorageSourceUpdateCapacity(virStorageSourcePtr src, if (src->encryption && meta->encryption) src->encryption->payload_offset = meta->encryption->payload_offset; } else { - goto cleanup; + return -1; } if (src->encryption && src->encryption->payload_offset != -1) src->capacity -= src->encryption->payload_offset * 512; - ret = 0; - - cleanup: - virStorageSourceFree(meta); - return ret; + return 0; } @@ -4821,8 +4801,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, int ret = -1; const char *uniqueName; VIR_AUTOFREE(char *) buf = NULL; + VIR_AUTOPTR(virStorageSource) backingStore = NULL; ssize_t headerLen; - virStorageSourcePtr backingStore = NULL; int backingFormat; int rv; @@ -4900,15 +4880,13 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, goto cleanup; } - src->backingStore = backingStore; - backingStore = NULL; + VIR_STEAL_PTR(src->backingStore, backingStore); ret = 0; cleanup: if (virStorageSourceHasBacking(src)) src->backingStore->id = depth; virStorageFileDeinit(src); - virStorageSourceFree(backingStore); return ret; } @@ -4977,10 +4955,9 @@ int virStorageFileGetBackingStoreStr(virStorageSourcePtr src, char **backing) { - virStorageSourcePtr tmp = NULL; + VIR_AUTOPTR(virStorageSource) tmp = NULL; VIR_AUTOFREE(char *) buf = NULL; ssize_t headerLen; - int ret = -1; int rv; *backing = NULL; @@ -5005,17 +4982,12 @@ virStorageFileGetBackingStoreStr(virStorageSourcePtr src, } if (!(tmp = virStorageSourceCopy(src, false))) - goto cleanup; + return -1; if (virStorageFileGetMetadataInternal(tmp, buf, headerLen, NULL) < 0) - goto cleanup; + return -1; VIR_STEAL_PTR(*backing, tmp->backingStoreRaw); - ret = 0; - - cleanup: - virStorageSourceFree(tmp); - - return ret; + return 0; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index f1164dde9b..6da8c2fdf9 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -544,4 +544,5 @@ void virStorageFileReportBrokenChain(int errcode, virStorageSourcePtr parent); VIR_DEFINE_AUTOPTR_FUNC(virStorageAuthDef, virStorageAuthDefFree); +VIR_DEFINE_AUTOPTR_FUNC(virStorageSource, virStorageSourceFree); #endif /* LIBVIRT_VIRSTORAGEFILE_H */ diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 5848f6b5b5..0171289dd9 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -46,8 +46,8 @@ testBackingXMLjsonXML(const void *args) xmlDocPtr xml = NULL; xmlXPathContextPtr ctxt = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - virStorageSourcePtr xmlsrc = NULL; - virStorageSourcePtr jsonsrc = NULL; + VIR_AUTOPTR(virStorageSource) xmlsrc = NULL; + VIR_AUTOPTR(virStorageSource) jsonsrc = NULL; virJSONValuePtr backendprops = NULL; virJSONValuePtr wrapper = NULL; char *propsstr = NULL; @@ -104,8 +104,6 @@ testBackingXMLjsonXML(const void *args) ret = 0; cleanup: - virStorageSourceFree(xmlsrc); - virStorageSourceFree(jsonsrc); VIR_FREE(propsstr); VIR_FREE(protocolwrapper); VIR_FREE(actualxml); diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 71c371891f..90a6dce50a 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -95,33 +95,31 @@ testStorageFileGetMetadata(const char *path, uid_t uid, gid_t gid) { struct stat st; + VIR_AUTOPTR(virStorageSource) def = NULL; virStorageSourcePtr ret = NULL; - if (VIR_ALLOC(ret) < 0) + if (VIR_ALLOC(def) < 0) return NULL; - ret->type = VIR_STORAGE_TYPE_FILE; - ret->format = format; + def->type = VIR_STORAGE_TYPE_FILE; + def->format = format; if (stat(path, &st) == 0) { if (S_ISDIR(st.st_mode)) { - ret->type = VIR_STORAGE_TYPE_DIR; + def->type = VIR_STORAGE_TYPE_DIR; } else if (S_ISBLK(st.st_mode)) { - ret->type = VIR_STORAGE_TYPE_BLOCK; + def->type = VIR_STORAGE_TYPE_BLOCK; } } - if (VIR_STRDUP(ret->path, path) < 0) - goto error; + if (VIR_STRDUP(def->path, path) < 0) + return NULL; - if (virStorageFileGetMetadata(ret, uid, gid, false) < 0) - goto error; + if (virStorageFileGetMetadata(def, uid, gid, false) < 0) + return NULL; + VIR_STEAL_PTR(ret, def); return ret; - - error: - virStorageSourceFree(ret); - return NULL; } static int @@ -308,41 +306,40 @@ static int testStorageChain(const void *args) { const struct testChainData *data = args; - int ret = -1; - virStorageSourcePtr meta; virStorageSourcePtr elt; size_t i = 0; + VIR_AUTOPTR(virStorageSource) meta = NULL; VIR_AUTOFREE(char *) broken = NULL; meta = testStorageFileGetMetadata(data->start, data->format, -1, -1); if (!meta) { if (data->flags & EXP_FAIL) { virResetLastError(); - ret = 0; + return 0; } - goto cleanup; + return -1; } else if (data->flags & EXP_FAIL) { fprintf(stderr, "call should have failed\n"); - goto cleanup; + return -1; } if (data->flags & EXP_WARN) { if (virGetLastErrorCode() == VIR_ERR_OK) { fprintf(stderr, "call should have warned\n"); - goto cleanup; + return -1; } virResetLastError(); if (virStorageFileChainGetBroken(meta, &broken) || !broken) { fprintf(stderr, "call should identify broken part of chain\n"); - goto cleanup; + return -1; } } else { if (virGetLastErrorCode()) { fprintf(stderr, "call should not have warned\n"); - goto cleanup; + return -1; } if (virStorageFileChainGetBroken(meta, &broken) || broken) { fprintf(stderr, "chain should not be identified as broken\n"); - goto cleanup; + return -1; } } @@ -353,7 +350,7 @@ testStorageChain(const void *args) if (i == data->nfiles) { fprintf(stderr, "probed chain was too long\n"); - goto cleanup; + return -1; } if (virAsprintf(&expect, @@ -378,24 +375,21 @@ testStorageChain(const void *args) elt->format, virStorageNetProtocolTypeToString(elt->protocol), NULLSTR(elt->nhosts ? elt->hosts[0].name : NULL)) < 0) { - goto cleanup; + return -1; } if (STRNEQ(expect, actual)) { virTestDifference(stderr, expect, actual); - goto cleanup; + return -1; } elt = elt->backingStore; i++; } if (i != data->nfiles) { fprintf(stderr, "probed chain was too short\n"); - goto cleanup; + return -1; } - ret = 0; - cleanup: - virStorageSourceFree(meta); - return ret; + return 0; } struct testLookupData @@ -646,7 +640,7 @@ testBackingParse(const void *args) { const struct testBackingParseData *data = args; virBuffer buf = VIR_BUFFER_INITIALIZER; - virStorageSourcePtr src = NULL; + VIR_AUTOPTR(virStorageSource) src = NULL; VIR_AUTOFREE(char *) xml = NULL; int ret = -1; @@ -680,7 +674,6 @@ testBackingParse(const void *args) ret = 0; cleanup: - virStorageSourceFree(src); virBufferFreeAndReset(&buf); return ret; @@ -697,7 +690,7 @@ mymain(void) struct testPathCanonicalizeData data3; struct testPathRelativeBacking data4; struct testBackingParseData data5; - virStorageSourcePtr chain = NULL; + VIR_AUTOPTR(virStorageSource) chain = NULL; virStorageSourcePtr chain2; /* short for chain->backingStore */ virStorageSourcePtr chain3; /* short for chain2->backingStore */ @@ -1580,7 +1573,6 @@ mymain(void) cleanup: /* Final cleanup */ - virStorageSourceFree(chain); testCleanupImages(); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; -- 2.20.1

On 2/8/19 3:07 AM, Erik Skultety wrote: Strange... Although I do have a recollection of seeing that inline error message though. Still I'll rework this one to extract out some stuff and repost as a v2 once I walk through all the changes I have made to date from code review to ensure I've covered all the comments raised. Thanks - John