[PATCH 00/25] Several g_auto* conversion and related small bugfixes
This started out with me noticing a memory leak in a patch that led to the realization that domain_conf.c hadn't been converted to use g_autofree yet, and each step of the way uncovered some other annoyance that I wanted to get rid of. Most of the changes are related to converting code to use g_auto*, g_new0, and g_free, but there is also a some of elimination of labels, fixing actual or theoretical memory leaks, modifications for style, etc. None of it should have any functional effect (except the fixing one or two memory leaks). Laine Stump (25): conf, vmx: check for OOM after calling xmlBufferCreate() use g_autoptr for all xmlBuffers conf: refactor virDomainBlkioDeviceParseXML to remove possible NULL dereference util: validate return from xmlNodeGetContent before use util: remove OOM error log from virGetHostnameImpl() conf: eliminate useless error label in virDomainFeaturesDefParse() util: eliminate error label in virDomainDefFormatInternalSetRootName() network: fix memory leak in networkBuildDhcpDaemonCommandLine() util: add g_autoptr cleanup function for virFirewall objects network: convert local pointers to g_auto* network: use g_free() in place of remaining VIR_FREE() network: make networkDnsmasqXmlNsDef private to bridge_driver.c define g_autoptr cleanup function for virNetworkDHCPLease network: replace VIR_ALLOC/REALLOC with g_new0/g_renew network: use proper arg type when calling virNetDevSetOnline() squash into 'network: convert local pointers to g_auto*' use g_autoptr() for all usages of virFirewallNew/Free nwfilter: define a typedef for struct ebtablesSubChainInst nwfilter replace VIR_ALLOC with g_new0 nwfilter: remove unnecessary code from ebtablesGetSubChainInsts() nwfilter: convert local pointers to use g_auto* nwfilter: convert remaining VIR_FREE() to g_free() nwfilter: transform logic in virNWFilterRuleInstSort to eliminate label nwfilter: use standard label names when reasonable replace g_new() with g_new0() for consistency src/conf/domain_conf.c | 254 +++++----- src/conf/network_conf.c | 10 +- src/datatypes.h | 2 + src/network/bridge_driver.c | 585 +++++++++------------- src/network/bridge_driver.h | 9 - src/network/bridge_driver_linux.c | 58 +-- src/network/leaseshelper.c | 16 +- src/nwfilter/nwfilter_dhcpsnoop.c | 150 +++--- src/nwfilter/nwfilter_driver.c | 13 +- src/nwfilter/nwfilter_ebiptables_driver.c | 277 ++++------ src/nwfilter/nwfilter_gentech_driver.c | 60 +-- src/nwfilter/nwfilter_learnipaddr.c | 45 +- src/qemu/qemu_backup.c | 2 +- src/util/virdnsmasq.h | 4 + src/util/virebtables.c | 24 +- src/util/virfirewall.h | 1 + src/util/viriptables.c | 14 +- src/util/virutil.c | 12 +- src/util/virxml.c | 12 +- src/vmx/vmx.c | 17 +- tests/qemuhotplugmock.c | 2 +- tests/virfirewalltest.c | 50 +- 22 files changed, 640 insertions(+), 977 deletions(-) -- 2.25.4
Although libvirt itself uses g_malloc0() and friends, which exit when there isn't enouogh memory, libxml2 uses standard malloc(), which just returns NULL on OOM - this means we must check for NULL on return from any libxml2 functions that allocate memory. xmlBufferCreate(), for example, might return NULL, and we don't always check for it. This patch adds checks where it isn't already done. (NB: Although libxml2 has a provision for changing behavior on OOM (by calling xmlMemSetup() to change what functions are used to allocating/freeing memory), we can't use that, since parts of libvirt code end up in libvirt.so, which is linked and called directly by applications that may themselves use libxml2 (and may have already set their own alternate malloc()), e.g. drivers like esx which live totally in the library rather than a separate process.) Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 6 +++++- src/conf/network_conf.c | 6 +++++- src/vmx/vmx.c | 7 +++++-- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fc7fcfb0c6..9d057f8c78 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -29589,7 +29589,11 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, * Thankfully, libxml maps what looks like globals into * thread-local uses, so we are thread-safe. */ xmlIndentTreeOutput = 1; - xmlbuf = xmlBufferCreate(); + if (!(xmlbuf = xmlBufferCreate())) { + virReportOOMError(); + goto error; + } + if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata, virBufferGetIndent(buf) / 2, 1) < 0) { xmlBufferFree(xmlbuf); diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 87d43de1e3..5b578f894c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2518,7 +2518,11 @@ virNetworkDefFormatBuf(virBufferPtr buf, * Thankfully, libxml maps what looks like globals into * thread-local uses, so we are thread-safe. */ xmlIndentTreeOutput = 1; - xmlbuf = xmlBufferCreate(); + if (!(xmlbuf = xmlBufferCreate())) { + virReportOOMError(); + return -1; + } + if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata, virBufferGetIndent(buf) / 2, 1) < 0) { xmlBufferFree(xmlbuf); diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index f2248cef53..fa9766995c 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -708,8 +708,11 @@ virVMXConvertToUTF8(const char *encoding, const char *string) return NULL; } - input = xmlBufferCreateStatic((char *)string, strlen(string)); - utf8 = xmlBufferCreate(); + if (!(input = xmlBufferCreateStatic((char *)string, strlen(string))) || + !(utf8 = xmlBufferCreate())) { + virReportOOMError(); + goto cleanup; + } if (xmlCharEncInFunc(handler, utf8, input) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, -- 2.25.4
On a Wednesday in 2020, Laine Stump wrote:
Although libvirt itself uses g_malloc0() and friends, which exit when there isn't enouogh memory, libxml2 uses standard malloc(), which just returns NULL on OOM - this means we must check for NULL on return from any libxml2 functions that allocate memory.
xmlBufferCreate(), for example, might return NULL, and we don't always check for it. This patch adds checks where it isn't already done.
(NB: Although libxml2 has a provision for changing behavior on OOM (by calling xmlMemSetup() to change what functions are used to allocating/freeing memory), we can't use that, since parts of libvirt code end up in libvirt.so, which is linked and called directly by applications that may themselves use libxml2 (and may have already set their own alternate malloc()), e.g. drivers like esx which live totally in the library rather than a separate process.)
Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 6 +++++- src/conf/network_conf.c | 6 +++++- src/vmx/vmx.c | 7 +++++-- 3 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index f2248cef53..fa9766995c 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -708,8 +708,11 @@ virVMXConvertToUTF8(const char *encoding, const char *string) return NULL; }
- input = xmlBufferCreateStatic((char *)string, strlen(string)); - utf8 = xmlBufferCreate(); + if (!(input = xmlBufferCreateStatic((char *)string, strlen(string))) || + !(utf8 = xmlBufferCreate())) {
My Clang complains that 'utf8' might be used uninitialized if the first part of the condition is true.
+ virReportOOMError(); + goto cleanup; + }
if (xmlCharEncInFunc(handler, utf8, input) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR,
With 'utf8' initialized: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
AUTOPTR_CLEANUP_FUNC is set to xmlBufferFree() in util/virxml.h (This is actually new - added accidentally (but fortunately harmlessly!) in commit 257aba2dafe. I had added it along with the hunks in this patch, then decided to remove it and submit separately, but missed taking out the hunk in virxml.h) Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 4 +--- src/conf/network_conf.c | 4 +--- src/util/virxml.c | 12 +++--------- src/vmx/vmx.c | 10 +++------- 4 files changed, 8 insertions(+), 22 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9d057f8c78..1916b51d38 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -29579,7 +29579,7 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, def->description); if (def->metadata) { - xmlBufferPtr xmlbuf; + g_autoptr(xmlBuffer) xmlbuf = NULL; int oldIndentTreeOutput = xmlIndentTreeOutput; /* Indentation on output requires that we previously set @@ -29596,12 +29596,10 @@ virDomainDefFormatInternalSetRootName(virDomainDefPtr def, if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata, virBufferGetIndent(buf) / 2, 1) < 0) { - xmlBufferFree(xmlbuf); xmlIndentTreeOutput = oldIndentTreeOutput; goto error; } virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf)); - xmlBufferFree(xmlbuf); xmlIndentTreeOutput = oldIndentTreeOutput; } diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 5b578f894c..4ebad1483c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2508,7 +2508,7 @@ virNetworkDefFormatBuf(virBufferPtr buf, virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuidstr); if (def->metadata) { - xmlBufferPtr xmlbuf; + g_autoptr(xmlBuffer) xmlbuf = NULL; int oldIndentTreeOutput = xmlIndentTreeOutput; /* Indentation on output requires that we previously set @@ -2525,12 +2525,10 @@ virNetworkDefFormatBuf(virBufferPtr buf, if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata, virBufferGetIndent(buf) / 2, 1) < 0) { - xmlBufferFree(xmlbuf); xmlIndentTreeOutput = oldIndentTreeOutput; return -1; } virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf)); - xmlBufferFree(xmlbuf); xmlIndentTreeOutput = oldIndentTreeOutput; } diff --git a/src/util/virxml.c b/src/util/virxml.c index 02b59ea2f8..848d549a8b 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -953,8 +953,7 @@ char * virXMLNodeToString(xmlDocPtr doc, xmlNodePtr node) { - xmlBufferPtr xmlbuf = NULL; - char *ret = NULL; + g_autoptr(xmlBuffer) xmlbuf = NULL; if (!(xmlbuf = xmlBufferCreate())) { virReportOOMError(); @@ -964,15 +963,10 @@ virXMLNodeToString(xmlDocPtr doc, if (xmlNodeDump(xmlbuf, doc, node, 0, 1) == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to convert the XML node tree")); - goto cleanup; + return NULL; } - ret = g_strdup((const char *)xmlBufferContent(xmlbuf)); - - cleanup: - xmlBufferFree(xmlbuf); - - return ret; + return g_strdup((const char *)xmlBufferContent(xmlbuf)); } diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index fa9766995c..67bbe27fde 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -697,8 +697,8 @@ virVMXConvertToUTF8(const char *encoding, const char *string) { char *result = NULL; xmlCharEncodingHandlerPtr handler; - xmlBufferPtr input; - xmlBufferPtr utf8; + g_autoptr(xmlBuffer) input = NULL; + g_autoptr(xmlBuffer) utf8 = NULL; handler = xmlFindCharEncodingHandler(encoding); @@ -720,14 +720,10 @@ virVMXConvertToUTF8(const char *encoding, const char *string) goto cleanup; } - result = (char *)utf8->content; - utf8->content = NULL; + result = (char *)g_steal_pointer(&utf8->content); cleanup: xmlCharEncCloseFunc(handler); - xmlBufferFree(input); - xmlBufferFree(utf8); - return result; } -- 2.25.4
On a Wednesday in 2020, Laine Stump wrote:
AUTOPTR_CLEANUP_FUNC is set to xmlBufferFree() in util/virxml.h (This is actually new - added accidentally (but fortunately harmlessly!) in commit 257aba2dafe. I had added it along with the hunks in this patch, then decided to remove it and submit separately, but missed taking out the hunk in virxml.h)
Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 4 +--- src/conf/network_conf.c | 4 +--- src/util/virxml.c | 12 +++--------- src/vmx/vmx.c | 10 +++------- 4 files changed, 8 insertions(+), 22 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virDomainBlkioDeviceParseXML() has multiple cases of sending the return from xmlNodeGetContent() directly to virStrToLong_xx() without checking for NULL. Although it is *very* rare for xmlNodeGetContent() to return NULL (possibly it only happens in an OOM condition? The documentation is unclear), it could happen, and the refactor in this patch manages to eliminate several lines of repeated code while adding in a (single) check for NULL. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1916b51d38..8cde1cd0e8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1628,73 +1628,64 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root, virBlkioDevicePtr dev) { xmlNodePtr node; - g_autofree char *c = NULL; + g_autofree char *path = NULL; node = root->children; while (node) { - if (node->type == XML_ELEMENT_NODE) { - if (virXMLNodeNameEqual(node, "path") && !dev->path) { - dev->path = (char *)xmlNodeGetContent(node); + g_autofree char *c = NULL; + + if (node->type == XML_ELEMENT_NODE && + (c = (char *)xmlNodeGetContent(node))) { + if (virXMLNodeNameEqual(node, "path") && !path) { + path = g_steal_pointer(&c); } else if (virXMLNodeNameEqual(node, "weight")) { - c = (char *)xmlNodeGetContent(node); if (virStrToLong_ui(c, NULL, 10, &dev->weight) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("could not parse weight %s"), c); - goto error; + return -1; } - VIR_FREE(c); } else if (virXMLNodeNameEqual(node, "read_bytes_sec")) { - c = (char *)xmlNodeGetContent(node); if (virStrToLong_ull(c, NULL, 10, &dev->rbps) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("could not parse read bytes sec %s"), c); - goto error; + return -1; } - VIR_FREE(c); } else if (virXMLNodeNameEqual(node, "write_bytes_sec")) { - c = (char *)xmlNodeGetContent(node); if (virStrToLong_ull(c, NULL, 10, &dev->wbps) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("could not parse write bytes sec %s"), c); - goto error; + return -1; } - VIR_FREE(c); } else if (virXMLNodeNameEqual(node, "read_iops_sec")) { - c = (char *)xmlNodeGetContent(node); if (virStrToLong_ui(c, NULL, 10, &dev->riops) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("could not parse read iops sec %s"), c); - goto error; + return -1; } - VIR_FREE(c); } else if (virXMLNodeNameEqual(node, "write_iops_sec")) { - c = (char *)xmlNodeGetContent(node); if (virStrToLong_ui(c, NULL, 10, &dev->wiops) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("could not parse write iops sec %s"), c); - goto error; + return -1; } - VIR_FREE(c); } } node = node->next; } - if (!dev->path) { + + if (!path) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("missing per-device path")); return -1; } + dev->path = g_steal_pointer(&path); return 0; - - error: - VIR_FREE(dev->path); - return -1; } -- 2.25.4
On a Wednesday in 2020, Laine Stump wrote:
virDomainBlkioDeviceParseXML() has multiple cases of sending the return from xmlNodeGetContent() directly to virStrToLong_xx() without checking for NULL. Although it is *very* rare for xmlNodeGetContent() to return NULL (possibly it only happens in an OOM condition? The documentation is unclear), it could happen, and the refactor in this patch manages to eliminate several lines of repeated code while adding in a (single) check for NULL.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1916b51d38..8cde1cd0e8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1628,73 +1628,64 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root, virBlkioDevicePtr dev) { xmlNodePtr node; - g_autofree char *c = NULL; + g_autofree char *path = NULL;
node = root->children; while (node) { - if (node->type == XML_ELEMENT_NODE) { - if (virXMLNodeNameEqual(node, "path") && !dev->path) { - dev->path = (char *)xmlNodeGetContent(node); + g_autofree char *c = NULL; + + if (node->type == XML_ELEMENT_NODE && + (c = (char *)xmlNodeGetContent(node))) {
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@redhat.com> Jano
On 6/25/20 6:44 PM, Ján Tomko wrote:
On a Wednesday in 2020, Laine Stump wrote:
virDomainBlkioDeviceParseXML() has multiple cases of sending the return from xmlNodeGetContent() directly to virStrToLong_xx() without checking for NULL. Although it is *very* rare for xmlNodeGetContent() to return NULL (possibly it only happens in an OOM condition? The documentation is unclear), it could happen, and the refactor in this patch manages to eliminate several lines of repeated code while adding in a (single) check for NULL.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1916b51d38..8cde1cd0e8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1628,73 +1628,64 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root, virBlkioDevicePtr dev) { xmlNodePtr node; - g_autofree char *c = NULL; + g_autofree char *path = NULL;
node = root->children; while (node) { - if (node->type == XML_ELEMENT_NODE) { - if (virXMLNodeNameEqual(node, "path") && !dev->path) { - dev->path = (char *)xmlNodeGetContent(node); + g_autofree char *c = NULL; + + if (node->type == XML_ELEMENT_NODE && + (c = (char *)xmlNodeGetContent(node))) {
Missing ErrorReport if xmlNodeGetContent fails.
Well, I was uncertain whether or not it was *always* an error. The API docs don't specifically say, a google search revealed people asking the question, but nobody answering it definitively (I think there may have been some snarky condescending reply on stackexchange (par for the course), but no actual information), and I stopped trying to figure it out by looking at the libxml2 source after just a couple layers - ain't nobody got time for that! But you apparently tried it out and determined that it will return "" rather than NULL as long as node->type == XML_ELEMENT_NODE, so I'll trust that and treat all NULL returns as OOM (including in a later patch).
Converting this open-coded for loop to an actual for loop would grant us 'continue' privileges, which would make the checks nicer
If you're averse to "else if" I guess.
and give a possibility of assigning the path directly to 'path', without the extra steal_pointer.
I don't follow there - if you assign directly from xmlNodeGetContent() into path, then you'll need to duplicate the virReportOOMError(). Anyway, I'll turn it into a for() loop make the NULL return from xmlNodeGetContent() an error (rather than ignoring it) and resubmit, since it's too many changes to trust me on it.
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@redhat.com>
Jano
On 6/26/20 6:54 PM, Laine Stump wrote:
On 6/25/20 6:44 PM, Ján Tomko wrote:
and give a possibility of assigning the path directly to 'path', without the extra steal_pointer.
I don't follow there - if you assign directly from xmlNodeGetContent() into path, then you'll need to duplicate the virReportOOMError().
Sigh. Nevermind. It had already been several days since I wrote the code, so I'd completely forgotten it and hadn't looked back at the bottom yet (where I would have seen the "extra steal pointer" you mention). Now I get it.
There were a few uses of xmlNodeGetContent() that didn't check for NULL before using the result. A NULL return from xmlNodeGetContent() *could* (probably does) mean that there was an Out of Memory condition, but it is unclear from the documentation if that is always the case, or if it could just indicate a missing value in the document, so we don't report an OOM error, but just don't try to use it for, e.g., conversion to an integer. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8cde1cd0e8..4d27c9caa8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10556,22 +10556,22 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, virXMLNodeNameEqual(cur, "wwn")) { wwn = (char *)xmlNodeGetContent(cur); - if (!virValidateWWN(wwn)) + if (wwn && !virValidateWWN(wwn)) goto error; } else if (!vendor && virXMLNodeNameEqual(cur, "vendor")) { - vendor = (char *)xmlNodeGetContent(cur); - - if (strlen(vendor) > VENDOR_LEN) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("disk vendor is more than 8 characters")); - goto error; - } + if ((vendor = (char *)xmlNodeGetContent(cur))) { + if (strlen(vendor) > VENDOR_LEN) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("disk vendor is more than 8 characters")); + goto error; + } - if (!virStringIsPrintable(vendor)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("disk vendor is not printable string")); - goto error; + if (!virStringIsPrintable(vendor)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("disk vendor is not printable string")); + goto error; + } } } else if (!product && virXMLNodeNameEqual(cur, "product")) { @@ -20374,8 +20374,8 @@ virDomainDefParseBootOptions(virDomainDefPtr def, if (STREQ_NULLABLE(tmp, "slic")) { VIR_FREE(tmp); - tmp = virXMLNodeContentString(nodes[0]); - def->os.slic_table = virFileSanitizePath(tmp); + if ((tmp = virXMLNodeContentString(nodes[0]))) + def->os.slic_table = virFileSanitizePath(tmp); } else { virReportError(VIR_ERR_XML_ERROR, _("Unknown acpi table type: %s"), -- 2.25.4
On a Wednesday in 2020, Laine Stump wrote:
There were a few uses of xmlNodeGetContent() that didn't check for NULL before using the result.
A NULL return from xmlNodeGetContent() *could* (probably does) mean that there was an Out of Memory condition, but it is unclear from the documentation if that is always the case, or if it could just indicate a missing value in the document, so we don't report an OOM error, but just don't try to use it for, e.g., conversion to an integer.
Is it possible to have an element with "no value"? Even <wwn/> gives me an empty string instead of NULL. Jano
Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
On 6/25/20 6:55 PM, Ján Tomko wrote:
On a Wednesday in 2020, Laine Stump wrote:
There were a few uses of xmlNodeGetContent() that didn't check for NULL before using the result.
A NULL return from xmlNodeGetContent() *could* (probably does) mean that there was an Out of Memory condition, but it is unclear from the documentation if that is always the case, or if it could just indicate a missing value in the document, so we don't report an OOM error, but just don't try to use it for, e.g., conversion to an integer.
Is it possible to have an element with "no value"?
I never found anywhere that said "No". But I also never found anywhere that says "yes", so I opted for "do no harm" (or something like that).
Even <wwn/> gives me an empty string instead of NULL.
Okay, *that* says "No". So I'll change the patch to always report an OOM error.
Jano
Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
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@redhat.com> --- 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
On a Wednesday in 2020, Laine Stump wrote:
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@redhat.com> --- src/util/virutil.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On a Wednesday in 2020, Laine Stump wrote:
The error: label in this function just does "return -1", so replace all the "goto error" in the function with "return -1".
I split this out from virDomainDefParse quickly as a build breaker fix and forgot to follow up with this cleanup.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 91 ++++++++++++++++++++---------------------- 1 file changed, 44 insertions(+), 47 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On a Wednesday in 2020, Laine Stump wrote:
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()).
qemuDomainDefFormatXMLInternal does not call it either.
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@redhat.com> --- src/conf/domain_conf.c | 88 ++++++++++++++++++++---------------------- 1 file changed, 42 insertions(+), 46 deletions(-)
With that fixed: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 6/25/20 7:08 PM, Ján Tomko wrote:
On a Wednesday in 2020, Laine Stump wrote:
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()).
qemuDomainDefFormatXMLInternal does not call it either.
Dang! I thought I had followed every call chain with cscope, but maybe I just searched in this one file? Anyway, it's especially embarrassing because not only did I miss qemuDomainFormatXMLInternal(), I also missed virDomainSnapshotDefFormat (which called virDomainSnapshotDefFormatInternal(), which calls virDomainDefFormatInternal()) :-( I think as a followup patch, I should convert every occurrence of "virBuffer blah = VIR_BUFFER_INITIALIZER" to "g_auto(virBuffer) blah = VIR_BUFFER_INITIALIZER" - in a quick search just now I already found a couple more (totally unrelated to virDomainDefFormat) that aren't properly cleared out on error. Thanks for taking the time to actually fact check my claims. #FakeCommitLogs
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@redhat.com> --- src/conf/domain_conf.c | 88 ++++++++++++++++++++---------------------- 1 file changed, 42 insertions(+), 46 deletions(-)
With that fixed: Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
hostsfilestr was not being freed. This will be turned into g_autofree in an upcoming patch converting a lot more of the same file to using g_auto*, but I wanted to make a separate patch for this first so the other patch is simpler to review. Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 47d5d95678..aff1b7b1bc 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1628,6 +1628,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkDriverStatePtr driver, virObjectUnref(dnsmasq_caps); VIR_FREE(configfile); VIR_FREE(configstr); + VIR_FREE(hostsfilestr); VIR_FREE(leaseshelper_path); return ret; } -- 2.25.4
On a Wednesday in 2020, Laine Stump wrote:
hostsfilestr was not being freed. This will be turned into g_autofree in an upcoming patch converting a lot more of the same file to using g_auto*, but I wanted to make a separate patch for this first so the other patch is simpler to review.
It also makes it easier to backport just the fix without the refactor. Fixes: 97a0aa246799c97d0a9ca9ecd6b4fd932ae4756c
Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver.c | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Put in a separate patch so that two future patches can be re-ordered / selectively backported independent of each other. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virfirewall.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h index 6148f46827..ff690b36f0 100644 --- a/src/util/virfirewall.h +++ b/src/util/virfirewall.h @@ -40,6 +40,7 @@ virFirewallPtr virFirewallNew(void); void virFirewallFree(virFirewallPtr firewall); + /** * virFirewallAddRule: * @firewall: firewall ruleset to add to -- 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:
Put in a separate patch so that two future patches can be re-ordered / selectively backported independent of each other.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virfirewall.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h index 6148f46827..ff690b36f0 100644 --- a/src/util/virfirewall.h +++ b/src/util/virfirewall.h @@ -40,6 +40,7 @@ virFirewallPtr virFirewallNew(void);
void virFirewallFree(virFirewallPtr firewall);
+
This is just a whitespace change that contradicts the prevailing style in this file. Jano
/** * virFirewallAddRule: * @firewall: firewall ruleset to add to -- 2.25.4
On 6/25/20 7:17 PM, Ján Tomko wrote:
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:
Put in a separate patch so that two future patches can be re-ordered / selectively backported independent of each other.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virfirewall.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h index 6148f46827..ff690b36f0 100644 --- a/src/util/virfirewall.h +++ b/src/util/virfirewall.h @@ -40,6 +40,7 @@ virFirewallPtr virFirewallNew(void);
void virFirewallFree(virFirewallPtr firewall);
+
This is just a whitespace change that contradicts the prevailing style in this file.
Right. Another patch that I intended to remove before I posted, but forgot because it was late and I wanted to end the day with a clean slate. Derp.
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@redhat.com> --- 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
On a Wednesday in 2020, Laine Stump wrote:
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@redhat.com> --- 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(-)
Since this patch touches way too many functions, it would be much easier to read in two separate commits: 1) add g_auto markers and remove the corresponding VIR_FREE's, possibly leaving empty cleanup sections 2) removing all the now unnecessary labels and gotos Jano
Signed-off-by: Laine Stump <laine@redhat.com> --- 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 Wed, Jun 24, 2020 at 23:34:00 -0400, Laine Stump wrote:
Signed-off-by: Laine Stump <laine@redhat.com> --- 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
[...]
@@ -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;
In general I'm agains senseless replacement of VIR_FREE for g_free. There is IMO no value to do so. VIR_FREE is now implemented via g_clear_pointer(&ptr, g_free) so g_free is actually used. Mass replacements are also substrate for adding bugs and need to be approached carefully, so doing this en-mass might lead to others attempting the same with possibly less care. In general, mass replacements should be done only to g_clear_pointer(&ptr, g_free) and I'm not sure it's worth it.
On 6/25/20 3:55 AM, Peter Krempa wrote:
On Wed, Jun 24, 2020 at 23:34:00 -0400, Laine Stump wrote:
Signed-off-by: Laine Stump <laine@redhat.com> --- 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 [...]
@@ -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; In general I'm agains senseless replacement of VIR_FREE for g_free. There is IMO no value to do so. VIR_FREE is now implemented via g_clear_pointer(&ptr, g_free) so g_free is actually used.
Mass replacements are also substrate for adding bugs and need to be approached carefully, so doing this en-mass might lead to others attempting the same with possibly less care.
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))
In general, mass replacements should be done only to
g_clear_pointer(&ptr, g_free)
and I'm not sure it's worth it.
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 Thu, Jun 25, 2020 at 11:01:48AM -0400, Laine Stump wrote:
On 6/25/20 3:55 AM, Peter Krempa wrote:
On Wed, Jun 24, 2020 at 23:34:00 -0400, Laine Stump wrote:
Signed-off-by: Laine Stump <laine@redhat.com> --- 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 [...]
@@ -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; In general I'm agains senseless replacement of VIR_FREE for g_free. There is IMO no value to do so. VIR_FREE is now implemented via g_clear_pointer(&ptr, g_free) so g_free is actually used.
Mass replacements are also substrate for adding bugs and need to be approached carefully, so doing this en-mass might lead to others attempting the same with possibly less care.
In general, mass replacements should be done only to
g_clear_pointer(&ptr, g_free)
and I'm not sure it's worth it.
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)
The point in getting rid of VIR_FREE is so that we reduce the libvirt specific wrappers in favour of standard APIs. A large portion of the VIR_FREE's will be eliminated by g_autoptr. Another large set of them are used in the virFooStructFree() methods. Those can all be converted to g_free safely, as all the methods do is free stuff. Most VIR_FREEs that occur at the exit of functions can also be safely converted to g_free, if g_autoptr isnt applicable. Sometimes needs a little care if you have multiple goto jumps between labels. The big danger cases are the VIR_FREE()s that occur in the middle of methods, especially in loop bodies. Those the ones that must use the g_clear_pointer, and that's not very many of those, so the ugly syntax isn't an issue. So I see no reason to keep VIR_FREE around long term. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 6/25/20 11:12 AM, Daniel P. Berrangé wrote:
On Thu, Jun 25, 2020 at 11:01:48AM -0400, Laine Stump wrote:
On 6/25/20 3:55 AM, Peter Krempa wrote:
On Wed, Jun 24, 2020 at 23:34:00 -0400, Laine Stump wrote:
Signed-off-by: Laine Stump <laine@redhat.com> --- 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 [...]
@@ -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; In general I'm agains senseless replacement of VIR_FREE for g_free. There is IMO no value to do so. VIR_FREE is now implemented via g_clear_pointer(&ptr, g_free) so g_free is actually used.
Mass replacements are also substrate for adding bugs and need to be approached carefully, so doing this en-mass might lead to others attempting the same with possibly less care.
In general, mass replacements should be done only to
g_clear_pointer(&ptr, g_free)
and I'm not sure it's worth it.
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)
The point in getting rid of VIR_FREE is so that we reduce the libvirt specific wrappers in favour of standard APIs.
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.)
A large portion of the VIR_FREE's will be eliminated by g_autoptr.
Another large set of them are used in the virFooStructFree() methods. Those can all be converted to g_free safely, as all the methods do is free stuff.
Most VIR_FREEs that occur at the exit of functions can also be safely converted to g_free, if g_autoptr isnt applicable. Sometimes needs a little care if you have multiple goto jumps between labels.
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.
The big danger cases are the VIR_FREE()s that occur in the middle of methods, especially in loop bodies. Those the ones that must use the g_clear_pointer, and that's not very many of those, so the ugly syntax isn't an issue.
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.
So I see no reason to keep VIR_FREE around long term.
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.
This struct isn't used anywhere else. Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver.c | 10 ++++++++++ src/network/bridge_driver.h | 9 --------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a1b2f5b6c7..275502b778 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -139,6 +139,16 @@ networkDnsmasqCapsRefresh(virNetworkDriverStatePtr driver) } +extern virXMLNamespace networkDnsmasqXMLNamespace; + +typedef struct _networkDnsmasqXmlNsDef networkDnsmasqXmlNsDef; +typedef networkDnsmasqXmlNsDef *networkDnsmasqXmlNsDefPtr; +struct _networkDnsmasqXmlNsDef { + size_t noptions; + char **options; +}; + + static void networkDnsmasqDefNamespaceFree(void *nsdata) { diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index fb0ccad4b1..2613fc629d 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -27,15 +27,6 @@ #include "virdnsmasq.h" #include "virnetworkobj.h" -extern virXMLNamespace networkDnsmasqXMLNamespace; - -typedef struct _networkDnsmasqXmlNsDef networkDnsmasqXmlNsDef; -typedef networkDnsmasqXmlNsDef *networkDnsmasqXmlNsDefPtr; -struct _networkDnsmasqXmlNsDef { - size_t noptions; - char **options; -}; - virNetworkXMLOptionPtr networkDnsmasqCreateXMLConf(void); -- 2.25.4
On a Wednesday in 2020, Laine Stump wrote:
This struct isn't used anywhere else.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver.c | 10 ++++++++++ src/network/bridge_driver.h | 9 --------- 2 files changed, 10 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virNetworkDHCPLease and virNetworkDHCPLeaseFree() are declared in the public API file libvirt-network.h, and we can't pollute that with glib macro invocations, so put this in src/datatypes.h next to the other virNetwork items. Signed-off-by: Laine Stump <laine@redhat.com> --- src/datatypes.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/datatypes.h b/src/datatypes.h index d58429ad6c..ade3779e43 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -635,6 +635,8 @@ struct _virNetworkPort { G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetworkPort, virObjectUnref); +/* virNetworkDHCPLease is defined in the public API - libvirt-network.h */ +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetworkDHCPLease, virNetworkDHCPLeaseFree); /** * _virInterface: -- 2.25.4
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@redhat.com> --- 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 = ¶ms[i]; -- 2.25.4
The 2nd arg to this function is a bool, not an int. Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1dee2fac6e..cbf5f05f30 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2427,7 +2427,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, goto error; /* Bring up the bridge interface */ - if (virNetDevSetOnline(def->bridge, 1) < 0) + if (virNetDevSetOnline(def->bridge, true) < 0) goto error; devOnline = true; @@ -2505,7 +2505,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, } if (devOnline) - ignore_value(virNetDevSetOnline(def->bridge, 0)); + ignore_value(virNetDevSetOnline(def->bridge, false)); if (firewalRulesAdded && def->forward.type != VIR_NETWORK_FORWARD_OPEN) @@ -2558,7 +2558,7 @@ networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver, ignore_value(virNetDevTapDelete(macTapIfName, NULL)); } - ignore_value(virNetDevSetOnline(def->bridge, 0)); + ignore_value(virNetDevSetOnline(def->bridge, false)); if (def->forward.type != VIR_NETWORK_FORWARD_OPEN) networkRemoveFirewallRules(def); -- 2.25.4
On a Wednesday in 2020, Laine Stump wrote:
The 2nd arg to this function is a bool, not an int.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver_linux.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 0d0ac730f2..7f765bcf99 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -834,7 +834,7 @@ int networkAddFirewallRules(virNetworkDefPtr def) { size_t i; virNetworkIPDefPtr ipdef; - g_autoptr(virFirewall) fw = NULL; + g_autoptr(virFirewall) fw = virFirewallNew(); if (virOnce(&createdOnce, networkSetupPrivateChains) < 0) return -1; @@ -920,8 +920,6 @@ int networkAddFirewallRules(virNetworkDefPtr def) } } - fw = virFirewallNew(); - virFirewallStartTransaction(fw, 0); networkAddGeneralFirewallRules(fw, def); @@ -946,10 +944,7 @@ int networkAddFirewallRules(virNetworkDefPtr def) virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); networkAddChecksumFirewallRules(fw, def); - if (virFirewallApply(fw) < 0) - return -1; - - return 0; + return virFirewallApply(fw); } /* Remove all rules for all ip addresses (and general rules) on a network */ -- 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:
Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver_linux.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 0d0ac730f2..7f765bcf99 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -834,7 +834,7 @@ int networkAddFirewallRules(virNetworkDefPtr def) { size_t i; virNetworkIPDefPtr ipdef; - g_autoptr(virFirewall) fw = NULL; + g_autoptr(virFirewall) fw = virFirewallNew();
if (virOnce(&createdOnce, networkSetupPrivateChains) < 0) return -1; @@ -920,8 +920,6 @@ int networkAddFirewallRules(virNetworkDefPtr def) } }
- fw = virFirewallNew(); - virFirewallStartTransaction(fw, 0);
networkAddGeneralFirewallRules(fw, def); @@ -946,10 +944,7 @@ int networkAddFirewallRules(virNetworkDefPtr def) virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); networkAddChecksumFirewallRules(fw, def);
- if (virFirewallApply(fw) < 0) - return -1; - - return 0; + return virFirewallApply(fw); }
/* Remove all rules for all ip addresses (and general rules) on a network */
On a Wednesday in 2020, Laine Stump wrote:
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 :-)
To me, these seem like changes unrelated to patch 10 and deserve their own commit. Jano
On 6/24/20 11:34 PM, Laine Stump wrote:
Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver_linux.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 0d0ac730f2..7f765bcf99 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -834,7 +834,7 @@ int networkAddFirewallRules(virNetworkDefPtr def) { size_t i; virNetworkIPDefPtr ipdef; - g_autoptr(virFirewall) fw = NULL; + g_autoptr(virFirewall) fw = virFirewallNew(); if (virOnce(&createdOnce, networkSetupPrivateChains) < 0) return -1; @@ -920,8 +920,6 @@ int networkAddFirewallRules(virNetworkDefPtr def) } } - fw = virFirewallNew(); - virFirewallStartTransaction(fw, 0); networkAddGeneralFirewallRules(fw, def); @@ -946,10 +944,7 @@ int networkAddFirewallRules(virNetworkDefPtr def) virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); networkAddChecksumFirewallRules(fw, def); - if (virFirewallApply(fw) < 0) - return -1; - - return 0; + return virFirewallApply(fw); } /* Remove all rules for all ip addresses (and general rules) on a network */
On 6/25/20 7:34 PM, Ján Tomko wrote:
On a Wednesday in 2020, Laine Stump wrote:
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 :-)
To me, these seem like changes unrelated to patch 10 and deserve their own commit.
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.
Jano
On 6/24/20 11:34 PM, Laine Stump wrote:
Signed-off-by: Laine Stump <laine@redhat.com> --- src/network/bridge_driver_linux.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 0d0ac730f2..7f765bcf99 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -834,7 +834,7 @@ int networkAddFirewallRules(virNetworkDefPtr def) { size_t i; virNetworkIPDefPtr ipdef; - g_autoptr(virFirewall) fw = NULL; + g_autoptr(virFirewall) fw = virFirewallNew(); if (virOnce(&createdOnce, networkSetupPrivateChains) < 0) return -1; @@ -920,8 +920,6 @@ int networkAddFirewallRules(virNetworkDefPtr def) } } - fw = virFirewallNew(); - virFirewallStartTransaction(fw, 0); networkAddGeneralFirewallRules(fw, def); @@ -946,10 +944,7 @@ int networkAddFirewallRules(virNetworkDefPtr def) virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); networkAddChecksumFirewallRules(fw, def); - if (virFirewallApply(fw) < 0) - return -1; - - return 0; + return virFirewallApply(fw); } /* Remove all rules for all ip addresses (and general rules) on a network */
Signed-off-by: Laine Stump <laine@redhat.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 63 +++++++---------------- src/util/virebtables.c | 24 ++------- src/util/viriptables.c | 14 ++--- tests/virfirewalltest.c | 50 ++++-------------- 4 files changed, 35 insertions(+), 116 deletions(-) diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 6fc8044c8d..8b77578117 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2863,7 +2863,7 @@ static int ebtablesApplyBasicRules(const char *ifname, const virMacAddr *macaddr) { - virFirewallPtr fw = virFirewallNew(); + g_autoptr(virFirewall) fw = virFirewallNew(); char chain[MAX_CHAINNAME_LENGTH]; char chainPrefix = CHAINPREFIX_HOST_IN_TEMP; char macaddr_str[VIR_MAC_STRING_BUFLEN]; @@ -2871,7 +2871,7 @@ ebtablesApplyBasicRules(const char *ifname, virMacAddrFormat(macaddr, macaddr_str); if (ebiptablesAllTeardown(ifname) < 0) - goto error; + return -1; virFirewallStartTransaction(fw, 0); @@ -2900,13 +2900,10 @@ ebtablesApplyBasicRules(const char *ifname, if (virFirewallApply(fw) < 0) goto tear_down_tmpebchains; - virFirewallFree(fw); return 0; tear_down_tmpebchains: ebtablesCleanAll(ifname); - error: - virFirewallFree(fw); return -1; } @@ -2939,12 +2936,12 @@ ebtablesApplyDHCPOnlyRules(const char *ifname, char macaddr_str[VIR_MAC_STRING_BUFLEN]; unsigned int idx = 0; unsigned int num_dhcpsrvrs; - virFirewallPtr fw = virFirewallNew(); + g_autoptr(virFirewall) fw = virFirewallNew(); virMacAddrFormat(macaddr, macaddr_str); if (ebiptablesAllTeardown(ifname) < 0) - goto error; + return -1; virFirewallStartTransaction(fw, 0); @@ -3019,14 +3016,10 @@ ebtablesApplyDHCPOnlyRules(const char *ifname, if (virFirewallApply(fw) < 0) goto tear_down_tmpebchains; - virFirewallFree(fw); - return 0; tear_down_tmpebchains: ebtablesCleanAll(ifname); - error: - virFirewallFree(fw); return -1; } @@ -3045,10 +3038,10 @@ ebtablesApplyDropAllRules(const char *ifname) { char chain_in [MAX_CHAINNAME_LENGTH], chain_out[MAX_CHAINNAME_LENGTH]; - virFirewallPtr fw = virFirewallNew(); + g_autoptr(virFirewall) fw = virFirewallNew(); if (ebiptablesAllTeardown(ifname) < 0) - goto error; + return -1; virFirewallStartTransaction(fw, 0); @@ -3074,13 +3067,10 @@ ebtablesApplyDropAllRules(const char *ifname) if (virFirewallApply(fw) < 0) goto tear_down_tmpebchains; - virFirewallFree(fw); return 0; tear_down_tmpebchains: ebtablesCleanAll(ifname); - error: - virFirewallFree(fw); return -1; } @@ -3095,8 +3085,7 @@ ebtablesRemoveBasicRules(const char *ifname) static int ebtablesCleanAll(const char *ifname) { - virFirewallPtr fw = virFirewallNew(); - int ret = -1; + g_autoptr(virFirewall) fw = virFirewallNew(); virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); @@ -3112,9 +3101,7 @@ ebtablesCleanAll(const char *ifname) ebtablesRemoveTmpRootChainFW(fw, true, ifname); ebtablesRemoveTmpRootChainFW(fw, false, ifname); - ret = virFirewallApply(fw); - virFirewallFree(fw); - return ret; + return virFirewallApply(fw); } @@ -3362,7 +3349,7 @@ ebiptablesApplyNewRules(const char *ifname, size_t nrules) { size_t i, j; - virFirewallPtr fw = virFirewallNew(); + g_autoptr(virFirewall) fw = virFirewallNew(); virHashTablePtr chains_in_set = virHashCreate(10, NULL); virHashTablePtr chains_out_set = virHashCreate(10, NULL); bool haveEbtables = false; @@ -3563,7 +3550,6 @@ ebiptablesApplyNewRules(const char *ifname, for (i = 0; i < nsubchains; i++) VIR_FREE(subchains[i]); VIR_FREE(subchains); - virFirewallFree(fw); virHashFree(chains_in_set); virHashFree(chains_out_set); @@ -3591,23 +3577,19 @@ ebiptablesTearNewRulesFW(virFirewallPtr fw, const char *ifname) static int ebiptablesTearNewRules(const char *ifname) { - virFirewallPtr fw = virFirewallNew(); - int ret = -1; + g_autoptr(virFirewall) fw = virFirewallNew(); virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); ebiptablesTearNewRulesFW(fw, ifname); - ret = virFirewallApply(fw); - virFirewallFree(fw); - return ret; + return virFirewallApply(fw); } static int ebiptablesTearOldRules(const char *ifname) { - virFirewallPtr fw = virFirewallNew(); - int ret = -1; + g_autoptr(virFirewall) fw = virFirewallNew(); virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); @@ -3626,9 +3608,7 @@ ebiptablesTearOldRules(const char *ifname) ebtablesRemoveRootChainFW(fw, false, ifname); ebtablesRenameTmpSubAndRootChainsFW(fw, ifname); - ret = virFirewallApply(fw); - virFirewallFree(fw); - return ret; + return virFirewallApply(fw); } @@ -3644,8 +3624,7 @@ ebiptablesTearOldRules(const char *ifname) static int ebiptablesAllTeardown(const char *ifname) { - virFirewallPtr fw = virFirewallNew(); - int ret = -1; + g_autoptr(virFirewall) fw = virFirewallNew(); virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); @@ -3667,9 +3646,7 @@ ebiptablesAllTeardown(const char *ifname) ebtablesRemoveRootChainFW(fw, true, ifname); ebtablesRemoveRootChainFW(fw, false, ifname); - ret = virFirewallApply(fw); - virFirewallFree(fw); - return ret; + return virFirewallApply(fw); } @@ -3754,8 +3731,7 @@ static int ebiptablesDriverProbeStateMatch(void) { unsigned long version; - virFirewallPtr fw = virFirewallNew(); - int ret = -1; + g_autoptr(virFirewall) fw = virFirewallNew(); virFirewallStartTransaction(fw, 0); virFirewallAddRuleFull(fw, VIR_FIREWALL_LAYER_IPV4, @@ -3763,7 +3739,7 @@ ebiptablesDriverProbeStateMatch(void) "--version", NULL); if (virFirewallApply(fw) < 0) - goto cleanup; + return -1; /* * since version 1.4.16 '-m state --state ...' will be converted to @@ -3772,10 +3748,7 @@ ebiptablesDriverProbeStateMatch(void) if (version >= 1 * 1000000 + 4 * 1000 + 16) newMatchState = true; - ret = 0; - cleanup: - virFirewallFree(fw); - return ret; + return 0; } static int diff --git a/src/util/virebtables.c b/src/util/virebtables.c index 14a922834a..610c399414 100644 --- a/src/util/virebtables.c +++ b/src/util/virebtables.c @@ -82,10 +82,8 @@ ebtablesContextFree(ebtablesContext *ctx) int ebtablesAddForwardPolicyReject(ebtablesContext *ctx) { - virFirewallPtr fw = NULL; - int ret = -1; + g_autoptr(virFirewall) fw = virFirewallNew(); - fw = virFirewallNew(); virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); virFirewallAddRule(fw, VIR_FIREWALL_LAYER_ETHERNET, "--new-chain", ctx->chain, @@ -99,13 +97,7 @@ ebtablesAddForwardPolicyReject(ebtablesContext *ctx) "-P", ctx->chain, "DROP", NULL); - if (virFirewallApply(fw) < 0) - goto cleanup; - - ret = 0; - cleanup: - virFirewallFree(fw); - return ret; + return virFirewallApply(fw); } @@ -118,10 +110,8 @@ ebtablesForwardAllowIn(ebtablesContext *ctx, const char *macaddr, int action) { - virFirewallPtr fw = NULL; - int ret = -1; + g_autoptr(virFirewall) fw = virFirewallNew(); - fw = virFirewallNew(); virFirewallStartTransaction(fw, 0); virFirewallAddRule(fw, VIR_FIREWALL_LAYER_ETHERNET, action == ADD ? "--insert" : "--delete", @@ -131,13 +121,7 @@ ebtablesForwardAllowIn(ebtablesContext *ctx, "--jump", "ACCEPT", NULL); - if (virFirewallApply(fw) < 0) - goto cleanup; - - ret = 0; - cleanup: - virFirewallFree(fw); - return ret; + return virFirewallApply(fw); } /** diff --git a/src/util/viriptables.c b/src/util/viriptables.c index 8ccce835b2..b5dd2edbd3 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -128,8 +128,7 @@ iptablesPrivateChainCreate(virFirewallPtr fw, int iptablesSetupPrivateChains(virFirewallLayer layer) { - virFirewallPtr fw = NULL; - int ret = -1; + g_autoptr(virFirewall) fw = virFirewallNew(); iptablesGlobalChain filter_chains[] = { {"INPUT", "LIBVIRT_INP"}, {"OUTPUT", "LIBVIRT_OUT"}, @@ -151,8 +150,6 @@ iptablesSetupPrivateChains(virFirewallLayer layer) }; size_t i; - fw = virFirewallNew(); - virFirewallStartTransaction(fw, 0); for (i = 0; i < G_N_ELEMENTS(data); i++) @@ -162,14 +159,9 @@ iptablesSetupPrivateChains(virFirewallLayer layer) "--list-rules", NULL); if (virFirewallApply(fw) < 0) - goto cleanup; - - ret = changed ? 1 : 0; - - cleanup: + return -1; - virFirewallFree(fw); - return ret; + return changed ? 1 : 0; } diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index c4827918c3..a11cbe0e23 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -200,7 +200,7 @@ static int testFirewallSingleGroup(const void *opaque) { virBuffer cmdbuf = VIR_BUFFER_INITIALIZER; - virFirewallPtr fw = NULL; + g_autoptr(virFirewall) fw = virFirewallNew(); int ret = -1; const char *actual = NULL; const char *expected = @@ -217,8 +217,6 @@ testFirewallSingleGroup(const void *opaque) else fwBuf = &cmdbuf; - fw = virFirewallNew(); - virFirewallStartTransaction(fw, 0); virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4, @@ -247,7 +245,6 @@ testFirewallSingleGroup(const void *opaque) virBufferFreeAndReset(&cmdbuf); fwBuf = NULL; virCommandSetDryRun(NULL, NULL, NULL); - virFirewallFree(fw); return ret; } @@ -256,7 +253,7 @@ static int testFirewallRemoveRule(const void *opaque) { virBuffer cmdbuf = VIR_BUFFER_INITIALIZER; - virFirewallPtr fw = NULL; + g_autoptr(virFirewall) fw = virFirewallNew(); int ret = -1; const char *actual = NULL; const char *expected = @@ -274,8 +271,6 @@ testFirewallRemoveRule(const void *opaque) else fwBuf = &cmdbuf; - fw = virFirewallNew(); - virFirewallStartTransaction(fw, 0); virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4, @@ -310,7 +305,6 @@ testFirewallRemoveRule(const void *opaque) virBufferFreeAndReset(&cmdbuf); fwBuf = NULL; virCommandSetDryRun(NULL, NULL, NULL); - virFirewallFree(fw); return ret; } @@ -319,7 +313,7 @@ static int testFirewallManyGroups(const void *opaque G_GNUC_UNUSED) { virBuffer cmdbuf = VIR_BUFFER_INITIALIZER; - virFirewallPtr fw = NULL; + g_autoptr(virFirewall) fw = virFirewallNew(); int ret = -1; const char *actual = NULL; const char *expected = @@ -338,8 +332,6 @@ testFirewallManyGroups(const void *opaque G_GNUC_UNUSED) else fwBuf = &cmdbuf; - fw = virFirewallNew(); - virFirewallStartTransaction(fw, 0); virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4, @@ -380,7 +372,6 @@ testFirewallManyGroups(const void *opaque G_GNUC_UNUSED) virBufferFreeAndReset(&cmdbuf); fwBuf = NULL; virCommandSetDryRun(NULL, NULL, NULL); - virFirewallFree(fw); return ret; } @@ -410,7 +401,7 @@ static int testFirewallIgnoreFailGroup(const void *opaque G_GNUC_UNUSED) { virBuffer cmdbuf = VIR_BUFFER_INITIALIZER; - virFirewallPtr fw = NULL; + g_autoptr(virFirewall) fw = virFirewallNew(); int ret = -1; const char *actual = NULL; const char *expected = @@ -431,8 +422,6 @@ testFirewallIgnoreFailGroup(const void *opaque G_GNUC_UNUSED) fwError = true; } - fw = virFirewallNew(); - virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS); virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4, @@ -473,7 +462,6 @@ testFirewallIgnoreFailGroup(const void *opaque G_GNUC_UNUSED) virBufferFreeAndReset(&cmdbuf); fwBuf = NULL; virCommandSetDryRun(NULL, NULL, NULL); - virFirewallFree(fw); return ret; } @@ -482,7 +470,7 @@ static int testFirewallIgnoreFailRule(const void *opaque G_GNUC_UNUSED) { virBuffer cmdbuf = VIR_BUFFER_INITIALIZER; - virFirewallPtr fw = NULL; + g_autoptr(virFirewall) fw = virFirewallNew(); int ret = -1; const char *actual = NULL; const char *expected = @@ -503,8 +491,6 @@ testFirewallIgnoreFailRule(const void *opaque G_GNUC_UNUSED) fwError = true; } - fw = virFirewallNew(); - virFirewallStartTransaction(fw, 0); virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4, @@ -544,7 +530,6 @@ testFirewallIgnoreFailRule(const void *opaque G_GNUC_UNUSED) virBufferFreeAndReset(&cmdbuf); fwBuf = NULL; virCommandSetDryRun(NULL, NULL, NULL); - virFirewallFree(fw); return ret; } @@ -553,7 +538,7 @@ static int testFirewallNoRollback(const void *opaque G_GNUC_UNUSED) { virBuffer cmdbuf = VIR_BUFFER_INITIALIZER; - virFirewallPtr fw = NULL; + g_autoptr(virFirewall) fw = virFirewallNew(); int ret = -1; const char *actual = NULL; const char *expected = @@ -572,8 +557,6 @@ testFirewallNoRollback(const void *opaque G_GNUC_UNUSED) fwError = true; } - fw = virFirewallNew(); - virFirewallStartTransaction(fw, 0); virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4, @@ -609,7 +592,6 @@ testFirewallNoRollback(const void *opaque G_GNUC_UNUSED) virBufferFreeAndReset(&cmdbuf); fwBuf = NULL; virCommandSetDryRun(NULL, NULL, NULL); - virFirewallFree(fw); return ret; } @@ -617,7 +599,7 @@ static int testFirewallSingleRollback(const void *opaque G_GNUC_UNUSED) { virBuffer cmdbuf = VIR_BUFFER_INITIALIZER; - virFirewallPtr fw = NULL; + g_autoptr(virFirewall) fw = virFirewallNew(); int ret = -1; const char *actual = NULL; const char *expected = @@ -639,8 +621,6 @@ testFirewallSingleRollback(const void *opaque G_GNUC_UNUSED) fwBuf = &cmdbuf; } - fw = virFirewallNew(); - virFirewallStartTransaction(fw, 0); virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4, @@ -693,7 +673,6 @@ testFirewallSingleRollback(const void *opaque G_GNUC_UNUSED) virBufferFreeAndReset(&cmdbuf); fwBuf = NULL; virCommandSetDryRun(NULL, NULL, NULL); - virFirewallFree(fw); return ret; } @@ -701,7 +680,7 @@ static int testFirewallManyRollback(const void *opaque G_GNUC_UNUSED) { virBuffer cmdbuf = VIR_BUFFER_INITIALIZER; - virFirewallPtr fw = NULL; + g_autoptr(virFirewall) fw = virFirewallNew(); int ret = -1; const char *actual = NULL; const char *expected = @@ -722,8 +701,6 @@ testFirewallManyRollback(const void *opaque G_GNUC_UNUSED) fwError = true; } - fw = virFirewallNew(); - virFirewallStartTransaction(fw, 0); virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4, @@ -780,7 +757,6 @@ testFirewallManyRollback(const void *opaque G_GNUC_UNUSED) virBufferFreeAndReset(&cmdbuf); fwBuf = NULL; virCommandSetDryRun(NULL, NULL, NULL); - virFirewallFree(fw); return ret; } @@ -788,7 +764,7 @@ static int testFirewallChainedRollback(const void *opaque G_GNUC_UNUSED) { virBuffer cmdbuf = VIR_BUFFER_INITIALIZER; - virFirewallPtr fw = NULL; + g_autoptr(virFirewall) fw = virFirewallNew(); int ret = -1; const char *actual = NULL; const char *expected = @@ -813,8 +789,6 @@ testFirewallChainedRollback(const void *opaque G_GNUC_UNUSED) fwError = true; } - fw = virFirewallNew(); - virFirewallStartTransaction(fw, 0); virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4, @@ -897,7 +871,6 @@ testFirewallChainedRollback(const void *opaque G_GNUC_UNUSED) virBufferFreeAndReset(&cmdbuf); fwBuf = NULL; virCommandSetDryRun(NULL, NULL, NULL); - virFirewallFree(fw); return ret; } @@ -982,7 +955,7 @@ static int testFirewallQuery(const void *opaque G_GNUC_UNUSED) { virBuffer cmdbuf = VIR_BUFFER_INITIALIZER; - virFirewallPtr fw = NULL; + g_autoptr(virFirewall) fw = virFirewallNew(); int ret = -1; const char *actual = NULL; const char *expected = @@ -1010,8 +983,6 @@ testFirewallQuery(const void *opaque G_GNUC_UNUSED) fwError = true; } - fw = virFirewallNew(); - virFirewallStartTransaction(fw, 0); virFirewallAddRule(fw, VIR_FIREWALL_LAYER_IPV4, @@ -1076,7 +1047,6 @@ testFirewallQuery(const void *opaque G_GNUC_UNUSED) virBufferFreeAndReset(&cmdbuf); fwBuf = NULL; virCommandSetDryRun(NULL, NULL, NULL); - virFirewallFree(fw); return ret; } -- 2.25.4
Signed-off-by: Laine Stump <laine@redhat.com> --- 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
Signed-off-by: Laine Stump <laine@redhat.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 9 +++------ src/nwfilter/nwfilter_driver.c | 3 +-- src/nwfilter/nwfilter_ebiptables_driver.c | 3 +-- src/nwfilter/nwfilter_gentech_driver.c | 3 +-- src/nwfilter/nwfilter_learnipaddr.c | 6 ++---- 5 files changed, 8 insertions(+), 16 deletions(-) diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index f530341872..f54e1a88e0 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -562,8 +562,7 @@ virNWFilterSnoopReqNew(const char *ifkey) return NULL; } - if (VIR_ALLOC(req) < 0) - return NULL; + req = g_new0(virNWFilterSnoopReq, 1); req->threadStatus = THREAD_STATUS_NONE; @@ -737,8 +736,7 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReqPtr req, virNWFilterSnoopReqUnlock(req); - if (VIR_ALLOC(pl) < 0) - return -1; + pl = g_new0(virNWFilterSnoopIPLease, 1); *pl = *plnew; /* protect req->threadkey */ @@ -1160,8 +1158,7 @@ virNWFilterSnoopDHCPDecodeJobSubmit(virThreadPoolPtr pool, if (len <= MIN_VALID_DHCP_PKT_SIZE || len > sizeof(job->packet)) return 0; - if (VIR_ALLOC(job) < 0) - return -1; + job = g_new0(virNWFilterDHCPDecodeJob, 1); memcpy(job->packet, pep, len); job->caplen = len; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 1c407727db..39d0a2128e 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -193,8 +193,7 @@ nwfilterStateInitialize(bool privileged, !(sysbus = virDBusGetSystemBus())) return VIR_DRV_STATE_INIT_ERROR; - if (VIR_ALLOC(driver) < 0) - return VIR_DRV_STATE_INIT_ERROR; + driver = g_new0(virNWFilterDriverState, 1); driver->lockFD = -1; if (virMutexInit(&driver->lock) < 0) diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index cc814235aa..89c131e17f 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -3318,8 +3318,7 @@ ebtablesGetSubChainInsts(virHashTablePtr chains, if ((int)idx < 0) continue; - if (VIR_ALLOC(inst) < 0) - goto cleanup; + inst = g_new0(ebtablesSubChainInst, 1); inst->priority = *(const virNWFilterChainPriority *)filter_names[i].value; inst->incoming = incoming; inst->protoidx = idx; diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 6789a4a3fa..8ba555358d 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -261,8 +261,7 @@ virNWFilterRuleDefToRuleInst(virNWFilterDefPtr def, virNWFilterRuleInstPtr ruleinst; int ret = -1; - if (VIR_ALLOC(ruleinst) < 0) - goto cleanup; + ruleinst = g_new0(virNWFilterRuleInst, 1); ruleinst->chainSuffix = def->chainsuffix; ruleinst->chainPriority = def->chainPriority; diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index 4ce8d5ba03..3bb8c27167 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -151,8 +151,7 @@ virNWFilterLockIface(const char *ifname) ifaceLock = virHashLookup(ifaceLockMap, ifname); if (!ifaceLock) { - if (VIR_ALLOC(ifaceLock) < 0) - goto err_exit; + ifaceLock = g_new0(virNWFilterIfaceLock, 1); if (virMutexInitRecursive(&ifaceLock->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -718,8 +717,7 @@ virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, return -1; } - if (VIR_ALLOC(req) < 0) - return -1; + req = g_new0(virNWFilterIPAddrLearnReq, 1); if (!(req->binding = virNWFilterBindingDefCopy(binding))) goto err_free_req; -- 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@redhat.com> --- 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@redhat.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 91 ++++-------- src/nwfilter/nwfilter_ebiptables_driver.c | 170 +++++++++------------- src/nwfilter/nwfilter_gentech_driver.c | 19 +-- src/nwfilter/nwfilter_learnipaddr.c | 9 +- 4 files changed, 108 insertions(+), 181 deletions(-) diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index f54e1a88e0..32cd6492ad 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -292,18 +292,17 @@ static const unsigned char dhcp_magic[4] = { 99, 130, 83, 99 }; static char * virNWFilterSnoopActivate(virNWFilterSnoopReqPtr req) { - char *key; - - key = g_strdup_printf("%p-%d", req, req->ifindex); + g_autofree char *key = g_strdup_printf("%p-%d", req, req->ifindex); + char *ret = NULL; virNWFilterSnoopActiveLock(); - if (virHashAddEntry(virNWFilterSnoopState.active, key, (void *)0x1) < 0) - VIR_FREE(key); + if (virHashAddEntry(virNWFilterSnoopState.active, key, (void *)0x1) == 0) + ret = g_steal_pointer(&key); virNWFilterSnoopActiveUnlock(); - return key; + return ret; } static void @@ -442,11 +441,10 @@ static int virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl, bool instantiate) { - char *ipaddr; + g_autofree char *ipaddr = virSocketAddrFormat(&ipl->ipAddress); int rc = -1; virNWFilterSnoopReqPtr req; - ipaddr = virSocketAddrFormat(&ipl->ipAddress); if (!ipaddr) return -1; @@ -473,9 +471,6 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl, exit_snooprequnlock: virNWFilterSnoopReqUnlock(req); - - VIR_FREE(ipaddr); - return rc; } @@ -551,7 +546,7 @@ virNWFilterSnoopReqGet(virNWFilterSnoopReqPtr req) static virNWFilterSnoopReqPtr virNWFilterSnoopReqNew(const char *ifkey) { - virNWFilterSnoopReqPtr req; + g_autofree virNWFilterSnoopReqPtr req = g_new0(virNWFilterSnoopReq, 1); if (ifkey == NULL || strlen(ifkey) != VIR_IFKEY_LEN - 1) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -562,28 +557,20 @@ virNWFilterSnoopReqNew(const char *ifkey) return NULL; } - req = g_new0(virNWFilterSnoopReq, 1); - req->threadStatus = THREAD_STATUS_NONE; - if (virStrcpyStatic(req->ifkey, ifkey) < 0|| - virMutexInitRecursive(&req->lock) < 0) - goto err_free_req; + if (virStrcpyStatic(req->ifkey, ifkey) < 0 || + virMutexInitRecursive(&req->lock) < 0) { + return NULL; + } - if (virCondInit(&req->threadStatusCond) < 0) - goto err_destroy_mutex; + if (virCondInit(&req->threadStatusCond) < 0) { + virMutexDestroy(&req->lock); + return NULL; + } virNWFilterSnoopReqGet(req); - - return req; - - err_destroy_mutex: - virMutexDestroy(&req->lock); - - err_free_req: - VIR_FREE(req); - - return NULL; + return g_steal_pointer(&req); } /* @@ -815,7 +802,7 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req, { int ret = 0; virNWFilterSnoopIPLeasePtr ipl; - char *ipstr = NULL; + g_autofree char *ipstr = NULL; /* protect req->start, req->ifname and the lease */ virNWFilterSnoopReqLock(req); @@ -868,8 +855,6 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req, ignore_value(!!g_atomic_int_dec_and_test(&virNWFilterSnoopState.nLeases)); lease_not_found: - VIR_FREE(ipstr); - virNWFilterSnoopReqUnlock(req); return ret; @@ -1045,7 +1030,7 @@ virNWFilterSnoopDHCPOpen(const char *ifname, virMacAddr *mac, pcap_t *handle = NULL; struct bpf_program fp; char pcap_errbuf[PCAP_ERRBUF_SIZE]; - char *ext_filter = NULL; + g_autofree char *ext_filter = NULL; char macaddr[VIR_MAC_STRING_BUFLEN]; virMacAddrFormat(mac, macaddr); @@ -1075,7 +1060,7 @@ virNWFilterSnoopDHCPOpen(const char *ifname, virMacAddr *mac, if (handle == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("pcap_create failed")); - goto cleanup_nohandle; + return NULL; } if (pcap_set_snaplen(handle, PCAP_PBUFSIZE) < 0 || @@ -1107,17 +1092,12 @@ virNWFilterSnoopDHCPOpen(const char *ifname, virMacAddr *mac, } pcap_freecode(&fp); - VIR_FREE(ext_filter); - return handle; cleanup_freecode: pcap_freecode(&fp); cleanup: pcap_close(handle); - cleanup_nohandle: - VIR_FREE(ext_filter); - return NULL; } @@ -1128,7 +1108,7 @@ virNWFilterSnoopDHCPOpen(const char *ifname, virMacAddr *mac, static void virNWFilterDHCPDecodeWorker(void *jobdata, void *opaque) { virNWFilterSnoopReqPtr req = opaque; - virNWFilterDHCPDecodeJobPtr job = jobdata; + g_autofree virNWFilterDHCPDecodeJobPtr job = jobdata; virNWFilterSnoopEthHdrPtr packet = (virNWFilterSnoopEthHdrPtr)job->packet; if (virNWFilterSnoopDHCPDecode(req, packet, @@ -1140,7 +1120,6 @@ static void virNWFilterDHCPDecodeWorker(void *jobdata, void *opaque) "interface '%s'"), req->binding->portdevname); } ignore_value(!!g_atomic_int_dec_and_test(job->qCtr)); - VIR_FREE(job); } /* @@ -1307,7 +1286,7 @@ virNWFilterDHCPSnoopThread(void *req0) int errcount = 0; int tmp = -1, rv, n, pollTo; size_t i; - char *threadkey = NULL; + g_autofree char *threadkey = NULL; virThreadPoolPtr worker = NULL; time_t last_displayed = 0, last_displayed_queue = 0; virNWFilterSnoopPcapConf pcapConf[] = { @@ -1533,8 +1512,6 @@ virNWFilterDHCPSnoopThread(void *req0) virNWFilterSnoopReqPut(req); - VIR_FREE(threadkey); - for (i = 0; i < G_N_ELEMENTS(pcapConf); i++) { if (pcapConf[i].handle) pcap_close(pcapConf[i].handle); @@ -1721,18 +1698,13 @@ static int virNWFilterSnoopLeaseFileWrite(int lfd, const char *ifkey, virNWFilterSnoopIPLeasePtr ipl) { - char *lbuf = NULL; - char *ipstr, *dhcpstr; + g_autofree char *lbuf = NULL; + g_autofree char *ipstr = virSocketAddrFormat(&ipl->ipAddress); + g_autofree char *dhcpstr = virSocketAddrFormat(&ipl->ipServer); int len; - int ret = 0; - ipstr = virSocketAddrFormat(&ipl->ipAddress); - dhcpstr = virSocketAddrFormat(&ipl->ipServer); - - if (!dhcpstr || !ipstr) { - ret = -1; - goto cleanup; - } + if (!dhcpstr || !ipstr) + return -1; /* time intf ip dhcpserver */ lbuf = g_strdup_printf("%u %s %s %s\n", ipl->timeout, ifkey, ipstr, dhcpstr); @@ -1740,18 +1712,11 @@ virNWFilterSnoopLeaseFileWrite(int lfd, const char *ifkey, if (safewrite(lfd, lbuf, len) != len) { virReportSystemError(errno, "%s", _("lease file write failed")); - ret = -1; - goto cleanup; + return -1; } ignore_value(g_fsync(lfd)); - - cleanup: - VIR_FREE(lbuf); - VIR_FREE(dhcpstr); - VIR_FREE(ipstr); - - return ret; + return 0; } /* diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 8fdc8e8897..b382b9405d 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -188,10 +188,10 @@ _printDataType(virNWFilterVarCombIterPtr vars, bool asHex, bool directionIn) { bool done; - char *data; + g_autofree char *data = NULL; uint8_t ctr; - virBuffer vb = VIR_BUFFER_INITIALIZER; - char *flags; + g_auto(virBuffer) vb = VIR_BUFFER_INITIALIZER; + g_autofree char *flags = NULL; if (printVar(vars, buf, bufsize, item, &done) < 0) return -1; @@ -207,10 +207,8 @@ _printDataType(virNWFilterVarCombIterPtr vars, if (g_snprintf(buf, bufsize, "%s", data) >= bufsize) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("buffer too small for IP address")); - VIR_FREE(data); return -1; } - VIR_FREE(data); break; case DATATYPE_IPV6ADDR: @@ -221,10 +219,8 @@ _printDataType(virNWFilterVarCombIterPtr vars, if (g_snprintf(buf, bufsize, "%s", data) >= bufsize) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("buffer too small for IPv6 address")); - VIR_FREE(data); return -1; } - VIR_FREE(data); break; case DATATYPE_MACADDR: @@ -308,10 +304,8 @@ _printDataType(virNWFilterVarCombIterPtr vars, if (virStrcpy(buf, flags, bufsize) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Buffer too small for IPSETFLAGS type")); - VIR_FREE(flags); return -1; } - VIR_FREE(flags); break; case DATATYPE_STRING: @@ -1187,19 +1181,19 @@ _iptablesCreateRuleInstance(virFirewallPtr fw, return -1; if (HAS_ENTRY_ITEM(&rule->p.tcpHdrFilter.dataTCPFlags)) { - char *flags; + g_autofree char *mask = NULL; + g_autofree char *flags = NULL; if (ENTRY_WANT_NEG_SIGN(&rule->p.tcpHdrFilter.dataTCPFlags)) virFirewallRuleAddArg(fw, fwrule, "!"); virFirewallRuleAddArg(fw, fwrule, "--tcp-flags"); - if (!(flags = virNWFilterPrintTCPFlags(rule->p.tcpHdrFilter.dataTCPFlags.u.tcpFlags.mask))) + if (!(mask = virNWFilterPrintTCPFlags(rule->p.tcpHdrFilter.dataTCPFlags.u.tcpFlags.mask))) return -1; - virFirewallRuleAddArg(fw, fwrule, flags); - VIR_FREE(flags); + virFirewallRuleAddArg(fw, fwrule, mask); + if (!(flags = virNWFilterPrintTCPFlags(rule->p.tcpHdrFilter.dataTCPFlags.u.tcpFlags.flags))) return -1; virFirewallRuleAddArg(fw, fwrule, flags); - VIR_FREE(flags); } if (iptablesHandlePortData(fw, fwrule, @@ -1528,7 +1522,7 @@ _iptablesCreateRuleInstance(virFirewallPtr fw, static int printStateMatchFlags(int32_t flags, char **bufptr) { - virBuffer buf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; virNWFilterPrintStateMatchFlags(&buf, "", flags, @@ -1548,7 +1542,9 @@ iptablesCreateRuleInstanceStateCtrl(virFirewallPtr fw, bool directionIn = false; char chainPrefix[2]; bool maySkipICMP, inout = false; - char *matchState = NULL; + g_autofree char *matchState1 = NULL; + g_autofree char *matchState2 = NULL; + g_autofree char *matchState3 = NULL; bool create; if ((rule->tt == VIR_NWFILTER_RULE_DIRECTION_IN) || @@ -1562,7 +1558,6 @@ iptablesCreateRuleInstanceStateCtrl(virFirewallPtr fw, maySkipICMP = directionIn || inout; create = true; - matchState = NULL; if (directionIn && !inout) { if ((rule->flags & IPTABLES_STATE_FLAGS)) @@ -1570,7 +1565,7 @@ iptablesCreateRuleInstanceStateCtrl(virFirewallPtr fw, } if (create && (rule->flags & IPTABLES_STATE_FLAGS)) { - if (printStateMatchFlags(rule->flags, &matchState) < 0) + if (printStateMatchFlags(rule->flags, &matchState1) < 0) return -1; } @@ -1583,11 +1578,10 @@ iptablesCreateRuleInstanceStateCtrl(virFirewallPtr fw, rule, ifname, vars, - matchState, false, + matchState1, false, "RETURN", maySkipICMP); - VIR_FREE(matchState); if (rc < 0) return rc; } @@ -1601,7 +1595,7 @@ iptablesCreateRuleInstanceStateCtrl(virFirewallPtr fw, } if (create && (rule->flags & IPTABLES_STATE_FLAGS)) { - if (printStateMatchFlags(rule->flags, &matchState) < 0) + if (printStateMatchFlags(rule->flags, &matchState2) < 0) return -1; } @@ -1614,12 +1608,9 @@ iptablesCreateRuleInstanceStateCtrl(virFirewallPtr fw, rule, ifname, vars, - matchState, false, + matchState2, false, "ACCEPT", maySkipICMP); - - VIR_FREE(matchState); - if (rc < 0) return rc; } @@ -1633,7 +1624,7 @@ iptablesCreateRuleInstanceStateCtrl(virFirewallPtr fw, create = false; } else { if ((rule->flags & IPTABLES_STATE_FLAGS)) { - if (printStateMatchFlags(rule->flags, &matchState) < 0) + if (printStateMatchFlags(rule->flags, &matchState3) < 0) return -1; } } @@ -1648,10 +1639,9 @@ iptablesCreateRuleInstanceStateCtrl(virFirewallPtr fw, rule, ifname, vars, - matchState, false, + matchState3, false, "RETURN", maySkipICMP); - VIR_FREE(matchState); } return rc; @@ -1797,8 +1787,6 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, const char *target; bool hasMask = false; virFirewallRulePtr fwrule; - int ret = -1; - virBuffer buf = VIR_BUFFER_INITIALIZER; if (STREQ(chainSuffix, virNWFilterChainSuffixTypeToString( @@ -1813,7 +1801,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, \ field, sizeof(field), \ &rule->p.STRUCT.ITEM) < 0) \ - goto cleanup; \ + return -1; \ virFirewallRuleAddArg(fw, fwrule, CLI); \ if (ENTRY_WANT_NEG_SIGN(&rule->p.STRUCT.ITEM)) \ virFirewallRuleAddArg(fw, fwrule, "!"); \ @@ -1825,7 +1813,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, \ field, sizeof(field), \ &rule->p.STRUCT.ITEM) < 0) \ - goto cleanup; \ + return -1; \ virFirewallRuleAddArg(fw, fwrule, CLI); \ if (ENTRY_WANT_NEG_SIGN(&rule->p.STRUCT.ITEM)) \ virFirewallRuleAddArg(fw, fwrule, "!"); \ @@ -1833,7 +1821,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, \ fieldalt, sizeof(fieldalt), \ &rule->p.STRUCT.ITEM_HI) < 0) \ - goto cleanup; \ + return -1; \ virFirewallRuleAddArgFormat(fw, fwrule, \ "%s%s%s", field, SEP, fieldalt); \ } else { \ @@ -1855,13 +1843,13 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, vars, &rule->p.ethHdrFilter.ethHdr, reverse) < 0) - goto cleanup; + return -1; if (HAS_ENTRY_ITEM(&rule->p.ethHdrFilter.dataProtocolID)) { if (printDataTypeAsHex(vars, number, sizeof(number), &rule->p.ethHdrFilter.dataProtocolID) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, "-p"); if (ENTRY_WANT_NEG_SIGN(&rule->p.ethHdrFilter.dataProtocolID)) virFirewallRuleAddArg(fw, fwrule, "!"); @@ -1877,7 +1865,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, vars, &rule->p.vlanHdrFilter.ethHdr, reverse) < 0) - goto cleanup; + return -1; virFirewallRuleAddArgList(fw, fwrule, "-p", "0x8100", NULL); @@ -1906,7 +1894,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, vars, &rule->p.stpHdrFilter.ethHdr, reverse) < 0) - goto cleanup; + return -1; virFirewallRuleAddArgList(fw, fwrule, "-d", NWFILTER_MAC_BGA, NULL); @@ -1942,7 +1930,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, vars, &rule->p.arpHdrFilter.ethHdr, reverse) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, "-p"); virFirewallRuleAddArgFormat(fw, fwrule, "0x%x", @@ -1954,7 +1942,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, number, sizeof(number), &rule->p.arpHdrFilter.dataHWType) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, "--arp-htype"); if (ENTRY_WANT_NEG_SIGN(&rule->p.arpHdrFilter.dataHWType)) virFirewallRuleAddArg(fw, fwrule, "!"); @@ -1965,7 +1953,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, number, sizeof(number), &rule->p.arpHdrFilter.dataOpcode) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, "--arp-opcode"); if (ENTRY_WANT_NEG_SIGN(&rule->p.arpHdrFilter.dataOpcode)) virFirewallRuleAddArg(fw, fwrule, "!"); @@ -1976,7 +1964,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataTypeAsHex(vars, number, sizeof(number), &rule->p.arpHdrFilter.dataProtocolType) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, "--arp-ptype"); if (ENTRY_WANT_NEG_SIGN(&rule->p.arpHdrFilter.dataProtocolType)) virFirewallRuleAddArg(fw, fwrule, "!"); @@ -1987,13 +1975,13 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, ipaddr, sizeof(ipaddr), &rule->p.arpHdrFilter.dataARPSrcIPAddr) < 0) - goto cleanup; + return -1; if (HAS_ENTRY_ITEM(&rule->p.arpHdrFilter.dataARPSrcIPMask)) { if (printDataType(vars, ipmask, sizeof(ipmask), &rule->p.arpHdrFilter.dataARPSrcIPMask) < 0) - goto cleanup; + return -1; hasMask = true; } @@ -2009,13 +1997,13 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, ipaddr, sizeof(ipaddr), &rule->p.arpHdrFilter.dataARPDstIPAddr) < 0) - goto cleanup; + return -1; if (HAS_ENTRY_ITEM(&rule->p.arpHdrFilter.dataARPDstIPMask)) { if (printDataType(vars, ipmask, sizeof(ipmask), &rule->p.arpHdrFilter.dataARPDstIPMask) < 0) - goto cleanup; + return -1; hasMask = true; } @@ -2031,7 +2019,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, macaddr, sizeof(macaddr), &rule->p.arpHdrFilter.dataARPSrcMACAddr) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, reverse ? "--arp-mac-dst" : "--arp-mac-src"); @@ -2044,7 +2032,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, macaddr, sizeof(macaddr), &rule->p.arpHdrFilter.dataARPDstMACAddr) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, reverse ? "--arp-mac-src" : "--arp-mac-dst"); @@ -2069,7 +2057,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, vars, &rule->p.ipHdrFilter.ethHdr, reverse) < 0) - goto cleanup; + return -1; virFirewallRuleAddArgList(fw, fwrule, "-p", "ipv4", NULL); @@ -2078,7 +2066,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, ipaddr, sizeof(ipaddr), &rule->p.ipHdrFilter.ipHdr.dataSrcIPAddr) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, reverse ? "--ip-destination" : "--ip-source"); @@ -2089,7 +2077,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, number, sizeof(number), &rule->p.ipHdrFilter.ipHdr.dataSrcIPMask) < 0) - goto cleanup; + return -1; virFirewallRuleAddArgFormat(fw, fwrule, "%s/%s", ipaddr, number); } else { @@ -2102,7 +2090,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, ipaddr, sizeof(ipaddr), &rule->p.ipHdrFilter.ipHdr.dataDstIPAddr) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, reverse ? "--ip-source" : "--ip-destination"); @@ -2113,7 +2101,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, number, sizeof(number), &rule->p.ipHdrFilter.ipHdr.dataDstIPMask) < 0) - goto cleanup; + return -1; virFirewallRuleAddArgFormat(fw, fwrule, "%s/%s", ipaddr, number); } else { @@ -2125,7 +2113,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, number, sizeof(number), &rule->p.ipHdrFilter.ipHdr.dataProtocolID) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, "--ip-protocol"); if (ENTRY_WANT_NEG_SIGN(&rule->p.ipHdrFilter.ipHdr.dataProtocolID)) @@ -2137,7 +2125,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, number, sizeof(number), &rule->p.ipHdrFilter.portData.dataSrcPortStart) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, reverse ? "--ip-destination-port" : "--ip-source-port"); @@ -2148,7 +2136,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, numberalt, sizeof(numberalt), &rule->p.ipHdrFilter.portData.dataSrcPortEnd) < 0) - goto cleanup; + return -1; virFirewallRuleAddArgFormat(fw, fwrule, "%s:%s", number, numberalt); @@ -2161,7 +2149,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, number, sizeof(number), &rule->p.ipHdrFilter.portData.dataDstPortStart) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, reverse ? "--ip-source-port" : "--ip-destination-port"); @@ -2172,7 +2160,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, numberalt, sizeof(numberalt), &rule->p.ipHdrFilter.portData.dataDstPortEnd) < 0) - goto cleanup; + return -1; virFirewallRuleAddArgFormat(fw, fwrule, "%s:%s", number, numberalt); @@ -2185,7 +2173,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataTypeAsHex(vars, number, sizeof(number), &rule->p.ipHdrFilter.ipHdr.dataDSCP) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, "--ip-tos"); if (ENTRY_WANT_NEG_SIGN(&rule->p.ipHdrFilter.ipHdr.dataDSCP)) @@ -2202,7 +2190,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, vars, &rule->p.ipv6HdrFilter.ethHdr, reverse) < 0) - goto cleanup; + return -1; virFirewallRuleAddArgList(fw, fwrule, "-p", "ipv6", NULL); @@ -2211,7 +2199,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, ipv6addr, sizeof(ipv6addr), &rule->p.ipv6HdrFilter.ipHdr.dataSrcIPAddr) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, reverse ? "--ip6-destination" : "--ip6-source"); @@ -2222,7 +2210,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, number, sizeof(number), &rule->p.ipv6HdrFilter.ipHdr.dataSrcIPMask) < 0) - goto cleanup; + return -1; virFirewallRuleAddArgFormat(fw, fwrule, "%s/%s", ipv6addr, number); } else { @@ -2235,7 +2223,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, ipv6addr, sizeof(ipv6addr), &rule->p.ipv6HdrFilter.ipHdr.dataDstIPAddr) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, reverse ? "--ip6-source" : "--ip6-destination"); @@ -2246,7 +2234,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, number, sizeof(number), &rule->p.ipv6HdrFilter.ipHdr.dataDstIPMask) < 0) - goto cleanup; + return -1; virFirewallRuleAddArgFormat(fw, fwrule, "%s/%s", ipv6addr, number); } else { @@ -2258,7 +2246,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, number, sizeof(number), &rule->p.ipv6HdrFilter.ipHdr.dataProtocolID) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, "--ip6-protocol"); if (ENTRY_WANT_NEG_SIGN(&rule->p.ipv6HdrFilter.ipHdr.dataProtocolID)) @@ -2271,7 +2259,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, number, sizeof(number), &rule->p.ipv6HdrFilter.portData.dataSrcPortStart) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, reverse ? "--ip6-destination-port" : "--ip6-source-port"); @@ -2282,7 +2270,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, numberalt, sizeof(numberalt), &rule->p.ipv6HdrFilter.portData.dataSrcPortEnd) < 0) - goto cleanup; + return -1; virFirewallRuleAddArgFormat(fw, fwrule, "%s:%s", number, numberalt); @@ -2296,7 +2284,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, number, sizeof(number), &rule->p.ipv6HdrFilter.portData.dataDstPortStart) < 0) - goto cleanup; + return -1; virFirewallRuleAddArg(fw, fwrule, reverse ? "--ip6-source-port" : "--ip6-destination-port"); @@ -2307,7 +2295,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, numberalt, sizeof(numberalt), &rule->p.ipv6HdrFilter.portData.dataDstPortEnd) < 0) - goto cleanup; + return -1; virFirewallRuleAddArgFormat(fw, fwrule, "%s:%s", number, numberalt); @@ -2321,7 +2309,8 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, HAS_ENTRY_ITEM(&rule->p.ipv6HdrFilter.dataICMPCodeStart) || HAS_ENTRY_ITEM(&rule->p.ipv6HdrFilter.dataICMPCodeEnd)) { bool lo = false; - char *r; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + g_autofree char *r = NULL; virFirewallRuleAddArg(fw, fwrule, "--ip6-icmp-type"); @@ -2330,7 +2319,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, number, sizeof(number), &rule->p.ipv6HdrFilter.dataICMPTypeStart) < 0) - goto cleanup; + return -1; lo = true; } else { ignore_value(virStrcpyStatic(number, "0")); @@ -2342,7 +2331,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, numberalt, sizeof(numberalt), &rule->p.ipv6HdrFilter.dataICMPTypeEnd) < 0) - goto cleanup; + return -1; } else { if (lo) ignore_value(virStrcpyStatic(numberalt, number)); @@ -2358,7 +2347,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, number, sizeof(number), &rule->p.ipv6HdrFilter.dataICMPCodeStart) < 0) - goto cleanup; + return -1; lo = true; } else { ignore_value(virStrcpyStatic(number, "0")); @@ -2370,7 +2359,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, if (printDataType(vars, numberalt, sizeof(numberalt), &rule->p.ipv6HdrFilter.dataICMPCodeEnd) < 0) - goto cleanup; + return -1; } else { if (lo) ignore_value(virStrcpyStatic(numberalt, number)); @@ -2386,8 +2375,6 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, r = virBufferContentAndReset(&buf); virFirewallRuleAddArg(fw, fwrule, r); - - VIR_FREE(r); } break; @@ -2421,11 +2408,7 @@ ebtablesCreateRuleInstance(virFirewallPtr fw, #undef INST_ITEM_2PARMS #undef INST_ITEM - ret = 0; - cleanup: - virBufferFreeAndReset(&buf); - - return ret; + return 0; } @@ -3301,9 +3284,8 @@ ebtablesGetSubChainInsts(virHashTablePtr chains, ebtablesSubChainInstPtr **insts, size_t *ninsts) { - virHashKeyValuePairPtr filter_names; + g_autofree virHashKeyValuePairPtr filter_names = NULL; size_t i; - int ret = -1; filter_names = virHashGetItems(chains, ebiptablesFilterOrderSort); @@ -3311,7 +3293,7 @@ ebtablesGetSubChainInsts(virHashTablePtr chains, return -1; for (i = 0; filter_names[i].key; i++) { - ebtablesSubChainInstPtr inst; + g_autofree ebtablesSubChainInstPtr inst = NULL; enum l3_proto_idx idx = ebtablesGetProtoIdxByFiltername( filter_names[i].key); @@ -3324,18 +3306,11 @@ ebtablesGetSubChainInsts(virHashTablePtr chains, inst->protoidx = idx; inst->filtername = filter_names[i].key; - if (VIR_APPEND_ELEMENT(*insts, *ninsts, inst) < 0) { - VIR_FREE(inst); - goto cleanup; - } + if (VIR_APPEND_ELEMENT(*insts, *ninsts, inst) < 0) + return -1; } - ret = 0; - - cleanup: - VIR_FREE(filter_names); - return ret; - + return 0; } static int @@ -3345,12 +3320,12 @@ ebiptablesApplyNewRules(const char *ifname, { size_t i, j; g_autoptr(virFirewall) fw = virFirewallNew(); - virHashTablePtr chains_in_set = virHashCreate(10, NULL); - virHashTablePtr chains_out_set = virHashCreate(10, NULL); + g_autoptr(virHashTable) chains_in_set = virHashCreate(10, NULL); + g_autoptr(virHashTable) chains_out_set = virHashCreate(10, NULL); bool haveEbtables = false; bool haveIptables = false; bool haveIp6tables = false; - ebtablesSubChainInstPtr *subchains = NULL; + g_autofree ebtablesSubChainInstPtr *subchains = NULL; size_t nsubchains = 0; int ret = -1; @@ -3544,9 +3519,6 @@ ebiptablesApplyNewRules(const char *ifname, cleanup: for (i = 0; i < nsubchains; i++) VIR_FREE(subchains[i]); - VIR_FREE(subchains); - virHashFree(chains_in_set); - virHashFree(chains_out_set); return ret; } diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 8ba555358d..f586c7e938 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -414,7 +414,6 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, virNWFilterDefPtr next_filter; virNWFilterDefPtr newNext_filter; virNWFilterVarValuePtr val; - virHashTablePtr tmpvars; for (i = 0; i < filter->nentries; i++) { virNWFilterRuleDefPtr rule = filter->filterEntries[i]->rule; @@ -424,20 +423,16 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, for (j = 0; j < rule->nVarAccess; j++) { if (!virNWFilterVarAccessIsAvailable(rule->varAccess[j], vars)) { - char *varAccess; - virBuffer buf = VIR_BUFFER_INITIALIZER; + g_autofree char *varAccess = NULL; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; virNWFilterVarAccessPrint(rule->varAccess[j], &buf); - val = virNWFilterVarValueCreateSimpleCopyValue("1"); - if (!val) { - virBufferFreeAndReset(&buf); + if (!(val = virNWFilterVarValueCreateSimpleCopyValue("1"))) return -1; - } varAccess = virBufferContentAndReset(&buf); rc = virHashUpdateEntry(missing_vars, varAccess, val); - VIR_FREE(varAccess); if (rc < 0) { virNWFilterVarValueFree(val); return -1; @@ -445,6 +440,8 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, } } } else if (inc) { + g_autoptr(virHashTable) tmpvars = NULL; + VIR_DEBUG("Following filter %s", inc->filterref); if (!(obj = virNWFilterObjListFindInstantiateFilter(driver->nwfilters, inc->filterref))) @@ -473,9 +470,6 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, missing_vars, useNewFilter, driver); - - virHashFree(tmpvars); - virNWFilterObjUnlock(obj); if (rc < 0) return -1; @@ -516,7 +510,7 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, int rc; virNWFilterInst inst; bool instantiate = true; - char *buf; + g_autofree char *buf = NULL; virNWFilterVarValuePtr lv; const char *learning; bool reportIP = false; @@ -636,7 +630,6 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot instantiate filter due to unresolvable " "variables or unavailable list elements: %s"), buf); - VIR_FREE(buf); } rc = -1; diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index 3bb8c27167..7bb39c3a66 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -396,8 +396,8 @@ learnIPAddressThread(void *arg) req->binding->portdevname); int dhcp_opts_len; char macaddr[VIR_MAC_STRING_BUFLEN]; - virBuffer buf = VIR_BUFFER_INITIALIZER; - char *filter = NULL; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + g_autofree char *filter = NULL; uint16_t etherType; bool showError = true; enum howDetect howDetected = 0; @@ -622,8 +622,6 @@ learnIPAddressThread(void *arg) } /* while */ done: - VIR_FREE(filter); - if (handle) pcap_close(handle); @@ -633,7 +631,7 @@ learnIPAddressThread(void *arg) sa.len = sizeof(sa.data.inet4); sa.data.inet4.sin_family = AF_INET; sa.data.inet4.sin_addr.s_addr = vmaddr; - char *inetaddr; + g_autofree char *inetaddr = NULL; /* It is necessary to unlock interface here to avoid updateMutex and * interface ordering deadlocks. Otherwise we are going to @@ -656,7 +654,6 @@ learnIPAddressThread(void *arg) req->ifindex); VIR_DEBUG("Result from applying firewall rules on " "%s with IP addr %s : %d", req->binding->portdevname, inetaddr, ret); - VIR_FREE(inetaddr); } } else { if (showError) -- 2.25.4
Signed-off-by: Laine Stump <laine@redhat.com> --- 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
This rewrite of a nested conditional produces the same results, but eliminate a goto and corresponding label. Signed-off-by: Laine Stump <laine@redhat.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 6e05e638aa..90afbe21f1 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -3101,13 +3101,12 @@ virNWFilterRuleInstSort(const void *a, const void *b) /* ensure root chain commands appear before all others since we will need them to create the child chains */ if (root_a) { - if (root_b) - goto normal; - return -1; /* a before b */ - } - if (root_b) + if (!root_b) + return -1; /* a before b */ + } else if (root_b) { return 1; /* b before a */ - normal: + } + /* priorities are limited to range [-1000, 1000] */ return insta->priority - instb->priority; } -- 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@redhat.com> --- 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
g_new() is used in only 3 places. Switching them to g_new0() will do no harm, reduces confusion, and helps me sleep better at night knowing that all allocated memory is initialized to 0 :-) (Yes, I *know* that in all three cases the associated memory is immediately assigned some other value. Today.) Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_backup.c | 2 +- src/util/virutil.c | 2 +- tests/qemuhotplugmock.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index 8dc9d2504d..dae9300567 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -64,7 +64,7 @@ qemuBackupPrepare(virDomainBackupDefPtr def) if (def->type == VIR_DOMAIN_BACKUP_TYPE_PULL) { if (!def->server) { - def->server = g_new(virStorageNetHostDef, 1); + def->server = g_new0(virStorageNetHostDef, 1); def->server->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; def->server->name = g_strdup("localhost"); diff --git a/src/util/virutil.c b/src/util/virutil.c index 04f882fda7..ff664ea778 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -962,7 +962,7 @@ virGetGroupList(uid_t uid, gid_t gid, gid_t **list) if (uid != (uid_t)-1 && virGetUserEnt(uid, &user, &primary, NULL, NULL, true) >= 0) { int nallocgrps = 10; - gid_t *grps = g_new(gid_t, nallocgrps); + gid_t *grps = g_new0(gid_t, nallocgrps); while (1) { int nprevallocgrps = nallocgrps; diff --git a/tests/qemuhotplugmock.c b/tests/qemuhotplugmock.c index d2324913cf..29fac8a598 100644 --- a/tests/qemuhotplugmock.c +++ b/tests/qemuhotplugmock.c @@ -57,7 +57,7 @@ virDevMapperGetTargets(const char *path, *devPaths = NULL; if (STREQ(path, "/dev/mapper/virt")) { - *devPaths = g_new(char *, 4); + *devPaths = g_new0(char *, 4); (*devPaths)[0] = g_strdup("/dev/block/8:0"); /* /dev/sda */ (*devPaths)[1] = g_strdup("/dev/block/8:16"); /* /dev/sdb */ (*devPaths)[2] = g_strdup("/dev/block/8:32"); /* /dev/sdc */ -- 2.25.4
participants (4)
-
Daniel P. Berrangé -
Ján Tomko -
Laine Stump -
Peter Krempa