4:08 p.m.
V1 was here:
https://www.redhat.com/archives/libvir-list/2020-June/msg01156.html
Some patches were ACKed and pushed. I re-ordered/re-organized most of
the rest, and removed some others to deal with separately (the
xmlNodeContent stuff)
What's left here is a few preliminary patches, then the standard set,
once for network and again for nwfilter:
1) convert from VIR_(RE)ALLOC(_N) to g_new0()/g_renew()
2) use g_auto*() where appropriate, removing unneeded free's
3) get rid of now-extraneous labels
4) (controversial) replace any remaining VIR_FREE() with g_free() (and
possibly g_clear_pointer() when needed
NB: these patches require my virBuffer "convert to g_auto" series
as a prerequisite:
https://www.redhat.com/archives/libvir-list/2020-July/msg00185.html
Changes from V1:
* move conversion of virFirewall and virBuffer automatics to another
series (see above)
* re-order to replace VIR_ALLOC first (without adding any g_auto*)
instead of doing it after g_auto conversion of automatics, then do
all g_auto additions at o
* separate label elimination into separate patches per jtomko's
suggestion.
Laine Stump (15):
replace g_new() with g_new0() for consistency
util: define g_autoptr cleanups for a couple dnsmasq objects
define g_autoptr cleanup function for virNetworkDHCPLease
network: replace VIR_ALLOC/REALLOC with g_new0/g_renew
network: use g_auto wherever appropriate
network: eliminate unnecessary labels
network: use g_free() in place of remaining VIR_FREE()
nwfilter: remove unnecessary code from ebtablesGetSubChainInsts()
nwfilter: clear nrules when resetting virNWFilterInst
nwfilter: define a typedef for struct ebtablesSubChainInst
nwfilter: transform logic in virNWFilterRuleInstSort to eliminate
label
nwfilter: use standard label names when reasonable
nwfilter: replace VIR_ALLOC with g_new0
nwfilter: convert local pointers to use g_auto*
nwfilter: convert remaining VIR_FREE() to g_free()
src/datatypes.h | 2 +
src/network/bridge_driver.c | 536 ++++++++--------------
src/network/bridge_driver_linux.c | 22 +-
src/network/leaseshelper.c | 16 +-
src/nwfilter/nwfilter_dhcpsnoop.c | 150 +++---
src/nwfilter/nwfilter_driver.c | 13 +-
src/nwfilter/nwfilter_ebiptables_driver.c | 119 ++---
src/nwfilter/nwfilter_gentech_driver.c | 57 ++-
src/nwfilter/nwfilter_learnipaddr.c | 43 +-
src/qemu/qemu_backup.c | 2 +-
src/util/virdnsmasq.h | 4 +
src/util/virutil.c | 2 +-
tests/qemuhotplugmock.c | 2 +-
13 files changed, 379 insertions(+), 589 deletions(-)
--
2.25.4
4:08 p.m.
New subject: [libvirt PATCH v2 01/15] replace g_new() with g_new0() for consistency
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(a)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
9:29 a.m.
New subject: [libvirt PATCH v2 01/15] replace g_new() with g_new0() for consistency
On a Tuesday in 2020, Laine Stump wrote:
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
Sweet dreams.
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(a)redhat.com>
---
src/qemu/qemu_backup.c | 2 +-
src/util/virutil.c | 2 +-
tests/qemuhotplugmock.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:08 p.m.
New subject: [libvirt PATCH v2 02/15] util: define g_autoptr cleanups for a couple dnsmasq objects
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virdnsmasq.h | 4 ++++
1 file changed, 4 insertions(+)
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
9:30 a.m.
New subject: [libvirt PATCH v2 02/15] util: define g_autoptr cleanups for a couple dnsmasq objects
On a Tuesday in 2020, Laine Stump wrote:
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virdnsmasq.h | 4 ++++
1 file changed, 4 insertions(+)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:08 p.m.
New subject: [libvirt PATCH v2 03/15] define g_autoptr cleanup function for virNetworkDHCPLease
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(a)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
9:33 a.m.
New subject: [libvirt PATCH v2 03/15] define g_autoptr cleanup function for virNetworkDHCPLease
On a Tuesday in 2020, Laine Stump wrote:
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(a)redhat.com>
---
src/datatypes.h | 2 ++
1 file changed, 2 insertions(+)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:08 p.m.
New subject: [libvirt PATCH v2 04/15] network: replace VIR_ALLOC/REALLOC with g_new0/g_renew
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/network/bridge_driver.c | 29 ++++++++++-------------------
1 file changed, 10 insertions(+), 19 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 713763130b..ab359acdb5 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -177,8 +177,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,12 +195,9 @@ static int
networkDnsmasqDefNamespaceParse(xmlXPathContextPtr ctxt,
void **data)
{
- networkDnsmasqXmlNsDefPtr nsdata = NULL;
+ networkDnsmasqXmlNsDefPtr nsdata = g_new0(networkDnsmasqXmlNsDef, 1);
int ret = -1;
- if (VIR_ALLOC(nsdata) < 0)
- return -1;
-
if (networkDnsmasqDefNamespaceParseOptions(nsdata, ctxt))
goto cleanup;
@@ -733,8 +729,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) {
@@ -2753,8 +2748,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];
@@ -4323,8 +4317,7 @@ networkGetDHCPLeases(virNetworkPtr net,
continue;
if (need_results) {
- if (VIR_ALLOC(lease) < 0)
- goto error;
+ lease = g_new0(virNetworkDHCPLease, 1);
lease->expirytime = expirytime_tmp;
@@ -4378,9 +4371,8 @@ networkGetDHCPLeases(virNetworkPtr net,
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;
@@ -5612,10 +5604,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
9:42 a.m.
New subject: [libvirt PATCH v2 04/15] network: replace VIR_ALLOC/REALLOC with g_new0/g_renew
On a Tuesday in 2020, Laine Stump wrote:
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/network/bridge_driver.c | 29 ++++++++++-------------------
1 file changed, 10 insertions(+), 19 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 713763130b..ab359acdb5 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -177,8 +177,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,12 +195,9 @@ static int
networkDnsmasqDefNamespaceParse(xmlXPathContextPtr ctxt,
void **data)
{
- networkDnsmasqXmlNsDefPtr nsdata = NULL;
+ networkDnsmasqXmlNsDefPtr nsdata = g_new0(networkDnsmasqXmlNsDef, 1);
int ret = -1;
- if (VIR_ALLOC(nsdata) < 0)
- return -1;
-
if (networkDnsmasqDefNamespaceParseOptions(nsdata, ctxt))
goto cleanup;
@@ -733,8 +729,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) {
@@ -2753,8 +2748,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];
@@ -4323,8 +4317,7 @@ networkGetDHCPLeases(virNetworkPtr net,
continue;
if (need_results) {
- if (VIR_ALLOC(lease) < 0)
- goto error;
+ lease = g_new0(virNetworkDHCPLease, 1);
lease->expirytime = expirytime_tmp;
@@ -4378,9 +4371,8 @@ networkGetDHCPLeases(virNetworkPtr net,
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);
Double space before nleases.
Also, this is a faithful conversion, but neither VIR_REALLOC_N nor
g_renew guarantee that the memory will be zeroed.
+ *leases = g_steal_pointer(&leases_ret);
}
rv = nleases;
With the double space removed:
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:08 p.m.
New subject: [libvirt PATCH v2 05/15] network: use g_auto wherever appropriate
This includes standard g_autofree() as well as other objects that have
a cleanup function defined to use via g_autoptr (virCommand,
virJSONValue)
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/network/bridge_driver.c | 206 ++++++++++--------------------
src/network/bridge_driver_linux.c | 7 +-
src/network/leaseshelper.c | 16 +--
3 files changed, 76 insertions(+), 153 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index ab359acdb5..31bd0dd92c 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -160,6 +160,7 @@ networkDnsmasqDefNamespaceFree(void *nsdata)
VIR_FREE(def);
}
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(networkDnsmasqXmlNsDef, networkDnsmasqDefNamespaceFree);
static int
@@ -195,7 +196,7 @@ static int
networkDnsmasqDefNamespaceParse(xmlXPathContextPtr ctxt,
void **data)
{
- networkDnsmasqXmlNsDefPtr nsdata = g_new0(networkDnsmasqXmlNsDef, 1);
+ g_autoptr(networkDnsmasqXmlNsDef) nsdata = g_new0(networkDnsmasqXmlNsDef, 1);
int ret = -1;
if (networkDnsmasqDefNamespaceParseOptions(nsdata, ctxt))
@@ -207,7 +208,6 @@ networkDnsmasqDefNamespaceParse(xmlXPathContextPtr ctxt,
ret = 0;
cleanup:
- networkDnsmasqDefNamespaceFree(nsdata);
return ret;
}
@@ -329,7 +329,7 @@ networkRunHook(virNetworkObjPtr obj,
{
virNetworkDefPtr def;
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
- char *xml = NULL;
+ g_autofree char *xml = NULL;
int hookret;
int ret = -1;
@@ -366,7 +366,6 @@ networkRunHook(virNetworkObjPtr obj,
ret = 0;
cleanup:
- VIR_FREE(xml);
return ret;
}
@@ -431,14 +430,14 @@ 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;
@@ -492,14 +491,6 @@ networkRemoveInactive(virNetworkDriverStatePtr driver,
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;
}
@@ -550,9 +541,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);
@@ -614,7 +605,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,
@@ -630,14 +621,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;
}
@@ -716,8 +704,8 @@ networkStateInitialize(bool privileged,
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;
@@ -845,8 +833,6 @@ networkStateInitialize(bool privileged,
ret = VIR_DRV_STATE_INIT_COMPLETE;
cleanup:
- VIR_FREE(configdir);
- VIR_FREE(rundir);
return ret;
error:
@@ -1047,10 +1033,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)
@@ -1071,7 +1058,6 @@ networkDnsmasqConfLocalPTRs(virBufferPtr buf,
}
virBufferAsprintf(buf, "local=/%s/\n", ptr);
- VIR_FREE(ptr);
}
return 0;
@@ -1095,7 +1081,6 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
bool wantDNS = dns->enable != VIR_TRISTATE_BOOL_NO;
virNetworkIPDefPtr tmpipdef, ipdef, ipv4def, ipv6def;
bool ipv6SLAAC;
- char *saddr = NULL, *eaddr = NULL;
*configstr = NULL;
@@ -1150,12 +1135,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;
virBufferAsprintf(&configbuf, "%s\n", addr);
- VIR_FREE(addr);
if (!fwd->domain)
addNoResolv = true;
} else {
@@ -1228,7 +1212,7 @@ 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;
@@ -1254,11 +1238,9 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
"(as described in
RFC1918/RFC3484/RFC4193)."),
ipaddr, (int)version / 1000000,
(int)(version % 1000000) / 1000);
- VIR_FREE(ipaddr);
goto cleanup;
}
virBufferAsprintf(&configbuf, "listen-address=%s\n", ipaddr);
- VIR_FREE(ipaddr);
}
}
@@ -1414,6 +1396,8 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
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)))
@@ -1446,8 +1430,6 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
virBufferAddLit(&configbuf, "\n");
- VIR_FREE(saddr);
- VIR_FREE(eaddr);
thisRange = virSocketAddrGetRange(&range.addr.start,
&range.addr.end,
&ipdef->address,
@@ -1464,7 +1446,7 @@ 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;
virBufferAsprintf(&configbuf, "dhcp-range=%s,static",
@@ -1472,7 +1454,6 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
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)
@@ -1492,13 +1473,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;
virBufferAsprintf(&configbuf, "dhcp-boot=%s%s%s\n",
ipdef->bootfile, ",,", bootserver);
- VIR_FREE(bootserver);
} else {
virBufferAsprintf(&configbuf, "dhcp-boot=%s\n",
ipdef->bootfile);
}
@@ -1543,12 +1523,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;
virBufferAsprintf(&configbuf,
"dhcp-range=%s,ra-only\n", bridgeaddr);
- VIR_FREE(bridgeaddr);
}
}
}
@@ -1569,8 +1548,6 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
ret = 0;
cleanup:
- VIR_FREE(saddr);
- VIR_FREE(eaddr);
return ret;
}
@@ -1584,13 +1561,13 @@ networkBuildDhcpDaemonCommandLine(virNetworkDriverStatePtr
driver,
dnsmasqContext *dctx)
{
virNetworkDefPtr def = virNetworkObjGetDef(obj);
- dnsmasqCapsPtr dnsmasq_caps = networkGetDnsmasqCaps(driver);
- virCommandPtr cmd = NULL;
+ g_autoptr(dnsmasqCaps) dnsmasq_caps = networkGetDnsmasqCaps(driver);
+ g_autoptr(virCommand) cmd = NULL;
int ret = -1;
- char *configfile = NULL;
- char *configstr = NULL;
- char *hostsfilestr = NULL;
- char *leaseshelper_path = 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);
@@ -1625,14 +1602,9 @@ networkBuildDhcpDaemonCommandLine(virNetworkDriverStatePtr driver,
virCommandAddArgFormat(cmd, "--dhcp-script=%s", leaseshelper_path);
virCommandAddEnvPair(cmd, "VIR_BRIDGE_NAME", def->bridge);
- *cmdout = cmd;
+ *cmdout = g_steal_pointer(&cmd);
ret = 0;
cleanup:
- virObjectUnref(dnsmasq_caps);
- VIR_FREE(configfile);
- VIR_FREE(configstr);
- VIR_FREE(hostsfilestr);
- VIR_FREE(leaseshelper_path);
return ret;
}
@@ -1645,11 +1617,11 @@ 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;
@@ -1722,9 +1694,6 @@ networkStartDhcpDaemon(virNetworkDriverStatePtr driver,
ret = 0;
cleanup:
- VIR_FREE(pidfile);
- virCommandFree(cmd);
- dnsmasqContextFree(dctx);
return ret;
}
@@ -1745,7 +1714,7 @@ networkRefreshDhcpDaemon(virNetworkDriverStatePtr driver,
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))
@@ -1797,7 +1766,6 @@ networkRefreshDhcpDaemon(virNetworkDriverStatePtr driver,
dnsmasqPid = virNetworkObjGetDnsmasqPid(obj);
ret = kill(dnsmasqPid, SIGHUP);
cleanup:
- dnsmasqContextFree(dctx);
return ret;
}
@@ -1872,7 +1840,7 @@ 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) {
@@ -1889,7 +1857,6 @@ networkRadvdConfContents(virNetworkObjPtr obj,
" {\n%s };\n",
netaddr, prefix,
dhcp6 ? radvd2 : radvd3);
- VIR_FREE(netaddr);
}
virBufferAddLit(&configbuf, "};\n");
@@ -1908,8 +1875,8 @@ networkRadvdConfWrite(virNetworkDriverStatePtr driver,
{
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;
@@ -1937,8 +1904,6 @@ networkRadvdConfWrite(virNetworkDriverStatePtr driver,
ret = 0;
cleanup:
- VIR_FREE(configStr);
- VIR_FREE(myConfigFile);
return ret;
}
@@ -1948,12 +1913,12 @@ 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;
+ g_autofree char *pidfile = NULL;
+ g_autofree char *radvdpidbase = NULL;
+ g_autofree char *configfile = NULL;
+ g_autoptr(virCommand) cmd = NULL;
int ret = -1;
virNetworkObjSetRadvdPid(obj, -1);
@@ -2026,11 +1991,6 @@ networkStartRadvd(virNetworkDriverStatePtr driver,
ret = 0;
cleanup:
- virObjectUnref(dnsmasq_caps);
- virCommandFree(cmd);
- VIR_FREE(configfile);
- VIR_FREE(radvdpidbase);
- VIR_FREE(pidfile);
return ret;
}
@@ -2040,14 +2000,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 */
@@ -2056,7 +2015,6 @@ networkRefreshRadvd(virNetworkDriverStatePtr driver,
}
return 0;
}
- virObjectUnref(dnsmasq_caps);
/* if there's no running radvd, just start it */
radvdPid = virNetworkObjGetRadvdPid(obj);
@@ -2248,7 +2206,7 @@ static int
networkSetIPv6Sysctls(virNetworkObjPtr obj)
{
virNetworkDefPtr def = virNetworkObjGetDef(obj);
- char *field = NULL;
+ g_autofree char *field = NULL;
int ret = -1;
bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0);
@@ -2273,7 +2231,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
"on bridge %s"), field, def->bridge);
goto cleanup;
}
- 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.
@@ -2282,6 +2239,7 @@ 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);
@@ -2290,11 +2248,11 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
_("cannot disable %s"), field);
goto cleanup;
}
- 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) {
@@ -2305,7 +2263,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
ret = 0;
cleanup:
- VIR_FREE(field);
return ret;
}
@@ -2384,7 +2341,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;
@@ -2399,7 +2357,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;
@@ -2416,9 +2373,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;
@@ -2465,7 +2422,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;
}
}
@@ -2581,9 +2537,6 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0)
goto error;
- VIR_FREE(macTapIfName);
- VIR_FREE(macMapFile);
-
return 0;
error:
@@ -2607,10 +2560,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));
@@ -2635,14 +2586,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);
@@ -2650,11 +2599,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, false));
@@ -2960,7 +2907,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);
@@ -2972,7 +2919,6 @@ networkShutdownNetwork(virNetworkDriverStatePtr driver,
return -1;
unlink(stateFile);
- VIR_FREE(stateFile);
switch ((virNetworkForwardType) def->forward.type) {
@@ -3245,7 +3191,6 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets,
virNetworkDefPtr def)
{
int ret = -1, id = 0;
- char *newname = NULL;
const char *templ = "virbr%d";
const char *p;
@@ -3255,7 +3200,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
@@ -3264,11 +3210,10 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets,
if (!(virNetworkObjBridgeInUse(nets, newname, def->name) ||
virNetDevExists(newname) == 1)) {
VIR_FREE(def->bridge); /*could contain template */
- def->bridge = newname;
+ def->bridge = g_steal_pointer(&newname);
ret = 0;
goto cleanup;
}
- VIR_FREE(newname);
} while (++id <= MAX_BRIDGE_ID);
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -3276,8 +3221,6 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets,
MAX_BRIDGE_ID);
ret = 0;
cleanup:
- if (ret < 0)
- VIR_FREE(newname);
return ret;
}
@@ -3421,7 +3364,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:
@@ -3462,10 +3405,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;
}
@@ -4141,7 +4082,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;
@@ -4198,8 +4140,6 @@ networkSetAutostart(virNetworkPtr net,
ret = 0;
cleanup:
- VIR_FREE(configFile);
- VIR_FREE(autostartLink);
virNetworkObjEndAPI(&obj);
return ret;
}
@@ -4221,14 +4161,13 @@ 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;
virNetworkObjPtr obj;
virNetworkDefPtr def;
@@ -4317,7 +4256,7 @@ networkGetDHCPLeases(virNetworkPtr net,
continue;
if (need_results) {
- lease = g_new0(virNetworkDHCPLease, 1);
+ g_autoptr(virNetworkDHCPLease) lease = g_new0(virNetworkDHCPLease, 1);
lease->expirytime = expirytime_tmp;
@@ -4365,8 +4304,6 @@ networkGetDHCPLeases(virNetworkPtr net,
} else {
nleases++;
}
-
- VIR_FREE(lease);
}
if (leases_ret) {
@@ -4378,13 +4315,7 @@ networkGetDHCPLeases(virNetworkPtr net,
rv = nleases;
cleanup:
- VIR_FREE(lease);
- VIR_FREE(lease_entries);
- VIR_FREE(custom_lease_file);
- virJSONValueFree(leases_array);
-
virNetworkObjEndAPI(&obj);
-
return rv;
error:
@@ -5577,7 +5508,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;
@@ -5654,7 +5585,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 5fc77785dc..58df15b5cf 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -218,7 +218,8 @@ void networkPostReloadFirewallRules(bool startup G_GNUC_UNUSED)
int networkCheckRouteCollision(virNetworkDefPtr def)
{
int ret = 0, len;
- char *cur, *buf = NULL;
+ char *cur;
+ g_autofree char *buf = NULL;
/* allow for up to 100000 routes (each line is 128 bytes) */
enum {MAX_ROUTE_SIZE = 128*100000};
@@ -315,14 +316,13 @@ 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;
}
@@ -330,7 +330,6 @@ int networkCheckRouteCollision(virNetworkDefPtr def)
}
out:
- VIR_FREE(buf);
return ret;
}
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;
}
--
2.25.4
10:10 a.m.
New subject: [libvirt PATCH v2 05/15] network: use g_auto wherever appropriate
On a Tuesday in 2020, Laine Stump wrote:
This includes standard g_autofree() as well as other objects that
have
a cleanup function defined to use via g_autoptr (virCommand,
virJSONValue)
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/network/bridge_driver.c | 206 ++++++++++--------------------
src/network/bridge_driver_linux.c | 7 +-
src/network/leaseshelper.c | 16 +--
3 files changed, 76 insertions(+), 153 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index ab359acdb5..31bd0dd92c 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
[...]
@@ -1095,7 +1081,6 @@ networkDnsmasqConfContents(virNetworkObjPtr
obj,
bool wantDNS = dns->enable != VIR_TRISTATE_BOOL_NO;
virNetworkIPDefPtr tmpipdef, ipdef, ipv4def, ipv6def;
bool ipv6SLAAC;
- char *saddr = NULL, *eaddr = NULL;
*configstr = NULL;
[...]
@@ -1414,6 +1396,8 @@ networkDnsmasqConfContents(virNetworkObjPtr
obj,
int thisRange;
virNetworkDHCPRangeDef range = ipdef->ranges[r];
g_autofree char *leasetime = NULL;
+ g_autofree char *saddr = NULL;
+ g_autofree char *eaddr = NULL;
300 lines below the original location. Long function is long. :)
if (!(saddr = virSocketAddrFormat(&range.addr.start)) ||
!(eaddr = virSocketAddrFormat(&range.addr.end)))
[...]
@@ -2248,7 +2206,7 @@ static int
networkSetIPv6Sysctls(virNetworkObjPtr obj)
{
virNetworkDefPtr def = virNetworkObjGetDef(obj);
- char *field = NULL;
+ g_autofree char *field = NULL;
Last time I tried manually freeing an autofree'd variable, I was told
not to do that O:-)
The clean way here seems to be refactoring the function. I can put that
somewhere into the depths of my TODO list.
int ret = -1;
bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0);
@@ -2273,7 +2231,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
"on bridge %s"), field, def->bridge);
goto cleanup;
}
- 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.
@@ -2282,6 +2239,7 @@ 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);
@@ -2290,11 +2248,11 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
_("cannot disable %s"), field);
goto cleanup;
}
- 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) {
@@ -2305,7 +2263,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
ret = 0;
cleanup:
- VIR_FREE(field);
return ret;
}
[...]
@@ -3276,8 +3221,6 @@ networkFindUnusedBridgeName(virNetworkObjListPtr
nets,
MAX_BRIDGE_ID);
ret = 0;
So this function returned 0 even on failure.
Introduced by a28d3e485f01d16320af15780bc935f54782a45d
cleanup:
- if (ret < 0)
- VIR_FREE(newname);
return ret;
}
Without the networkSetIPv6Sysctls changes:
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
11:18 a.m.
New subject: [libvirt PATCH v2 05/15] network: use g_auto wherever appropriate
On 7/15/20 11:10 AM, Ján Tomko wrote:
On a Tuesday in 2020, Laine Stump wrote:
> This includes standard g_autofree() as well as other objects that have
> a cleanup function defined to use via g_autoptr (virCommand,
> virJSONValue)
>
> Signed-off-by: Laine Stump <laine(a)redhat.com>
> ---
> src/network/bridge_driver.c | 206 ++++++++++--------------------
> src/network/bridge_driver_linux.c | 7 +-
> src/network/leaseshelper.c | 16 +--
> 3 files changed, 76 insertions(+), 153 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index ab359acdb5..31bd0dd92c 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
[...]
> @@ -1095,7 +1081,6 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
> bool wantDNS = dns->enable != VIR_TRISTATE_BOOL_NO;
> virNetworkIPDefPtr tmpipdef, ipdef, ipv4def, ipv6def;
> bool ipv6SLAAC;
> - char *saddr = NULL, *eaddr = NULL;
>
> *configstr = NULL;
>
[...]
> @@ -1414,6 +1396,8 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
> int thisRange;
> virNetworkDHCPRangeDef range = ipdef->ranges[r];
> g_autofree char *leasetime = NULL;
> + g_autofree char *saddr = NULL;
> + g_autofree char *eaddr = NULL;
300 lines below the original location. Long function is long. :)
At least there were no unrelated changes in be... oh, wait. Nevermind.
A long time ago (1988) in a galaxy far far away (Lake City, Minnesota) I
worked with a guy who told me that any function that wouldn't fit on a
single screen was too long and needed to be broken up (this was in the
80x25 ASCII terminal days). He would probably rip out his moustache and
scream if he saw some of the functions in libvirt.
>
> if (!(saddr = virSocketAddrFormat(&range.addr.start)) ||
> !(eaddr = virSocketAddrFormat(&range.addr.end)))
[...]
> @@ -2248,7 +2206,7 @@ static int
> networkSetIPv6Sysctls(virNetworkObjPtr obj)
> {
> virNetworkDefPtr def = virNetworkObjGetDef(obj);
> - char *field = NULL;
> + g_autofree char *field = NULL;
Last time I tried manually freeing an autofree'd variable, I was told
not to do that O:-)
Yeah, there's a few places where we re-use a pointer for "temporary"
strings. In a different patch I sent a few weeks ago, I fixed it by just
declaring multiple separate autofree variables, one for each usage, and
I think that is the least future-bug-prone method of dealing with it.
(I hadn't seen anyone scolding about not manually freeing and autofree'd
variable, but since doing so made me uneasy anyway, I'm happy to jump on
the bandwagon :-)
The clean way here seems to be refactoring the function. I can put that
somewhere into the depths of my TODO list.
If you really want to, you can. Otherwise I can send a patch for that to
be pushed along with this series, just so that I won't have the icky
feeling of leaving a job not quite done.
> int ret = -1;
> bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0);
>
> @@ -2273,7 +2231,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> "on bridge %s"), field, def->bridge);
> goto cleanup;
> }
> - 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.
> @@ -2282,6 +2239,7 @@ 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);
>
> @@ -2290,11 +2248,11 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> _("cannot disable %s"), field);
> goto cleanup;
> }
> - 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) {
> @@ -2305,7 +2263,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
>
> ret = 0;
> cleanup:
> - VIR_FREE(field);
> return ret;
> }
>
[...]
> @@ -3276,8 +3221,6 @@
> networkFindUnusedBridgeName(virNetworkObjListPtr nets,
> MAX_BRIDGE_ID);
> ret = 0;
So this function returned 0 even on failure.
Introduced by a28d3e485f01d16320af15780bc935f54782a45d
> cleanup:
> - if (ret < 0)
> - VIR_FREE(newname);
> return ret;
> }
>
Without the networkSetIPv6Sysctls changes:
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:08 p.m.
New subject: [libvirt PATCH v2 06/15] network: eliminate unnecessary labels
All these cleanup/error labels were reduced to having just "return
ret" by a previous patch, so get rid of them and return directly.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/network/bridge_driver.c | 264 ++++++++++++------------------
src/network/bridge_driver_linux.c | 15 +-
2 files changed, 113 insertions(+), 166 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 31bd0dd92c..79b2ca3330 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -197,18 +197,14 @@ networkDnsmasqDefNamespaceParse(xmlXPathContextPtr ctxt,
void **data)
{
g_autoptr(networkDnsmasqXmlNsDef) nsdata = g_new0(networkDnsmasqXmlNsDef, 1);
- int ret = -1;
if (networkDnsmasqDefNamespaceParseOptions(nsdata, ctxt))
- goto cleanup;
+ return -1;
if (nsdata->noptions > 0)
*data = g_steal_pointer(&nsdata);
- ret = 0;
-
- cleanup:
- return ret;
+ return 0;
}
@@ -331,22 +327,20 @@ networkRunHook(virNetworkObjPtr obj,
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>");
@@ -359,14 +353,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:
- return ret;
+ return 0;
}
@@ -440,34 +432,32 @@ networkRemoveInactive(virNetworkDriverStatePtr driver,
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);
@@ -488,10 +478,7 @@ networkRemoveInactive(virNetworkDriverStatePtr driver,
/* remove the network definition */
virNetworkObjRemoveInactive(driver->networks, obj);
- ret = 0;
-
- cleanup:
- return ret;
+ return 0;
}
@@ -703,7 +690,6 @@ networkStateInitialize(bool privileged,
virStateInhibitCallback callback G_GNUC_UNUSED,
void *opaque G_GNUC_UNUSED)
{
- int ret = VIR_DRV_STATE_INIT_ERROR;
g_autofree char *configdir = NULL;
g_autofree char *rundir = NULL;
bool autostart = true;
@@ -831,13 +817,12 @@ networkStateInitialize(bool privileged,
}
#endif
- ret = VIR_DRV_STATE_INIT_COMPLETE;
- cleanup:
- return ret;
+ return VIR_DRV_STATE_INIT_COMPLETE;
+
error:
networkStateCleanup();
- goto cleanup;
+ return VIR_DRV_STATE_INIT_ERROR;
}
@@ -1074,7 +1059,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
{
virNetworkDefPtr def = virNetworkObjGetDef(obj);
g_auto(virBuffer) configbuf = VIR_BUFFER_INITIALIZER;
- int r, ret = -1;
+ int r;
int nbleases = 0;
size_t i;
virNetworkDNSDefPtr dns = &def->dns;
@@ -1138,7 +1123,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
g_autofree char *addr = virSocketAddrFormat(&fwd->addr);
if (!addr)
- goto cleanup;
+ return -1;
virBufferAsprintf(&configbuf, "%s\n", addr);
if (!fwd->domain)
addNoResolv = true;
@@ -1165,7 +1150,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");
@@ -1215,7 +1200,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
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
@@ -1238,7 +1223,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
"(as described in
RFC1918/RFC3484/RFC4193)."),
ipaddr, (int)version / 1000000,
(int)(version % 1000000) / 1000);
- goto cleanup;
+ return -1;
}
virBufferAsprintf(&configbuf, "listen-address=%s\n", ipaddr);
}
@@ -1279,14 +1264,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.
@@ -1335,7 +1320,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;
}
@@ -1355,13 +1340,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;
}
@@ -1390,7 +1375,7 @@ 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;
@@ -1401,7 +1386,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
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",
@@ -1416,11 +1401,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);
}
@@ -1435,7 +1420,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
&ipdef->address,
virNetworkIPDefPrefix(ipdef));
if (thisRange < 0)
- goto cleanup;
+ return -1;
nbleases += thisRange;
}
@@ -1448,7 +1433,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
if (!ipdef->nranges && ipdef->nhosts) {
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))
@@ -1457,7 +1442,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
}
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)) {
@@ -1476,7 +1461,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
g_autofree char *bootserver =
virSocketAddrFormat(&ipdef->bootserver);
if (!bootserver)
- goto cleanup;
+ return -1;
virBufferAsprintf(&configbuf, "dhcp-boot=%s%s%s\n",
ipdef->bootfile, ",,", bootserver);
} else {
@@ -1492,7 +1477,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
@@ -1525,7 +1510,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
if (!(ipdef->nranges || ipdef->nhosts)) {
g_autofree char *bridgeaddr =
virSocketAddrFormat(&ipdef->address);
if (!bridgeaddr)
- goto cleanup;
+ return -1;
virBufferAsprintf(&configbuf,
"dhcp-range=%s,ra-only\n", bridgeaddr);
}
@@ -1540,15 +1525,12 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
}
if (!(*configstr = virBufferContentAndReset(&configbuf)))
- goto cleanup;
+ return -1;
*hostsfilestr = dnsmasqDhcpHostsToString(dctx->hostsfile->hosts,
dctx->hostsfile->nhosts);
- ret = 0;
-
- cleanup:
- return ret;
+ return 0;
}
@@ -1563,7 +1545,6 @@ networkBuildDhcpDaemonCommandLine(virNetworkDriverStatePtr driver,
virNetworkDefPtr def = virNetworkObjGetDef(obj);
g_autoptr(dnsmasqCaps) dnsmasq_caps = networkGetDnsmasqCaps(driver);
g_autoptr(virCommand) cmd = NULL;
- int ret = -1;
g_autofree char *configfile = NULL;
g_autofree char *configstr = NULL;
g_autofree char *hostsfilestr = NULL;
@@ -1573,27 +1554,27 @@ networkBuildDhcpDaemonCommandLine(virNetworkDriverStatePtr
driver,
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);
@@ -1603,9 +1584,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkDriverStatePtr driver,
virCommandAddEnvPair(cmd, "VIR_BRIDGE_NAME", def->bridge);
*cmdout = g_steal_pointer(&cmd);
- ret = 0;
- cleanup:
- return ret;
+ return 0;
}
@@ -1620,7 +1599,6 @@ networkStartDhcpDaemon(virNetworkDriverStatePtr driver,
g_autoptr(virCommand) cmd = NULL;
g_autofree char *pidfile = NULL;
pid_t dnsmasqPid;
- int ret = -1;
g_autoptr(dnsmasqContext) dctx = NULL;
/* see if there are any IP addresses that need a dhcp server */
@@ -1631,53 +1609,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
@@ -1687,14 +1658,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:
- return ret;
+ return 0;
}
@@ -1710,7 +1679,6 @@ networkRefreshDhcpDaemon(virNetworkDriverStatePtr driver,
virNetworkObjPtr obj)
{
virNetworkDefPtr def = virNetworkObjGetDef(obj);
- int ret = -1;
size_t i;
pid_t dnsmasqPid;
virNetworkIPDefPtr ipdef, ipv4def, ipv6def;
@@ -1726,10 +1694,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
@@ -1752,21 +1718,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:
- return ret;
+ return kill(dnsmasqPid, SIGHUP);
+
}
@@ -1874,7 +1839,6 @@ networkRadvdConfWrite(virNetworkDriverStatePtr driver,
char **configFile)
{
virNetworkDefPtr def = virNetworkObjGetDef(obj);
- int ret = -1;
g_autofree char *configStr = NULL;
g_autofree char *myConfigFile = NULL;
@@ -1884,27 +1848,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:
- return ret;
+ return 0;
}
@@ -1919,20 +1880,16 @@ networkStartRadvd(virNetworkDriverStatePtr driver,
g_autofree char *radvdpidbase = NULL;
g_autofree char *configfile = NULL;
g_autoptr(virCommand) cmd = NULL;
- int ret = -1;
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)) {
@@ -1940,30 +1897,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
@@ -1983,15 +1942,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:
- return ret;
+ virNetworkObjSetRadvdPid(obj, radvdPid);
+ return 0;
}
@@ -2207,7 +2164,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
{
virNetworkDefPtr def = virNetworkObjGetDef(obj);
g_autofree char *field = NULL;
- int ret = -1;
bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0);
/* set disable_ipv6 if there are no ipv6 addresses defined for the
@@ -2221,15 +2177,14 @@ 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;
}
/* The rest of the ipv6 sysctl tunables should always be set the
@@ -2246,7 +2201,7 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
if (virFileWriteStr(field, "0", 0) < 0) {
virReportSystemError(errno,
_("cannot disable %s"), field);
- goto cleanup;
+ return -1;
}
/* All interfaces used as a gateway (which is what this is, by
@@ -2258,12 +2213,10 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
if (virFileWriteStr(field, "0", 0) < 0) {
virReportSystemError(errno,
_("cannot disable %s"), field);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- return ret;
+ return 0;
}
@@ -3190,7 +3143,7 @@ static int
networkFindUnusedBridgeName(virNetworkObjListPtr nets,
virNetworkDefPtr def)
{
- int ret = -1, id = 0;
+ int id = 0;
const char *templ = "virbr%d";
const char *p;
@@ -3211,17 +3164,14 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets,
virNetDevExists(newname) == 1)) {
VIR_FREE(def->bridge); /*could contain template */
def->bridge = g_steal_pointer(&newname);
- ret = 0;
- goto cleanup;
+ return 0;
}
} while (++id <= MAX_BRIDGE_ID);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Bridge generation exceeded max id %d"),
MAX_BRIDGE_ID);
- ret = 0;
- cleanup:
- return ret;
+ return -1;
}
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
index 58df15b5cf..7f765bcf99 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -217,7 +217,7 @@ void networkPostReloadFirewallRules(bool startup G_GNUC_UNUSED)
*/
int networkCheckRouteCollision(virNetworkDefPtr def)
{
- int ret = 0, len;
+ int len;
char *cur;
g_autofree char *buf = NULL;
/* allow for up to 100000 routes (each line is 128 bytes) */
@@ -225,7 +225,7 @@ int networkCheckRouteCollision(virNetworkDefPtr def)
/* 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)
@@ -234,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');
@@ -296,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;
}
}
@@ -323,14 +322,12 @@ int networkCheckRouteCollision(virNetworkDefPtr def)
_("Route address '%s' conflicts "
"with IP address for '%s'"),
NULLSTR(addr_str), iface);
- ret = -1;
- goto out;
+ return -1;
}
}
}
- out:
- return ret;
+ return 0;
}
static const char networkLocalMulticastIPv4[] = "224.0.0.0/24";
--
2.25.4
10:16 a.m.
New subject: [libvirt PATCH v2 06/15] network: eliminate unnecessary labels
On a Tuesday in 2020, Laine Stump wrote:
All these cleanup/error labels were reduced to having just
"return
ret" by a previous patch, so get rid of them and return directly.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/network/bridge_driver.c | 264 ++++++++++++------------------
src/network/bridge_driver_linux.c | 15 +-
2 files changed, 113 insertions(+), 166 deletions(-)
@@ -3190,7 +3143,7 @@ static int
networkFindUnusedBridgeName(virNetworkObjListPtr nets,
virNetworkDefPtr def)
{
- int ret = -1, id = 0;
+ int id = 0;
const char *templ = "virbr%d";
const char *p;
@@ -3211,17 +3164,14 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets,
virNetDevExists(newname) == 1)) {
VIR_FREE(def->bridge); /*could contain template */
def->bridge = g_steal_pointer(&newname);
- ret = 0;
- goto cleanup;
+ return 0;
}
} while (++id <= MAX_BRIDGE_ID);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Bridge generation exceeded max id %d"),
MAX_BRIDGE_ID);
- ret = 0;
- cleanup:
- return ret;
+ return -1;
This fix deserves a separate commit. Or at least a mention in the commit
message.
}
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
7:04 a.m.
New subject: [libvirt PATCH v2 06/15] network: eliminate unnecessary labels
On 7/7/20 5:08 PM, Laine Stump wrote:
All these cleanup/error labels were reduced to having just
"return
ret" by a previous patch, so get rid of them and return directly.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/network/bridge_driver.c | 264 ++++++++++++------------------
src/network/bridge_driver_linux.c | 15 +-
2 files changed, 113 insertions(+), 166 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 31bd0dd92c..79b2ca3330 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
[...]
Coverity noted there's a leak with this part of the change for @field...
@@ -2207,7 +2164,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
{
virNetworkDefPtr def = virNetworkObjGetDef(obj);
g_autofree char *field = NULL;
- int ret = -1;
bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0);
/* set disable_ipv6 if there are no ipv6 addresses defined for the
@@ -2221,15 +2177,14 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
if (!enableIPv6)
VIR_DEBUG("ipv6 appears to already be disabled on %s",
def->bridge);
- ret = 0;
- goto cleanup;
+ return 0;
}
Below here doesn't match w/ current source, but I assume you know that.
Looks like a mis-merge with the review from the previous patch.
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;
}
/* The rest of the ipv6 sysctl tunables should always be set the
@@ -2246,7 +2201,7 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
if (virFileWriteStr(field, "0", 0) < 0) {
virReportSystemError(errno,
_("cannot disable %s"), field);
- goto cleanup;
+ return -1;
}
/* All interfaces used as a gateway (which is what this is, by
@@ -2258,12 +2213,10 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
if (virFileWriteStr(field, "0", 0) < 0) {
virReportSystemError(errno,
_("cannot disable %s"), field);
- goto cleanup;
+ return -1;
}
- ret = 0;
- cleanup:
- return ret;
+ return 0;
}
[...]
8:07 a.m.
New subject: [libvirt PATCH v2 06/15] network: eliminate unnecessary labels
On a Tuesday in 2020, John Ferlan wrote:
On 7/7/20 5:08 PM, Laine Stump wrote:
> All these cleanup/error labels were reduced to having just "return
> ret" by a previous patch, so get rid of them and return directly.
>
> Signed-off-by: Laine Stump <laine(a)redhat.com>
> ---
> src/network/bridge_driver.c | 264 ++++++++++++------------------
> src/network/bridge_driver_linux.c | 15 +-
> 2 files changed, 113 insertions(+), 166 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 31bd0dd92c..79b2ca3330 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
[...]
Coverity noted there's a leak with this part of the change for @field...
>
> @@ -2207,7 +2164,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> {
> virNetworkDefPtr def = virNetworkObjGetDef(obj);
> g_autofree char *field = NULL;
> - int ret = -1;
> bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0);
>
> /* set disable_ipv6 if there are no ipv6 addresses defined for the
> @@ -2221,15 +2177,14 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> if (!enableIPv6)
> VIR_DEBUG("ipv6 appears to already be disabled on %s",
> def->bridge);
> - ret = 0;
> - goto cleanup;
> + return 0;
> }
Below here doesn't match w/ current source, but I assume you know that.
Looks like a mis-merge with the review from the previous patch.
Should be fixed in git master by:
commit 5c50d1dda5664d480e6370111c0218947706bd31
Author: Ján Tomko <jtomko(a)redhat.com>
CommitDate: 2020-07-21 14:55:00 +0200
network: split out networkSetIPv6Sysctl
https://gitlab.com/libvirt/libvirt/-/commit/5c50d1dda5664d480e6370111c021...
Jano
9:13 a.m.
New subject: [libvirt PATCH v2 06/15] network: eliminate unnecessary labels
On 7/21/20 8:04 AM, John Ferlan wrote:
On 7/7/20 5:08 PM, Laine Stump wrote:
> All these cleanup/error labels were reduced to having just "return
> ret" by a previous patch, so get rid of them and return directly.
>
> Signed-off-by: Laine Stump <laine(a)redhat.com>
> ---
> src/network/bridge_driver.c | 264 ++++++++++++------------------
> src/network/bridge_driver_linux.c | 15 +-
> 2 files changed, 113 insertions(+), 166 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 31bd0dd92c..79b2ca3330 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
[...]
Coverity noted there's a leak with this part of the change for @field...
>
> @@ -2207,7 +2164,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> {
> virNetworkDefPtr def = virNetworkObjGetDef(obj);
> g_autofree char *field = NULL;
> - int ret = -1;
> bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0);
>
> /* set disable_ipv6 if there are no ipv6 addresses defined for the
> @@ -2221,15 +2177,14 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> if (!enableIPv6)
> VIR_DEBUG("ipv6 appears to already be disabled on %s",
> def->bridge);
> - ret = 0;
> - goto cleanup;
> + return 0;
> }
Below here doesn't match w/ current source, but I assume you know that.
Looks like a mis-merge with the review from the previous patch.
Sigh.
I *thought* I had removed all the changes to this function when I
rebased the series the last time (since Jan already had a better patch
for it), but I guess I didn't look carefully enough at the diffs before
I pushed :-(
Fortunately, Jan has pushed his patch, which completely replaces the
function.
>
> 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;
> }
>
> /* The rest of the ipv6 sysctl tunables should always be set the
> @@ -2246,7 +2201,7 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> if (virFileWriteStr(field, "0", 0) < 0) {
> virReportSystemError(errno,
> _("cannot disable %s"), field);
> - goto cleanup;
> + return -1;
> }
>
> /* All interfaces used as a gateway (which is what this is, by
> @@ -2258,12 +2213,10 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> if (virFileWriteStr(field, "0", 0) < 0) {
> virReportSystemError(errno,
> _("cannot disable %s"), field);
> - goto cleanup;
> + return -1;
> }
>
> - ret = 0;
> - cleanup:
> - return ret;
> + return 0;
> }
[...]
4:08 p.m.
New subject: [libvirt PATCH v2 07/15] network: use g_free() in place of remaining VIR_FREE()
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/network/bridge_driver.c | 45 +++++++++++++++++++------------------
1 file changed, 23 insertions(+), 22 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 79b2ca3330..7d81d4dd78 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -158,7 +158,7 @@ networkDnsmasqDefNamespaceFree(void *nsdata)
virStringListFreeCount(def->options, def->noptions);
- VIR_FREE(def);
+ g_free(def);
}
G_DEFINE_AUTOPTR_CLEANUP_FUNC(networkDnsmasqXmlNsDef, networkDnsmasqDefNamespaceFree);
@@ -707,7 +707,7 @@ networkStateInitialize(bool privileged,
network_driver->lockFD = -1;
if (virMutexInit(&network_driver->lock) < 0) {
- VIR_FREE(network_driver);
+ g_clear_pointer(&network_driver, g_free);
goto error;
}
@@ -875,18 +875,18 @@ 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_clear_pointer(&network_driver, g_free);
return 0;
}
@@ -2194,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);
@@ -2207,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) {
@@ -2714,19 +2714,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_clear_pointer(&netdef->forward.ifs, g_free);
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;
}
@@ -3162,7 +3162,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;
}
@@ -4272,7 +4272,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;
}
@@ -4396,7 +4396,7 @@ networkAllocatePort(virNetworkObjPtr obj,
return -1;
}
if (portprofile) {
- VIR_FREE(port->virtPortProfile);
+ g_free(port->virtPortProfile);
port->virtPortProfile = portprofile;
}
@@ -5513,9 +5513,10 @@ networkPortSetParameters(virNetworkPortPtr port,
* 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);
+ g_clear_pointer(&bandwidth->in, g_free);
+
if (!bandwidth->out->average)
- VIR_FREE(bandwidth->out);
+ g_clear_pointer(&bandwidth->out, g_free);
if (networkUpdatePortBandwidth(obj,
&portdef->mac,
--
2.25.4
10:19 a.m.
New subject: [libvirt PATCH v2 07/15] network: use g_free() in place of remaining VIR_FREE()
On a Tuesday in 2020, Laine Stump wrote:
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/network/bridge_driver.c | 45 +++++++++++++++++++------------------
1 file changed, 23 insertions(+), 22 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:08 p.m.
New subject: [libvirt PATCH v2 08/15] nwfilter: remove unnecessary code from ebtablesGetSubChainInsts()
On failure, this function would clear out and free the list of
subchains it had been called with. This is unnecessary, because the
*only* caller of this function will also clear out and free the list
of subchains if it gets a failure from ebtablesGetSubChainInsts().
(It also makes more logical sense for the function that is creating
the entire list to be the one freeing the entire list, rather than
having a function whose purpose is only to create *one item* on the
list freeing the entire list).
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
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 78a52408b2..426212e0dc 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3328,12 +3328,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
10:30 a.m.
New subject: [libvirt PATCH v2 08/15] nwfilter: remove unnecessary code from ebtablesGetSubChainInsts()
On a Tuesday in 2020, Laine Stump wrote:
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).
This is the function creating the list, I think it makes sense
to not leave anything allocated in case of failure.
Jano
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/nwfilter/nwfilter_ebiptables_driver.c | 6 ------
1 file changed, 6 deletions(-)
11:25 p.m.
New subject: [libvirt PATCH v2 08/15] nwfilter: remove unnecessary code from ebtablesGetSubChainInsts()
On 7/15/20 11:30 AM, Ján Tomko wrote:
On a Tuesday in 2020, Laine Stump wrote:
> 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).
This is the function creating the list,
I disagree with that characterization. The list is created, with 0
elements, when the caller (ebiptablesApplyNewRules()) defines it. Then
each time ebtablesGetSubChainInsts() is called, it doesn't create the
list anew, it just adds to whatever is already on the existing list - as
a matter of fact it is called multiple times and each time it adds more
items to the list without re=initializing it.
This is very much like what happens with a virBuffer - some function
creates a virBuffer by defining it and initializing it to empty, then
each time a virBuffer function is called, it adds more text to the
buffer. But if there is an error in a virBuffer function, it doesn't
clear out the buffer before returning, it just returns an error leaving
the buffer in whatever state it was in when the error occurred; it is
then up to the caller, who is the owner of the virBuffer, to clear it out.
I think it makes sense
to not leave anything allocated in case of failure.
Aside from making the code simpler and cleaner, I think it doesn't make
sense for one invocation of the function to clear out anything that was
put into the list by *a different* invocation of the function. If you're
going to be a purist about it, then a failed ebtablesGetSubChainInsts()
should remove from the list *only those items that were added during the
current call* and nothing else.
But that's just pedantic nitpicking (Hey, *you* started the nitpicking
though :-P)
(Also, there is only one caller of ebtablesGetSubChainInsts(), and
whenever ebtablesGetSubChainInsts() fails, the *very next thing* that
caller does is to clear out the entire list. So in fact, "nothing is
left allocated in case of failure".)
Jano
>
> Signed-off-by: Laine Stump <laine(a)redhat.com>
My S-o-b stands. I still think this is the right thing to do.
>
> ---
> src/nwfilter/nwfilter_ebiptables_driver.c | 6 ------
> 1 file changed, 6 deletions(-)
>
4:04 p.m.
New subject: [libvirt PATCH v2 08/15] nwfilter: remove unnecessary code from ebtablesGetSubChainInsts()
On a Saturday in 2020, Laine Stump wrote:
On 7/15/20 11:30 AM, Ján Tomko wrote:
>On a Tuesday in 2020, Laine Stump wrote:
>>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).
>
>This is the function creating the list,
I disagree with that characterization. The list is created, with 0
elements, when the caller (ebiptablesApplyNewRules()) defines it. Then
each time ebtablesGetSubChainInsts() is called, it doesn't create the
list anew, it just adds to whatever is already on the existing list -
as a matter of fact it is called multiple times and each time it adds
more items to the list without re=initializing it.
Oh, I missed that it's called twice - I thought it was either one or the
other call.
This is very much like what happens with a virBuffer - some function
creates a virBuffer by defining it and initializing it to empty, then
each time a virBuffer function is called, it adds more text to the
buffer. But if there is an error in a virBuffer function, it doesn't
clear out the buffer before returning, it just returns an error
leaving the buffer in whatever state it was in when the error
occurred; it is then up to the caller, who is the owner of the
virBuffer, to clear it out.
>I think it makes sense
>to not leave anything allocated in case of failure.
Aside from making the code simpler and cleaner, I think it doesn't
make sense for one invocation of the function to clear out anything
that was put into the list by *a different* invocation of the
function. If you're going to be a purist about it, then a failed
ebtablesGetSubChainInsts() should remove from the list *only those
items that were added during the current call* and nothing else.
Yeah, that would be wrong.
But that's just pedantic nitpicking (Hey, *you* started the nitpicking
though :-P)
(Also, there is only one caller of ebtablesGetSubChainInsts(), and
whenever ebtablesGetSubChainInsts() fails, the *very next thing* that
caller does is to clear out the entire list. So in fact, "nothing is
left allocated in case of failure".)
>
>Jano
>
>>
>>Signed-off-by: Laine Stump <laine(a)redhat.com>
My S-o-b stands. I still think this is the right thing to do.
S-o-b merely means that you are the author and/or have the author's
permission to use the code. I don't think you can revoke a S-o-b,
even if you don't think the code is right.
>>
>>---
>>src/nwfilter/nwfilter_ebiptables_driver.c | 6 ------
>>1 file changed, 6 deletions(-)
>>
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:40 p.m.
New subject: [libvirt PATCH v2 08/15] nwfilter: remove unnecessary code from ebtablesGetSubChainInsts()
On 7/20/20 5:04 PM, Ján Tomko wrote:
On a Saturday in 2020, Laine Stump wrote:
> On 7/15/20 11:30 AM, Ján Tomko wrote:
>> On a Tuesday in 2020, Laine Stump wrote:
>>>
>>> Signed-off-by: Laine Stump <laine(a)redhat.com>
>
>
> My S-o-b stands. I still think this is the right thing to do.
>
S-o-b merely means that you are the author and/or have the author's
permission to use the code. I don't think you can revoke a S-o-b,
even if you don't think the code is right.
Yeah, I know that a misuse of S-o-b, I just like the idea of putting
something at the end of the commit message that's equivalent to a
politician at the end of their election ad: "I'm Zaphod Beeblebrox, and
I approve this message". :-)
4:08 p.m.
New subject: [libvirt PATCH v2 09/15] nwfilter: clear nrules when resetting virNWFilterInst
It's possible/probable the callers to virNWFilterInstReset() make it
unnecessary to set the object's nrules to 0 after freeing all its
rules, but that same function is setting nfilters to 0, so let's do
the same for the sake of consistency.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/nwfilter/nwfilter_gentech_driver.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/nwfilter/nwfilter_gentech_driver.c
b/src/nwfilter/nwfilter_gentech_driver.c
index b7633eb10a..aff42cbfb0 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -240,6 +240,7 @@ virNWFilterInstReset(virNWFilterInstPtr inst)
for (i = 0; i < inst->nrules; i++)
virNWFilterRuleInstFree(inst->rules[i]);
VIR_FREE(inst->rules);
+ inst->nrules = 0;
}
--
2.25.4
10:30 a.m.
New subject: [libvirt PATCH v2 09/15] nwfilter: clear nrules when resetting virNWFilterInst
On a Tuesday in 2020, Laine Stump wrote:
It's possible/probable the callers to virNWFilterInstReset() make
it
unnecessary to set the object's nrules to 0 after freeing all its
rules, but that same function is setting nfilters to 0, so let's do
the same for the sake of consistency.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/nwfilter/nwfilter_gentech_driver.c | 1 +
1 file changed, 1 insertion(+)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:08 p.m.
New subject: [libvirt PATCH v2 10/15] nwfilter: define a typedef for struct ebtablesSubChainInst
Signed-off-by: Laine Stump <laine(a)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 426212e0dc..cc0f3f93d9 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3269,7 +3269,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;
@@ -3280,8 +3282,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;
@@ -3291,7 +3293,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;
@@ -3304,7 +3306,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);
@@ -3344,7 +3346,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
10:31 a.m.
New subject: [libvirt PATCH v2 10/15] nwfilter: define a typedef for struct ebtablesSubChainInst
On a Tuesday in 2020, Laine Stump wrote:
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/nwfilter/nwfilter_ebiptables_driver.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:08 p.m.
New subject: [libvirt PATCH v2 11/15] nwfilter: transform logic in virNWFilterRuleInstSort to eliminate label
This rewrite of a nested conditional produces the same results, but
eliminate a goto and corresponding label.
Signed-off-by: Laine Stump <laine(a)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 cc0f3f93d9..94eaac927a 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3113,13 +3113,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
10:33 a.m.
New subject: [libvirt PATCH v2 11/15] nwfilter: transform logic in virNWFilterRuleInstSort to eliminate label
On a Tuesday in 2020, Laine Stump wrote:
This rewrite of a nested conditional produces the same results, but
eliminate a goto and corresponding label.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/nwfilter/nwfilter_ebiptables_driver.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:08 p.m.
New subject: [libvirt PATCH v2 12/15] nwfilter: use standard label names when reasonable
Rather than having labels named exit, done, exit_snooprequnlock,
skip_rename, etc, use the standard "cleanup" label. And instead of
err_exit, malformed, tear_down_tmpebchains, use "error".
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
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 | 24 +++++++--------
4 files changed, 52 insertions(+), 52 deletions(-)
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
index f530341872..6de41ff209 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -456,11 +456,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 */
@@ -471,7 +471,7 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl,
req->ifindex);
}
- exit_snooprequnlock:
+ cleanup:
virNWFilterSnoopReqUnlock(req);
VIR_FREE(ipaddr);
@@ -732,7 +732,7 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReqPtr req,
virNWFilterSnoopReqUnlock(req);
- goto exit;
+ goto cleanup;
}
virNWFilterSnoopReqUnlock(req);
@@ -757,7 +757,7 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReqPtr req,
g_atomic_int_add(&virNWFilterSnoopState.nLeases, 1);
- exit:
+ cleanup:
if (update_leasefile)
virNWFilterSnoopLeaseFileSave(pl);
@@ -902,7 +902,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));
@@ -910,7 +910,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];
@@ -922,12 +922,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;
}
@@ -1386,7 +1386,7 @@ virNWFilterDHCPSnoopThread(void *req0)
virNWFilterSnoopReqUnlock(req);
if (req->threadStatus != THREAD_STATUS_OK)
- goto exit;
+ goto cleanup;
while (!error) {
if (virNWFilterSnoopAdjustPoll(pcapConf,
@@ -1414,7 +1414,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)
@@ -1531,7 +1531,7 @@ virNWFilterDHCPSnoopThread(void *req0)
virNWFilterSnoopReqUnlock(req);
virNWFilterSnoopUnlock();
- exit:
+ cleanup:
virThreadPoolFree(worker);
virNWFilterSnoopReqPut(req);
@@ -1774,14 +1774,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();
}
@@ -1876,7 +1876,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) {
@@ -1886,7 +1886,7 @@ virNWFilterSnoopLeaseFileRefresh(void)
}
g_atomic_int_set(&virNWFilterSnoopState.wLeases, 0);
- skip_rename:
+ cleanup:
virNWFilterSnoopLeaseFileOpen();
}
@@ -2051,14 +2051,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 94eaac927a..8ac3a7271e 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -2893,11 +2893,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;
}
@@ -3009,11 +3009,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;
}
@@ -3060,11 +3060,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 aff42cbfb0..400d064724 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;
}
@@ -527,7 +527,7 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver,
if (!missing_vars) {
rc = -1;
- goto err_exit;
+ goto error;
}
rc = virNWFilterDetermineMissingVarsRec(filter,
@@ -536,7 +536,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)
@@ -558,7 +558,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,
@@ -567,14 +567,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;
@@ -583,7 +583,7 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver,
goto err_unresolvable_vars;
} else if (!forceWithPendingReq &&
virNWFilterHasLearnReq(ifindex)) {
- goto err_exit;
+ goto error;
}
rc = virNWFilterDefToInst(driver,
@@ -593,7 +593,7 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver,
&inst);
if (rc < 0)
- goto err_exit;
+ goto error;
switch (useNewFilter) {
case INSTANTIATE_FOLLOW_NEWFILTER:
@@ -606,7 +606,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);
@@ -623,7 +623,7 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver,
virNWFilterUnlockIface(binding->portdevname);
}
- err_exit:
+ error:
virNWFilterInstReset(&inst);
virHashFree(missing_vars);
@@ -640,7 +640,7 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver,
}
rc = -1;
- goto err_exit;
+ goto error;
}
@@ -707,14 +707,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);
@@ -737,7 +737,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 14c66cff35..95e21050b4 100644
--- a/src/nwfilter/nwfilter_learnipaddr.c
+++ b/src/nwfilter/nwfilter_learnipaddr.c
@@ -152,13 +152,13 @@ virNWFilterLockIface(const char *ifname)
ifaceLock = virHashLookup(ifaceLockMap, ifname);
if (!ifaceLock) {
if (VIR_ALLOC(ifaceLock) < 0)
- goto err_exit;
+ goto error;
if (virMutexInitRecursive(&ifaceLock->lock) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("mutex initialization failed"));
VIR_FREE(ifaceLock);
- goto err_exit;
+ goto error;
}
if (virStrcpyStatic(ifaceLock->ifname, ifname) < 0) {
@@ -167,12 +167,12 @@ virNWFilterLockIface(const char *ifname)
"buffer "),
ifaceLock->ifname);
VIR_FREE(ifaceLock);
- goto err_exit;
+ goto error;
}
while (virHashAddEntry(ifaceLockMap, ifname, ifaceLock)) {
VIR_FREE(ifaceLock);
- goto err_exit;
+ goto error;
}
ifaceLock->refctr = 0;
@@ -186,7 +186,7 @@ virNWFilterLockIface(const char *ifname)
return 0;
- err_exit:
+ error:
virMutexUnlock(&ifaceMapLock);
return -1;
@@ -414,7 +414,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);
@@ -422,7 +422,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);
@@ -436,7 +436,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 {
@@ -444,7 +444,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);
@@ -455,14 +455,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);
@@ -622,7 +622,7 @@ learnIPAddressThread(void *arg)
}
} /* while */
- done:
+ cleanup:
VIR_FREE(filter);
if (handle)
--
2.25.4
10:34 a.m.
New subject: [libvirt PATCH v2 12/15] nwfilter: use standard label names when reasonable
On a Tuesday in 2020, Laine Stump wrote:
Rather than having labels named exit, done, exit_snooprequnlock,
skip_rename, etc, use the standard "cleanup" label. And instead of
err_exit, malformed, tear_down_tmpebchains, use "error".
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
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 | 24 +++++++--------
4 files changed, 52 insertions(+), 52 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:08 p.m.
New subject: [libvirt PATCH v2 13/15] nwfilter: replace VIR_ALLOC with g_new0
Signed-off-by: Laine Stump <laine(a)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 6de41ff209..4bc1607694 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 8ac3a7271e..177e7e62b9 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3312,8 +3312,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 400d064724..acd5614987 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -262,8 +262,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 95e21050b4..63fac37132 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 error;
+ 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
10:35 a.m.
New subject: [libvirt PATCH v2 13/15] nwfilter: replace VIR_ALLOC with g_new0
On a Tuesday in 2020, Laine Stump wrote:
Signed-off-by: Laine Stump <laine(a)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(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:08 p.m.
New subject: [libvirt PATCH v2 14/15] nwfilter: convert local pointers to use g_auto*
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/nwfilter/nwfilter_dhcpsnoop.c | 91 +++++++----------------
src/nwfilter/nwfilter_ebiptables_driver.c | 75 +++++++------------
src/nwfilter/nwfilter_gentech_driver.c | 15 ++--
src/nwfilter/nwfilter_learnipaddr.c | 7 +-
4 files changed, 61 insertions(+), 127 deletions(-)
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
index 4bc1607694..64671af135 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,
cleanup:
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 177e7e62b9..9c9d63f14b 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;
g_auto(virBuffer) vb = VIR_BUFFER_INITIALIZER;
- char *flags;
+ 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,
@@ -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,7 +1787,6 @@ ebtablesCreateRuleInstance(virFirewallPtr fw,
const char *target;
bool hasMask = false;
virFirewallRulePtr fwrule;
- g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
if (STREQ(chainSuffix,
virNWFilterChainSuffixTypeToString(
@@ -2320,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");
@@ -2385,8 +2375,6 @@ ebtablesCreateRuleInstance(virFirewallPtr fw,
r = virBufferContentAndReset(&buf);
virFirewallRuleAddArg(fw, fwrule, r);
-
- VIR_FREE(r);
}
break;
@@ -3295,9 +3283,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);
@@ -3305,7 +3292,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);
@@ -3318,18 +3305,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
@@ -3339,12 +3319,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;
@@ -3538,9 +3518,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 acd5614987..071f15caea 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -415,7 +415,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;
@@ -425,18 +424,16 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter,
for (j = 0; j < rule->nVarAccess; j++) {
if (!virNWFilterVarAccessIsAvailable(rule->varAccess[j],
vars)) {
- char *varAccess;
+ g_autofree char *varAccess = NULL;
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
virNWFilterVarAccessPrint(rule->varAccess[j], &buf);
- val = virNWFilterVarValueCreateSimpleCopyValue("1");
- if (!val)
+ 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;
@@ -444,6 +441,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)))
@@ -472,9 +471,6 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter,
missing_vars,
useNewFilter,
driver);
-
- virHashFree(tmpvars);
-
virNWFilterObjUnlock(obj);
if (rc < 0)
return -1;
@@ -515,7 +511,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;
@@ -635,7 +631,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 63fac37132..abef0b6457 100644
--- a/src/nwfilter/nwfilter_learnipaddr.c
+++ b/src/nwfilter/nwfilter_learnipaddr.c
@@ -397,7 +397,7 @@ learnIPAddressThread(void *arg)
int dhcp_opts_len;
char macaddr[VIR_MAC_STRING_BUFLEN];
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
- char *filter = NULL;
+ g_autofree char *filter = NULL;
uint16_t etherType;
bool showError = true;
enum howDetect howDetected = 0;
@@ -622,8 +622,6 @@ learnIPAddressThread(void *arg)
} /* while */
cleanup:
- 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
10:43 a.m.
New subject: [libvirt PATCH v2 14/15] nwfilter: convert local pointers to use g_auto*
On a Tuesday in 2020, Laine Stump wrote:
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/nwfilter/nwfilter_dhcpsnoop.c | 91 +++++++----------------
src/nwfilter/nwfilter_ebiptables_driver.c | 75 +++++++------------
src/nwfilter/nwfilter_gentech_driver.c | 15 ++--
src/nwfilter/nwfilter_learnipaddr.c | 7 +-
4 files changed, 61 insertions(+), 127 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:08 p.m.
New subject: [libvirt PATCH v2 15/15] nwfilter: convert remaining VIR_FREE() to g_free()
Signed-off-by: Laine Stump <laine(a)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 64671af135..aafa6de322 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_clear_pointer(&req->binding->portdevname, g_free);
virNWFilterSnoopReqUnlock(req);
virNWFilterSnoopUnlock();
@@ -1970,7 +1970,7 @@ virNWFilterSnoopRemAllReqIter(const void *payload,
*/
virNWFilterIPAddrMapDelIPAddr(req->binding->portdevname, NULL);
- VIR_FREE(req->binding->portdevname);
+ g_clear_pointer(&req->binding->portdevname, g_free);
}
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_clear_pointer(&req->binding->portdevname, g_free);
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 9c9d63f14b..6aefbe226b 100644
--- a/src/nwfilter/nwfilter_ebiptables_driver.c
+++ b/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -3517,7 +3517,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 071f15caea..c93f2ed18f 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);
inst->nrules = 0;
}
diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c
index abef0b6457..c6497450eb 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 error;
}
@@ -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 error;
}
while (virHashAddEntry(ifaceLockMap, ifname, ifaceLock)) {
- VIR_FREE(ifaceLock);
+ g_free(ifaceLock);
goto error;
}
@@ -221,7 +221,7 @@ virNWFilterIPAddrLearnReqFree(virNWFilterIPAddrLearnReqPtr req)
virNWFilterBindingDefFree(req->binding);
- VIR_FREE(req);
+ g_free(req);
}
--
2.25.4
10:49 a.m.
New subject: [libvirt PATCH v2 15/15] nwfilter: convert remaining VIR_FREE() to g_free()
On a Tuesday in 2020, Laine Stump wrote:
Signed-off-by: Laine Stump <laine(a)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 64671af135..aafa6de322 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);
This one should use g_clear_pointer.
virNWFilterSnoopActiveUnlock();
}
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);
Same here.
return VIR_DRV_STATE_INIT_ERROR;
}
@@ -379,7 +379,7 @@ nwfilterStateCleanup(void)
virNWFilterObjListFree(driver->nwfilters);
virMutexDestroy(&driver->lock);
- VIR_FREE(driver);
+ g_free(driver);
Possibly here.
return 0;
}
(I have not verified whether the double use of the pointer may
practically happen, but we do a non-NULL check of these in a few cases)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
12:29 p.m.
ping
On 7/7/20 5:08 PM, Laine Stump wrote:
V1 was here:
https://www.redhat.com/archives/libvir-list/2020-June/msg01156.html
Some patches were ACKed and pushed. I re-ordered/re-organized most of
the rest, and removed some others to deal with separately (the
xmlNodeContent stuff)
What's left here is a few preliminary patches, then the standard set,
once for network and again for nwfilter:
1) convert from VIR_(RE)ALLOC(_N) to g_new0()/g_renew()
2) use g_auto*() where appropriate, removing unneeded free's
3) get rid of now-extraneous labels
4) (controversial) replace any remaining VIR_FREE() with g_free() (and
possibly g_clear_pointer() when needed
NB: these patches require my virBuffer "convert to g_auto" series
as a prerequisite:
https://www.redhat.com/archives/libvir-list/2020-July/msg00185.html
^^ This has been pushed, so there are no longer any extra prerequisites.
Changes from V1:
* move conversion of virFirewall and virBuffer automatics to another
series (see above)
* re-order to replace VIR_ALLOC first (without adding any g_auto*)
instead of doing it after g_auto conversion of automatics, then do
all g_auto additions at o
* separate label elimination into separate patches per jtomko's
suggestion.
Laine Stump (15):
replace g_new() with g_new0() for consistency
util: define g_autoptr cleanups for a couple dnsmasq objects
define g_autoptr cleanup function for virNetworkDHCPLease
network: replace VIR_ALLOC/REALLOC with g_new0/g_renew
network: use g_auto wherever appropriate
network: eliminate unnecessary labels
network: use g_free() in place of remaining VIR_FREE()
nwfilter: remove unnecessary code from ebtablesGetSubChainInsts()
nwfilter: clear nrules when resetting virNWFilterInst
nwfilter: define a typedef for struct ebtablesSubChainInst
nwfilter: transform logic in virNWFilterRuleInstSort to eliminate
label
nwfilter: use standard label names when reasonable
nwfilter: replace VIR_ALLOC with g_new0
nwfilter: convert local pointers to use g_auto*
nwfilter: convert remaining VIR_FREE() to g_free()
src/datatypes.h | 2 +
src/network/bridge_driver.c | 536 ++++++++--------------
src/network/bridge_driver_linux.c | 22 +-
src/network/leaseshelper.c | 16 +-
src/nwfilter/nwfilter_dhcpsnoop.c | 150 +++---
src/nwfilter/nwfilter_driver.c | 13 +-
src/nwfilter/nwfilter_ebiptables_driver.c | 119 ++---
src/nwfilter/nwfilter_gentech_driver.c | 57 ++-
src/nwfilter/nwfilter_learnipaddr.c | 43 +-
src/qemu/qemu_backup.c | 2 +-
src/util/virdnsmasq.h | 4 +
src/util/virutil.c | 2 +-
tests/qemuhotplugmock.c | 2 +-
13 files changed, 379 insertions(+), 589 deletions(-)
1585
days inactive
1599
days old
38 comments
3 participants
participants (3)
-
John Ferlan -
Ján Tomko -
Laine Stump