[PATCH] util: convert char pointers to use g_autofree
From: Barrett Schonefeld <bschoney@utexas.edu> additional conversions to the GLib API in src/util per issue #11. Related issue: https://gitlab.com/libvirt/libvirt/-/issues/11 Signed-off-by: Barrett Schonefeld <bschoney@utexas.edu> --- src/util/vircgroupv1.c | 3 +- src/util/virdnsmasq.c | 43 ++++++++------------- src/util/virfile.c | 9 ++--- src/util/virhostcpu.c | 4 +- src/util/virlockspace.c | 6 +-- src/util/virlog.c | 12 ++---- src/util/virmacmap.c | 3 +- src/util/virnetdevbandwidth.c | 48 ++++++++--------------- src/util/virresctrl.c | 25 ++++-------- src/util/virrotatingfile.c | 11 ++---- src/util/virscsihost.c | 25 +++++------- src/util/virsecret.c | 14 +++---- src/util/virstorageencryption.c | 19 +++------ src/util/virstoragefilebackend.c | 4 +- src/util/virsysinfo.c | 18 +++------ src/util/viruri.c | 7 +--- src/util/virutil.c | 66 +++++++++++--------------------- src/util/virvhba.c | 57 ++++++++++----------------- src/util/virxml.c | 7 +--- 19 files changed, 130 insertions(+), 251 deletions(-) diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index 731e9d61d4..984cd50409 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -1549,7 +1549,7 @@ virCgroupV1GetMemoryStat(virCgroupPtr group, unsigned long long *unevictable) { int ret = -1; - char *stat = NULL; + g_autofree char *stat = NULL; char *line = NULL; unsigned long long cacheVal = 0; unsigned long long activeAnonVal = 0; @@ -1614,7 +1614,6 @@ virCgroupV1GetMemoryStat(virCgroupPtr group, ret = 0; cleanup: - VIR_FREE(stat); return ret; } diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 9030f9218a..93bc4a129f 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -164,7 +164,7 @@ addnhostsWrite(const char *path, dnsmasqAddnHost *hosts, unsigned int nhosts) { - char *tmp; + g_autofree char *tmp = NULL; FILE *f; bool istmp = true; size_t i, j; @@ -180,7 +180,7 @@ addnhostsWrite(const char *path, istmp = false; if (!(f = fopen(path, "w"))) { rc = -errno; - goto cleanup; + return rc; } } @@ -192,7 +192,7 @@ addnhostsWrite(const char *path, if (istmp) unlink(tmp); - goto cleanup; + return rc; } for (j = 0; j < hosts[i].nhostnames; j++) { @@ -203,7 +203,7 @@ addnhostsWrite(const char *path, if (istmp) unlink(tmp); - goto cleanup; + return rc; } } @@ -214,24 +214,21 @@ addnhostsWrite(const char *path, if (istmp) unlink(tmp); - goto cleanup; + return rc; } } if (VIR_FCLOSE(f) == EOF) { rc = -errno; - goto cleanup; + return rc; } if (istmp && rename(tmp, path) < 0) { rc = -errno; unlink(tmp); - goto cleanup; + return rc; } - cleanup: - VIR_FREE(tmp); - return rc; } @@ -364,7 +361,7 @@ hostsfileWrite(const char *path, dnsmasqDhcpHost *hosts, unsigned int nhosts) { - char *tmp; + g_autofree char *tmp = NULL; FILE *f; bool istmp = true; size_t i; @@ -380,7 +377,7 @@ hostsfileWrite(const char *path, istmp = false; if (!(f = fopen(path, "w"))) { rc = -errno; - goto cleanup; + return rc; } } @@ -392,24 +389,21 @@ hostsfileWrite(const char *path, if (istmp) unlink(tmp); - goto cleanup; + return rc; } } if (VIR_FCLOSE(f) == EOF) { rc = -errno; - goto cleanup; + return rc; } if (istmp && rename(tmp, path) < 0) { rc = -errno; unlink(tmp); - goto cleanup; + return rc; } - cleanup: - VIR_FREE(tmp); - return rc; } @@ -686,15 +680,13 @@ static int dnsmasqCapsSetFromFile(dnsmasqCapsPtr caps, const char *path) { int ret = -1; - char *buf = NULL; + g_autofree char *buf = NULL; if (virFileReadAll(path, 1024 * 1024, &buf) < 0) - goto cleanup; + return ret; ret = dnsmasqCapsSetFromBuffer(caps, buf); - cleanup: - VIR_FREE(buf); return ret; } @@ -704,7 +696,9 @@ dnsmasqCapsRefreshInternal(dnsmasqCapsPtr caps, bool force) int ret = -1; struct stat sb; virCommandPtr cmd = NULL; - char *help = NULL, *version = NULL, *complete = NULL; + g_autofree char *help = NULL; + g_autofree char *version = NULL; + g_autofree char *complete = NULL; if (!caps || caps->noRefresh) return 0; @@ -749,9 +743,6 @@ dnsmasqCapsRefreshInternal(dnsmasqCapsPtr caps, bool force) cleanup: virCommandFree(cmd); - VIR_FREE(help); - VIR_FREE(version); - VIR_FREE(complete); return ret; } diff --git a/src/util/virfile.c b/src/util/virfile.c index f57272ca2f..38207f1948 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3328,9 +3328,9 @@ virFileIsSharedFixFUSE(const char *path, FILE *f = NULL; struct mntent mb; char mntbuf[1024]; - char *mntDir = NULL; - char *mntType = NULL; - char *canonPath = NULL; + g_autofree char *mntDir = NULL; + g_autofree char *mntType = NULL; + g_autofree char *canonPath = NULL; size_t maxMatching = 0; int ret = -1; @@ -3381,9 +3381,6 @@ virFileIsSharedFixFUSE(const char *path, ret = 0; cleanup: - VIR_FREE(canonPath); - VIR_FREE(mntType); - VIR_FREE(mntDir); endmntent(f); return ret; } diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index c531d65f86..4f6c3390ce 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -87,7 +87,7 @@ virHostCPUGetStatsFreeBSD(int cpuNum, int *nparams) { const char *sysctl_name; - long *cpu_times; + g_autofree long *cpu_times = NULL; struct clockinfo clkinfo; size_t i, j, cpu_times_size, clkinfo_size; int cpu_times_num, offset, hz, stathz, ret = -1; @@ -172,8 +172,6 @@ virHostCPUGetStatsFreeBSD(int cpuNum, ret = 0; cleanup: - VIR_FREE(cpu_times); - return ret; } diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c index b90e13f506..71d5dfb83e 100644 --- a/src/util/virlockspace.c +++ b/src/util/virlockspace.c @@ -515,7 +515,7 @@ int virLockSpaceCreateResource(virLockSpacePtr lockspace, const char *resname) { int ret = -1; - char *respath = NULL; + g_autofree char *respath = NULL; VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname); @@ -538,7 +538,6 @@ int virLockSpaceCreateResource(virLockSpacePtr lockspace, cleanup: virMutexUnlock(&lockspace->lock); - VIR_FREE(respath); return ret; } @@ -547,7 +546,7 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace, const char *resname) { int ret = -1; - char *respath = NULL; + g_autofree char *respath = NULL; VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname); @@ -575,7 +574,6 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace, cleanup: virMutexUnlock(&lockspace->lock); - VIR_FREE(respath); return ret; } diff --git a/src/util/virlog.c b/src/util/virlog.c index 6b7a4512e9..2bf606b8c5 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -505,8 +505,8 @@ virLogVMessage(virLogSourcePtr source, va_list vargs) { static bool logInitMessageStderr = true; - char *str = NULL; - char *msg = NULL; + g_autofree char *str = NULL; + g_autofree char *msg = NULL; char timestamp[VIR_TIME_STRING_BUFLEN]; size_t i; int saved_errno = errno; @@ -528,7 +528,8 @@ virLogVMessage(virLogSourcePtr source, if (source->serial < virLogFiltersSerial) virLogSourceUpdate(source); if (priority < source->priority) - goto cleanup; + errno = saved_errno; + return; /* * serialize the error message, add level and timestamp @@ -601,11 +602,6 @@ virLogVMessage(virLogSourcePtr source, str, msg, (void *) STDERR_FILENO); } virLogUnlock(); - - cleanup: - VIR_FREE(str); - VIR_FREE(msg); - errno = saved_errno; } diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c index f9047d0fb1..e68742de10 100644 --- a/src/util/virmacmap.c +++ b/src/util/virmacmap.c @@ -129,7 +129,7 @@ static int virMacMapLoadFile(virMacMapPtr mgr, const char *file) { - char *map_str = NULL; + g_autofree char *map_str = NULL; virJSONValuePtr map = NULL; int map_str_len = 0; size_t i; @@ -189,7 +189,6 @@ virMacMapLoadFile(virMacMapPtr mgr, ret = 0; cleanup: - VIR_FREE(map_str); virJSONValueFree(map); return ret; } diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index c8eb5cfc79..2ae03e8edc 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -102,7 +102,7 @@ virNetDevBandwidthManipulateFilter(const char *ifname, bool create_new) { int ret = -1; - char *filter_id = NULL; + g_autofree char *filter_id = NULL; virCommandPtr cmd = NULL; unsigned char ifmac[VIR_MAC_BUFLEN]; char *mac[2] = {NULL, NULL}; @@ -157,7 +157,6 @@ virNetDevBandwidthManipulateFilter(const char *ifname, cleanup: VIR_FREE(mac[1]); VIR_FREE(mac[0]); - VIR_FREE(filter_id); virCommandFree(cmd); return ret; } @@ -195,9 +194,9 @@ virNetDevBandwidthSet(const char *ifname, int ret = -1; virNetDevBandwidthRatePtr rx = NULL, tx = NULL; /* From domain POV */ virCommandPtr cmd = NULL; - char *average = NULL; - char *peak = NULL; - char *burst = NULL; + g_autofree char *average = NULL; + g_autofree char *peak = NULL; + g_autofree char *burst = NULL; if (!bandwidth) { /* nothing to be enabled */ @@ -385,9 +384,6 @@ virNetDevBandwidthSet(const char *ifname, cleanup: virCommandFree(cmd); - VIR_FREE(average); - VIR_FREE(peak); - VIR_FREE(burst); return ret; } @@ -533,10 +529,10 @@ virNetDevBandwidthPlug(const char *brname, { int ret = -1; virCommandPtr cmd = NULL; - char *class_id = NULL; - char *qdisc_id = NULL; - char *floor = NULL; - char *ceil = NULL; + g_autofree char *class_id = NULL; + g_autofree char *qdisc_id = NULL; + g_autofree char *floor = NULL; + g_autofree char *ceil = NULL; char ifmacStr[VIR_MAC_STRING_BUFLEN]; if (id <= 2) { @@ -586,10 +582,6 @@ virNetDevBandwidthPlug(const char *brname, ret = 0; cleanup: - VIR_FREE(ceil); - VIR_FREE(floor); - VIR_FREE(qdisc_id); - VIR_FREE(class_id); virCommandFree(cmd); return ret; } @@ -610,8 +602,8 @@ virNetDevBandwidthUnplug(const char *brname, int ret = -1; int cmd_ret = 0; virCommandPtr cmd = NULL; - char *class_id = NULL; - char *qdisc_id = NULL; + g_autofree char *class_id = NULL; + g_autofree char *qdisc_id = NULL; if (id <= 2) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid class ID %d"), id); @@ -645,8 +637,6 @@ virNetDevBandwidthUnplug(const char *brname, ret = 0; cleanup: - VIR_FREE(qdisc_id); - VIR_FREE(class_id); virCommandFree(cmd); return ret; } @@ -673,9 +663,9 @@ virNetDevBandwidthUpdateRate(const char *ifname, { int ret = -1; virCommandPtr cmd = NULL; - char *class_id = NULL; - char *rate = NULL; - char *ceil = NULL; + g_autofree char *class_id = NULL; + g_autofree char *rate = NULL; + g_autofree char *ceil = NULL; class_id = g_strdup_printf("1:%x", id); rate = g_strdup_printf("%llukbps", new_rate); @@ -696,9 +686,6 @@ virNetDevBandwidthUpdateRate(const char *ifname, cleanup: virCommandFree(cmd); - VIR_FREE(class_id); - VIR_FREE(rate); - VIR_FREE(ceil); return ret; } @@ -725,16 +712,13 @@ virNetDevBandwidthUpdateFilter(const char *ifname, unsigned int id) { int ret = -1; - char *class_id = NULL; + g_autofree char *class_id = NULL; class_id = g_strdup_printf("1:%x", id); if (virNetDevBandwidthManipulateFilter(ifname, ifmac_ptr, id, class_id, true, true) < 0) - goto cleanup; + return ret; - ret = 0; - cleanup: - VIR_FREE(class_id); - return ret; + return 0; } diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index d3087b98c1..1c2d175295 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -709,7 +709,7 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) { int ret = -1; int rv = -1; - char *featurestr = NULL; + g_autofree char *featurestr = NULL; char **features = NULL; size_t nfeatures = 0; virResctrlInfoMongrpPtr info_monitor = NULL; @@ -771,7 +771,6 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) ret = 0; cleanup: - VIR_FREE(featurestr); g_strfreev(features); VIR_FREE(info_monitor); return ret; @@ -1736,7 +1735,7 @@ virResctrlAllocGetGroup(virResctrlInfoPtr resctrl, const char *groupname, virResctrlAllocPtr *alloc) { - char *schemata = NULL; + g_autofree char *schemata = NULL; int rv = virFileReadValueString(&schemata, SYSFS_RESCTRL_PATH "/%s/schemata", groupname); @@ -1753,11 +1752,9 @@ virResctrlAllocGetGroup(virResctrlInfoPtr resctrl, if (virResctrlAllocParse(resctrl, *alloc, schemata) < 0) goto error; - VIR_FREE(schemata); return 0; error: - VIR_FREE(schemata); virObjectUnref(*alloc); *alloc = NULL; return -1; @@ -2354,8 +2351,8 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, virResctrlAllocPtr alloc, const char *machinename) { - char *schemata_path = NULL; - char *alloc_str = NULL; + g_autofree char *schemata_path = NULL; + g_autofree char *alloc_str = NULL; int ret = -1; int lockfd = -1; @@ -2403,8 +2400,6 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, ret = 0; cleanup: virResctrlUnlock(lockfd); - VIR_FREE(alloc_str); - VIR_FREE(schemata_path); return ret; } @@ -2413,8 +2408,8 @@ static int virResctrlAddPID(const char *path, pid_t pid) { - char *tasks = NULL; - char *pidstr = NULL; + g_autofree char *tasks = NULL; + g_autofree char *pidstr = NULL; int ret = 0; if (!path) { @@ -2436,8 +2431,6 @@ virResctrlAddPID(const char *path, ret = 0; cleanup: - VIR_FREE(tasks); - VIR_FREE(pidstr); return ret; } @@ -2657,8 +2650,8 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, size_t i = 0; unsigned long long val = 0; g_autoptr(DIR) dirp = NULL; - char *datapath = NULL; - char *filepath = NULL; + g_autofree char *datapath = NULL; + g_autofree char *filepath = NULL; struct dirent *ent = NULL; virResctrlMonitorStatsPtr stat = NULL; @@ -2737,8 +2730,6 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, ret = 0; cleanup: - VIR_FREE(datapath); - VIR_FREE(filepath); virResctrlMonitorStatsFree(stat); return ret; } diff --git a/src/util/virrotatingfile.c b/src/util/virrotatingfile.c index 9f1ef17c3e..6fe5fa3154 100644 --- a/src/util/virrotatingfile.c +++ b/src/util/virrotatingfile.c @@ -362,8 +362,8 @@ static int virRotatingFileWriterRollover(virRotatingFileWriterPtr file) { size_t i; - char *nextpath = NULL; - char *thispath = NULL; + g_autofree char *nextpath = NULL; + g_autofree char *thispath = NULL; int ret = -1; VIR_DEBUG("Rollover %s", file->basepath); @@ -373,7 +373,7 @@ virRotatingFileWriterRollover(virRotatingFileWriterPtr file) virReportSystemError(errno, _("Unable to remove %s"), file->basepath); - goto cleanup; + return ret; } } else { nextpath = g_strdup_printf("%s.%zu", file->basepath, file->maxbackup - 1); @@ -391,7 +391,7 @@ virRotatingFileWriterRollover(virRotatingFileWriterPtr file) virReportSystemError(errno, _("Unable to rename %s to %s"), thispath, nextpath); - goto cleanup; + return ret; } VIR_FREE(nextpath); @@ -402,9 +402,6 @@ virRotatingFileWriterRollover(virRotatingFileWriterPtr file) VIR_DEBUG("Rollover done %s", file->basepath); ret = 0; - cleanup: - VIR_FREE(nextpath); - VIR_FREE(thispath); return ret; } diff --git a/src/util/virscsihost.c b/src/util/virscsihost.c index 969cdd9f79..cad48b4059 100644 --- a/src/util/virscsihost.c +++ b/src/util/virscsihost.c @@ -95,12 +95,12 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_HOST_PATH; struct dirent *entry = NULL; g_autoptr(DIR) dir = NULL; - char *host_link = NULL; - char *host_path = NULL; + g_autofree char *host_link = NULL; + g_autofree char *host_path = NULL; char *p = NULL; char *ret = NULL; - char *buf = NULL; - char *unique_path = NULL; + g_autofree char *buf = NULL; + g_autofree char *unique_path = NULL; unsigned int read_unique_id; if (virDirOpen(&dir, prefix) < 0) @@ -113,7 +113,7 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, host_link = g_strdup_printf("%s/%s", prefix, entry->d_name); if (virFileResolveLink(host_link, &host_path) < 0) - goto cleanup; + return ret; if (!strstr(host_path, parentaddr)) { VIR_FREE(host_link); @@ -131,13 +131,13 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, } if (virFileReadAll(unique_path, 1024, &buf) < 0) - goto cleanup; + return ret; if ((p = strchr(buf, '\n'))) *p = '\0'; if (virStrToLong_ui(buf, NULL, 10, &read_unique_id) < 0) - goto cleanup; + return ret; VIR_FREE(buf); @@ -150,11 +150,6 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, break; } - cleanup: - VIR_FREE(unique_path); - VIR_FREE(host_link); - VIR_FREE(host_path); - VIR_FREE(buf); return ret; } @@ -226,7 +221,7 @@ virSCSIHostGetNameByParentaddr(unsigned int domain, unsigned int unique_id) { char *name = NULL; - char *parentaddr = NULL; + g_autofree char *parentaddr = NULL; parentaddr = g_strdup_printf("%04x:%02x:%02x.%01x", domain, bus, slot, function); @@ -235,11 +230,9 @@ virSCSIHostGetNameByParentaddr(unsigned int domain, _("Failed to find scsi_host using PCI '%s' " "and unique_id='%u'"), parentaddr, unique_id); - goto cleanup; + return name; } - cleanup: - VIR_FREE(parentaddr); return name; } diff --git a/src/util/virsecret.c b/src/util/virsecret.c index 54d6bbcb7c..67c9b68215 100644 --- a/src/util/virsecret.c +++ b/src/util/virsecret.c @@ -65,8 +65,8 @@ int virSecretLookupParseSecret(xmlNodePtr secretnode, virSecretLookupTypeDefPtr def) { - char *uuid; - char *usage; + g_autofree char *uuid = NULL; + g_autofree char *usage = NULL; int ret = -1; uuid = virXMLPropString(secretnode, "uuid"); @@ -74,20 +74,20 @@ virSecretLookupParseSecret(xmlNodePtr secretnode, if (uuid == NULL && usage == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing secret uuid or usage attribute")); - goto cleanup; + return ret; } if (uuid && usage) { virReportError(VIR_ERR_XML_ERROR, "%s", _("either secret uuid or usage expected")); - goto cleanup; + return ret; } if (uuid) { if (virUUIDParse(uuid, def->u.uuid) < 0) { virReportError(VIR_ERR_XML_ERROR, _("invalid secret uuid '%s'"), uuid); - goto cleanup; + return ret; } def->type = VIR_SECRET_LOOKUP_TYPE_UUID; } else { @@ -96,10 +96,6 @@ virSecretLookupParseSecret(xmlNodePtr secretnode, def->type = VIR_SECRET_LOOKUP_TYPE_USAGE; } ret = 0; - - cleanup: - VIR_FREE(uuid); - VIR_FREE(usage); return ret; } diff --git a/src/util/virstorageencryption.c b/src/util/virstorageencryption.c index 399c6926bd..db886c1e76 100644 --- a/src/util/virstorageencryption.c +++ b/src/util/virstorageencryption.c @@ -142,7 +142,7 @@ virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt, { VIR_XPATH_NODE_AUTORESTORE(ctxt) virStorageEncryptionSecretPtr ret; - char *type_str = NULL; + g_autofree char *type_str = NULL; ret = g_new0(virStorageEncryptionSecret, 1); @@ -164,12 +164,9 @@ virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt, if (virSecretLookupParseSecret(node, &ret->seclookupdef) < 0) goto cleanup; - VIR_FREE(type_str); - return ret; cleanup: - VIR_FREE(type_str); virStorageEncryptionSecretFree(ret); return NULL; } @@ -180,12 +177,12 @@ virStorageEncryptionInfoParseCipher(xmlNodePtr info_node, virStorageEncryptionInfoDefPtr info) { int ret = -1; - char *size_str = NULL; + g_autofree char *size_str = NULL; if (!(info->cipher_name = virXMLPropString(info_node, "name"))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("cipher info missing 'name' attribute")); - goto cleanup; + return ret; } if ((size_str = virXMLPropString(info_node, "size")) && @@ -193,22 +190,19 @@ virStorageEncryptionInfoParseCipher(xmlNodePtr info_node, virReportError(VIR_ERR_XML_ERROR, _("cannot parse cipher size: '%s'"), size_str); - goto cleanup; + return ret; } if (!size_str) { virReportError(VIR_ERR_XML_ERROR, "%s", _("cipher info missing 'size' attribute")); - goto cleanup; + return ret; } info->cipher_mode = virXMLPropString(info_node, "mode"); info->cipher_hash = virXMLPropString(info_node, "hash"); ret = 0; - - cleanup: - VIR_FREE(size_str); return ret; } @@ -237,7 +231,7 @@ virStorageEncryptionParseNode(xmlNodePtr node, xmlNodePtr *nodes = NULL; virStorageEncryptionPtr encdef = NULL; virStorageEncryptionPtr ret = NULL; - char *format_str = NULL; + g_autofree char *format_str = NULL; int n; size_t i; @@ -297,7 +291,6 @@ virStorageEncryptionParseNode(xmlNodePtr node, ret = g_steal_pointer(&encdef); cleanup: - VIR_FREE(format_str); VIR_FREE(nodes); virStorageEncryptionFree(encdef); diff --git a/src/util/virstoragefilebackend.c b/src/util/virstoragefilebackend.c index 2779b5c307..55c62b0212 100644 --- a/src/util/virstoragefilebackend.c +++ b/src/util/virstoragefilebackend.c @@ -51,7 +51,7 @@ virStorageFileLoadBackendModule(const char *name, const char *regfunc, bool forceload) { - char *modfile = NULL; + g_autofree char *modfile = NULL; int ret; if (!(modfile = virFileFindResourceFull(name, @@ -64,8 +64,6 @@ virStorageFileLoadBackendModule(const char *name, ret = virModuleLoad(modfile, regfunc, forceload); - VIR_FREE(modfile); - return ret; } #endif /* WITH_STORAGE_DIR || WITH_STORAGE_FS || WITH_STORAGE_GLUSTER */ diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index 217f842a37..45a950c85a 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -393,7 +393,7 @@ virSysinfoParseARMProcessor(const char *base, virSysinfoDefPtr ret) const char *cur; char *eol, *tmp_base; virSysinfoProcessorDefPtr processor; - char *processor_type = NULL; + g_autofree char *processor_type = NULL; if (!(tmp_base = strstr(base, "model name")) && !(tmp_base = strstr(base, "Processor"))) @@ -411,7 +411,7 @@ virSysinfoParseARMProcessor(const char *base, virSysinfoDefPtr ret) cur = strchr(base, ':') + 1; if (VIR_EXPAND_N(ret->processor, ret->nprocessor, 1) < 0) - goto error; + return -1; processor = &ret->processor[ret->nprocessor - 1]; virSkipSpaces(&cur); @@ -424,12 +424,7 @@ virSysinfoParseARMProcessor(const char *base, virSysinfoDefPtr ret) base = cur; } - VIR_FREE(processor_type); return 0; - - error: - VIR_FREE(processor_type); - return -1; } /* virSysinfoRead for ARMv7 @@ -532,9 +527,9 @@ static int virSysinfoParseS390Processor(const char *base, virSysinfoDefPtr ret) { const char *tmp_base; - char *manufacturer = NULL; - char *procline = NULL; - char *ncpu = NULL; + g_autofree char *manufacturer = NULL; + g_autofree char *procline = NULL; + g_autofree char *ncpu = NULL; int result = -1; virSysinfoProcessorDefPtr processor; @@ -593,9 +588,6 @@ virSysinfoParseS390Processor(const char *base, virSysinfoDefPtr ret) result = 0; error: - VIR_FREE(manufacturer); - VIR_FREE(procline); - VIR_FREE(ncpu); return result; } diff --git a/src/util/viruri.c b/src/util/viruri.c index 11753a0aef..d49821451e 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -209,7 +209,7 @@ char * virURIFormat(virURIPtr uri) { xmlURI xmluri; - char *tmpserver = NULL; + g_autofree char *tmpserver = NULL; char *ret; memset(&xmluri, 0, sizeof(xmluri)); @@ -241,12 +241,9 @@ virURIFormat(virURIPtr uri) ret = (char *)xmlSaveUri(&xmluri); if (!ret) { virReportOOMError(); - goto cleanup; + return ret; } - cleanup: - VIR_FREE(tmpserver); - return ret; } diff --git a/src/util/virutil.c b/src/util/virutil.c index a0cd0f1bcd..a7c163ab94 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -598,7 +598,7 @@ char *virGetUserRuntimeDirectory(void) static int virGetUserEnt(uid_t uid, char **name, gid_t *group, char **dir, char **shell, bool quiet) { - char *strbuf; + g_autofree char *strbuf = NULL; struct passwd pwbuf; struct passwd *pw = NULL; long val = sysconf(_SC_GETPW_R_SIZE_MAX); @@ -668,13 +668,12 @@ virGetUserEnt(uid_t uid, char **name, gid_t *group, char **dir, char **shell, bo if (shell) VIR_FREE(*shell); } - VIR_FREE(strbuf); return ret; } static char *virGetGroupEnt(gid_t gid) { - char *strbuf; + g_autofree char *strbuf = NULL; char *ret; struct group grbuf; struct group *gr = NULL; @@ -717,7 +716,6 @@ static char *virGetGroupEnt(gid_t gid) } ret = g_strdup(gr->gr_name); - VIR_FREE(strbuf); return ret; } @@ -759,7 +757,7 @@ char *virGetGroupName(gid_t gid) static int virGetUserIDByName(const char *name, uid_t *uid, bool missing_ok) { - char *strbuf = NULL; + g_autofree char *strbuf = NULL; struct passwd pwbuf; struct passwd *pw = NULL; long val = sysconf(_SC_GETPW_R_SIZE_MAX); @@ -775,7 +773,7 @@ virGetUserIDByName(const char *name, uid_t *uid, bool missing_ok) while ((rc = getpwnam_r(name, &pwbuf, strbuf, strbuflen, &pw)) == ERANGE) { if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) - goto cleanup; + return ret; } if (!pw) { @@ -788,16 +786,13 @@ virGetUserIDByName(const char *name, uid_t *uid, bool missing_ok) } ret = 1; - goto cleanup; + return ret; } if (uid) *uid = pw->pw_uid; ret = 0; - cleanup: - VIR_FREE(strbuf); - return ret; } @@ -840,7 +835,7 @@ virGetUserID(const char *user, uid_t *uid) static int virGetGroupIDByName(const char *name, gid_t *gid, bool missing_ok) { - char *strbuf = NULL; + g_autofree char *strbuf = NULL; struct group grbuf; struct group *gr = NULL; long val = sysconf(_SC_GETGR_R_SIZE_MAX); @@ -856,7 +851,7 @@ virGetGroupIDByName(const char *name, gid_t *gid, bool missing_ok) while ((rc = getgrnam_r(name, &grbuf, strbuf, strbuflen, &gr)) == ERANGE) { if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) - goto cleanup; + return ret; } if (!gr) { @@ -869,16 +864,13 @@ virGetGroupIDByName(const char *name, gid_t *gid, bool missing_ok) } ret = 1; - goto cleanup; + return ret; } if (gid) *gid = gr->gr_gid; ret = 0; - cleanup: - VIR_FREE(strbuf); - return ret; } @@ -949,7 +941,7 @@ int virGetGroupList(uid_t uid, gid_t gid, gid_t **list) { int ret = 0; - char *user = NULL; + g_autofree char *user = NULL; gid_t primary; *list = NULL; @@ -987,19 +979,17 @@ virGetGroupList(uid_t uid, gid_t gid, gid_t **list) for (i = 0; i < ret; i++) { if ((*list)[i] == gid) - goto cleanup; + return ret; } if (VIR_APPEND_ELEMENT(*list, i, gid) < 0) { ret = -1; VIR_FREE(*list); - goto cleanup; + return ret; } else { ret = i; } } - cleanup: - VIR_FREE(user); return ret; } @@ -1405,8 +1395,8 @@ virSetDeviceUnprivSGIO(const char *path, const char *sysfs_dir, int unpriv_sgio) { - char *sysfs_path = NULL; - char *val = NULL; + g_autofree char *sysfs_path = NULL; + g_autofree char *val = NULL; int ret = -1; int rc; @@ -1416,20 +1406,17 @@ virSetDeviceUnprivSGIO(const char *path, if (!virFileExists(sysfs_path)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("unpriv_sgio is not supported by this kernel")); - goto cleanup; + return ret; } val = g_strdup_printf("%d", unpriv_sgio); if ((rc = virFileWriteStr(sysfs_path, val, 0)) < 0) { virReportSystemError(-rc, _("failed to set %s"), sysfs_path); - goto cleanup; + return ret; } ret = 0; - cleanup: - VIR_FREE(sysfs_path); - VIR_FREE(val); return ret; } @@ -1438,8 +1425,8 @@ virGetDeviceUnprivSGIO(const char *path, const char *sysfs_dir, int *unpriv_sgio) { - char *sysfs_path = NULL; - char *buf = NULL; + g_autofree char *sysfs_path = NULL; + g_autofree char *buf = NULL; char *tmp = NULL; int ret = -1; @@ -1449,11 +1436,11 @@ virGetDeviceUnprivSGIO(const char *path, if (!virFileExists(sysfs_path)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("unpriv_sgio is not supported by this kernel")); - goto cleanup; + return ret; } if (virFileReadAll(sysfs_path, 1024, &buf) < 0) - goto cleanup; + return ret; if ((tmp = strchr(buf, '\n'))) *tmp = '\0'; @@ -1461,13 +1448,10 @@ virGetDeviceUnprivSGIO(const char *path, if (virStrToLong_i(buf, NULL, 10, unpriv_sgio) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse value of %s"), sysfs_path); - goto cleanup; + return ret; } ret = 0; - cleanup: - VIR_FREE(sysfs_path); - VIR_FREE(buf); return ret; } @@ -1488,7 +1472,7 @@ virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) int rc = -1; uid_t theuid; gid_t thegid; - char *tmp_label = NULL; + g_autofree char *tmp_label = NULL; char *sep = NULL; char *owner = NULL; char *group = NULL; @@ -1501,7 +1485,7 @@ virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse uid and gid from '%s'"), label); - goto cleanup; + return rc; } *sep = '\0'; owner = tmp_label; @@ -1512,7 +1496,7 @@ virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) */ if (virGetUserID(owner, &theuid) < 0 || virGetGroupID(group, &thegid) < 0) - goto cleanup; + return rc; if (uidPtr) *uidPtr = theuid; @@ -1520,10 +1504,6 @@ virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) *gidPtr = thegid; rc = 0; - - cleanup: - VIR_FREE(tmp_label); - return rc; } diff --git a/src/util/virvhba.c b/src/util/virvhba.c index a4e88024d1..d8642ea041 100644 --- a/src/util/virvhba.c +++ b/src/util/virvhba.c @@ -49,7 +49,7 @@ bool virVHBAPathExists(const char *sysfs_prefix, int host) { - char *sysfs_path = NULL; + g_autofree char *sysfs_path = NULL; bool ret = false; sysfs_path = g_strdup_printf("%s/host%d", @@ -58,7 +58,6 @@ virVHBAPathExists(const char *sysfs_prefix, if (virFileExists(sysfs_path)) ret = true; - VIR_FREE(sysfs_path); return ret; } @@ -79,8 +78,8 @@ bool virVHBAIsVportCapable(const char *sysfs_prefix, int host) { - char *scsi_host_path = NULL; - char *fc_host_path = NULL; + g_autofree char *scsi_host_path = NULL; + g_autofree char *fc_host_path = NULL; bool ret = false; fc_host_path = g_strdup_printf("%s/host%d/%s", @@ -94,8 +93,6 @@ virVHBAIsVportCapable(const char *sysfs_prefix, if (virFileExists(fc_host_path) || virFileExists(scsi_host_path)) ret = true; - VIR_FREE(fc_host_path); - VIR_FREE(scsi_host_path); return ret; } @@ -115,19 +112,19 @@ virVHBAGetConfig(const char *sysfs_prefix, int host, const char *entry) { - char *sysfs_path = NULL; + g_autofree char *sysfs_path = NULL; char *p = NULL; - char *buf = NULL; + g_autofree char *buf = NULL; char *result = NULL; sysfs_path = g_strdup_printf("%s/host%d/%s", sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH, host, entry); if (!virFileExists(sysfs_path)) - goto cleanup; + return result; if (virFileReadAll(sysfs_path, 1024, &buf) < 0) - goto cleanup; + return result; if ((p = strchr(buf, '\n'))) *p = '\0'; @@ -139,9 +136,6 @@ virVHBAGetConfig(const char *sysfs_prefix, result = g_strdup(p); - cleanup: - VIR_FREE(sysfs_path); - VIR_FREE(buf); return result; } @@ -160,8 +154,8 @@ virVHBAFindVportHost(const char *sysfs_prefix) const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH; g_autoptr(DIR) dir = NULL; struct dirent *entry = NULL; - char *max_vports = NULL; - char *vports = NULL; + g_autofree char *max_vports = NULL; + g_autofree char *vports = NULL; char *state = NULL; char *ret = NULL; @@ -212,16 +206,13 @@ virVHBAFindVportHost(const char *sysfs_prefix) ((strlen(max_vports) == strlen(vports)) && strcmp(max_vports, vports) > 0)) { ret = g_strdup(entry->d_name); - goto cleanup; + return ret; } VIR_FREE(max_vports); VIR_FREE(vports); } - cleanup: - VIR_FREE(max_vports); - VIR_FREE(vports); return ret; } @@ -241,7 +232,8 @@ virVHBAManageVport(const int parent_host, int operation) { int ret = -1; - char *operation_path = NULL, *vport_name = NULL; + g_autofree char *operation_path = NULL; + g_autofree char *vport_name = NULL; const char *operation_file = NULL; switch (operation) { @@ -254,7 +246,7 @@ virVHBAManageVport(const int parent_host, default: virReportError(VIR_ERR_OPERATION_INVALID, _("Invalid vport operation (%d)"), operation); - goto cleanup; + return ret; } operation_path = g_strdup_printf("%s/host%d/%s", SYSFS_FC_HOST_PATH, @@ -270,7 +262,7 @@ virVHBAManageVport(const int parent_host, _("vport operation '%s' is not supported " "for host%d"), operation_file, parent_host); - goto cleanup; + return ret; } } @@ -290,9 +282,6 @@ virVHBAManageVport(const int parent_host, "vport create/delete failed"), vport_name, operation_path); - cleanup: - VIR_FREE(vport_name); - VIR_FREE(operation_path); return ret; } @@ -315,8 +304,8 @@ vhbaReadCompareWWN(const char *prefix, const char *f_name, const char *wwn) { - char *path; - char *buf = NULL; + g_autofree char *path = NULL; + g_autofree char *buf = NULL; char *p; int ret = -1; @@ -324,11 +313,11 @@ vhbaReadCompareWWN(const char *prefix, if (!virFileExists(path)) { ret = 0; - goto cleanup; + return ret; } if (virFileReadAll(path, 1024, &buf) < 0) - goto cleanup; + return ret; if ((p = strchr(buf, '\n'))) *p = '\0'; @@ -342,10 +331,6 @@ vhbaReadCompareWWN(const char *prefix, else ret = 1; - cleanup: - VIR_FREE(path); - VIR_FREE(buf); - return ret; } @@ -407,7 +392,7 @@ virVHBAGetHostByFabricWWN(const char *sysfs_prefix, const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH; struct dirent *entry = NULL; g_autoptr(DIR) dir = NULL; - char *vport_create_path = NULL; + g_autofree char *vport_create_path = NULL; char *ret = NULL; if (virDirOpen(&dir, prefix) < 0) @@ -428,7 +413,7 @@ virVHBAGetHostByFabricWWN(const char *sysfs_prefix, if ((rc = vhbaReadCompareWWN(prefix, entry->d_name, "fabric_name", fabric_wwn)) < 0) - goto cleanup; + return ret; if (rc == 0) continue; @@ -437,8 +422,6 @@ virVHBAGetHostByFabricWWN(const char *sysfs_prefix, break; } - cleanup: - VIR_FREE(vport_create_path); return ret; } diff --git a/src/util/virxml.c b/src/util/virxml.c index a3b819d85c..7df50e4b4d 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -696,8 +696,8 @@ catchXMLError(void *ctx, const char *msg G_GNUC_UNUSED, ...) unsigned int n, col; /* GCC warns if signed, because compared with sizeof() */ int domcode = VIR_FROM_XML; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - char *contextstr = NULL; - char *pointerstr = NULL; + g_autofree char *contextstr = NULL; + g_autofree char *pointerstr = NULL; /* conditions for error printing */ @@ -763,9 +763,6 @@ catchXMLError(void *ctx, const char *msg G_GNUC_UNUSED, ...) contextstr, pointerstr); } - - VIR_FREE(contextstr); - VIR_FREE(pointerstr); } /** -- 2.29.0
On Wed, 2020-11-18 at 16:47 -0600, Ryan Gahagan wrote:
> From: Barrett Schonefeld <bschoney@utexas.edu>
>
> additional conversions to the GLib API in src/util per issue #11.
>
> Related issue: https://gitlab.com/libvirt/libvirt/-/issues/11
>
> Signed-off-by: Barrett Schonefeld <bschoney@utexas.edu>
> ---
> src/util/vircgroupv1.c | 3 +-
> src/util/virdnsmasq.c | 43 ++++++++-------------
> src/util/virfile.c | 9 ++---
> src/util/virhostcpu.c | 4 +-
> src/util/virlockspace.c | 6 +--
> src/util/virlog.c | 12 ++----
> src/util/virmacmap.c | 3 +-
> src/util/virnetdevbandwidth.c | 48 ++++++++---------------
> src/util/virresctrl.c | 25 ++++--------
> src/util/virrotatingfile.c | 11 ++----
> src/util/virscsihost.c | 25 +++++-------
> src/util/virsecret.c | 14 +++----
> src/util/virstorageencryption.c | 19 +++------
> src/util/virstoragefilebackend.c | 4 +-
> src/util/virsysinfo.c | 18 +++------
> src/util/viruri.c | 7 +---
> src/util/virutil.c | 66 +++++++++++-------------------
> --
> src/util/virvhba.c | 57 ++++++++++-----------------
> src/util/virxml.c | 7 +---
> 19 files changed, 130 insertions(+), 251 deletions(-)
>
> diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
> index 731e9d61d4..984cd50409 100644
> --- a/src/util/vircgroupv1.c
> +++ b/src/util/vircgroupv1.c
> @@ -1549,7 +1549,7 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
> unsigned long long *unevictable)
> {
> int ret = -1;
> - char *stat = NULL;
> + g_autofree char *stat = NULL;
> char *line = NULL;
> unsigned long long cacheVal = 0;
> unsigned long long activeAnonVal = 0;
> @@ -1614,7 +1614,6 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,
> ret = 0;
>
> cleanup:
> - VIR_FREE(stat);
> return ret;
> }
>
I believe you can remove "cleanup" and "ret" as well. More instances
below.
> diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
> index 9030f9218a..93bc4a129f 100644
> --- a/src/util/virdnsmasq.c
> +++ b/src/util/virdnsmasq.c
> @@ -164,7 +164,7 @@ addnhostsWrite(const char *path,
> dnsmasqAddnHost *hosts,
> unsigned int nhosts)
> {
> - char *tmp;
> + g_autofree char *tmp = NULL;
> FILE *f;
> bool istmp = true;
> size_t i, j;
> @@ -180,7 +180,7 @@ addnhostsWrite(const char *path,
> istmp = false;
> if (!(f = fopen(path, "w"))) {
> rc = -errno;
> - goto cleanup;
> + return rc;
Why not "return -errno;"? More instances below.
You also might want to split the patch up, e.g. go function by
function, to create self-contained changes.
Cheers,
Tim
> }
> }
>
> @@ -192,7 +192,7 @@ addnhostsWrite(const char *path,
> if (istmp)
> unlink(tmp);
>
> - goto cleanup;
> + return rc;
> }
>
> for (j = 0; j < hosts[i].nhostnames; j++) {
> @@ -203,7 +203,7 @@ addnhostsWrite(const char *path,
> if (istmp)
> unlink(tmp);
>
> - goto cleanup;
> + return rc;
> }
> }
>
> @@ -214,24 +214,21 @@ addnhostsWrite(const char *path,
> if (istmp)
> unlink(tmp);
>
> - goto cleanup;
> + return rc;
> }
> }
>
> if (VIR_FCLOSE(f) == EOF) {
> rc = -errno;
> - goto cleanup;
> + return rc;
> }
>
> if (istmp && rename(tmp, path) < 0) {
> rc = -errno;
> unlink(tmp);
> - goto cleanup;
> + return rc;
> }
>
> - cleanup:
> - VIR_FREE(tmp);
> -
> return rc;
> }
>
> @@ -364,7 +361,7 @@ hostsfileWrite(const char *path,
> dnsmasqDhcpHost *hosts,
> unsigned int nhosts)
> {
> - char *tmp;
> + g_autofree char *tmp = NULL;
> FILE *f;
> bool istmp = true;
> size_t i;
> @@ -380,7 +377,7 @@ hostsfileWrite(const char *path,
> istmp = false;
> if (!(f = fopen(path, "w"))) {
> rc = -errno;
> - goto cleanup;
> + return rc;
> }
> }
>
> @@ -392,24 +389,21 @@ hostsfileWrite(const char *path,
> if (istmp)
> unlink(tmp);
>
> - goto cleanup;
> + return rc;
> }
> }
>
> if (VIR_FCLOSE(f) == EOF) {
> rc = -errno;
> - goto cleanup;
> + return rc;
> }
>
> if (istmp && rename(tmp, path) < 0) {
> rc = -errno;
> unlink(tmp);
> - goto cleanup;
> + return rc;
> }
>
> - cleanup:
> - VIR_FREE(tmp);
> -
> return rc;
> }
>
> @@ -686,15 +680,13 @@ static int
> dnsmasqCapsSetFromFile(dnsmasqCapsPtr caps, const char *path)
> {
> int ret = -1;
> - char *buf = NULL;
> + g_autofree char *buf = NULL;
>
> if (virFileReadAll(path, 1024 * 1024, &buf) < 0)
> - goto cleanup;
> + return ret;
>
> ret = dnsmasqCapsSetFromBuffer(caps, buf);
>
> - cleanup:
> - VIR_FREE(buf);
> return ret;
> }
>
> @@ -704,7 +696,9 @@ dnsmasqCapsRefreshInternal(dnsmasqCapsPtr caps,
> bool force)
> int ret = -1;
> struct stat sb;
> virCommandPtr cmd = NULL;
> - char *help = NULL, *version = NULL, *complete = NULL;
> + g_autofree char *help = NULL;
> + g_autofree char *version = NULL;
> + g_autofree char *complete = NULL;
>
> if (!caps || caps->noRefresh)
> return 0;
> @@ -749,9 +743,6 @@ dnsmasqCapsRefreshInternal(dnsmasqCapsPtr caps,
> bool force)
>
> cleanup:
> virCommandFree(cmd);
> - VIR_FREE(help);
> - VIR_FREE(version);
> - VIR_FREE(complete);
> return ret;
> }
>
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index f57272ca2f..38207f1948 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -3328,9 +3328,9 @@ virFileIsSharedFixFUSE(const char *path,
> FILE *f = NULL;
> struct mntent mb;
> char mntbuf[1024];
> - char *mntDir = NULL;
> - char *mntType = NULL;
> - char *canonPath = NULL;
> + g_autofree char *mntDir = NULL;
> + g_autofree char *mntType = NULL;
> + g_autofree char *canonPath = NULL;
> size_t maxMatching = 0;
> int ret = -1;
>
> @@ -3381,9 +3381,6 @@ virFileIsSharedFixFUSE(const char *path,
>
> ret = 0;
> cleanup:
> - VIR_FREE(canonPath);
> - VIR_FREE(mntType);
> - VIR_FREE(mntDir);
> endmntent(f);
> return ret;
> }
> diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> index c531d65f86..4f6c3390ce 100644
> --- a/src/util/virhostcpu.c
> +++ b/src/util/virhostcpu.c
> @@ -87,7 +87,7 @@ virHostCPUGetStatsFreeBSD(int cpuNum,
> int *nparams)
> {
> const char *sysctl_name;
> - long *cpu_times;
> + g_autofree long *cpu_times = NULL;
> struct clockinfo clkinfo;
> size_t i, j, cpu_times_size, clkinfo_size;
> int cpu_times_num, offset, hz, stathz, ret = -1;
> @@ -172,8 +172,6 @@ virHostCPUGetStatsFreeBSD(int cpuNum,
> ret = 0;
>
> cleanup:
> - VIR_FREE(cpu_times);
> -
> return ret;
> }
>
> diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c
> index b90e13f506..71d5dfb83e 100644
> --- a/src/util/virlockspace.c
> +++ b/src/util/virlockspace.c
> @@ -515,7 +515,7 @@ int virLockSpaceCreateResource(virLockSpacePtr
> lockspace,
> const char *resname)
> {
> int ret = -1;
> - char *respath = NULL;
> + g_autofree char *respath = NULL;
>
> VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname);
>
> @@ -538,7 +538,6 @@ int virLockSpaceCreateResource(virLockSpacePtr
> lockspace,
>
> cleanup:
> virMutexUnlock(&lockspace->lock);
> - VIR_FREE(respath);
> return ret;
> }
>
> @@ -547,7 +546,7 @@ int virLockSpaceDeleteResource(virLockSpacePtr
> lockspace,
> const char *resname)
> {
> int ret = -1;
> - char *respath = NULL;
> + g_autofree char *respath = NULL;
>
> VIR_DEBUG("lockspace=%p resname=%s", lockspace, resname);
>
> @@ -575,7 +574,6 @@ int virLockSpaceDeleteResource(virLockSpacePtr
> lockspace,
>
> cleanup:
> virMutexUnlock(&lockspace->lock);
> - VIR_FREE(respath);
> return ret;
> }
>
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index 6b7a4512e9..2bf606b8c5 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -505,8 +505,8 @@ virLogVMessage(virLogSourcePtr source,
> va_list vargs)
> {
> static bool logInitMessageStderr = true;
> - char *str = NULL;
> - char *msg = NULL;
> + g_autofree char *str = NULL;
> + g_autofree char *msg = NULL;
> char timestamp[VIR_TIME_STRING_BUFLEN];
> size_t i;
> int saved_errno = errno;
> @@ -528,7 +528,8 @@ virLogVMessage(virLogSourcePtr source,
> if (source->serial < virLogFiltersSerial)
> virLogSourceUpdate(source);
> if (priority < source->priority)
> - goto cleanup;
> + errno = saved_errno;
> + return;
>
> /*
> * serialize the error message, add level and timestamp
> @@ -601,11 +602,6 @@ virLogVMessage(virLogSourcePtr source,
> str, msg, (void *) STDERR_FILENO);
> }
> virLogUnlock();
> -
> - cleanup:
> - VIR_FREE(str);
> - VIR_FREE(msg);
> - errno = saved_errno;
> }
>
>
> diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c
> index f9047d0fb1..e68742de10 100644
> --- a/src/util/virmacmap.c
> +++ b/src/util/virmacmap.c
> @@ -129,7 +129,7 @@ static int
> virMacMapLoadFile(virMacMapPtr mgr,
> const char *file)
> {
> - char *map_str = NULL;
> + g_autofree char *map_str = NULL;
> virJSONValuePtr map = NULL;
> int map_str_len = 0;
> size_t i;
> @@ -189,7 +189,6 @@ virMacMapLoadFile(virMacMapPtr mgr,
>
> ret = 0;
> cleanup:
> - VIR_FREE(map_str);
> virJSONValueFree(map);
> return ret;
> }
> diff --git a/src/util/virnetdevbandwidth.c
> b/src/util/virnetdevbandwidth.c
> index c8eb5cfc79..2ae03e8edc 100644
> --- a/src/util/virnetdevbandwidth.c
> +++ b/src/util/virnetdevbandwidth.c
> @@ -102,7 +102,7 @@ virNetDevBandwidthManipulateFilter(const char
> *ifname,
> bool create_new)
> {
> int ret = -1;
> - char *filter_id = NULL;
> + g_autofree char *filter_id = NULL;
> virCommandPtr cmd = NULL;
> unsigned char ifmac[VIR_MAC_BUFLEN];
> char *mac[2] = {NULL, NULL};
> @@ -157,7 +157,6 @@ virNetDevBandwidthManipulateFilter(const char
> *ifname,
> cleanup:
> VIR_FREE(mac[1]);
> VIR_FREE(mac[0]);
> - VIR_FREE(filter_id);
> virCommandFree(cmd);
> return ret;
> }
> @@ -195,9 +194,9 @@ virNetDevBandwidthSet(const char *ifname,
> int ret = -1;
> virNetDevBandwidthRatePtr rx = NULL, tx = NULL; /* From domain
> POV */
> virCommandPtr cmd = NULL;
> - char *average = NULL;
> - char *peak = NULL;
> - char *burst = NULL;
> + g_autofree char *average = NULL;
> + g_autofree char *peak = NULL;
> + g_autofree char *burst = NULL;
>
> if (!bandwidth) {
> /* nothing to be enabled */
> @@ -385,9 +384,6 @@ virNetDevBandwidthSet(const char *ifname,
>
> cleanup:
> virCommandFree(cmd);
> - VIR_FREE(average);
> - VIR_FREE(peak);
> - VIR_FREE(burst);
> return ret;
> }
>
> @@ -533,10 +529,10 @@ virNetDevBandwidthPlug(const char *brname,
> {
> int ret = -1;
> virCommandPtr cmd = NULL;
> - char *class_id = NULL;
> - char *qdisc_id = NULL;
> - char *floor = NULL;
> - char *ceil = NULL;
> + g_autofree char *class_id = NULL;
> + g_autofree char *qdisc_id = NULL;
> + g_autofree char *floor = NULL;
> + g_autofree char *ceil = NULL;
> char ifmacStr[VIR_MAC_STRING_BUFLEN];
>
> if (id <= 2) {
> @@ -586,10 +582,6 @@ virNetDevBandwidthPlug(const char *brname,
> ret = 0;
>
> cleanup:
> - VIR_FREE(ceil);
> - VIR_FREE(floor);
> - VIR_FREE(qdisc_id);
> - VIR_FREE(class_id);
> virCommandFree(cmd);
> return ret;
> }
> @@ -610,8 +602,8 @@ virNetDevBandwidthUnplug(const char *brname,
> int ret = -1;
> int cmd_ret = 0;
> virCommandPtr cmd = NULL;
> - char *class_id = NULL;
> - char *qdisc_id = NULL;
> + g_autofree char *class_id = NULL;
> + g_autofree char *qdisc_id = NULL;
>
> if (id <= 2) {
> virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid class ID
> %d"), id);
> @@ -645,8 +637,6 @@ virNetDevBandwidthUnplug(const char *brname,
> ret = 0;
>
> cleanup:
> - VIR_FREE(qdisc_id);
> - VIR_FREE(class_id);
> virCommandFree(cmd);
> return ret;
> }
> @@ -673,9 +663,9 @@ virNetDevBandwidthUpdateRate(const char *ifname,
> {
> int ret = -1;
> virCommandPtr cmd = NULL;
> - char *class_id = NULL;
> - char *rate = NULL;
> - char *ceil = NULL;
> + g_autofree char *class_id = NULL;
> + g_autofree char *rate = NULL;
> + g_autofree char *ceil = NULL;
>
> class_id = g_strdup_printf("1:%x", id);
> rate = g_strdup_printf("%llukbps", new_rate);
> @@ -696,9 +686,6 @@ virNetDevBandwidthUpdateRate(const char *ifname,
>
> cleanup:
> virCommandFree(cmd);
> - VIR_FREE(class_id);
> - VIR_FREE(rate);
> - VIR_FREE(ceil);
> return ret;
> }
>
> @@ -725,16 +712,13 @@ virNetDevBandwidthUpdateFilter(const char
> *ifname,
> unsigned int id)
> {
> int ret = -1;
> - char *class_id = NULL;
> + g_autofree char *class_id = NULL;
>
> class_id = g_strdup_printf("1:%x", id);
>
> if (virNetDevBandwidthManipulateFilter(ifname, ifmac_ptr, id,
> class_id, true, true) <
> 0)
> - goto cleanup;
> + return ret;
>
> - ret = 0;
> - cleanup:
> - VIR_FREE(class_id);
> - return ret;
> + return 0;
> }
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index d3087b98c1..1c2d175295 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -709,7 +709,7 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr
> resctrl)
> {
> int ret = -1;
> int rv = -1;
> - char *featurestr = NULL;
> + g_autofree char *featurestr = NULL;
> char **features = NULL;
> size_t nfeatures = 0;
> virResctrlInfoMongrpPtr info_monitor = NULL;
> @@ -771,7 +771,6 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr
> resctrl)
>
> ret = 0;
> cleanup:
> - VIR_FREE(featurestr);
> g_strfreev(features);
> VIR_FREE(info_monitor);
> return ret;
> @@ -1736,7 +1735,7 @@ virResctrlAllocGetGroup(virResctrlInfoPtr
> resctrl,
> const char *groupname,
> virResctrlAllocPtr *alloc)
> {
> - char *schemata = NULL;
> + g_autofree char *schemata = NULL;
> int rv = virFileReadValueString(&schemata,
> SYSFS_RESCTRL_PATH
> "/%s/schemata",
> groupname);
> @@ -1753,11 +1752,9 @@ virResctrlAllocGetGroup(virResctrlInfoPtr
> resctrl,
> if (virResctrlAllocParse(resctrl, *alloc, schemata) < 0)
> goto error;
>
> - VIR_FREE(schemata);
> return 0;
>
> error:
> - VIR_FREE(schemata);
> virObjectUnref(*alloc);
> *alloc = NULL;
> return -1;
> @@ -2354,8 +2351,8 @@ virResctrlAllocCreate(virResctrlInfoPtr
> resctrl,
> virResctrlAllocPtr alloc,
> const char *machinename)
> {
> - char *schemata_path = NULL;
> - char *alloc_str = NULL;
> + g_autofree char *schemata_path = NULL;
> + g_autofree char *alloc_str = NULL;
> int ret = -1;
> int lockfd = -1;
>
> @@ -2403,8 +2400,6 @@ virResctrlAllocCreate(virResctrlInfoPtr
> resctrl,
> ret = 0;
> cleanup:
> virResctrlUnlock(lockfd);
> - VIR_FREE(alloc_str);
> - VIR_FREE(schemata_path);
> return ret;
> }
>
> @@ -2413,8 +2408,8 @@ static int
> virResctrlAddPID(const char *path,
> pid_t pid)
> {
> - char *tasks = NULL;
> - char *pidstr = NULL;
> + g_autofree char *tasks = NULL;
> + g_autofree char *pidstr = NULL;
> int ret = 0;
>
> if (!path) {
> @@ -2436,8 +2431,6 @@ virResctrlAddPID(const char *path,
>
> ret = 0;
> cleanup:
> - VIR_FREE(tasks);
> - VIR_FREE(pidstr);
> return ret;
> }
>
> @@ -2657,8 +2650,8 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr
> monitor,
> size_t i = 0;
> unsigned long long val = 0;
> g_autoptr(DIR) dirp = NULL;
> - char *datapath = NULL;
> - char *filepath = NULL;
> + g_autofree char *datapath = NULL;
> + g_autofree char *filepath = NULL;
> struct dirent *ent = NULL;
> virResctrlMonitorStatsPtr stat = NULL;
>
> @@ -2737,8 +2730,6 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr
> monitor,
>
> ret = 0;
> cleanup:
> - VIR_FREE(datapath);
> - VIR_FREE(filepath);
> virResctrlMonitorStatsFree(stat);
> return ret;
> }
> diff --git a/src/util/virrotatingfile.c b/src/util/virrotatingfile.c
> index 9f1ef17c3e..6fe5fa3154 100644
> --- a/src/util/virrotatingfile.c
> +++ b/src/util/virrotatingfile.c
> @@ -362,8 +362,8 @@ static int
> virRotatingFileWriterRollover(virRotatingFileWriterPtr file)
> {
> size_t i;
> - char *nextpath = NULL;
> - char *thispath = NULL;
> + g_autofree char *nextpath = NULL;
> + g_autofree char *thispath = NULL;
> int ret = -1;
>
> VIR_DEBUG("Rollover %s", file->basepath);
> @@ -373,7 +373,7 @@
> virRotatingFileWriterRollover(virRotatingFileWriterPtr file)
> virReportSystemError(errno,
> _("Unable to remove %s"),
> file->basepath);
> - goto cleanup;
> + return ret;
> }
> } else {
> nextpath = g_strdup_printf("%s.%zu", file->basepath, file-
> >maxbackup - 1);
> @@ -391,7 +391,7 @@
> virRotatingFileWriterRollover(virRotatingFileWriterPtr file)
> virReportSystemError(errno,
> _("Unable to rename %s to %s"),
> thispath, nextpath);
> - goto cleanup;
> + return ret;
> }
>
> VIR_FREE(nextpath);
> @@ -402,9 +402,6 @@
> virRotatingFileWriterRollover(virRotatingFileWriterPtr file)
> VIR_DEBUG("Rollover done %s", file->basepath);
>
> ret = 0;
> - cleanup:
> - VIR_FREE(nextpath);
> - VIR_FREE(thispath);
> return ret;
> }
>
> diff --git a/src/util/virscsihost.c b/src/util/virscsihost.c
> index 969cdd9f79..cad48b4059 100644
> --- a/src/util/virscsihost.c
> +++ b/src/util/virscsihost.c
> @@ -95,12 +95,12 @@ virSCSIHostFindByPCI(const char *sysfs_prefix,
> const char *prefix = sysfs_prefix ? sysfs_prefix :
> SYSFS_SCSI_HOST_PATH;
> struct dirent *entry = NULL;
> g_autoptr(DIR) dir = NULL;
> - char *host_link = NULL;
> - char *host_path = NULL;
> + g_autofree char *host_link = NULL;
> + g_autofree char *host_path = NULL;
> char *p = NULL;
> char *ret = NULL;
> - char *buf = NULL;
> - char *unique_path = NULL;
> + g_autofree char *buf = NULL;
> + g_autofree char *unique_path = NULL;
> unsigned int read_unique_id;
>
> if (virDirOpen(&dir, prefix) < 0)
> @@ -113,7 +113,7 @@ virSCSIHostFindByPCI(const char *sysfs_prefix,
> host_link = g_strdup_printf("%s/%s", prefix, entry->d_name);
>
> if (virFileResolveLink(host_link, &host_path) < 0)
> - goto cleanup;
> + return ret;
>
> if (!strstr(host_path, parentaddr)) {
> VIR_FREE(host_link);
> @@ -131,13 +131,13 @@ virSCSIHostFindByPCI(const char *sysfs_prefix,
> }
>
> if (virFileReadAll(unique_path, 1024, &buf) < 0)
> - goto cleanup;
> + return ret;
>
> if ((p = strchr(buf, '\n')))
> *p = '\0';
>
> if (virStrToLong_ui(buf, NULL, 10, &read_unique_id) < 0)
> - goto cleanup;
> + return ret;
>
> VIR_FREE(buf);
>
> @@ -150,11 +150,6 @@ virSCSIHostFindByPCI(const char *sysfs_prefix,
> break;
> }
>
> - cleanup:
> - VIR_FREE(unique_path);
> - VIR_FREE(host_link);
> - VIR_FREE(host_path);
> - VIR_FREE(buf);
> return ret;
> }
>
> @@ -226,7 +221,7 @@ virSCSIHostGetNameByParentaddr(unsigned int
> domain,
> unsigned int unique_id)
> {
> char *name = NULL;
> - char *parentaddr = NULL;
> + g_autofree char *parentaddr = NULL;
>
> parentaddr = g_strdup_printf("%04x:%02x:%02x.%01x", domain, bus,
> slot,
> function);
> @@ -235,11 +230,9 @@ virSCSIHostGetNameByParentaddr(unsigned int
> domain,
> _("Failed to find scsi_host using PCI '%s' "
> "and unique_id='%u'"),
> parentaddr, unique_id);
> - goto cleanup;
> + return name;
> }
>
> - cleanup:
> - VIR_FREE(parentaddr);
> return name;
> }
>
> diff --git a/src/util/virsecret.c b/src/util/virsecret.c
> index 54d6bbcb7c..67c9b68215 100644
> --- a/src/util/virsecret.c
> +++ b/src/util/virsecret.c
> @@ -65,8 +65,8 @@ int
> virSecretLookupParseSecret(xmlNodePtr secretnode,
> virSecretLookupTypeDefPtr def)
> {
> - char *uuid;
> - char *usage;
> + g_autofree char *uuid = NULL;
> + g_autofree char *usage = NULL;
> int ret = -1;
>
> uuid = virXMLPropString(secretnode, "uuid");
> @@ -74,20 +74,20 @@ virSecretLookupParseSecret(xmlNodePtr secretnode,
> if (uuid == NULL && usage == NULL) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> _("missing secret uuid or usage attribute"));
> - goto cleanup;
> + return ret;
> }
>
> if (uuid && usage) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> _("either secret uuid or usage expected"));
> - goto cleanup;
> + return ret;
> }
>
> if (uuid) {
> if (virUUIDParse(uuid, def->u.uuid) < 0) {
> virReportError(VIR_ERR_XML_ERROR,
> _("invalid secret uuid '%s'"), uuid);
> - goto cleanup;
> + return ret;
> }
> def->type = VIR_SECRET_LOOKUP_TYPE_UUID;
> } else {
> @@ -96,10 +96,6 @@ virSecretLookupParseSecret(xmlNodePtr secretnode,
> def->type = VIR_SECRET_LOOKUP_TYPE_USAGE;
> }
> ret = 0;
> -
> - cleanup:
> - VIR_FREE(uuid);
> - VIR_FREE(usage);
> return ret;
> }
>
> diff --git a/src/util/virstorageencryption.c
> b/src/util/virstorageencryption.c
> index 399c6926bd..db886c1e76 100644
> --- a/src/util/virstorageencryption.c
> +++ b/src/util/virstorageencryption.c
> @@ -142,7 +142,7 @@
> virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt,
> {
> VIR_XPATH_NODE_AUTORESTORE(ctxt)
> virStorageEncryptionSecretPtr ret;
> - char *type_str = NULL;
> + g_autofree char *type_str = NULL;
>
> ret = g_new0(virStorageEncryptionSecret, 1);
>
> @@ -164,12 +164,9 @@
> virStorageEncryptionSecretParse(xmlXPathContextPtr ctxt,
> if (virSecretLookupParseSecret(node, &ret->seclookupdef) < 0)
> goto cleanup;
>
> - VIR_FREE(type_str);
> -
> return ret;
>
> cleanup:
> - VIR_FREE(type_str);
> virStorageEncryptionSecretFree(ret);
> return NULL;
> }
> @@ -180,12 +177,12 @@ virStorageEncryptionInfoParseCipher(xmlNodePtr
> info_node,
> virStorageEncryptionInfoDefPtr
> info)
> {
> int ret = -1;
> - char *size_str = NULL;
> + g_autofree char *size_str = NULL;
>
> if (!(info->cipher_name = virXMLPropString(info_node, "name")))
> {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> _("cipher info missing 'name' attribute"));
> - goto cleanup;
> + return ret;
> }
>
> if ((size_str = virXMLPropString(info_node, "size")) &&
> @@ -193,22 +190,19 @@ virStorageEncryptionInfoParseCipher(xmlNodePtr
> info_node,
> virReportError(VIR_ERR_XML_ERROR,
> _("cannot parse cipher size: '%s'"),
> size_str);
> - goto cleanup;
> + return ret;
> }
>
> if (!size_str) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> _("cipher info missing 'size' attribute"));
> - goto cleanup;
> + return ret;
> }
>
> info->cipher_mode = virXMLPropString(info_node, "mode");
> info->cipher_hash = virXMLPropString(info_node, "hash");
>
> ret = 0;
> -
> - cleanup:
> - VIR_FREE(size_str);
> return ret;
> }
>
> @@ -237,7 +231,7 @@ virStorageEncryptionParseNode(xmlNodePtr node,
> xmlNodePtr *nodes = NULL;
> virStorageEncryptionPtr encdef = NULL;
> virStorageEncryptionPtr ret = NULL;
> - char *format_str = NULL;
> + g_autofree char *format_str = NULL;
> int n;
> size_t i;
>
> @@ -297,7 +291,6 @@ virStorageEncryptionParseNode(xmlNodePtr node,
> ret = g_steal_pointer(&encdef);
>
> cleanup:
> - VIR_FREE(format_str);
> VIR_FREE(nodes);
> virStorageEncryptionFree(encdef);
>
> diff --git a/src/util/virstoragefilebackend.c
> b/src/util/virstoragefilebackend.c
> index 2779b5c307..55c62b0212 100644
> --- a/src/util/virstoragefilebackend.c
> +++ b/src/util/virstoragefilebackend.c
> @@ -51,7 +51,7 @@ virStorageFileLoadBackendModule(const char *name,
> const char *regfunc,
> bool forceload)
> {
> - char *modfile = NULL;
> + g_autofree char *modfile = NULL;
> int ret;
>
> if (!(modfile = virFileFindResourceFull(name,
> @@ -64,8 +64,6 @@ virStorageFileLoadBackendModule(const char *name,
>
> ret = virModuleLoad(modfile, regfunc, forceload);
>
> - VIR_FREE(modfile);
> -
> return ret;
> }
> #endif /* WITH_STORAGE_DIR || WITH_STORAGE_FS ||
> WITH_STORAGE_GLUSTER */
> diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c
> index 217f842a37..45a950c85a 100644
> --- a/src/util/virsysinfo.c
> +++ b/src/util/virsysinfo.c
> @@ -393,7 +393,7 @@ virSysinfoParseARMProcessor(const char *base,
> virSysinfoDefPtr ret)
> const char *cur;
> char *eol, *tmp_base;
> virSysinfoProcessorDefPtr processor;
> - char *processor_type = NULL;
> + g_autofree char *processor_type = NULL;
>
> if (!(tmp_base = strstr(base, "model name")) &&
> !(tmp_base = strstr(base, "Processor")))
> @@ -411,7 +411,7 @@ virSysinfoParseARMProcessor(const char *base,
> virSysinfoDefPtr ret)
> cur = strchr(base, ':') + 1;
>
> if (VIR_EXPAND_N(ret->processor, ret->nprocessor, 1) < 0)
> - goto error;
> + return -1;
> processor = &ret->processor[ret->nprocessor - 1];
>
> virSkipSpaces(&cur);
> @@ -424,12 +424,7 @@ virSysinfoParseARMProcessor(const char *base,
> virSysinfoDefPtr ret)
> base = cur;
> }
>
> - VIR_FREE(processor_type);
> return 0;
> -
> - error:
> - VIR_FREE(processor_type);
> - return -1;
> }
>
> /* virSysinfoRead for ARMv7
> @@ -532,9 +527,9 @@ static int
> virSysinfoParseS390Processor(const char *base, virSysinfoDefPtr ret)
> {
> const char *tmp_base;
> - char *manufacturer = NULL;
> - char *procline = NULL;
> - char *ncpu = NULL;
> + g_autofree char *manufacturer = NULL;
> + g_autofree char *procline = NULL;
> + g_autofree char *ncpu = NULL;
> int result = -1;
> virSysinfoProcessorDefPtr processor;
>
> @@ -593,9 +588,6 @@ virSysinfoParseS390Processor(const char *base,
> virSysinfoDefPtr ret)
> result = 0;
>
> error:
> - VIR_FREE(manufacturer);
> - VIR_FREE(procline);
> - VIR_FREE(ncpu);
> return result;
> }
>
> diff --git a/src/util/viruri.c b/src/util/viruri.c
> index 11753a0aef..d49821451e 100644
> --- a/src/util/viruri.c
> +++ b/src/util/viruri.c
> @@ -209,7 +209,7 @@ char *
> virURIFormat(virURIPtr uri)
> {
> xmlURI xmluri;
> - char *tmpserver = NULL;
> + g_autofree char *tmpserver = NULL;
> char *ret;
>
> memset(&xmluri, 0, sizeof(xmluri));
> @@ -241,12 +241,9 @@ virURIFormat(virURIPtr uri)
> ret = (char *)xmlSaveUri(&xmluri);
> if (!ret) {
> virReportOOMError();
> - goto cleanup;
> + return ret;
> }
>
> - cleanup:
> - VIR_FREE(tmpserver);
> -
> return ret;
> }
>
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index a0cd0f1bcd..a7c163ab94 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -598,7 +598,7 @@ char *virGetUserRuntimeDirectory(void)
> static int
> virGetUserEnt(uid_t uid, char **name, gid_t *group, char **dir, char
> **shell, bool quiet)
> {
> - char *strbuf;
> + g_autofree char *strbuf = NULL;
> struct passwd pwbuf;
> struct passwd *pw = NULL;
> long val = sysconf(_SC_GETPW_R_SIZE_MAX);
> @@ -668,13 +668,12 @@ virGetUserEnt(uid_t uid, char **name, gid_t
> *group, char **dir, char **shell, bo
> if (shell)
> VIR_FREE(*shell);
> }
> - VIR_FREE(strbuf);
> return ret;
> }
>
> static char *virGetGroupEnt(gid_t gid)
> {
> - char *strbuf;
> + g_autofree char *strbuf = NULL;
> char *ret;
> struct group grbuf;
> struct group *gr = NULL;
> @@ -717,7 +716,6 @@ static char *virGetGroupEnt(gid_t gid)
> }
>
> ret = g_strdup(gr->gr_name);
> - VIR_FREE(strbuf);
> return ret;
> }
>
> @@ -759,7 +757,7 @@ char *virGetGroupName(gid_t gid)
> static int
> virGetUserIDByName(const char *name, uid_t *uid, bool missing_ok)
> {
> - char *strbuf = NULL;
> + g_autofree char *strbuf = NULL;
> struct passwd pwbuf;
> struct passwd *pw = NULL;
> long val = sysconf(_SC_GETPW_R_SIZE_MAX);
> @@ -775,7 +773,7 @@ virGetUserIDByName(const char *name, uid_t *uid,
> bool missing_ok)
>
> while ((rc = getpwnam_r(name, &pwbuf, strbuf, strbuflen, &pw))
> == ERANGE) {
> if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) <
> 0)
> - goto cleanup;
> + return ret;
> }
>
> if (!pw) {
> @@ -788,16 +786,13 @@ virGetUserIDByName(const char *name, uid_t
> *uid, bool missing_ok)
> }
>
> ret = 1;
> - goto cleanup;
> + return ret;
> }
>
> if (uid)
> *uid = pw->pw_uid;
> ret = 0;
>
> - cleanup:
> - VIR_FREE(strbuf);
> -
> return ret;
> }
>
> @@ -840,7 +835,7 @@ virGetUserID(const char *user, uid_t *uid)
> static int
> virGetGroupIDByName(const char *name, gid_t *gid, bool missing_ok)
> {
> - char *strbuf = NULL;
> + g_autofree char *strbuf = NULL;
> struct group grbuf;
> struct group *gr = NULL;
> long val = sysconf(_SC_GETGR_R_SIZE_MAX);
> @@ -856,7 +851,7 @@ virGetGroupIDByName(const char *name, gid_t *gid,
> bool missing_ok)
>
> while ((rc = getgrnam_r(name, &grbuf, strbuf, strbuflen, &gr))
> == ERANGE) {
> if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) <
> 0)
> - goto cleanup;
> + return ret;
> }
>
> if (!gr) {
> @@ -869,16 +864,13 @@ virGetGroupIDByName(const char *name, gid_t
> *gid, bool missing_ok)
> }
>
> ret = 1;
> - goto cleanup;
> + return ret;
> }
>
> if (gid)
> *gid = gr->gr_gid;
> ret = 0;
>
> - cleanup:
> - VIR_FREE(strbuf);
> -
> return ret;
> }
>
> @@ -949,7 +941,7 @@ int
> virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
> {
> int ret = 0;
> - char *user = NULL;
> + g_autofree char *user = NULL;
> gid_t primary;
>
> *list = NULL;
> @@ -987,19 +979,17 @@ virGetGroupList(uid_t uid, gid_t gid, gid_t
> **list)
>
> for (i = 0; i < ret; i++) {
> if ((*list)[i] == gid)
> - goto cleanup;
> + return ret;
> }
> if (VIR_APPEND_ELEMENT(*list, i, gid) < 0) {
> ret = -1;
> VIR_FREE(*list);
> - goto cleanup;
> + return ret;
> } else {
> ret = i;
> }
> }
>
> - cleanup:
> - VIR_FREE(user);
> return ret;
> }
>
> @@ -1405,8 +1395,8 @@ virSetDeviceUnprivSGIO(const char *path,
> const char *sysfs_dir,
> int unpriv_sgio)
> {
> - char *sysfs_path = NULL;
> - char *val = NULL;
> + g_autofree char *sysfs_path = NULL;
> + g_autofree char *val = NULL;
> int ret = -1;
> int rc;
>
> @@ -1416,20 +1406,17 @@ virSetDeviceUnprivSGIO(const char *path,
> if (!virFileExists(sysfs_path)) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("unpriv_sgio is not supported by this
> kernel"));
> - goto cleanup;
> + return ret;
> }
>
> val = g_strdup_printf("%d", unpriv_sgio);
>
> if ((rc = virFileWriteStr(sysfs_path, val, 0)) < 0) {
> virReportSystemError(-rc, _("failed to set %s"),
> sysfs_path);
> - goto cleanup;
> + return ret;
> }
>
> ret = 0;
> - cleanup:
> - VIR_FREE(sysfs_path);
> - VIR_FREE(val);
> return ret;
> }
>
> @@ -1438,8 +1425,8 @@ virGetDeviceUnprivSGIO(const char *path,
> const char *sysfs_dir,
> int *unpriv_sgio)
> {
> - char *sysfs_path = NULL;
> - char *buf = NULL;
> + g_autofree char *sysfs_path = NULL;
> + g_autofree char *buf = NULL;
> char *tmp = NULL;
> int ret = -1;
>
> @@ -1449,11 +1436,11 @@ virGetDeviceUnprivSGIO(const char *path,
> if (!virFileExists(sysfs_path)) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("unpriv_sgio is not supported by this
> kernel"));
> - goto cleanup;
> + return ret;
> }
>
> if (virFileReadAll(sysfs_path, 1024, &buf) < 0)
> - goto cleanup;
> + return ret;
>
> if ((tmp = strchr(buf, '\n')))
> *tmp = '\0';
> @@ -1461,13 +1448,10 @@ virGetDeviceUnprivSGIO(const char *path,
> if (virStrToLong_i(buf, NULL, 10, unpriv_sgio) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("failed to parse value of %s"),
> sysfs_path);
> - goto cleanup;
> + return ret;
> }
>
> ret = 0;
> - cleanup:
> - VIR_FREE(sysfs_path);
> - VIR_FREE(buf);
> return ret;
> }
>
> @@ -1488,7 +1472,7 @@ virParseOwnershipIds(const char *label, uid_t
> *uidPtr, gid_t *gidPtr)
> int rc = -1;
> uid_t theuid;
> gid_t thegid;
> - char *tmp_label = NULL;
> + g_autofree char *tmp_label = NULL;
> char *sep = NULL;
> char *owner = NULL;
> char *group = NULL;
> @@ -1501,7 +1485,7 @@ virParseOwnershipIds(const char *label, uid_t
> *uidPtr, gid_t *gidPtr)
> virReportError(VIR_ERR_INVALID_ARG,
> _("Failed to parse uid and gid from '%s'"),
> label);
> - goto cleanup;
> + return rc;
> }
> *sep = '\0';
> owner = tmp_label;
> @@ -1512,7 +1496,7 @@ virParseOwnershipIds(const char *label, uid_t
> *uidPtr, gid_t *gidPtr)
> */
> if (virGetUserID(owner, &theuid) < 0 ||
> virGetGroupID(group, &thegid) < 0)
> - goto cleanup;
> + return rc;
>
> if (uidPtr)
> *uidPtr = theuid;
> @@ -1520,10 +1504,6 @@ virParseOwnershipIds(const char *label, uid_t
> *uidPtr, gid_t *gidPtr)
> *gidPtr = thegid;
>
> rc = 0;
> -
> - cleanup:
> - VIR_FREE(tmp_label);
> -
> return rc;
> }
>
> diff --git a/src/util/virvhba.c b/src/util/virvhba.c
> index a4e88024d1..d8642ea041 100644
> --- a/src/util/virvhba.c
> +++ b/src/util/virvhba.c
> @@ -49,7 +49,7 @@ bool
> virVHBAPathExists(const char *sysfs_prefix,
> int host)
> {
> - char *sysfs_path = NULL;
> + g_autofree char *sysfs_path = NULL;
> bool ret = false;
>
> sysfs_path = g_strdup_printf("%s/host%d",
> @@ -58,7 +58,6 @@ virVHBAPathExists(const char *sysfs_prefix,
> if (virFileExists(sysfs_path))
> ret = true;
>
> - VIR_FREE(sysfs_path);
> return ret;
> }
>
> @@ -79,8 +78,8 @@ bool
> virVHBAIsVportCapable(const char *sysfs_prefix,
> int host)
> {
> - char *scsi_host_path = NULL;
> - char *fc_host_path = NULL;
> + g_autofree char *scsi_host_path = NULL;
> + g_autofree char *fc_host_path = NULL;
> bool ret = false;
>
> fc_host_path = g_strdup_printf("%s/host%d/%s",
> @@ -94,8 +93,6 @@ virVHBAIsVportCapable(const char *sysfs_prefix,
> if (virFileExists(fc_host_path) ||
> virFileExists(scsi_host_path))
> ret = true;
>
> - VIR_FREE(fc_host_path);
> - VIR_FREE(scsi_host_path);
> return ret;
> }
>
> @@ -115,19 +112,19 @@ virVHBAGetConfig(const char *sysfs_prefix,
> int host,
> const char *entry)
> {
> - char *sysfs_path = NULL;
> + g_autofree char *sysfs_path = NULL;
> char *p = NULL;
> - char *buf = NULL;
> + g_autofree char *buf = NULL;
> char *result = NULL;
>
> sysfs_path = g_strdup_printf("%s/host%d/%s",
> sysfs_prefix ? sysfs_prefix :
> SYSFS_FC_HOST_PATH, host, entry);
>
> if (!virFileExists(sysfs_path))
> - goto cleanup;
> + return result;
>
> if (virFileReadAll(sysfs_path, 1024, &buf) < 0)
> - goto cleanup;
> + return result;
>
> if ((p = strchr(buf, '\n')))
> *p = '\0';
> @@ -139,9 +136,6 @@ virVHBAGetConfig(const char *sysfs_prefix,
>
> result = g_strdup(p);
>
> - cleanup:
> - VIR_FREE(sysfs_path);
> - VIR_FREE(buf);
> return result;
> }
>
> @@ -160,8 +154,8 @@ virVHBAFindVportHost(const char *sysfs_prefix)
> const char *prefix = sysfs_prefix ? sysfs_prefix :
> SYSFS_FC_HOST_PATH;
> g_autoptr(DIR) dir = NULL;
> struct dirent *entry = NULL;
> - char *max_vports = NULL;
> - char *vports = NULL;
> + g_autofree char *max_vports = NULL;
> + g_autofree char *vports = NULL;
> char *state = NULL;
> char *ret = NULL;
>
> @@ -212,16 +206,13 @@ virVHBAFindVportHost(const char *sysfs_prefix)
> ((strlen(max_vports) == strlen(vports)) &&
> strcmp(max_vports, vports) > 0)) {
> ret = g_strdup(entry->d_name);
> - goto cleanup;
> + return ret;
> }
>
> VIR_FREE(max_vports);
> VIR_FREE(vports);
> }
>
> - cleanup:
> - VIR_FREE(max_vports);
> - VIR_FREE(vports);
> return ret;
> }
>
> @@ -241,7 +232,8 @@ virVHBAManageVport(const int parent_host,
> int operation)
> {
> int ret = -1;
> - char *operation_path = NULL, *vport_name = NULL;
> + g_autofree char *operation_path = NULL;
> + g_autofree char *vport_name = NULL;
> const char *operation_file = NULL;
>
> switch (operation) {
> @@ -254,7 +246,7 @@ virVHBAManageVport(const int parent_host,
> default:
> virReportError(VIR_ERR_OPERATION_INVALID,
> _("Invalid vport operation (%d)"),
> operation);
> - goto cleanup;
> + return ret;
> }
>
> operation_path = g_strdup_printf("%s/host%d/%s",
> SYSFS_FC_HOST_PATH,
> @@ -270,7 +262,7 @@ virVHBAManageVport(const int parent_host,
> _("vport operation '%s' is not supported
> "
> "for host%d"),
> operation_file, parent_host);
> - goto cleanup;
> + return ret;
> }
> }
>
> @@ -290,9 +282,6 @@ virVHBAManageVport(const int parent_host,
> "vport create/delete failed"),
> vport_name, operation_path);
>
> - cleanup:
> - VIR_FREE(vport_name);
> - VIR_FREE(operation_path);
> return ret;
> }
>
> @@ -315,8 +304,8 @@ vhbaReadCompareWWN(const char *prefix,
> const char *f_name,
> const char *wwn)
> {
> - char *path;
> - char *buf = NULL;
> + g_autofree char *path = NULL;
> + g_autofree char *buf = NULL;
> char *p;
> int ret = -1;
>
> @@ -324,11 +313,11 @@ vhbaReadCompareWWN(const char *prefix,
>
> if (!virFileExists(path)) {
> ret = 0;
> - goto cleanup;
> + return ret;
> }
>
> if (virFileReadAll(path, 1024, &buf) < 0)
> - goto cleanup;
> + return ret;
>
> if ((p = strchr(buf, '\n')))
> *p = '\0';
> @@ -342,10 +331,6 @@ vhbaReadCompareWWN(const char *prefix,
> else
> ret = 1;
>
> - cleanup:
> - VIR_FREE(path);
> - VIR_FREE(buf);
> -
> return ret;
> }
>
> @@ -407,7 +392,7 @@ virVHBAGetHostByFabricWWN(const char
> *sysfs_prefix,
> const char *prefix = sysfs_prefix ? sysfs_prefix :
> SYSFS_FC_HOST_PATH;
> struct dirent *entry = NULL;
> g_autoptr(DIR) dir = NULL;
> - char *vport_create_path = NULL;
> + g_autofree char *vport_create_path = NULL;
> char *ret = NULL;
>
> if (virDirOpen(&dir, prefix) < 0)
> @@ -428,7 +413,7 @@ virVHBAGetHostByFabricWWN(const char
> *sysfs_prefix,
>
> if ((rc = vhbaReadCompareWWN(prefix, entry->d_name,
> "fabric_name", fabric_wwn)) <
> 0)
> - goto cleanup;
> + return ret;
>
> if (rc == 0)
> continue;
> @@ -437,8 +422,6 @@ virVHBAGetHostByFabricWWN(const char
> *sysfs_prefix,
> break;
> }
>
> - cleanup:
> - VIR_FREE(vport_create_path);
> return ret;
> }
>
> diff --git a/src/util/virxml.c b/src/util/virxml.c
> index a3b819d85c..7df50e4b4d 100644
> --- a/src/util/virxml.c
> +++ b/src/util/virxml.c
> @@ -696,8 +696,8 @@ catchXMLError(void *ctx, const char *msg
> G_GNUC_UNUSED, ...)
> unsigned int n, col; /* GCC warns if signed, because
> compared with sizeof() */
> int domcode = VIR_FROM_XML;
> g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
> - char *contextstr = NULL;
> - char *pointerstr = NULL;
> + g_autofree char *contextstr = NULL;
> + g_autofree char *pointerstr = NULL;
>
>
> /* conditions for error printing */
> @@ -763,9 +763,6 @@ catchXMLError(void *ctx, const char *msg
> G_GNUC_UNUSED, ...)
> contextstr,
> pointerstr);
> }
> -
> - VIR_FREE(contextstr);
> - VIR_FREE(pointerstr);
> }
>
> /**
On 11/19/20 6:46 AM, Tim Wiederhake wrote:
On Wed, 2020-11-18 at 16:47 -0600, Ryan Gahagan wrote:
From: Barrett Schonefeld <bschoney@utexas.edu>
additional conversions to the GLib API in src/util per issue #11.
[...]
return ret; }
I believe you can remove "cleanup" and "ret" as well. More instances below.
The method recommended to me by a few reviewers was to make *only* the conversions to g_autofree in one patch, then have a separate patch that short-circuits the "goto cleanup"s and removes the cleanup: label from those functions that now have an "empty" cleanup. This makes review more mechanical, and thus easier to verify during review.
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 9030f9218a..93bc4a129f 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -164,7 +164,7 @@ addnhostsWrite(const char *path, dnsmasqAddnHost *hosts, unsigned int nhosts) { - char *tmp; + g_autofree char *tmp = NULL; FILE *f; bool istmp = true; size_t i, j; @@ -180,7 +180,7 @@ addnhostsWrite(const char *path, istmp = false; if (!(f = fopen(path, "w"))) { rc = -errno; - goto cleanup; + return rc; Why not "return -errno;"? More instances below.
Yeah, that's another that would be done in the 2nd "remove cleanup: labels" patch - usually you end up with all the returns just directly returning some value (usually 0 or -1, but it could be something like -errno or some other variable), and that very often makes the variable "ret" (or sometimes, as in this case, "rc") unused, so it would be removed as a part of the same patch.
You also might want to split the patch up, e.g. go function by function, to create self-contained changes.
My opinion is that for a mechanical change like this, a separate patch for each function is overkill. And I'm on the fence about even having a separate patch for each file (it depends on how many chunks there are and how similar/simple the chunks are). Having several identical changes in one patch makes it easier to scan through all the chunks without needing to hop from one message to the next (and then either give a blanket R-b: to the series, or else reply to every single one of the tiny patches). (The downside to including too many pieces of too many files in one patch is that if some other unrelated future patch needs to be backported to a distro's stable maintenance branch of libvirt, and that patch created a merge conflict if the patch you made hadn't also been backported to the branch, then the maintainer would either need to backport one large patch rather than one small patch. In the case of g_autofree conversion patches )and other similar simple changes), I'd say backporting the larger patch would create any extra problems, but then I don't backport a lot of patches to downstream branches :-)
I appreciate the feedback on this patch! I will work on splitting this into multiple patches. I believe this will involve redoing much of the work because I will need to split this patch (a single commit) into many commits. Hence, I'd like to get some confirmation on how I should approach the patch. I plan to: 1. Address the feedback on returning `-errno`, `0`, `-1`, etc. directly instead of setting the local variable, `ret`, and returning `ret`. 2. Submit a patch per file with only the g_autofree changes. 3. Submit a patch per file that removes the cleanup sections. On Thu, Nov 19, 2020 at 9:57 AM Laine Stump <laine@redhat.com> wrote:
On 11/19/20 6:46 AM, Tim Wiederhake wrote:
On Wed, 2020-11-18 at 16:47 -0600, Ryan Gahagan wrote:
From: Barrett Schonefeld <bschoney@utexas.edu>
additional conversions to the GLib API in src/util per issue #11.
[...]
return ret; }
I believe you can remove "cleanup" and "ret" as well. More instances below.
The method recommended to me by a few reviewers was to make *only* the conversions to g_autofree in one patch, then have a separate patch that short-circuits the "goto cleanup"s and removes the cleanup: label from those functions that now have an "empty" cleanup. This makes review more mechanical, and thus easier to verify during review.
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 9030f9218a..93bc4a129f 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -164,7 +164,7 @@ addnhostsWrite(const char *path, dnsmasqAddnHost *hosts, unsigned int nhosts) { - char *tmp; + g_autofree char *tmp = NULL; FILE *f; bool istmp = true; size_t i, j; @@ -180,7 +180,7 @@ addnhostsWrite(const char *path, istmp = false; if (!(f = fopen(path, "w"))) { rc = -errno; - goto cleanup; + return rc; Why not "return -errno;"? More instances below.
Yeah, that's another that would be done in the 2nd "remove cleanup: labels" patch - usually you end up with all the returns just directly returning some value (usually 0 or -1, but it could be something like -errno or some other variable), and that very often makes the variable "ret" (or sometimes, as in this case, "rc") unused, so it would be removed as a part of the same patch.
You also might want to split the patch up, e.g. go function by function, to create self-contained changes.
My opinion is that for a mechanical change like this, a separate patch for each function is overkill. And I'm on the fence about even having a separate patch for each file (it depends on how many chunks there are and how similar/simple the chunks are). Having several identical changes in one patch makes it easier to scan through all the chunks without needing to hop from one message to the next (and then either give a blanket R-b: to the series, or else reply to every single one of the tiny patches).
(The downside to including too many pieces of too many files in one patch is that if some other unrelated future patch needs to be backported to a distro's stable maintenance branch of libvirt, and that patch created a merge conflict if the patch you made hadn't also been backported to the branch, then the maintainer would either need to backport one large patch rather than one small patch. In the case of g_autofree conversion patches )and other similar simple changes), I'd say backporting the larger patch would create any extra problems, but then I don't backport a lot of patches to downstream branches :-)
On 11/20/20 11:43 AM, Barrett J Schonefeld wrote:
I appreciate the feedback on this patch!
I will work on splitting this into multiple patches. I believe this will involve redoing much of the work because I will need to split this patch (a single commit) into many commits.
One suggestion on how to more easily split one patch into multiple patches (keeping in mind there's probably a much cleaner way of doing the same thing; this is just how I've evolved to do it): 1) make a new branch "X-v2" based off the branch "X" that has this current patch 2) "git reset HEAD^" on the new branch - this will remove the last commit from git, but leave the working copies of the file unchanged. 3) use a tool like "git meld" to interactively go through all the changes (hunks) you've made in this single commit, *un*doing the ones that aren't related to basic g_autofree conversion. 4) git add / git commit -m"convert to g_autofree" 5) "git meld X" to compare the original commit on the *old* branch to the tip of the new branch, interactively re-applying all the hunks that you had just removed. 5) git add / git commit -m"remove unnecessary cleanup labels and return variables"
Hence, I'd like to get some confirmation on how I should approach the patch.
I plan to:
1. Address the feedback on returning `-errno`, `0`, `-1`, etc. directly instead of setting the local variable, `ret`, and returning `ret`. 2. Submit a patch per file with only the g_autofree changes. 3. Submit a patch per file that removes the cleanup sections.
A couple of qualifiers: a) changing the return values to constants will of course happen as a part of the "cleanup label removal" patch (item (3)), not on its own. b) a recent patch from jtomko reminded me of two occasions when you *would* want a separate patch for the g_autofree changed in a single function all by itself: ii) if that change fixes a bug (which would usually be a memory leak), and/or iii) if it requires any change in the logic of the function beyond simply adding "g_autofree virBlahPtr blah = NULL;" and removing the VIR_FREE(blah); at the end of the scope. Both of these would warrant extra explanation in the commit log, and that's easier to follow if it's isolated.
Updated the patch with the suggested changes. The last commit for the new patch is https://www.redhat.com/archives/libvir-list/2020-November/msg01324.html. On Fri, Nov 20, 2020 at 2:55 PM Laine Stump <laine@redhat.com> wrote:
On 11/20/20 11:43 AM, Barrett J Schonefeld wrote:
I appreciate the feedback on this patch!
I will work on splitting this into multiple patches. I believe this will involve redoing much of the work because I will need to split this patch (a single commit) into many commits.
One suggestion on how to more easily split one patch into multiple patches (keeping in mind there's probably a much cleaner way of doing the same thing; this is just how I've evolved to do it):
1) make a new branch "X-v2" based off the branch "X" that has this current patch
2) "git reset HEAD^" on the new branch - this will remove the last commit from git, but leave the working copies of the file unchanged.
3) use a tool like "git meld" to interactively go through all the changes (hunks) you've made in this single commit, *un*doing the ones that aren't related to basic g_autofree conversion.
4) git add / git commit -m"convert to g_autofree"
5) "git meld X" to compare the original commit on the *old* branch to the tip of the new branch, interactively re-applying all the hunks that you had just removed.
5) git add / git commit -m"remove unnecessary cleanup labels and return variables"
Hence, I'd like to get some confirmation on how I should approach the patch.
I plan to:
1. Address the feedback on returning `-errno`, `0`, `-1`, etc. directly instead of setting the local variable, `ret`, and returning `ret`. 2. Submit a patch per file with only the g_autofree changes. 3. Submit a patch per file that removes the cleanup sections.
A couple of qualifiers:
a) changing the return values to constants will of course happen as a part of the "cleanup label removal" patch (item (3)), not on its own.
b) a recent patch from jtomko reminded me of two occasions when you *would* want a separate patch for the g_autofree changed in a single function all by itself:
ii) if that change fixes a bug (which would usually be a memory leak),
and/or
iii) if it requires any change in the logic of the function beyond simply adding "g_autofree virBlahPtr blah = NULL;" and removing the VIR_FREE(blah); at the end of the scope.
Both of these would warrant extra explanation in the commit log, and that's easier to follow if it's isolated.
participants (4)
-
Barrett J Schonefeld -
Laine Stump -
Ryan Gahagan -
Tim Wiederhake