On a Wednesday in 2020, Laine Stump wrote: My Clang complains that 'utf8' might be used uninitialized if the first part of the condition is true. With 'utf8' initialized: Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Jano

On a Wednesday in 2020, Laine Stump wrote: Missing ErrorReport if xmlNodeGetContent fails. Converting this open-coded for loop to an actual for loop would grant us 'continue' privileges, which would make the checks nicer and give a possibility of assigning the path directly to 'path', without the extra steal_pointer. Alternatively, the minimum diff where I'd consider this patch to be a strict improvement is: } else if (node->type == XML_ELEMENT_NODE && !c) { virReportOOMError(); return -1; } With that: Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Jano

The strings allocated in virGetHostnameImpl() are all allocated via g_strdup(), which will exit on OOM anyway, so the call to virReportOOMError() is redundant, and removing it allows slight modification to the code, in particular the cleanup label can be eliminated. Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/util/virutil.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index f39122e8dd..04f882fda7 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -503,8 +503,7 @@ virGetHostnameImpl(bool quiet) * string as-is; it's up to callers to check whether "localhost" * is allowed. */ - result = g_strdup(hostname); - goto cleanup; + return g_strdup(hostname); } /* otherwise, it's a shortened, non-localhost, hostname. Attempt to @@ -519,8 +518,7 @@ virGetHostnameImpl(bool quiet) if (!quiet) VIR_WARN("getaddrinfo failed for '%s': %s", hostname, gai_strerror(r)); - result = g_strdup(hostname); - goto cleanup; + return g_strdup(hostname); } /* Tell static analyzers about getaddrinfo semantics. */ @@ -538,10 +536,6 @@ virGetHostnameImpl(bool quiet) result = g_strdup(info->ai_canonname); freeaddrinfo(info); - - cleanup: - if (!result) - virReportOOMError(); return result; } -- 2.25.4

The error: label in this function just does "return -1", so replace all the "goto error" in the function with "return -1". Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/conf/domain_conf.c | 91 ++++++++++++++++++++---------------------- 1 file changed, 44 insertions(+), 47 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4d27c9caa8..243590854f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19312,14 +19312,14 @@ virDomainFeaturesDefParse(virDomainDefPtr def, int n; if ((n = virXPathNodeSet("./features/*", ctxt, &nodes)) < 0) - goto error; + return -1; for (i = 0; i < n; i++) { int val = virDomainFeatureTypeFromString((const char *)nodes[i]->name); if (val < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unexpected feature '%s'"), nodes[i]->name); - goto error; + return -1; } switch ((virDomainFeature) val) { @@ -19330,7 +19330,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown value for attribute eoi: '%s'"), tmp); - goto error; + return -1; } def->apic_eoi = eoi; VIR_FREE(tmp); @@ -19353,7 +19353,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown policy attribute '%s' of feature '%s'"), tmp, virDomainFeatureTypeToString(val)); - goto error; + return -1; } VIR_FREE(tmp); } else { @@ -19372,7 +19372,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown state attribute '%s' of feature '%s'"), tmp, virDomainFeatureTypeToString(val)); - goto error; + return -1; } VIR_FREE(tmp); } else { @@ -19386,7 +19386,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, if (gic_version < 0 || gic_version == VIR_GIC_VERSION_NONE) { virReportError(VIR_ERR_XML_ERROR, _("malformed gic version: %s"), tmp); - goto error; + return -1; } def->gic_version = gic_version; VIR_FREE(tmp); @@ -19402,7 +19402,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown driver mode: %s"), tmp); - goto error; + return -1; } def->features[val] = value; VIR_FREE(tmp); @@ -19417,7 +19417,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown HPT resizing setting: %s"), tmp); - goto error; + return -1; } def->hpt_resizing = (virDomainHPTResizing) value; VIR_FREE(tmp); @@ -19433,7 +19433,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Unable to parse HPT maxpagesize setting")); - goto error; + return -1; } def->hpt_maxpagesize = VIR_DIV_UP(def->hpt_maxpagesize, 1024); @@ -19451,7 +19451,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown value: %s"), tmp); - goto error; + return -1; } def->features[val] = value; VIR_FREE(tmp); @@ -19466,7 +19466,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown value: %s"), tmp); - goto error; + return -1; } def->features[val] = value; VIR_FREE(tmp); @@ -19481,7 +19481,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown value: %s"), tmp); - goto error; + return -1; } def->features[val] = value; VIR_FREE(tmp); @@ -19495,13 +19495,13 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("missing state attribute '%s' of feature '%s'"), tmp, virDomainFeatureTypeToString(val)); - goto error; + return -1; } if ((def->features[val] = virTristateSwitchTypeFromString(tmp)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown state attribute '%s' of feature '%s'"), tmp, virDomainFeatureTypeToString(val)); - goto error; + return -1; } VIR_FREE(tmp); break; @@ -19518,7 +19518,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, int value; node = ctxt->node; if ((n = virXPathNodeSet("./features/hyperv/*", ctxt, &nodes)) < 0) - goto error; + return -1; for (i = 0; i < n; i++) { feature = virDomainHypervTypeFromString((const char *)nodes[i]->name); @@ -19526,7 +19526,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported HyperV Enlightenment feature: %s"), nodes[i]->name); - goto error; + return -1; } ctxt->node = nodes[i]; @@ -19536,7 +19536,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, _("missing 'state' attribute for " "HyperV Enlightenment feature '%s'"), nodes[i]->name); - goto error; + return -1; } if ((value = virTristateSwitchTypeFromString(tmp)) < 0) { @@ -19544,7 +19544,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, _("invalid value of state argument " "for HyperV Enlightenment feature '%s'"), nodes[i]->name); - goto error; + return -1; } VIR_FREE(tmp); @@ -19573,14 +19573,14 @@ virDomainFeaturesDefParse(virDomainDefPtr def, &def->hyperv_spinlocks) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid HyperV spinlock retry count")); - goto error; + return -1; } if (def->hyperv_spinlocks < 0xFFF) { virReportError(VIR_ERR_XML_ERROR, "%s", _("HyperV spinlock retry count must be " "at least 4095")); - goto error; + return -1; } break; @@ -19593,7 +19593,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_XML_ERROR, "%s", _("missing 'value' attribute for " "HyperV feature 'vendor_id'")); - goto error; + return -1; } if (strlen(def->hyperv_vendor_id) > VIR_DOMAIN_HYPERV_VENDOR_ID_MAX) { @@ -19601,14 +19601,14 @@ virDomainFeaturesDefParse(virDomainDefPtr def, _("HyperV vendor_id value must not be more " "than %d characters."), VIR_DOMAIN_HYPERV_VENDOR_ID_MAX); - goto error; + return -1; } /* ensure that the string can be passed to qemu */ if (strchr(def->hyperv_vendor_id, ',')) { virReportError(VIR_ERR_XML_ERROR, "%s", _("HyperV vendor_id value is invalid")); - goto error; + return -1; } /* coverity[dead_error_begin] */ @@ -19623,28 +19623,28 @@ virDomainFeaturesDefParse(virDomainDefPtr def, if (def->features[VIR_DOMAIN_HYPERV_STIMER] == VIR_TRISTATE_SWITCH_ON) { int value; if ((n = virXPathNodeSet("./features/hyperv/stimer/*", ctxt, &nodes)) < 0) - goto error; + return -1; for (i = 0; i < n; i++) { if (STRNEQ((const char *)nodes[i]->name, "direct")) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported Hyper-V stimer feature: %s"), nodes[i]->name); - goto error; + return -1; } if (!(tmp = virXMLPropString(nodes[i], "state"))) { virReportError(VIR_ERR_XML_ERROR, _("missing 'state' attribute for " "Hyper-V stimer '%s' feature"), "direct"); - goto error; + return -1; } if ((value = virTristateSwitchTypeFromString(tmp)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("invalid value of state argument " "for Hyper-V stimer '%s' feature"), "direct"); - goto error; + return -1; } VIR_FREE(tmp); @@ -19657,7 +19657,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, int feature; int value; if ((n = virXPathNodeSet("./features/kvm/*", ctxt, &nodes)) < 0) - goto error; + return -1; for (i = 0; i < n; i++) { feature = virDomainKVMTypeFromString((const char *)nodes[i]->name); @@ -19665,7 +19665,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported KVM feature: %s"), nodes[i]->name); - goto error; + return -1; } switch ((virDomainKVM) feature) { @@ -19676,7 +19676,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, _("missing 'state' attribute for " "KVM feature '%s'"), nodes[i]->name); - goto error; + return -1; } if ((value = virTristateSwitchTypeFromString(tmp)) < 0) { @@ -19684,7 +19684,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, _("invalid value of state argument " "for KVM feature '%s'"), nodes[i]->name); - goto error; + return -1; } VIR_FREE(tmp); @@ -19705,7 +19705,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, g_autofree char *ptval = NULL; if ((n = virXPathNodeSet("./features/xen/*", ctxt, &nodes)) < 0) - goto error; + return -1; for (i = 0; i < n; i++) { feature = virDomainXenTypeFromString((const char *)nodes[i]->name); @@ -19713,7 +19713,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported Xen feature: %s"), nodes[i]->name); - goto error; + return -1; } if (!(tmp = virXMLPropString(nodes[i], "state"))) { @@ -19721,7 +19721,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, _("missing 'state' attribute for " "Xen feature '%s'"), nodes[i]->name); - goto error; + return -1; } if ((value = virTristateSwitchTypeFromString(tmp)) < 0) { @@ -19729,7 +19729,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, _("invalid value of state argument " "for Xen feature '%s'"), nodes[i]->name); - goto error; + return -1; } VIR_FREE(tmp); @@ -19750,7 +19750,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported mode '%s' for Xen passthrough feature"), ptval); - goto error; + return -1; } if (mode != VIR_DOMAIN_XEN_PASSTHROUGH_MODE_SYNC_PT && @@ -19758,7 +19758,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_XML_ERROR, "%s", _("'mode' attribute for Xen feature " "'passthrough' must be 'sync_pt' or 'share_pt'")); - goto error; + return -1; } def->xen_passthrough_mode = mode; } @@ -19781,39 +19781,39 @@ virDomainFeaturesDefParse(virDomainDefPtr def, ULLONG_MAX, false); if (rv < 0) - goto error; + return -1; def->tseg_specified = rv; } if (def->features[VIR_DOMAIN_FEATURE_MSRS] == VIR_TRISTATE_SWITCH_ON) { if ((node = virXPathNode("./features/msrs", ctxt)) == NULL) - goto error; + return -1; if (!(tmp = virXMLPropString(node, "unknown"))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("missing 'unknown' attribute for feature '%s'"), virDomainFeatureTypeToString(VIR_DOMAIN_FEATURE_MSRS)); - goto error; + return -1; } if ((def->msrs_features[VIR_DOMAIN_MSRS_UNKNOWN] = virDomainMsrsUnknownTypeFromString(tmp)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown 'unknown' value '%s'"), tmp); - goto error; + return -1; } VIR_FREE(tmp); } if ((n = virXPathNodeSet("./features/capabilities/*", ctxt, &nodes)) < 0) - goto error; + return -1; for (i = 0; i < n; i++) { int val = virDomainProcessCapsFeatureTypeFromString((const char *)nodes[i]->name); if (val < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unexpected capability feature '%s'"), nodes[i]->name); - goto error; + return -1; } if ((tmp = virXMLPropString(nodes[i], "state"))) { @@ -19821,7 +19821,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown state attribute '%s' of feature capability '%s'"), tmp, virDomainProcessCapsFeatureTypeToString(val)); - goto error; + return -1; } VIR_FREE(tmp); } else { @@ -19830,9 +19830,6 @@ virDomainFeaturesDefParse(virDomainDefPtr def, } VIR_FREE(nodes); return 0; - - error: - return -1; } -- 2.25.4

The only reason for the error label in this function is to call virBufferFreeAndReset(). It's actually more common for a failed format function to just leave the virBuffer alone and let the caller free it when there is a failure, and in fact the only caller of this function that *wasn't* already calling virBufferFreeAndReset() on failure was virDomainDefFormat() (via virDomainDefFormatInternal()). That is easily solved by modifying virDomainDefFormat() to declare its virBuffer buf with g_auto(), so that virBufferFreeAndReset() is automatically called. Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/conf/domain_conf.c | 88 ++++++++++++++++++++---------------------- 1 file changed, 42 insertions(+), 46 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 243590854f..0307ffcbd6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -29534,7 +29534,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, if (!(type = virDomainVirtTypeToString(def->virtType))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected domain type %d"), def->virtType); - goto error; + return -1; } if (def->id == -1) @@ -29579,13 +29579,13 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, xmlIndentTreeOutput = 1; if (!(xmlbuf = xmlBufferCreate())) { virReportOOMError(); - goto error; + return -1; } if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata, virBufferGetIndent(buf) / 2, 1) < 0) { xmlIndentTreeOutput = oldIndentTreeOutput; - goto error; + return -1; } virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf)); xmlIndentTreeOutput = oldIndentTreeOutput; @@ -29608,13 +29608,13 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, def->mem.cur_balloon); if (virDomainDefFormatBlkiotune(buf, def) < 0) - goto error; + return -1; virDomainMemtuneFormat(buf, &def->mem); virDomainMemorybackingFormat(buf, &def->mem); if (virDomainCpuDefFormat(buf, def) < 0) - goto error; + return -1; if (def->niothreadids > 0) { virBufferAsprintf(buf, "<iothreads>%zu</iothreads>\n", @@ -29632,10 +29632,10 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, } if (virDomainCputuneDefFormat(buf, def, flags) < 0) - goto error; + return -1; if (virDomainNumatuneFormatXML(buf, def->numa) < 0) - goto error; + return -1; if (def->resource) virDomainResourceDefFormat(buf, def->resource); @@ -29720,7 +29720,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected boot device type %d"), def->os.bootDevs[n]); - goto error; + return -1; } virBufferAsprintf(buf, "<boot dev='%s'/>\n", boottype); } @@ -29752,7 +29752,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, if (mode == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected smbios mode %d"), def->os.smbios_mode); - goto error; + return -1; } virBufferAsprintf(buf, "<smbios mode='%s'/>\n", mode); } @@ -29783,10 +29783,10 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, } if (virDomainDefFormatFeatures(buf, def) < 0) - goto error; + return -1; if (virCPUDefFormatBufFull(buf, def->cpu, def->numa) < 0) - goto error; + return -1; virBufferAsprintf(buf, "<clock offset='%s'", virDomainClockOffsetTypeToString(def->clock.offset)); @@ -29817,7 +29817,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, virBufferAdjustIndent(buf, 2); for (n = 0; n < def->clock.ntimers; n++) { if (virDomainTimerDefFormat(buf, def->clock.timers[n]) < 0) - goto error; + return -1; } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</clock>\n"); @@ -29826,20 +29826,20 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, if (virDomainEventActionDefFormat(buf, def->onPoweroff, "on_poweroff", virDomainLifecycleActionTypeToString) < 0) - goto error; + return -1; if (virDomainEventActionDefFormat(buf, def->onReboot, "on_reboot", virDomainLifecycleActionTypeToString) < 0) - goto error; + return -1; if (virDomainEventActionDefFormat(buf, def->onCrash, "on_crash", virDomainLifecycleActionTypeToString) < 0) - goto error; + return -1; if (def->onLockFailure != VIR_DOMAIN_LOCK_FAILURE_DEFAULT && virDomainEventActionDefFormat(buf, def->onLockFailure, "on_lockfailure", virDomainLockFailureTypeToString) < 0) - goto error; + return -1; if (def->pm.s3 || def->pm.s4) { virBufferAddLit(buf, "<pm>\n"); @@ -29866,35 +29866,35 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, for (n = 0; n < def->ndisks; n++) if (virDomainDiskDefFormat(buf, def->disks[n], flags, xmlopt) < 0) - goto error; + return -1; for (n = 0; n < def->ncontrollers; n++) if (virDomainControllerDefFormat(buf, def->controllers[n], flags) < 0) - goto error; + return -1; for (n = 0; n < def->nleases; n++) if (virDomainLeaseDefFormat(buf, def->leases[n]) < 0) - goto error; + return -1; for (n = 0; n < def->nfss; n++) if (virDomainFSDefFormat(buf, def->fss[n], flags) < 0) - goto error; + return -1; for (n = 0; n < def->nnets; n++) if (virDomainNetDefFormat(buf, def->nets[n], xmlopt, flags) < 0) - goto error; + return -1; for (n = 0; n < def->nsmartcards; n++) if (virDomainSmartcardDefFormat(buf, def->smartcards[n], flags) < 0) - goto error; + return -1; for (n = 0; n < def->nserials; n++) if (virDomainChrDefFormat(buf, def->serials[n], flags) < 0) - goto error; + return -1; for (n = 0; n < def->nparallels; n++) if (virDomainChrDefFormat(buf, def->parallels[n], flags) < 0) - goto error; + return -1; for (n = 0; n < def->nconsoles; n++) { virDomainChrDef console; @@ -29912,36 +29912,36 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, memcpy(&console, def->consoles[n], sizeof(console)); } if (virDomainChrDefFormat(buf, &console, flags) < 0) - goto error; + return -1; } for (n = 0; n < def->nchannels; n++) if (virDomainChrDefFormat(buf, def->channels[n], flags) < 0) - goto error; + return -1; for (n = 0; n < def->ninputs; n++) { if (virDomainInputDefFormat(buf, def->inputs[n], flags) < 0) - goto error; + return -1; } for (n = 0; n < def->ntpms; n++) { if (virDomainTPMDefFormat(buf, def->tpms[n], flags) < 0) - goto error; + return -1; } for (n = 0; n < def->ngraphics; n++) { if (virDomainGraphicsDefFormat(buf, def->graphics[n], flags) < 0) - goto error; + return -1; } for (n = 0; n < def->nsounds; n++) { if (virDomainSoundDefFormat(buf, def->sounds[n], flags) < 0) - goto error; + return -1; } for (n = 0; n < def->nvideos; n++) { if (virDomainVideoDefFormat(buf, def->videos[n], flags) < 0) - goto error; + return -1; } for (n = 0; n < def->nhostdevs; n++) { @@ -29951,13 +29951,13 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, */ if (!def->hostdevs[n]->parentnet && virDomainHostdevDefFormat(buf, def->hostdevs[n], flags) < 0) { - goto error; + return -1; } } for (n = 0; n < def->nredirdevs; n++) { if (virDomainRedirdevDefFormat(buf, def->redirdevs[n], flags) < 0) - goto error; + return -1; } if (def->redirfilter) @@ -29965,7 +29965,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, for (n = 0; n < def->nhubs; n++) { if (virDomainHubDefFormat(buf, def->hubs[n], flags) < 0) - goto error; + return -1; } if (def->watchdog) @@ -29976,7 +29976,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, for (n = 0; n < def->nrngs; n++) { if (virDomainRNGDefFormat(buf, def->rngs[n], flags)) - goto error; + return -1; } if (def->nvram) @@ -29984,26 +29984,26 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, for (n = 0; n < def->npanics; n++) { if (virDomainPanicDefFormat(buf, def->panics[n]) < 0) - goto error; + return -1; } for (n = 0; n < def->nshmems; n++) { if (virDomainShmemDefFormat(buf, def->shmems[n], flags) < 0) - goto error; + return -1; } for (n = 0; n < def->nmems; n++) { if (virDomainMemoryDefFormat(buf, def->mems[n], def, flags) < 0) - goto error; + return -1; } if (def->iommu && virDomainIOMMUDefFormat(buf, def->iommu) < 0) - goto error; + return -1; if (def->vsock && virDomainVsockDefFormat(buf, def->vsock) < 0) - goto error; + return -1; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</devices>\n"); @@ -30018,17 +30018,13 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, if (def->namespaceData && def->ns.format) { if ((def->ns.format)(buf, def->namespaceData) < 0) - goto error; + return -1; } virBufferAdjustIndent(buf, -2); virBufferAsprintf(buf, "</%s>\n", rootname); return 0; - - error: - virBufferFreeAndReset(buf); - return -1; } /* Converts VIR_DOMAIN_XML_COMMON_FLAGS into VIR_DOMAIN_DEF_FORMAT_* @@ -30056,7 +30052,7 @@ virDomainDefFormat(virDomainDefPtr def, virDomainXMLOptionPtr xmlopt, unsigned int flags) { - virBuffer buf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; virCheckFlags(VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS, NULL); if (virDomainDefFormatInternal(def, xmlopt, &buf, flags) < 0) -- 2.25.4

The cleanup function was already added by: commit 2ad0284627ea3d6c123e0a266b9c7bb00aea4576 CommitDate: 2018-07-27 17:21:04 +0200 util: firewall: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC On a Wednesday in 2020, Laine Stump wrote: This is just a whitespace change that contradicts the prevailing style in this file. Jano

This includes those that use plain VIR_FREE() as well as those that have a cleanup function defined for use via g_auto/g_autoptr (virCommand, virFirewall, virBuffer, virJSONValue etc). Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/network/bridge_driver.c | 477 +++++++++++------------------- src/network/bridge_driver_linux.c | 55 ++-- src/network/leaseshelper.c | 16 +- src/util/virdnsmasq.h | 4 + 4 files changed, 209 insertions(+), 343 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index aff1b7b1bc..668aa9ca88 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -322,25 +322,23 @@ networkRunHook(virNetworkObjPtr obj, int sub_op) { virNetworkDefPtr def; - virBuffer buf = VIR_BUFFER_INITIALIZER; - char *xml = NULL; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + g_autofree char *xml = NULL; int hookret; - int ret = -1; if (virHookPresent(VIR_HOOK_DRIVER_NETWORK)) { if (!obj) { VIR_DEBUG("Not running hook as @obj is NULL"); - ret = 0; - goto cleanup; + return 0; } def = virNetworkObjGetDef(obj); virBufferAddLit(&buf, "<hookData>\n"); virBufferAdjustIndent(&buf, 2); if (virNetworkDefFormatBuf(&buf, def, network_driver->xmlopt, 0) < 0) - goto cleanup; + return -1; if (port && virNetworkPortDefFormatBuf(&buf, port) < 0) - goto cleanup; + return -1; virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</hookData>"); @@ -353,16 +351,12 @@ networkRunHook(virNetworkObjPtr obj, * If the script raised an error, pass it to the callee. */ if (hookret < 0) - goto cleanup; + return -1; networkNetworkObjTaint(obj, VIR_NETWORK_TAINT_HOOK); } - ret = 0; - cleanup: - virBufferFreeAndReset(&buf); - VIR_FREE(xml); - return ret; + return 0; } @@ -426,44 +420,42 @@ static int networkRemoveInactive(virNetworkDriverStatePtr driver, virNetworkObjPtr obj) { - char *leasefile = NULL; - char *customleasefile = NULL; - char *radvdconfigfile = NULL; - char *configfile = NULL; - char *radvdpidbase = NULL; - char *statusfile = NULL; - char *macMapFile = NULL; - dnsmasqContext *dctx = NULL; + g_autofree char *leasefile = NULL; + g_autofree char *customleasefile = NULL; + g_autofree char *radvdconfigfile = NULL; + g_autofree char *configfile = NULL; + g_autofree char *radvdpidbase = NULL; + g_autofree char *statusfile = NULL; + g_autofree char *macMapFile = NULL; + g_autoptr(dnsmasqContext) dctx = NULL; virNetworkDefPtr def = virNetworkObjGetPersistentDef(obj); - int ret = -1; - /* remove the (possibly) existing dnsmasq and radvd files */ if (!(dctx = dnsmasqContextNew(def->name, driver->dnsmasqStateDir))) { - goto cleanup; + return -1; } if (!(leasefile = networkDnsmasqLeaseFileNameDefault(driver, def->name))) - goto cleanup; + return -1; if (!(customleasefile = networkDnsmasqLeaseFileNameCustom(driver, def->bridge))) - goto cleanup; + return -1; if (!(radvdconfigfile = networkRadvdConfigFileName(driver, def->name))) - goto cleanup; + return -1; if (!(radvdpidbase = networkRadvdPidfileBasename(def->name))) - goto cleanup; + return -1; if (!(configfile = networkDnsmasqConfigFileName(driver, def->name))) - goto cleanup; + return -1; if (!(statusfile = virNetworkConfigFile(driver->stateDir, def->name))) - goto cleanup; + return -1; if (!(macMapFile = virMacMapFileName(driver->dnsmasqStateDir, def->bridge))) - goto cleanup; + return -1; /* dnsmasq */ dnsmasqDelete(dctx); @@ -484,18 +476,7 @@ networkRemoveInactive(virNetworkDriverStatePtr driver, /* remove the network definition */ virNetworkObjRemoveInactive(driver->networks, obj); - ret = 0; - - cleanup: - VIR_FREE(leasefile); - VIR_FREE(configfile); - VIR_FREE(customleasefile); - VIR_FREE(radvdconfigfile); - VIR_FREE(radvdpidbase); - VIR_FREE(statusfile); - VIR_FREE(macMapFile); - dnsmasqContextFree(dctx); - return ret; + return 0; } @@ -545,9 +526,9 @@ networkUpdateState(virNetworkObjPtr obj, { virNetworkDefPtr def; virNetworkDriverStatePtr driver = opaque; - dnsmasqCapsPtr dnsmasq_caps = networkGetDnsmasqCaps(driver); + g_autoptr(dnsmasqCaps) dnsmasq_caps = networkGetDnsmasqCaps(driver); virMacMapPtr macmap; - char *macMapFile = NULL; + g_autofree char *macMapFile = NULL; int ret = -1; virObjectLock(obj); @@ -609,7 +590,7 @@ networkUpdateState(virNetworkObjPtr obj, if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) { pid_t radvdPid; pid_t dnsmasqPid; - char *radvdpidbase; + g_autofree char *radvdpidbase = NULL; ignore_value(virPidFileReadIfAlive(driver->pidDir, def->name, @@ -625,14 +606,11 @@ networkUpdateState(virNetworkObjPtr obj, radvdpidbase, &radvdPid, RADVD)); virNetworkObjSetRadvdPid(obj, radvdPid); - VIR_FREE(radvdpidbase); } ret = 0; cleanup: virObjectUnlock(obj); - virObjectUnref(dnsmasq_caps); - VIR_FREE(macMapFile); return ret; } @@ -710,9 +688,8 @@ networkStateInitialize(bool privileged, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { - int ret = VIR_DRV_STATE_INIT_ERROR; - char *configdir = NULL; - char *rundir = NULL; + g_autofree char *configdir = NULL; + g_autofree char *rundir = NULL; bool autostart = true; #ifdef WITH_FIREWALLD DBusConnection *sysbus = NULL; @@ -839,15 +816,12 @@ networkStateInitialize(bool privileged, } #endif - ret = VIR_DRV_STATE_INIT_COMPLETE; - cleanup: - VIR_FREE(configdir); - VIR_FREE(rundir); - return ret; + return VIR_DRV_STATE_INIT_COMPLETE; + error: networkStateCleanup(); - goto cleanup; + return VIR_DRV_STATE_INIT_ERROR; } @@ -1043,10 +1017,11 @@ networkDnsmasqConfLocalPTRs(virBufferPtr buf, { virNetworkIPDefPtr ip; size_t i; - char *ptr = NULL; int rc; for (i = 0; i < def->nips; i++) { + g_autofree char *ptr = NULL; + ip = def->ips + i; if (ip->localPTR != VIR_TRISTATE_BOOL_YES) @@ -1067,7 +1042,6 @@ networkDnsmasqConfLocalPTRs(virBufferPtr buf, } virBufferAsprintf(buf, "local=/%s/\n", ptr); - VIR_FREE(ptr); } return 0; @@ -1083,15 +1057,14 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, dnsmasqCapsPtr caps G_GNUC_UNUSED) { virNetworkDefPtr def = virNetworkObjGetDef(obj); - virBuffer configbuf = VIR_BUFFER_INITIALIZER; - int r, ret = -1; + g_auto(virBuffer) configbuf = VIR_BUFFER_INITIALIZER; + int r; int nbleases = 0; size_t i; virNetworkDNSDefPtr dns = &def->dns; bool wantDNS = dns->enable != VIR_TRISTATE_BOOL_NO; virNetworkIPDefPtr tmpipdef, ipdef, ipv4def, ipv6def; bool ipv6SLAAC; - char *saddr = NULL, *eaddr = NULL; *configstr = NULL; @@ -1146,12 +1119,11 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, if (fwd->domain) virBufferAsprintf(&configbuf, "/%s/", fwd->domain); if (VIR_SOCKET_ADDR_VALID(&fwd->addr)) { - char *addr = virSocketAddrFormat(&fwd->addr); + g_autofree char *addr = virSocketAddrFormat(&fwd->addr); if (!addr) - goto cleanup; + return -1; virBufferAsprintf(&configbuf, "%s\n", addr); - VIR_FREE(addr); if (!fwd->domain) addNoResolv = true; } else { @@ -1177,7 +1149,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, if (wantDNS && networkDnsmasqConfLocalPTRs(&configbuf, def) < 0) - goto cleanup; + return -1; if (wantDNS && def->dns.forwardPlainNames == VIR_TRISTATE_BOOL_NO) { virBufferAddLit(&configbuf, "domain-needed\n"); @@ -1224,10 +1196,10 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, for (i = 0; (tmpipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i)); i++) { - char *ipaddr = virSocketAddrFormat(&tmpipdef->address); + g_autofree char *ipaddr = virSocketAddrFormat(&tmpipdef->address); if (!ipaddr) - goto cleanup; + return -1; /* also part of CVE 2012-3411 - if the host's version of * dnsmasq doesn't have bind-dynamic, only allow listening on @@ -1250,11 +1222,9 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, "(as described in RFC1918/RFC3484/RFC4193)."), ipaddr, (int)version / 1000000, (int)(version % 1000000) / 1000); - VIR_FREE(ipaddr); - goto cleanup; + return -1; } virBufferAsprintf(&configbuf, "listen-address=%s\n", ipaddr); - VIR_FREE(ipaddr); } } @@ -1293,14 +1263,14 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, _("Missing required 'service' " "attribute in SRV record of network '%s'"), def->name); - goto cleanup; + return -1; } if (!dns->srvs[i].protocol) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Missing required 'service' " "attribute in SRV record of network '%s'"), def->name); - goto cleanup; + return -1; } /* RFC2782 requires that service and protocol be preceded by * an underscore. @@ -1349,7 +1319,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("For IPv4, multiple DHCP definitions " "cannot be specified.")); - goto cleanup; + return -1; } else { ipv4def = ipdef; } @@ -1369,13 +1339,13 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, (int)(version % 1000000) / 1000, DNSMASQ_DHCPv6_MAJOR_REQD, DNSMASQ_DHCPv6_MINOR_REQD); - goto cleanup; + return -1; } if (ipv6def) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("For IPv6, multiple DHCP definitions " "cannot be specified.")); - goto cleanup; + return -1; } else { ipv6def = ipdef; } @@ -1404,16 +1374,18 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, virReportError(VIR_ERR_INTERNAL_ERROR, _("bridge '%s' has an invalid prefix"), def->bridge); - goto cleanup; + return -1; } for (r = 0; r < ipdef->nranges; r++) { int thisRange; virNetworkDHCPRangeDef range = ipdef->ranges[r]; g_autofree char *leasetime = NULL; + g_autofree char *saddr = NULL; + g_autofree char *eaddr = NULL; if (!(saddr = virSocketAddrFormat(&range.addr.start)) || !(eaddr = virSocketAddrFormat(&range.addr.end))) - goto cleanup; + return -1; if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%d", @@ -1428,11 +1400,11 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, _("Failed to translate bridge '%s' " "prefix %d to netmask"), def->bridge, prefix); - goto cleanup; + return -1; } if (!(netmaskStr = virSocketAddrFormat(&netmask))) - goto cleanup; + return -1; virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%s", saddr, eaddr, netmaskStr); } @@ -1442,14 +1414,12 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, virBufferAddLit(&configbuf, "\n"); - VIR_FREE(saddr); - VIR_FREE(eaddr); thisRange = virSocketAddrGetRange(&range.addr.start, &range.addr.end, &ipdef->address, virNetworkIPDefPrefix(ipdef)); if (thisRange < 0) - goto cleanup; + return -1; nbleases += thisRange; } @@ -1460,19 +1430,18 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, * support) */ if (!ipdef->nranges && ipdef->nhosts) { - char *bridgeaddr = virSocketAddrFormat(&ipdef->address); + g_autofree char *bridgeaddr = virSocketAddrFormat(&ipdef->address); if (!bridgeaddr) - goto cleanup; + return -1; virBufferAsprintf(&configbuf, "dhcp-range=%s,static", bridgeaddr); if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) virBufferAsprintf(&configbuf, ",%d", prefix); virBufferAddLit(&configbuf, "\n"); - VIR_FREE(bridgeaddr); } if (networkBuildDnsmasqDhcpHostsList(dctx, ipdef) < 0) - goto cleanup; + return -1; /* Note: the following is IPv4 only */ if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) { @@ -1488,13 +1457,12 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, if (ipdef->bootfile) { if (VIR_SOCKET_ADDR_VALID(&ipdef->bootserver)) { - char *bootserver = virSocketAddrFormat(&ipdef->bootserver); + g_autofree char *bootserver = virSocketAddrFormat(&ipdef->bootserver); if (!bootserver) - goto cleanup; + return -1; virBufferAsprintf(&configbuf, "dhcp-boot=%s%s%s\n", ipdef->bootfile, ",,", bootserver); - VIR_FREE(bootserver); } else { virBufferAsprintf(&configbuf, "dhcp-boot=%s\n", ipdef->bootfile); } @@ -1508,7 +1476,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, /* this is done once per interface */ if (networkBuildDnsmasqHostsList(dctx, dns) < 0) - goto cleanup; + return -1; /* Even if there are currently no static hosts, if we're * listening for DHCP, we should write a 0-length hosts @@ -1539,12 +1507,11 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, (ipdef = virNetworkDefGetIPByIndex(def, AF_INET6, i)); i++) { if (!(ipdef->nranges || ipdef->nhosts)) { - char *bridgeaddr = virSocketAddrFormat(&ipdef->address); + g_autofree char *bridgeaddr = virSocketAddrFormat(&ipdef->address); if (!bridgeaddr) - goto cleanup; + return -1; virBufferAsprintf(&configbuf, "dhcp-range=%s,ra-only\n", bridgeaddr); - VIR_FREE(bridgeaddr); } } } @@ -1557,18 +1524,12 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, } if (!(*configstr = virBufferContentAndReset(&configbuf))) - goto cleanup; + return -1; *hostsfilestr = dnsmasqDhcpHostsToString(dctx->hostsfile->hosts, dctx->hostsfile->nhosts); - ret = 0; - - cleanup: - VIR_FREE(saddr); - VIR_FREE(eaddr); - virBufferFreeAndReset(&configbuf); - return ret; + return 0; } @@ -1581,39 +1542,38 @@ networkBuildDhcpDaemonCommandLine(virNetworkDriverStatePtr driver, dnsmasqContext *dctx) { virNetworkDefPtr def = virNetworkObjGetDef(obj); - dnsmasqCapsPtr dnsmasq_caps = networkGetDnsmasqCaps(driver); - virCommandPtr cmd = NULL; - int ret = -1; - char *configfile = NULL; - char *configstr = NULL; - char *hostsfilestr = NULL; - char *leaseshelper_path = NULL; + g_autoptr(dnsmasqCaps) dnsmasq_caps = networkGetDnsmasqCaps(driver); + g_autoptr(virCommand) cmd = NULL; + g_autofree char *configfile = NULL; + g_autofree char *configstr = NULL; + g_autofree char *hostsfilestr = NULL; + g_autofree char *leaseshelper_path = NULL; virNetworkObjSetDnsmasqPid(obj, -1); if (networkDnsmasqConfContents(obj, pidfile, &configstr, &hostsfilestr, dctx, dnsmasq_caps) < 0) - goto cleanup; + return -1; if (!configstr) - goto cleanup; + return -1; /* construct the filename */ if (!(configfile = networkDnsmasqConfigFileName(driver, def->name))) - goto cleanup; + return -1; /* Write the file */ if (virFileWriteStr(configfile, configstr, 0600) < 0) { virReportSystemError(errno, _("couldn't write dnsmasq config file '%s'"), configfile); - goto cleanup; + return -1; } /* This helper is used to create custom leases file for libvirt */ if (!(leaseshelper_path = virFileFindResource("libvirt_leaseshelper", abs_top_builddir "/src", LIBEXECDIR))) - goto cleanup; + return -1; cmd = virCommandNew(dnsmasqCapsGetBinaryPath(dnsmasq_caps)); virCommandAddArgFormat(cmd, "--conf-file=%s", configfile); @@ -1622,15 +1582,8 @@ networkBuildDhcpDaemonCommandLine(virNetworkDriverStatePtr driver, virCommandAddArgFormat(cmd, "--dhcp-script=%s", leaseshelper_path); virCommandAddEnvPair(cmd, "VIR_BRIDGE_NAME", def->bridge); - *cmdout = cmd; - ret = 0; - cleanup: - virObjectUnref(dnsmasq_caps); - VIR_FREE(configfile); - VIR_FREE(configstr); - VIR_FREE(hostsfilestr); - VIR_FREE(leaseshelper_path); - return ret; + *cmdout = g_steal_pointer(&cmd); + return 0; } @@ -1642,11 +1595,10 @@ networkStartDhcpDaemon(virNetworkDriverStatePtr driver, virNetworkIPDefPtr ipdef; size_t i; bool needDnsmasq = false; - virCommandPtr cmd = NULL; - char *pidfile = NULL; + g_autoptr(virCommand) cmd = NULL; + g_autofree char *pidfile = NULL; pid_t dnsmasqPid; - int ret = -1; - dnsmasqContext *dctx = NULL; + g_autoptr(dnsmasqContext) dctx = NULL; /* see if there are any IP addresses that need a dhcp server */ i = 0; @@ -1656,53 +1608,46 @@ networkStartDhcpDaemon(virNetworkDriverStatePtr driver, needDnsmasq = true; } - if (i == 0) { - /* no IP addresses at all, so we don't need to run */ - ret = 0; - goto cleanup; - } + /* no IP addresses at all, so we don't need to run */ + if (i == 0) + return 0; - if (!needDnsmasq && def->dns.enable == VIR_TRISTATE_BOOL_NO) { - /* no DHCP services needed, and user disabled DNS service */ - ret = 0; - goto cleanup; - } + /* no DHCP services needed, and user disabled DNS service */ + if (!needDnsmasq && def->dns.enable == VIR_TRISTATE_BOOL_NO) + return 0; if (virFileMakePath(driver->pidDir) < 0) { virReportSystemError(errno, _("cannot create directory %s"), driver->pidDir); - goto cleanup; + return -1; } if (!(pidfile = virPidFileBuildPath(driver->pidDir, def->name))) - goto cleanup; + return -1; if (virFileMakePath(driver->dnsmasqStateDir) < 0) { virReportSystemError(errno, _("cannot create directory %s"), driver->dnsmasqStateDir); - goto cleanup; + return -1; } dctx = dnsmasqContextNew(def->name, driver->dnsmasqStateDir); if (dctx == NULL) - goto cleanup; + return -1; if (networkDnsmasqCapsRefresh(driver) < 0) - goto cleanup; + return -1; - ret = networkBuildDhcpDaemonCommandLine(driver, obj, &cmd, pidfile, dctx); - if (ret < 0) - goto cleanup; + if (networkBuildDhcpDaemonCommandLine(driver, obj, &cmd, pidfile, dctx) < 0) + return -1; - ret = dnsmasqSave(dctx); - if (ret < 0) - goto cleanup; + if (dnsmasqSave(dctx) < 0) + return -1; - ret = virCommandRun(cmd, NULL); - if (ret < 0) - goto cleanup; + if (virCommandRun(cmd, NULL) < 0) + return -1; /* * There really is no race here - when dnsmasq daemonizes, its @@ -1712,17 +1657,12 @@ networkStartDhcpDaemon(virNetworkDriverStatePtr driver, * pid */ - ret = virPidFileRead(driver->pidDir, def->name, &dnsmasqPid); - if (ret < 0) - goto cleanup; + if (virPidFileRead(driver->pidDir, def->name, &dnsmasqPid) < 0) + return -1; + virNetworkObjSetDnsmasqPid(obj, dnsmasqPid); - ret = 0; - cleanup: - VIR_FREE(pidfile); - virCommandFree(cmd); - dnsmasqContextFree(dctx); - return ret; + return 0; } @@ -1738,11 +1678,10 @@ networkRefreshDhcpDaemon(virNetworkDriverStatePtr driver, virNetworkObjPtr obj) { virNetworkDefPtr def = virNetworkObjGetDef(obj); - int ret = -1; size_t i; pid_t dnsmasqPid; virNetworkIPDefPtr ipdef, ipv4def, ipv6def; - dnsmasqContext *dctx = NULL; + g_autoptr(dnsmasqContext) dctx = NULL; /* if no IP addresses specified, nothing to do */ if (!virNetworkDefGetIPByIndex(def, AF_UNSPEC, 0)) @@ -1754,10 +1693,8 @@ networkRefreshDhcpDaemon(virNetworkDriverStatePtr driver, return networkStartDhcpDaemon(driver, obj); VIR_INFO("Refreshing dnsmasq for network %s", def->bridge); - if (!(dctx = dnsmasqContextNew(def->name, - driver->dnsmasqStateDir))) { - goto cleanup; - } + if (!(dctx = dnsmasqContextNew(def->name, driver->dnsmasqStateDir))) + return -1; /* Look for first IPv4 address that has dhcp defined. * We only support dhcp-host config on one IPv4 subnetwork @@ -1780,22 +1717,20 @@ networkRefreshDhcpDaemon(virNetworkDriverStatePtr driver, } if (ipv4def && (networkBuildDnsmasqDhcpHostsList(dctx, ipv4def) < 0)) - goto cleanup; + return -1; if (ipv6def && (networkBuildDnsmasqDhcpHostsList(dctx, ipv6def) < 0)) - goto cleanup; + return -1; if (networkBuildDnsmasqHostsList(dctx, &def->dns) < 0) - goto cleanup; + return -1; - if ((ret = dnsmasqSave(dctx)) < 0) - goto cleanup; + if (dnsmasqSave(dctx) < 0) + return -1; dnsmasqPid = virNetworkObjGetDnsmasqPid(obj); - ret = kill(dnsmasqPid, SIGHUP); - cleanup: - dnsmasqContextFree(dctx); - return ret; + return kill(dnsmasqPid, SIGHUP); + } @@ -1833,8 +1768,7 @@ networkRadvdConfContents(virNetworkObjPtr obj, char **configstr) { virNetworkDefPtr def = virNetworkObjGetDef(obj); - virBuffer configbuf = VIR_BUFFER_INITIALIZER; - int ret = -1; + g_auto(virBuffer) configbuf = VIR_BUFFER_INITIALIZER; size_t i; virNetworkIPDefPtr ipdef; bool v6present = false, dhcp6 = false; @@ -1851,10 +1785,8 @@ networkRadvdConfContents(virNetworkObjPtr obj, } /* If there are no IPv6 addresses, then we are done */ - if (!v6present) { - ret = 0; - goto cleanup; - } + if (!v6present) + return 0; /* create radvd config file appropriate for this network; * IgnoreIfMissing allows radvd to start even when the bridge is down @@ -1872,33 +1804,29 @@ networkRadvdConfContents(virNetworkObjPtr obj, /* add a section for each IPv6 address in the config */ for (i = 0; (ipdef = virNetworkDefGetIPByIndex(def, AF_INET6, i)); i++) { int prefix; - char *netaddr; + g_autofree char *netaddr = NULL; prefix = virNetworkIPDefPrefix(ipdef); if (prefix < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("bridge '%s' has an invalid prefix"), def->bridge); - goto cleanup; + return -1; } if (!(netaddr = virSocketAddrFormat(&ipdef->address))) - goto cleanup; + return -1; + virBufferAsprintf(&configbuf, " prefix %s/%d\n" " {\n%s };\n", netaddr, prefix, dhcp6 ? radvd2 : radvd3); - VIR_FREE(netaddr); } virBufferAddLit(&configbuf, "};\n"); - *configstr = virBufferContentAndReset(&configbuf); + return 0; - ret = 0; - cleanup: - virBufferFreeAndReset(&configbuf); - return ret; } @@ -1909,9 +1837,8 @@ networkRadvdConfWrite(virNetworkDriverStatePtr driver, char **configFile) { virNetworkDefPtr def = virNetworkObjGetDef(obj); - int ret = -1; - char *configStr = NULL; - char *myConfigFile = NULL; + g_autofree char *configStr = NULL; + g_autofree char *myConfigFile = NULL; if (!configFile) configFile = &myConfigFile; @@ -1919,29 +1846,24 @@ networkRadvdConfWrite(virNetworkDriverStatePtr driver, *configFile = NULL; if (networkRadvdConfContents(obj, &configStr) < 0) - goto cleanup; + return -1; - if (!configStr) { - ret = 0; - goto cleanup; - } + if (!configStr) + return 0; /* construct the filename */ if (!(*configFile = networkRadvdConfigFileName(driver, def->name))) - goto cleanup; + return -1; + /* write the file */ if (virFileWriteStr(*configFile, configStr, 0600) < 0) { virReportSystemError(errno, _("couldn't write radvd config file '%s'"), *configFile); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FREE(configStr); - VIR_FREE(myConfigFile); - return ret; + return 0; } @@ -1950,26 +1872,22 @@ networkStartRadvd(virNetworkDriverStatePtr driver, virNetworkObjPtr obj) { virNetworkDefPtr def = virNetworkObjGetDef(obj); - dnsmasqCapsPtr dnsmasq_caps = networkGetDnsmasqCaps(driver); + g_autoptr(dnsmasqCaps) dnsmasq_caps = networkGetDnsmasqCaps(driver); pid_t radvdPid; - char *pidfile = NULL; - char *radvdpidbase = NULL; - char *configfile = NULL; - virCommandPtr cmd = NULL; - int ret = -1; + g_autofree char *pidfile = NULL; + g_autofree char *radvdpidbase = NULL; + g_autofree char *configfile = NULL; + g_autoptr(virCommand) cmd = NULL; virNetworkObjSetRadvdPid(obj, -1); /* Is dnsmasq handling RA? */ - if (DNSMASQ_RA_SUPPORT(dnsmasq_caps)) { - ret = 0; - goto cleanup; - } + if (DNSMASQ_RA_SUPPORT(dnsmasq_caps)) + return 0; if (!virNetworkDefGetIPByIndex(def, AF_INET6, 0)) { /* no IPv6 addresses, so we don't need to run radvd */ - ret = 0; - goto cleanup; + return 0; } if (!virFileIsExecutable(RADVD)) { @@ -1977,30 +1895,32 @@ networkStartRadvd(virNetworkDriverStatePtr driver, _("Cannot find %s - " "Possibly the package isn't installed"), RADVD); - goto cleanup; + return -1; } if (virFileMakePath(driver->pidDir) < 0) { virReportSystemError(errno, _("cannot create directory %s"), driver->pidDir); - goto cleanup; + return -1; } + if (virFileMakePath(driver->radvdStateDir) < 0) { virReportSystemError(errno, _("cannot create directory %s"), driver->radvdStateDir); - goto cleanup; + return -1; } /* construct pidfile name */ if (!(radvdpidbase = networkRadvdPidfileBasename(def->name))) - goto cleanup; + return -1; + if (!(pidfile = virPidFileBuildPath(driver->pidDir, radvdpidbase))) - goto cleanup; + return -1; if (networkRadvdConfWrite(driver, obj, &configfile) < 0) - goto cleanup; + return -1; /* prevent radvd from daemonizing itself with "--debug 1", and use * a dummy pidfile name - virCommand will create the pidfile we @@ -2020,20 +1940,13 @@ networkStartRadvd(virNetworkDriverStatePtr driver, virCommandDaemonize(cmd); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return -1; if (virPidFileRead(driver->pidDir, radvdpidbase, &radvdPid) < 0) - goto cleanup; - virNetworkObjSetRadvdPid(obj, radvdPid); + return -1; - ret = 0; - cleanup: - virObjectUnref(dnsmasq_caps); - virCommandFree(cmd); - VIR_FREE(configfile); - VIR_FREE(radvdpidbase); - VIR_FREE(pidfile); - return ret; + virNetworkObjSetRadvdPid(obj, radvdPid); + return 0; } @@ -2042,14 +1955,13 @@ networkRefreshRadvd(virNetworkDriverStatePtr driver, virNetworkObjPtr obj) { virNetworkDefPtr def = virNetworkObjGetDef(obj); - dnsmasqCapsPtr dnsmasq_caps = networkGetDnsmasqCaps(driver); + g_autoptr(dnsmasqCaps) dnsmasq_caps = networkGetDnsmasqCaps(driver); g_autofree char *radvdpidbase = NULL; g_autofree char *pidfile = NULL; pid_t radvdPid; /* Is dnsmasq handling RA? */ if (DNSMASQ_RA_SUPPORT(dnsmasq_caps)) { - virObjectUnref(dnsmasq_caps); if ((radvdpidbase = networkRadvdPidfileBasename(def->name)) && (pidfile = virPidFileBuildPath(driver->pidDir, radvdpidbase))) { /* radvd should not be running but in case it is */ @@ -2058,7 +1970,6 @@ networkRefreshRadvd(virNetworkDriverStatePtr driver, } return 0; } - virObjectUnref(dnsmasq_caps); /* if there's no running radvd, just start it */ radvdPid = virNetworkObjGetRadvdPid(obj); @@ -2250,8 +2161,7 @@ static int networkSetIPv6Sysctls(virNetworkObjPtr obj) { virNetworkDefPtr def = virNetworkObjGetDef(obj); - char *field = NULL; - int ret = -1; + g_autofree char *field = NULL; bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0); /* set disable_ipv6 if there are no ipv6 addresses defined for the @@ -2265,17 +2175,15 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj) if (!enableIPv6) VIR_DEBUG("ipv6 appears to already be disabled on %s", def->bridge); - ret = 0; - goto cleanup; + return 0; } if (virFileWriteStr(field, enableIPv6 ? "0" : "1", 0) < 0) { virReportSystemError(errno, _("cannot write to %s to enable/disable IPv6 " "on bridge %s"), field, def->bridge); - goto cleanup; + return -1; } - VIR_FREE(field); /* The rest of the ipv6 sysctl tunables should always be set the * same, whether or not we're using ipv6 on this bridge. @@ -2284,31 +2192,29 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj) /* Prevent guests from hijacking the host network by sending out * their own router advertisements. */ + VIR_FREE(field); field = g_strdup_printf(SYSCTL_PATH "/net/ipv6/conf/%s/accept_ra", def->bridge); if (virFileWriteStr(field, "0", 0) < 0) { virReportSystemError(errno, _("cannot disable %s"), field); - goto cleanup; + return -1; } - VIR_FREE(field); /* All interfaces used as a gateway (which is what this is, by * definition), must always have autoconf=0. */ + VIR_FREE(field); field = g_strdup_printf(SYSCTL_PATH "/net/ipv6/conf/%s/autoconf", def->bridge); if (virFileWriteStr(field, "0", 0) < 0) { virReportSystemError(errno, _("cannot disable %s"), field); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FREE(field); - return ret; + return 0; } @@ -2386,7 +2292,8 @@ networkWaitDadFinish(virNetworkObjPtr obj) { virNetworkDefPtr def = virNetworkObjGetDef(obj); virNetworkIPDefPtr ipdef; - virSocketAddrPtr *addrs = NULL, addr = NULL; + g_autofree virSocketAddrPtr *addrs = NULL; + virSocketAddrPtr addr = NULL; size_t naddrs = 0; int ret = -1; @@ -2401,7 +2308,6 @@ networkWaitDadFinish(virNetworkObjPtr obj) ret = (naddrs == 0) ? 0 : virNetDevIPWaitDadFinish(addrs, naddrs); cleanup: - VIR_FREE(addrs); VIR_DEBUG("Finished waiting for IPv6 DAD on network %s with status %d", def->name, ret); return ret; @@ -2418,9 +2324,9 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, virErrorPtr save_err = NULL; virNetworkIPDefPtr ipdef; virNetDevIPRoutePtr routedef; - char *macTapIfName = NULL; + g_autofree char *macTapIfName = NULL; virMacMapPtr macmap; - char *macMapFile = NULL; + g_autofree char *macMapFile = NULL; int tapfd = -1; bool dnsmasqStarted = false; bool devOnline = false; @@ -2467,7 +2373,6 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE | VIR_NETDEV_TAP_CREATE_IFUP | VIR_NETDEV_TAP_CREATE_PERSIST) < 0) { - VIR_FREE(macTapIfName); goto error; } } @@ -2583,9 +2488,6 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) goto error; - VIR_FREE(macTapIfName); - VIR_FREE(macMapFile); - return 0; error: @@ -2609,10 +2511,8 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, if (macTapIfName) { VIR_FORCE_CLOSE(tapfd); ignore_value(virNetDevTapDelete(macTapIfName, NULL)); - VIR_FREE(macTapIfName); } virNetworkObjUnrefMacMap(obj); - VIR_FREE(macMapFile); ignore_value(virNetDevBridgeDelete(def->bridge)); @@ -2637,14 +2537,12 @@ networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver, radvdPid = virNetworkObjGetRadvdPid(obj); if (radvdPid > 0) { - char *radvdpidbase; + g_autofree char *radvdpidbase = NULL; kill(radvdPid, SIGTERM); /* attempt to delete the pidfile we created */ - if ((radvdpidbase = networkRadvdPidfileBasename(def->name))) { + if ((radvdpidbase = networkRadvdPidfileBasename(def->name))) virPidFileDelete(driver->pidDir, radvdpidbase); - VIR_FREE(radvdpidbase); - } } dnsmasqPid = virNetworkObjGetDnsmasqPid(obj); @@ -2652,11 +2550,9 @@ networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver, kill(dnsmasqPid, SIGTERM); if (def->mac_specified) { - char *macTapIfName = networkBridgeDummyNicName(def->bridge); - if (macTapIfName) { + g_autofree char *macTapIfName = networkBridgeDummyNicName(def->bridge); + if (macTapIfName) ignore_value(virNetDevTapDelete(macTapIfName, NULL)); - VIR_FREE(macTapIfName); - } } ignore_value(virNetDevSetOnline(def->bridge, 0)); @@ -2963,7 +2859,7 @@ networkShutdownNetwork(virNetworkDriverStatePtr driver, { virNetworkDefPtr def = virNetworkObjGetDef(obj); int ret = 0; - char *stateFile; + g_autofree char *stateFile = NULL; VIR_INFO("Shutting down network '%s'", def->name); @@ -2975,7 +2871,6 @@ networkShutdownNetwork(virNetworkDriverStatePtr driver, return -1; unlink(stateFile); - VIR_FREE(stateFile); switch ((virNetworkForwardType) def->forward.type) { @@ -3247,8 +3142,7 @@ static int networkFindUnusedBridgeName(virNetworkObjListPtr nets, virNetworkDefPtr def) { - int ret = -1, id = 0; - char *newname = NULL; + int id = 0; const char *templ = "virbr%d"; const char *p; @@ -3258,7 +3152,8 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets, templ = def->bridge; do { - newname = g_strdup_printf(templ, id); + g_autofree char *newname = g_strdup_printf(templ, id); + /* check if this name is used in another libvirt network or * there is an existing device with that name. ignore errors * from virNetDevExists(), just in case it isn't implemented @@ -3267,21 +3162,15 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets, if (!(virNetworkObjBridgeInUse(nets, newname, def->name) || virNetDevExists(newname) == 1)) { VIR_FREE(def->bridge); /*could contain template */ - def->bridge = newname; - ret = 0; - goto cleanup; + def->bridge = g_steal_pointer(&newname); + return 0; } - VIR_FREE(newname); } while (++id <= MAX_BRIDGE_ID); virReportError(VIR_ERR_INTERNAL_ERROR, _("Bridge generation exceeded max id %d"), MAX_BRIDGE_ID); - ret = 0; - cleanup: - if (ret < 0) - VIR_FREE(newname); - return ret; + return -1; } @@ -3424,7 +3313,7 @@ networkValidate(virNetworkDriverStatePtr driver, */ for (i = 0; i < def->forward.nifs; i++) { virNetworkForwardIfDefPtr iface = &def->forward.ifs[i]; - char *sysfs_path = NULL; + g_autofree char *sysfs_path = NULL; switch ((virNetworkForwardHostdevDeviceType)iface->type) { case VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV: @@ -3465,10 +3354,8 @@ networkValidate(virNetworkDriverStatePtr driver, _("device '%s' in network '%s' is not " "an SR-IOV Virtual Function"), sysfs_path, def->name); - VIR_FREE(sysfs_path); return -1; } - VIR_FREE(sysfs_path); break; } @@ -4144,7 +4031,8 @@ networkSetAutostart(virNetworkPtr net, virNetworkDriverStatePtr driver = networkGetDriver(); virNetworkObjPtr obj; virNetworkDefPtr def; - char *configFile = NULL, *autostartLink = NULL; + g_autofree char *configFile = NULL; + g_autofree char *autostartLink = NULL; bool new_autostart; bool cur_autostart; int ret = -1; @@ -4201,8 +4089,6 @@ networkSetAutostart(virNetworkPtr net, ret = 0; cleanup: - VIR_FREE(configFile); - VIR_FREE(autostartLink); virNetworkObjEndAPI(&obj); return ret; } @@ -4224,12 +4110,12 @@ networkGetDHCPLeases(virNetworkPtr net, long long currtime = 0; long long expirytime_tmp = -1; bool ipv6 = false; - char *lease_entries = NULL; - char *custom_lease_file = NULL; + g_autofree char *lease_entries = NULL; + g_autofree char *custom_lease_file = NULL; const char *ip_tmp = NULL; const char *mac_tmp = NULL; virJSONValuePtr lease_tmp = NULL; - virJSONValuePtr leases_array = NULL; + g_autoptr(virJSONValue) leases_array = NULL; virNetworkIPDefPtr ipdef_tmp = NULL; virNetworkDHCPLeasePtr lease = NULL; virNetworkDHCPLeasePtr *leases_ret = NULL; @@ -4384,10 +4270,6 @@ networkGetDHCPLeases(virNetworkPtr net, cleanup: VIR_FREE(lease); - VIR_FREE(lease_entries); - VIR_FREE(custom_lease_file); - virJSONValueFree(leases_array); - virNetworkObjEndAPI(&obj); return rv; @@ -5582,7 +5464,7 @@ networkPortSetParameters(virNetworkPortPtr port, virNetworkDefPtr def; virNetworkPortDefPtr portdef; virNetDevBandwidthPtr bandwidth = NULL; - char *dir = NULL; + g_autofree char *dir = NULL; int ret = -1; size_t i; @@ -5660,7 +5542,6 @@ networkPortSetParameters(virNetworkPortPtr port, cleanup: virNetDevBandwidthFree(bandwidth); virNetworkObjEndAPI(&obj); - VIR_FREE(dir); return ret; } diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 30f6aa8fe1..0d0ac730f2 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -217,14 +217,15 @@ void networkPostReloadFirewallRules(bool startup G_GNUC_UNUSED) */ int networkCheckRouteCollision(virNetworkDefPtr def) { - int ret = 0, len; - char *cur, *buf = NULL; + int len; + char *cur; + g_autofree char *buf = NULL; /* allow for up to 100000 routes (each line is 128 bytes) */ enum {MAX_ROUTE_SIZE = 128*100000}; /* Read whole routing table into memory */ if ((len = virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE, &buf)) < 0) - goto out; + return 0; /* Dropping the last character shouldn't hurt */ if (len > 0) @@ -233,7 +234,7 @@ int networkCheckRouteCollision(virNetworkDefPtr def) VIR_DEBUG("%s output:\n%s", PROC_NET_ROUTE, buf); if (!STRPREFIX(buf, "Iface")) - goto out; + return 0; /* First line is just headings, skip it */ cur = strchr(buf, '\n'); @@ -295,8 +296,7 @@ int networkCheckRouteCollision(virNetworkDefPtr def) virReportError(VIR_ERR_INTERNAL_ERROR, _("Network is already in use by interface %s"), iface); - ret = -1; - goto out; + return -1; } } @@ -315,23 +315,19 @@ int networkCheckRouteCollision(virNetworkDefPtr def) if ((r_addr.data.inet4.sin_addr.s_addr == addr_val) && (r_mask.data.inet4.sin_addr.s_addr == mask_val)) { - char *addr_str = virSocketAddrFormat(&r_addr); + g_autofree char *addr_str = virSocketAddrFormat(&r_addr); if (!addr_str) virResetLastError(); virReportError(VIR_ERR_INTERNAL_ERROR, _("Route address '%s' conflicts " "with IP address for '%s'"), NULLSTR(addr_str), iface); - VIR_FREE(addr_str); - ret = -1; - goto out; + return -1; } } } - out: - VIR_FREE(buf); - return ret; + return 0; } static const char networkLocalMulticastIPv4[] = "224.0.0.0/24"; @@ -838,8 +834,7 @@ int networkAddFirewallRules(virNetworkDefPtr def) { size_t i; virNetworkIPDefPtr ipdef; - virFirewallPtr fw = NULL; - int ret = -1; + g_autoptr(virFirewall) fw = NULL; if (virOnce(&createdOnce, networkSetupPrivateChains) < 0) return -1; @@ -869,11 +864,11 @@ int networkAddFirewallRules(virNetworkDefPtr def) _("zone %s requested for network %s " "but firewalld is not active"), def->bridgeZone, def->name); - goto cleanup; + return -1; } if (virFirewallDInterfaceSetZone(def->bridge, def->bridgeZone) < 0) - goto cleanup; + return -1; } else { @@ -893,13 +888,13 @@ int networkAddFirewallRules(virNetworkDefPtr def) */ if (virFirewallDZoneExists("libvirt")) { if (virFirewallDInterfaceSetZone(def->bridge, "libvirt") < 0) - goto cleanup; + return -1; } else { unsigned long version; int vresult = virFirewallDGetVersion(&version); if (vresult < 0) - goto cleanup; + return -1; /* Support for nftables backend was added in firewalld * 0.6.0. Support for rule priorities (required by the @@ -919,7 +914,7 @@ int networkAddFirewallRules(virNetworkDefPtr def) "version supporting rule priorities " "(0.7.0+) and/or rebuilding " "libvirt with --with-firewalld-zone")); - goto cleanup; + return -1; } } } @@ -935,7 +930,7 @@ int networkAddFirewallRules(virNetworkDefPtr def) (ipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i)); i++) { if (networkAddIPSpecificFirewallRules(fw, def, ipdef) < 0) - goto cleanup; + return -1; } virFirewallStartRollback(fw, 0); @@ -944,7 +939,7 @@ int networkAddFirewallRules(virNetworkDefPtr def) (ipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i)); i++) { if (networkRemoveIPSpecificFirewallRules(fw, def, ipdef) < 0) - goto cleanup; + return -1; } networkRemoveGeneralFirewallRules(fw, def); @@ -952,12 +947,9 @@ int networkAddFirewallRules(virNetworkDefPtr def) networkAddChecksumFirewallRules(fw, def); if (virFirewallApply(fw) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - virFirewallFree(fw); - return ret; + return 0; } /* Remove all rules for all ip addresses (and general rules) on a network */ @@ -965,9 +957,7 @@ void networkRemoveFirewallRules(virNetworkDefPtr def) { size_t i; virNetworkIPDefPtr ipdef; - virFirewallPtr fw = NULL; - - fw = virFirewallNew(); + g_autoptr(virFirewall) fw = virFirewallNew(); virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); networkRemoveChecksumFirewallRules(fw, def); @@ -978,12 +968,9 @@ void networkRemoveFirewallRules(virNetworkDefPtr def) (ipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i)); i++) { if (networkRemoveIPSpecificFirewallRules(fw, def, ipdef) < 0) - goto cleanup; + return; } networkRemoveGeneralFirewallRules(fw, def); virFirewallApply(fw); - - cleanup: - virFirewallFree(fw); } diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index 2b5fc0f442..732dd09610 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c @@ -82,8 +82,8 @@ VIR_ENUM_IMPL(virLeaseAction, int main(int argc, char **argv) { - char *pid_file = NULL; - char *custom_lease_file = NULL; + g_autofree char *pid_file = NULL; + g_autofree char *custom_lease_file = NULL; const char *ip = NULL; const char *mac = NULL; const char *leases_str = NULL; @@ -91,13 +91,13 @@ main(int argc, char **argv) const char *clientid = getenv("DNSMASQ_CLIENT_ID"); const char *interface = getenv("DNSMASQ_INTERFACE"); const char *hostname = getenv("DNSMASQ_SUPPLIED_HOSTNAME"); - char *server_duid = NULL; + g_autofree char *server_duid = NULL; int action = -1; int pid_file_fd = -1; int rv = EXIT_FAILURE; bool delete = false; - virJSONValuePtr lease_new = NULL; - virJSONValuePtr leases_array_new = NULL; + g_autoptr(virJSONValue) lease_new = NULL; + g_autoptr(virJSONValue) leases_array_new = NULL; virSetErrorFunc(NULL, NULL); virSetErrorLogPriorityFunc(NULL); @@ -256,11 +256,5 @@ main(int argc, char **argv) if (pid_file_fd != -1) virPidFileReleasePath(pid_file, pid_file_fd); - VIR_FREE(pid_file); - VIR_FREE(server_duid); - VIR_FREE(custom_lease_file); - virJSONValueFree(lease_new); - virJSONValueFree(leases_array_new); - return rv; } diff --git a/src/util/virdnsmasq.h b/src/util/virdnsmasq.h index 4c14bc6ca7..e3814c2eb1 100644 --- a/src/util/virdnsmasq.h +++ b/src/util/virdnsmasq.h @@ -78,10 +78,14 @@ typedef enum { typedef struct _dnsmasqCaps dnsmasqCaps; typedef dnsmasqCaps *dnsmasqCapsPtr; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(dnsmasqCaps, virObjectUnref); + dnsmasqContext * dnsmasqContextNew(const char *network_name, const char *config_dir); void dnsmasqContextFree(dnsmasqContext *ctx); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(dnsmasqContext, dnsmasqContextFree); + int dnsmasqAddDhcpHost(dnsmasqContext *ctx, const char *mac, virSocketAddr *ip, -- 2.25.4

Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/network/bridge_driver.c | 59 +++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 26 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 668aa9ca88..a1b2f5b6c7 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -148,7 +148,7 @@ networkDnsmasqDefNamespaceFree(void *nsdata) virStringListFreeCount(def->options, def->noptions); - VIR_FREE(def); + g_free(def); } @@ -706,7 +706,8 @@ networkStateInitialize(bool privileged, network_driver->lockFD = -1; if (virMutexInit(&network_driver->lock) < 0) { - VIR_FREE(network_driver); + g_free(network_driver); + network_driver = NULL; goto error; } @@ -874,18 +875,19 @@ networkStateCleanup(void) virPidFileRelease(network_driver->stateDir, "driver", network_driver->lockFD); - VIR_FREE(network_driver->networkConfigDir); - VIR_FREE(network_driver->networkAutostartDir); - VIR_FREE(network_driver->stateDir); - VIR_FREE(network_driver->pidDir); - VIR_FREE(network_driver->dnsmasqStateDir); - VIR_FREE(network_driver->radvdStateDir); + g_free(network_driver->networkConfigDir); + g_free(network_driver->networkAutostartDir); + g_free(network_driver->stateDir); + g_free(network_driver->pidDir); + g_free(network_driver->dnsmasqStateDir); + g_free(network_driver->radvdStateDir); virObjectUnref(network_driver->dnsmasqCaps); virMutexDestroy(&network_driver->lock); - VIR_FREE(network_driver); + g_free(network_driver); + network_driver = NULL; return 0; } @@ -2192,7 +2194,7 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj) /* Prevent guests from hijacking the host network by sending out * their own router advertisements. */ - VIR_FREE(field); + g_free(field); field = g_strdup_printf(SYSCTL_PATH "/net/ipv6/conf/%s/accept_ra", def->bridge); @@ -2205,7 +2207,7 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj) /* All interfaces used as a gateway (which is what this is, by * definition), must always have autoconf=0. */ - VIR_FREE(field); + g_free(field); field = g_strdup_printf(SYSCTL_PATH "/net/ipv6/conf/%s/autoconf", def->bridge); if (virFileWriteStr(field, "0", 0) < 0) { @@ -2713,19 +2715,19 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) for (i = 0; i < netdef->forward.nifs; i++) { if (netdef->forward.ifs[i].type == VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV) - VIR_FREE(netdef->forward.ifs[i].device.dev); + g_free(netdef->forward.ifs[i].device.dev); } netdef->forward.nifs = 0; } if (netdef->forward.nifs == 0) - VIR_FREE(netdef->forward.ifs); + g_free(netdef->forward.ifs); for (i = 0; i < numVirtFns; i++) { - VIR_FREE(vfNames[i]); - VIR_FREE(virtFns[i]); + g_free(vfNames[i]); + g_free(virtFns[i]); } - VIR_FREE(vfNames); - VIR_FREE(virtFns); + g_free(vfNames); + g_free(virtFns); return ret; } @@ -3161,7 +3163,7 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets, */ if (!(virNetworkObjBridgeInUse(nets, newname, def->name) || virNetDevExists(newname) == 1)) { - VIR_FREE(def->bridge); /*could contain template */ + g_free(def->bridge); /*could contain template */ def->bridge = g_steal_pointer(&newname); return 0; } @@ -4256,7 +4258,8 @@ networkGetDHCPLeases(virNetworkPtr net, nleases++; } - VIR_FREE(lease); + g_free(lease); + lease = NULL; } if (leases_ret) { @@ -4269,7 +4272,7 @@ networkGetDHCPLeases(virNetworkPtr net, rv = nleases; cleanup: - VIR_FREE(lease); + g_free(lease); virNetworkObjEndAPI(&obj); return rv; @@ -4278,7 +4281,7 @@ networkGetDHCPLeases(virNetworkPtr net, if (leases_ret) { for (i = 0; i < nleases; i++) virNetworkDHCPLeaseFree(leases_ret[i]); - VIR_FREE(leases_ret); + g_free(leases_ret); } goto cleanup; } @@ -4402,7 +4405,7 @@ networkAllocatePort(virNetworkObjPtr obj, return -1; } if (portprofile) { - VIR_FREE(port->virtPortProfile); + g_free(port->virtPortProfile); port->virtPortProfile = portprofile; } @@ -5519,10 +5522,14 @@ networkPortSetParameters(virNetworkPortPtr port, /* average or floor are mandatory, peak and burst are optional. * So if no average or floor is given, we free inbound/outbound * here which causes inbound/outbound to not be set. */ - if (!bandwidth->in->average && !bandwidth->in->floor) - VIR_FREE(bandwidth->in); - if (!bandwidth->out->average) - VIR_FREE(bandwidth->out); + if (!bandwidth->in->average && !bandwidth->in->floor) { + g_free(bandwidth->in); + bandwidth->in = NULL; + } + if (!bandwidth->out->average) { + g_free(bandwidth->out); + bandwidth->out = NULL; + } if (networkUpdatePortBandwidth(obj, &portdef->mac, -- 2.25.4

On 6/25/20 3:55 AM, Peter Krempa wrote: Actually I agree with you :-) When we started into all this glib stuff, I thought that it was kind of unproductive to churn the code around in cases where it was just renaming one thing to something else - aside from (as you point out) being a siren call for regressions, this also makes backports to old branches more annoying, obscures *actual* functional history, and besides, what happens the *next* time we want to change how we do [whatever thing we're changing]? Do we do yet another global replacement for the "new new hotness"? But this was one of those things that didn't seem worth getting in the way of (and in balance it was definitely a net win), so I mostly ignored it (including not going out of my way to convert any code over just for the sake of converting). In the meantime, lots and lots of patches have come in converting this stuff piecemeal over the codebase, and it's all becoming more and more g_*-centric. I still didn't really bother with it much. Then I saw a memory leak in a patch a couple weeks ago that wouldn't have occurred if the existing function had used g_autofree (and thus reminded the author to use g_autofree for their additions to this existing function). This led me to make a patch to convert that file to use g_autofree and g_autoptr wherever possible, which in turn got me to look at xmlBuffer allocation/free and notice a couple bugs, which led to noticing something else inconsistent with current style, which led to noticing some other existing bug, and from there to something else ad infinitum. So this one recognition of a single memory leak organically led to a bunch of drive-by patches, but the drive-by patches left everything in an in-between limbo state - half of things were the "old way" and half were the "new way". Somewhere in the middle of all this, I looked back at a recent set of patches from danpb for reference, and saw that along with making locals g_auto*, and changing VIR_ALLOC to g_new0, he had also replaced VIR_FREE with g_free, so I figured I should probably do that too while I was already churning things. The semantic change (no longer setting the pointer to the freed memory to NULL) was bothered me, but since it was already being used, I assumed there must have been discussion about it among all the glib conversion mails I skipped over, and decided to make my patches consistent with "current convention", and just carefully examine each usage to assure that either the pointer wasn't referenced after free, or that it was explicitly set to NULL. I do recognize your concern that "some other people" (thanks for explicitly, though incorrectly, excluding me! :-)) may not be as diligent when doing a similar replacement though, and even after doing it myself I have concern that I may have missed something. And now you point out the new implementation to VIR_FREE() (*yet another* change missed by me, as with so many other things that whiz by on the mailing list) that uses g_clear_pointer (which, having not read through the glib reference manual nor worked on other projects using glib, I didn't know about until today)! This validates my original apprehension (in the before before time) about replacing VIR_* with g_* macros - when we use our own macros it may be slightly more difficult for first-time readers of the code who *might* have already been familiar with glib (or maybe not), but it allows us to easily change the underlying implementation in the future without yet again churning through all the code. This convinces me that VIR_FREE shouldn't be replaced with g_free in *any* circumstance. As a matter of fact, I would even go so far as to say that use of g_free() should be ...... er "prohibited" with a syntax check (or would that be limiting free speech?). (BTW, in case someone might bring up the argument of "g_free() should be used in cases when it's not important to null out the pointer, because it's more efficient!", my response is that 1) nobody should have to be burdened with manually verifying that their pointer isn't referenced after its freed just to determine which function to call, and 2) consistency is more important than an undetectably tiny amount of extra efficiency (and this comes from someone who spent a significant amount of time in his early career writing device drivers and even simple application programs in assembly language, and internally shudders whenever automatic garbage collection is mentioned)) There's no getting around it - that looks ugly. And who wants to replace 5506 occurences of one simple-looking thing with something else that's functionally equivalent but more painful to look at? I would vote for just documenting that, for safety and consistency reasons, VIR_FREE() should always be used instead of g_free(), and eliminating all direct use of g_free() (along with the aforementioned syntax check). (BTW, I had assumed there had been more changes to g_free(), but when I looked at my current tree just now, there were only 228 occurences, including the changes in this patch) So what about the rest of these patches? Should I repost the entire series without any g_free() additions? Or will you (or someone else) ACK the ones here that don't include g_free()? (Maybe there's something else I've done that, despite my purist intentions, doesn't fit with current sentiment?)

On 6/25/20 11:12 AM, Daniel P. Berrangé wrote: Is this just to make the code more accessible/understandable to new contributors? Or is there some other reason that I missed due to being incapable of reading all the messages on all the lists? (I guess there's also the issue of reducing support burden by reproducing identical code to something that someone else is already maintaining in a different library. But in this case we're just talking about a few lines that enforces good behavior.) It still requires thought + diligence = time. And what if new code is added to the end of a function, thus making those things that had been "at the end" now in the middle. The more thought and decision making is needed to get something right, the more likely it is that someone will get it wrong. 1) Maybe I'll feel differently after more of the code has been converted to use g_auto* and eliminated more of the existing explicit frees, but with currently > 5000 uses of VIR_FREE still in the code, I fear that "not many of those" may be more than we're expecting, and especially with many of them left, it would give me more warm fuzzies to be able to say "We can verifiably state that no pointers will be used after free , because their values have been NULLed, and any access will either be a NOP, or cause an immediate segfault" rather than "We believe that the contributors to libvirt have been diligent in their manual auditing of all cases of free'ing memory to assure that none of the freed pointers are ever used at any later point, because.... well, just *because*". (on the other hand, admittedly any pointer to something with its own vir*Free() function already requires diligence on the part of the programmer, since vir*Free() doesn't NULL the pointer. In that case, what's a little extra burden?) 2) Speaking from my experience with the occurrences I converted here, the worst offenders were the places where someone re-used a local pointer multiple times in a function (sometimes naming the multiply-used variable something generic like "tmp", other times naming it specifically (e.g. "flags", then using it once for a matching purpose (e.g. a string containing the flags arg for an ebtables command option), and again for something only tangentially related (e.g. the *mask* arg for an ebtables command option). I used to think that re-using an automatic was clever and efficient because it conserved stack usage, but now I think it's just another source of making the code confusing, error prone, and time consuming to modify. Now I'm torn. I agree with Peter's assertion that moving to g_free could introduce new bugs in spite of our diligence, and that in general it's safer to rely on the programmatically verifiable safety of VIR_FREE rather than said diligence. But if that's the way we're going, I also don't want my patches to leave the conversion "half done", and pawn the last bit off on someone else. (actually, I already noticed that, of all the things to leave out (!), I didn't even fully convert the one file that started this all - domain_conf.c; I failed to even include a patch to eliminate VIR_ALLOC/VIR_REALLOC (much less VIR_FREE). I guess that just shows how Kerouac-esque stream-of-consciousness the patches in this series were :-)) I guess I'll separate out all the g_free() conversions and deal with them separately after all the rest is finished.

most of these are long-lived or attached to some other object, but a couple are automatics, and can take advantage of g_autoptr. Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/network/bridge_driver.c | 44 +++++++++++-------------------------- 1 file changed, 13 insertions(+), 31 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 275502b778..1dee2fac6e 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -160,6 +160,7 @@ networkDnsmasqDefNamespaceFree(void *nsdata) g_free(def); } +G_DEFINE_AUTOPTR_CLEANUP_FUNC(networkDnsmasqXmlNsDefPtr, networkDnsmasqDefNamespaceFree); static int @@ -177,8 +178,7 @@ networkDnsmasqDefNamespaceParseOptions(networkDnsmasqXmlNsDefPtr nsdef, if (nnodes == 0) return 0; - if (VIR_ALLOC_N(nsdef->options, nnodes) < 0) - return -1; + nsdef->options = g_new0(char *, nnodes); for (i = 0; i < nnodes; i++) { if (!(nsdef->options[nsdef->noptions++] = virXMLPropString(nodes[i], "value"))) { @@ -196,23 +196,15 @@ static int networkDnsmasqDefNamespaceParse(xmlXPathContextPtr ctxt, void **data) { - networkDnsmasqXmlNsDefPtr nsdata = NULL; - int ret = -1; - - if (VIR_ALLOC(nsdata) < 0) - return -1; + networkDnsmasqXmlNsDefPtr nsdata = g_new0(networkDnsmasqXmlNsDef, 1); if (networkDnsmasqDefNamespaceParseOptions(nsdata, ctxt)) - goto cleanup; + return -1; if (nsdata->noptions > 0) *data = g_steal_pointer(&nsdata); - ret = 0; - - cleanup: - networkDnsmasqDefNamespaceFree(nsdata); - return ret; + return 0; } @@ -711,8 +703,7 @@ networkStateInitialize(bool privileged, return -1; } - if (VIR_ALLOC(network_driver) < 0) - goto error; + network_driver = g_new0(virNetworkDriverState, 1); network_driver->lockFD = -1; if (virMutexInit(&network_driver->lock) < 0) { @@ -2658,8 +2649,7 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) goto cleanup; } - if (VIR_ALLOC_N(netdef->forward.ifs, numVirtFns) < 0) - goto cleanup; + netdef->forward.ifs = g_new0(virNetworkForwardIfDef, numVirtFns); for (i = 0; i < numVirtFns; i++) { virPCIDeviceAddressPtr thisVirtFn = virtFns[i]; @@ -4129,7 +4119,6 @@ networkGetDHCPLeases(virNetworkPtr net, virJSONValuePtr lease_tmp = NULL; g_autoptr(virJSONValue) leases_array = NULL; virNetworkIPDefPtr ipdef_tmp = NULL; - virNetworkDHCPLeasePtr lease = NULL; virNetworkDHCPLeasePtr *leases_ret = NULL; virNetworkObjPtr obj; virNetworkDefPtr def; @@ -4218,8 +4207,7 @@ networkGetDHCPLeases(virNetworkPtr net, continue; if (need_results) { - if (VIR_ALLOC(lease) < 0) - goto error; + g_autoptr(virNetworkDHCPLease) lease = g_new0(virNetworkDHCPLease, 1); lease->expirytime = expirytime_tmp; @@ -4267,22 +4255,17 @@ networkGetDHCPLeases(virNetworkPtr net, } else { nleases++; } - - g_free(lease); - lease = NULL; } if (leases_ret) { /* NULL terminated array */ - ignore_value(VIR_REALLOC_N(leases_ret, nleases + 1)); - *leases = leases_ret; - leases_ret = NULL; + leases_ret = g_renew(virNetworkDHCPLeasePtr, leases_ret, nleases + 1); + *leases = g_steal_pointer(&leases_ret); } rv = nleases; cleanup: - g_free(lease); virNetworkObjEndAPI(&obj); return rv; @@ -5504,10 +5487,9 @@ networkPortSetParameters(virNetworkPortPtr port, if (!(dir = virNetworkObjGetPortStatusDir(obj, driver->stateDir))) goto cleanup; - if ((VIR_ALLOC(bandwidth) < 0) || - (VIR_ALLOC(bandwidth->in) < 0) || - (VIR_ALLOC(bandwidth->out) < 0)) - goto cleanup; + bandwidth = g_new0(virNetDevBandwidth, 1); + bandwidth->in = g_new0(virNetDevBandwidthRate, 1); + bandwidth->out = g_new0(virNetDevBandwidthRate, 1); for (i = 0; i < nparams; i++) { virTypedParameterPtr param = &params[i]; -- 2.25.4

OOPS!! I meant to squash this into patch 10 before posting. If you want to just review it separately I can squash it in before push. Or if you want to be pedantic I can squash it in and resend :-) On 6/24/20 11:34 PM, Laine Stump wrote:

On 6/25/20 7:34 PM, Ján Tomko wrote: Well, they're all related to the cleanup that naturally follows from making a pointer g_autofree - 1) you need to initialize it when you define it (and so you might as well initialize it with the first real value it's going to get, as long as that value would end up *always* being assigned anyway, and 2) code reduction related to removing now-empty cleanup: label. But I see you just responded to patch 10 saying you thought the patch was too long and should be split, so I suppose I could make this the 2nd of the 2 that you suggest.

Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/nwfilter/nwfilter_ebiptables_driver.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 8b77578117..cc814235aa 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -3274,7 +3274,9 @@ ebtablesRuleInstCommand(virFirewallPtr fw, return ret; } -struct ebtablesSubChainInst { +typedef struct _ebtablesSubChainInst ebtablesSubChainInst; +typedef ebtablesSubChainInst *ebtablesSubChainInstPtr; +struct _ebtablesSubChainInst { virNWFilterChainPriority priority; bool incoming; enum l3_proto_idx protoidx; @@ -3285,8 +3287,8 @@ struct ebtablesSubChainInst { static int ebtablesSubChainInstSort(const void *a, const void *b) { - const struct ebtablesSubChainInst **insta = (const struct ebtablesSubChainInst **)a; - const struct ebtablesSubChainInst **instb = (const struct ebtablesSubChainInst **)b; + const ebtablesSubChainInst **insta = (const ebtablesSubChainInst **)a; + const ebtablesSubChainInst **instb = (const ebtablesSubChainInst **)b; /* priorities are limited to range [-1000, 1000] */ return (*insta)->priority - (*instb)->priority; @@ -3296,7 +3298,7 @@ ebtablesSubChainInstSort(const void *a, const void *b) static int ebtablesGetSubChainInsts(virHashTablePtr chains, bool incoming, - struct ebtablesSubChainInst ***insts, + ebtablesSubChainInstPtr **insts, size_t *ninsts) { virHashKeyValuePairPtr filter_names; @@ -3309,7 +3311,7 @@ ebtablesGetSubChainInsts(virHashTablePtr chains, return -1; for (i = 0; filter_names[i].key; i++) { - struct ebtablesSubChainInst *inst; + ebtablesSubChainInstPtr inst; enum l3_proto_idx idx = ebtablesGetProtoIdxByFiltername( filter_names[i].key); @@ -3355,7 +3357,7 @@ ebiptablesApplyNewRules(const char *ifname, bool haveEbtables = false; bool haveIptables = false; bool haveIp6tables = false; - struct ebtablesSubChainInst **subchains = NULL; + ebtablesSubChainInstPtr *subchains = NULL; size_t nsubchains = 0; int ret = -1; -- 2.25.4

On failure, this function would clear out and free the list of subchains it had been called with. This is unnecessary, because the *only* caller of this function will also clear out and free the list of subchains if it gets a failure from ebtablesGetSubChainInsts(). (It also makes more logical sense for the function that is creating the entire list to be the one freeing the entire list, rather than having a function whose purpose is only to create *one item* on the list freeing the entire list). Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/nwfilter/nwfilter_ebiptables_driver.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 89c131e17f..8fdc8e8897 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -3334,12 +3334,6 @@ ebtablesGetSubChainInsts(virHashTablePtr chains, cleanup: VIR_FREE(filter_names); - if (ret < 0) { - for (i = 0; i < *ninsts; i++) - VIR_FREE(*insts[i]); - VIR_FREE(*insts); - *ninsts = 0; - } return ret; } -- 2.25.4

Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/nwfilter/nwfilter_dhcpsnoop.c | 16 ++++++++-------- src/nwfilter/nwfilter_driver.c | 10 +++++----- src/nwfilter/nwfilter_ebiptables_driver.c | 2 +- src/nwfilter/nwfilter_gentech_driver.c | 6 +++--- src/nwfilter/nwfilter_learnipaddr.c | 8 ++++---- 5 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index 32cd6492ad..e41062feca 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -314,7 +314,7 @@ virNWFilterSnoopCancel(char **threadKey) virNWFilterSnoopActiveLock(); ignore_value(virHashRemoveEntry(virNWFilterSnoopState.active, *threadKey)); - VIR_FREE(*threadKey); + g_free(*threadKey); virNWFilterSnoopActiveUnlock(); } @@ -600,7 +600,7 @@ virNWFilterSnoopReqFree(virNWFilterSnoopReqPtr req) virCondDestroy(&req->threadStatusCond); virFreeError(req->threadError); - VIR_FREE(req); + g_free(req); } /* @@ -731,7 +731,7 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReqPtr req, if (req->threadkey && virNWFilterSnoopIPLeaseInstallRule(pl, true) < 0) { virNWFilterSnoopReqUnlock(req); - VIR_FREE(pl); + g_free(pl); return -1; } @@ -850,7 +850,7 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req, } skip_instantiate: - VIR_FREE(ipl); + g_free(ipl); ignore_value(!!g_atomic_int_dec_and_test(&virNWFilterSnoopState.nLeases)); @@ -1149,7 +1149,7 @@ virNWFilterSnoopDHCPDecodeJobSubmit(virThreadPoolPtr pool, if (ret == 0) g_atomic_int_add(qCtr, 1); else - VIR_FREE(job); + g_free(job); return ret; } @@ -1502,7 +1502,7 @@ virNWFilterDHCPSnoopThread(void *req0) ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, req->binding->portdevname)); - VIR_FREE(req->binding->portdevname); + g_free(req->binding->portdevname); virNWFilterSnoopReqUnlock(req); virNWFilterSnoopUnlock(); @@ -1970,7 +1970,7 @@ virNWFilterSnoopRemAllReqIter(const void *payload, */ virNWFilterIPAddrMapDelIPAddr(req->binding->portdevname, NULL); - VIR_FREE(req->binding->portdevname); + g_free(req->binding->portdevname); } virNWFilterSnoopReqUnlock(req); @@ -2079,7 +2079,7 @@ virNWFilterDHCPSnoopEnd(const char *ifname) /* keep valid lease req; drop interface association */ virNWFilterSnoopCancel(&req->threadkey); - VIR_FREE(req->binding->portdevname); + g_free(req->binding->portdevname); virNWFilterSnoopReqUnlock(req); diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 39d0a2128e..7853ad59fa 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -303,7 +303,7 @@ nwfilterStateInitialize(bool privileged, err_free_driverstate: virNWFilterObjListFree(driver->nwfilters); - VIR_FREE(driver); + g_free(driver); return VIR_DRV_STATE_INIT_ERROR; } @@ -367,9 +367,9 @@ nwfilterStateCleanup(void) if (driver->lockFD != -1) virPidFileRelease(driver->stateDir, "driver", driver->lockFD); - VIR_FREE(driver->stateDir); - VIR_FREE(driver->configDir); - VIR_FREE(driver->bindingDir); + g_free(driver->stateDir); + g_free(driver->configDir); + g_free(driver->bindingDir); nwfilterDriverUnlock(); } @@ -379,7 +379,7 @@ nwfilterStateCleanup(void) virNWFilterObjListFree(driver->nwfilters); virMutexDestroy(&driver->lock); - VIR_FREE(driver); + g_free(driver); return 0; } diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index b382b9405d..6e05e638aa 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -3518,7 +3518,7 @@ ebiptablesApplyNewRules(const char *ifname, cleanup: for (i = 0; i < nsubchains; i++) - VIR_FREE(subchains[i]); + g_free(subchains[i]); return ret; } diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index f586c7e938..183e2f0a91 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -122,7 +122,7 @@ virNWFilterRuleInstFree(virNWFilterRuleInstPtr inst) return; virHashFree(inst->vars); - VIR_FREE(inst); + g_free(inst); } @@ -234,12 +234,12 @@ virNWFilterInstReset(virNWFilterInstPtr inst) for (i = 0; i < inst->nfilters; i++) virNWFilterObjUnlock(inst->filters[i]); - VIR_FREE(inst->filters); + g_free(inst->filters); inst->nfilters = 0; for (i = 0; i < inst->nrules; i++) virNWFilterRuleInstFree(inst->rules[i]); - VIR_FREE(inst->rules); + g_free(inst->rules); } diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index 7bb39c3a66..c72ae23e4c 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -156,7 +156,7 @@ virNWFilterLockIface(const char *ifname) if (virMutexInitRecursive(&ifaceLock->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("mutex initialization failed")); - VIR_FREE(ifaceLock); + g_free(ifaceLock); goto err_exit; } @@ -165,12 +165,12 @@ virNWFilterLockIface(const char *ifname) _("interface name %s does not fit into " "buffer "), ifaceLock->ifname); - VIR_FREE(ifaceLock); + g_free(ifaceLock); goto err_exit; } while (virHashAddEntry(ifaceLockMap, ifname, ifaceLock)) { - VIR_FREE(ifaceLock); + g_free(ifaceLock); goto err_exit; } @@ -221,7 +221,7 @@ virNWFilterIPAddrLearnReqFree(virNWFilterIPAddrLearnReqPtr req) virNWFilterBindingDefFree(req->binding); - VIR_FREE(req); + g_free(req); } -- 2.25.4

Rather than having labels named exit, done, exit_snooprequnlock, skip_rename, etc, use the standard "cleanup" label. And instead of err_exit, malformed, tear_down_tmpebchains, use "error". Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/nwfilter/nwfilter_dhcpsnoop.c | 36 +++++++++++------------ src/nwfilter/nwfilter_ebiptables_driver.c | 12 ++++---- src/nwfilter/nwfilter_gentech_driver.c | 32 ++++++++++---------- src/nwfilter/nwfilter_learnipaddr.c | 22 +++++++------- 4 files changed, 51 insertions(+), 51 deletions(-) diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index e41062feca..efb3257e92 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -454,11 +454,11 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl, virNWFilterSnoopReqLock(req); if (virNWFilterIPAddrMapAddIPAddr(req->binding->portdevname, ipaddr) < 0) - goto exit_snooprequnlock; + goto cleanup; if (!instantiate) { rc = 0; - goto exit_snooprequnlock; + goto cleanup; } /* instantiate the filters */ @@ -469,7 +469,7 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl, req->ifindex); } - exit_snooprequnlock: + cleanup: virNWFilterSnoopReqUnlock(req); return rc; } @@ -718,7 +718,7 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReqPtr req, virNWFilterSnoopReqUnlock(req); - goto exit; + goto cleanup; } virNWFilterSnoopReqUnlock(req); @@ -742,7 +742,7 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReqPtr req, g_atomic_int_add(&virNWFilterSnoopState.nLeases, 1); - exit: + cleanup: if (update_leasefile) virNWFilterSnoopLeaseFileSave(pl); @@ -885,7 +885,7 @@ virNWFilterSnoopDHCPGetOpt(virNWFilterSnoopDHCPHdrPtr pd, int len, switch (pd->d_opts[oind]) { case DHCPO_LEASE: if (olen - oind < 6) - goto malformed; + goto error; if (*pleasetime) return -1; /* duplicate lease time */ memcpy(&nwint, (char *)pd->d_opts + oind + 2, sizeof(nwint)); @@ -893,7 +893,7 @@ virNWFilterSnoopDHCPGetOpt(virNWFilterSnoopDHCPHdrPtr pd, int len, break; case DHCPO_MTYPE: if (olen - oind < 3) - goto malformed; + goto error; if (*pmtype) return -1; /* duplicate message type */ *pmtype = pd->d_opts[oind + 2]; @@ -905,12 +905,12 @@ virNWFilterSnoopDHCPGetOpt(virNWFilterSnoopDHCPHdrPtr pd, int len, return 0; default: if (olen - oind < 2) - goto malformed; + goto error; } oind += pd->d_opts[oind + 1] + 2; } return 0; - malformed: + error: VIR_WARN("got lost in the options!"); return -1; } @@ -1362,7 +1362,7 @@ virNWFilterDHCPSnoopThread(void *req0) virNWFilterSnoopReqUnlock(req); if (req->threadStatus != THREAD_STATUS_OK) - goto exit; + goto cleanup; while (!error) { if (virNWFilterSnoopAdjustPoll(pcapConf, @@ -1390,7 +1390,7 @@ virNWFilterDHCPSnoopThread(void *req0) */ if (!virNWFilterSnoopIsActive(threadkey) || req->jobCompletionStatus != 0) - goto exit; + goto cleanup; for (i = 0; n > 0 && i < G_N_ELEMENTS(fds); i++) { if (!fds[i].revents) @@ -1507,7 +1507,7 @@ virNWFilterDHCPSnoopThread(void *req0) virNWFilterSnoopReqUnlock(req); virNWFilterSnoopUnlock(); - exit: + cleanup: virThreadPoolFree(worker); virNWFilterSnoopReqPut(req); @@ -1736,14 +1736,14 @@ virNWFilterSnoopLeaseFileSave(virNWFilterSnoopIPLeasePtr ipl) virNWFilterSnoopLeaseFileOpen(); if (virNWFilterSnoopLeaseFileWrite(virNWFilterSnoopState.leaseFD, req->ifkey, ipl) < 0) - goto err_exit; + goto error; /* keep dead leases at < ~95% of file size */ if (g_atomic_int_add(&virNWFilterSnoopState.wLeases, 1) >= g_atomic_int_get(&virNWFilterSnoopState.nLeases) * 20) virNWFilterSnoopLeaseFileLoad(); /* load & refresh lease file */ - err_exit: + error: virNWFilterSnoopUnlock(); } @@ -1838,7 +1838,7 @@ virNWFilterSnoopLeaseFileRefresh(void) if (VIR_CLOSE(tfd) < 0) { virReportSystemError(errno, _("unable to close %s"), TMPLEASEFILE); /* assuming the old lease file is still better, skip the renaming */ - goto skip_rename; + goto cleanup; } if (rename(TMPLEASEFILE, LEASEFILE) < 0) { @@ -1848,7 +1848,7 @@ virNWFilterSnoopLeaseFileRefresh(void) } g_atomic_int_set(&virNWFilterSnoopState.wLeases, 0); - skip_rename: + cleanup: virNWFilterSnoopLeaseFileOpen(); } @@ -2013,14 +2013,14 @@ virNWFilterDHCPSnoopInit(void) if (!virNWFilterSnoopState.ifnameToKey || !virNWFilterSnoopState.snoopReqs || !virNWFilterSnoopState.active) - goto err_exit; + goto error; virNWFilterSnoopLeaseFileLoad(); virNWFilterSnoopLeaseFileOpen(); return 0; - err_exit: + error: virHashFree(virNWFilterSnoopState.ifnameToKey); virNWFilterSnoopState.ifnameToKey = NULL; diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 90afbe21f1..6aefbe226b 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2881,11 +2881,11 @@ ebtablesApplyBasicRules(const char *ifname, ebtablesRenameTmpRootChainFW(fw, true, ifname); if (virFirewallApply(fw) < 0) - goto tear_down_tmpebchains; + goto error; return 0; - tear_down_tmpebchains: + error: ebtablesCleanAll(ifname); return -1; } @@ -2997,11 +2997,11 @@ ebtablesApplyDHCPOnlyRules(const char *ifname, } if (virFirewallApply(fw) < 0) - goto tear_down_tmpebchains; + goto error; return 0; - tear_down_tmpebchains: + error: ebtablesCleanAll(ifname); return -1; } @@ -3048,11 +3048,11 @@ ebtablesApplyDropAllRules(const char *ifname) ebtablesRenameTmpRootChainFW(fw, false, ifname); if (virFirewallApply(fw) < 0) - goto tear_down_tmpebchains; + goto error; return 0; - tear_down_tmpebchains: + error: ebtablesCleanAll(ifname); return -1; } diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 183e2f0a91..5433761548 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -204,14 +204,14 @@ virNWFilterCreateVarsFrom(virHashTablePtr vars1, return NULL; if (virNWFilterHashTablePutAll(vars1, res) < 0) - goto err_exit; + goto error; if (virNWFilterHashTablePutAll(vars2, res) < 0) - goto err_exit; + goto error; return res; - err_exit: + error: virHashFree(res); return NULL; } @@ -521,7 +521,7 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, if (!missing_vars) { rc = -1; - goto err_exit; + goto error; } rc = virNWFilterDetermineMissingVarsRec(filter, @@ -530,7 +530,7 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, useNewFilter, driver); if (rc < 0) - goto err_exit; + goto error; lv = virHashLookup(binding->filterparams, NWFILTER_VARNAME_CTRL_IP_LEARNING); if (lv) @@ -552,7 +552,7 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, rc = virNWFilterDHCPSnoopReq(techdriver, binding, driver); - goto err_exit; + goto error; } else if (STRCASEEQ(learning, "any")) { if (!virNWFilterHasLearnReq(ifindex)) { rc = virNWFilterLearnIPAddress(techdriver, @@ -561,14 +561,14 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, driver, DETECT_DHCP|DETECT_STATIC); } - goto err_exit; + goto error; } else { rc = -1; virReportError(VIR_ERR_PARSE_FAILED, _("filter '%s' " "learning value '%s' invalid."), filter->name, learning); - goto err_exit; + goto error; } } else { goto err_unresolvable_vars; @@ -577,7 +577,7 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, goto err_unresolvable_vars; } else if (!forceWithPendingReq && virNWFilterHasLearnReq(ifindex)) { - goto err_exit; + goto error; } rc = virNWFilterDefToInst(driver, @@ -587,7 +587,7 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, &inst); if (rc < 0) - goto err_exit; + goto error; switch (useNewFilter) { case INSTANTIATE_FOLLOW_NEWFILTER: @@ -600,7 +600,7 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, if (instantiate) { if (virNWFilterLockIface(binding->portdevname) < 0) - goto err_exit; + goto error; rc = techdriver->applyNewRules(binding->portdevname, inst.rules, inst.nrules); @@ -617,7 +617,7 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, virNWFilterUnlockIface(binding->portdevname); } - err_exit: + error: virNWFilterInstReset(&inst); virHashFree(missing_vars); @@ -633,7 +633,7 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, } rc = -1; - goto err_exit; + goto error; } @@ -700,14 +700,14 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, if (virNWFilterVarHashmapAddStdValue(binding->filterparams, NWFILTER_STD_VAR_MAC, vmmacaddr) < 0) - goto err_exit; + goto error; ipaddr = virNWFilterIPAddrMapGetIPAddr(binding->portdevname); if (ipaddr && virNWFilterVarHashmapAddStdValue(binding->filterparams, NWFILTER_STD_VAR_IP, virNWFilterVarValueGetSimple(ipaddr)) < 0) - goto err_exit; + goto error; filter = virNWFilterObjGetDef(obj); @@ -730,7 +730,7 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, teardownOld, driver, forceWithPendingReq); - err_exit: + error: virNWFilterObjUnlock(obj); return rc; diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index c72ae23e4c..c6497450eb 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -157,7 +157,7 @@ virNWFilterLockIface(const char *ifname) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("mutex initialization failed")); g_free(ifaceLock); - goto err_exit; + goto error; } if (virStrcpyStatic(ifaceLock->ifname, ifname) < 0) { @@ -166,12 +166,12 @@ virNWFilterLockIface(const char *ifname) "buffer "), ifaceLock->ifname); g_free(ifaceLock); - goto err_exit; + goto error; } while (virHashAddEntry(ifaceLockMap, ifname, ifaceLock)) { g_free(ifaceLock); - goto err_exit; + goto error; } ifaceLock->refctr = 0; @@ -185,7 +185,7 @@ virNWFilterLockIface(const char *ifname) return 0; - err_exit: + error: virMutexUnlock(&ifaceMapLock); return -1; @@ -413,7 +413,7 @@ learnIPAddressThread(void *arg) if (virNetDevValidateConfig(req->binding->portdevname, NULL, req->ifindex) <= 0) { virResetLastError(); req->status = ENODEV; - goto done; + goto cleanup; } handle = pcap_open_live(listen_if, BUFSIZ, 0, PKT_TIMEOUT_MS, errbuf); @@ -421,7 +421,7 @@ learnIPAddressThread(void *arg) if (handle == NULL) { VIR_DEBUG("Couldn't open device %s: %s", listen_if, errbuf); req->status = ENODEV; - goto done; + goto cleanup; } fds[0].fd = pcap_fileno(handle); @@ -435,7 +435,7 @@ learnIPAddressThread(void *arg) NULL, false) < 0) { VIR_DEBUG("Unable to apply DHCP only rules"); req->status = EINVAL; - goto done; + goto cleanup; } virBufferAddLit(&buf, "src port 67 and dst port 68"); } else { @@ -443,7 +443,7 @@ learnIPAddressThread(void *arg) &req->binding->mac) < 0) { VIR_DEBUG("Unable to apply basic rules"); req->status = EINVAL; - goto done; + goto cleanup; } virBufferAsprintf(&buf, "ether host %s or ether dst ff:ff:ff:ff:ff:ff", macaddr); @@ -454,14 +454,14 @@ learnIPAddressThread(void *arg) if (pcap_compile(handle, &fp, filter, 1, 0) != 0) { VIR_DEBUG("Couldn't compile filter '%s'", filter); req->status = EINVAL; - goto done; + goto cleanup; } if (pcap_setfilter(handle, &fp) != 0) { VIR_DEBUG("Couldn't set filter '%s'", filter); req->status = EINVAL; pcap_freecode(&fp); - goto done; + goto cleanup; } pcap_freecode(&fp); @@ -621,7 +621,7 @@ learnIPAddressThread(void *arg) } } /* while */ - done: + cleanup: if (handle) pcap_close(handle); -- 2.25.4