Re: [PATCH 11/25] network: use g_free() in place of remaining VIR_FREE()
On 6/25/20 11:12 AM, Daniel P. Berrangé wrote:
On Thu, Jun 25, 2020 at 11:01:48AM -0400, Laine Stump wrote:
On 6/25/20 3:55 AM, Peter Krempa wrote:
On Wed, Jun 24, 2020 at 23:34:00 -0400, Laine Stump wrote:
Signed-off-by: Laine Stump
---
src/network/bridge_driver.c | 59 +
1 file changed, 33 insertions(+), 26 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 668aa9ca88..a1b2f5b6c7 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
[...]
@@ -706,7 +706,8 @@ networkStateInitialize(bool privileged,
network_driver->lockFD = -1;
if (virMutexInit(_driver->lock) < 0) {
-VIR_FREE(network_driver);
+g_free(network_driver);
+network_driver = NULL;
goto error;
In general I'm agains senseless replacement of VIR_FREE for g_free.
There is IMO no value to do so. VIR_FREE is now implemented via
g_clear_pointer(, g_free) so g_free is actually used.
Mass replacements are also substrate for adding bugs and need to be
approached carefully, so doing this en-mass might lead to others
attempting the same with possibly less care.
In general, mass replacements should be done only to
g_clear_pointer(, g_free)
and I'm not sure it's worth it.
There's no getting around it - that looks ugly. And who wants to replace
5506 occurences of one simple-looking thing with something else that's
functionally equivalent but more painful to look at?
I would vote for just documenting that, for safety and consistency reasons,
VIR_FREE() should always be used instead of g_free(), and eliminating all
direct use of g_free() (along with the aforementioned syntax check). (BTW, I
had assumed there had been more changes to g_free(), but when I looked at my
current tree just now, there were only 228 occurences, including the changes
in this patch)
The point in getting rid of VIR_FREE is so that we reduce the libvirt
specific wrappers in favour of standard APIs.
Is this just to make the code more accessible/understandable to new
contributors? Or is there some other reason that I missed due to being
incapable of reading all the messages on all the lists? (I guess there's
also the issue of reducing support burden by reproducing identical code
to something that someone else is already maintaining in a different
library. But in this case we're just talking about a few lines that
enforces good behavior.)
A large portion of the VIR_FREE's will be eliminated by g_autoptr.
Another large set of them are used in the virFooStructFree() methods.
Those can all be converted to g_free safely, as all the methods do
is free stuff.
Most VIR_FREEs that occur at the exit of functions can also be
safely converted to g_free, if g_autoptr isnt applicable. Sometimes
needs a little care if you have multiple goto jumps between labels.
It still requires thought + diligence = time. And what if new code is
added to the end of a function, thus making those things that had been
"at the end" now in the middle. The more thought and decision making is
needed to get something right, the more likely it is that someone will
get it wrong.
The big danger cases are the VIR_FREE()s that occur in the middle
of methods, especially in loop bodies. Those the ones that must
use the g_clear_pointer, and that's not very many of those, so the
ugly syntax isn't an issue.
1) Maybe I'll feel differently after more of the code has been converted
to use g_auto* and eliminated more of the existing explicit frees, but
with currently > 5000 uses of VIR_FREE still in the code, I fear that
"not many of those" may be more than we're expecting, and especially
with many of them left, it would give me more warm fuzzies to be able to
say
"We can verifiably state that no pointers will be used
after free , because their values have been NULLed,
and any access will either be a NOP, or cause an
immediate segfault"
rather than
"We believe that the contributors to libvirt have been
diligent in their manual auditing of all cases of
free'ing memory to assure that none of the freed
pointers are ever used at any later point,
because well, just *because*".
(on the other hand, admittedly any pointer to something with its own
vir*Free() function already requires diligence on the part of the
programmer, since vir*Free() doesn't NULL the pointer. In that case,
what's a little extra burden?)
2) Speaking from my experience with the occurrences I converted here,
the worst offenders were the places where someone re-used a local
pointer multiple times in a function (sometimes naming the multiply-used
variable something generic like "tmp", other times naming it
specifically (e.g. "flags", then using it once for a matching purpose
(e.g. a string containing the flags arg for an ebtables command option),
and again for something only tangentially related (e.g. the *mask* arg
for an ebtables command
Re: [PATCH 11/25] network: use g_free() in place of remaining VIR_FREE()
On Thu, Jun 25, 2020 at 11:01:48AM -0400, Laine Stump wrote:
> On 6/25/20 3:55 AM, Peter Krempa wrote:
> > On Wed, Jun 24, 2020 at 23:34:00 -0400, Laine Stump wrote:
> > > Signed-off-by: Laine Stump
> > > ---
> > > src/network/bridge_driver.c | 59 +
> > > 1 file changed, 33 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > > index 668aa9ca88..a1b2f5b6c7 100644
> > > --- a/src/network/bridge_driver.c
> > > +++ b/src/network/bridge_driver.c
> > [...]
> >
> > > @@ -706,7 +706,8 @@ networkStateInitialize(bool privileged,
> > > network_driver->lockFD = -1;
> > > if (virMutexInit(_driver->lock) < 0) {
> > > -VIR_FREE(network_driver);
> > > +g_free(network_driver);
> > > +network_driver = NULL;
> > > goto error;
> > In general I'm agains senseless replacement of VIR_FREE for g_free.
> > There is IMO no value to do so. VIR_FREE is now implemented via
> > g_clear_pointer(, g_free) so g_free is actually used.
> >
> > Mass replacements are also substrate for adding bugs and need to be
> > approached carefully, so doing this en-mass might lead to others
> > attempting the same with possibly less care.
> > In general, mass replacements should be done only to
> >
> > g_clear_pointer(, g_free)
> >
> > and I'm not sure it's worth it.
> >
>
> There's no getting around it - that looks ugly. And who wants to replace
> 5506 occurences of one simple-looking thing with something else that's
> functionally equivalent but more painful to look at?
>
>
> I would vote for just documenting that, for safety and consistency reasons,
> VIR_FREE() should always be used instead of g_free(), and eliminating all
> direct use of g_free() (along with the aforementioned syntax check). (BTW, I
> had assumed there had been more changes to g_free(), but when I looked at my
> current tree just now, there were only 228 occurences, including the changes
> in this patch)
The point in getting rid of VIR_FREE is so that we reduce the libvirt
specific wrappers in favour of standard APIs.
A large portion of the VIR_FREE's will be eliminated by g_autoptr.
Another large set of them are used in the virFooStructFree() methods.
Those can all be converted to g_free safely, as all the methods do
is free stuff.
Most VIR_FREEs that occur at the exit of functions can also be
safely converted to g_free, if g_autoptr isnt applicable. Sometimes
needs a little care if you have multiple goto jumps between labels.
The big danger cases are the VIR_FREE()s that occur in the middle
of methods, especially in loop bodies. Those the ones that must
use the g_clear_pointer, and that's not very many of those, so the
ugly syntax isn't an issue.
So I see no reason to keep VIR_FREE around long term.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 11/25] network: use g_free() in place of remaining VIR_FREE()
On 6/25/20 3:55 AM, Peter Krempa wrote:
On Wed, Jun 24, 2020 at 23:34:00 -0400, Laine Stump wrote:
Signed-off-by: Laine Stump
---
src/network/bridge_driver.c | 59 +
1 file changed, 33 insertions(+), 26 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 668aa9ca88..a1b2f5b6c7 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
[...]
@@ -706,7 +706,8 @@ networkStateInitialize(bool privileged,
network_driver->lockFD = -1;
if (virMutexInit(_driver->lock) < 0) {
-VIR_FREE(network_driver);
+g_free(network_driver);
+network_driver = NULL;
goto error;
In general I'm agains senseless replacement of VIR_FREE for g_free.
There is IMO no value to do so. VIR_FREE is now implemented via
g_clear_pointer(, g_free) so g_free is actually used.
Mass replacements are also substrate for adding bugs and need to be
approached carefully, so doing this en-mass might lead to others
attempting the same with possibly less care.
Actually I agree with you :-)
When we started into all this glib stuff, I thought that it was kind of
unproductive to churn the code around in cases where it was just
renaming one thing to something else - aside from (as you point out)
being a siren call for regressions, this also makes backports to old
branches more annoying, obscures *actual* functional history, and
besides, what happens the *next* time we want to change how we do
[whatever thing we're changing]? Do we do yet another global replacement
for the "new new hotness"? But this was one of those things that didn't
seem worth getting in the way of (and in balance it was definitely a net
win), so I mostly ignored it (including not going out of my way to
convert any code over just for the sake of converting).
In the meantime, lots and lots of patches have come in converting this
stuff piecemeal over the codebase, and it's all becoming more and more
g_*-centric. I still didn't really bother with it much.
Then I saw a memory leak in a patch a couple weeks ago that wouldn't
have occurred if the existing function had used g_autofree (and thus
reminded the author to use g_autofree for their additions to this
existing function). This led me to make a patch to convert that file to
use g_autofree and g_autoptr wherever possible, which in turn got me to
look at xmlBuffer allocation/free and notice a couple bugs, which led to
noticing something else inconsistent with current style, which led to
noticing some other existing bug, and from there to something else ad
infinitum.
So this one recognition of a single memory leak organically led to a
bunch of drive-by patches, but the drive-by patches left everything in
an in-between limbo state - half of things were the "old way" and half
were the "new way". Somewhere in the middle of all this, I looked back
at a recent set of patches from danpb for reference, and saw that along
with making locals g_auto*, and changing VIR_ALLOC to g_new0, he had
also replaced VIR_FREE with g_free, so I figured I should probably do
that too while I was already churning things. The semantic change (no
longer setting the pointer to the freed memory to NULL) was bothered me,
but since it was already being used, I assumed there must have been
discussion about it among all the glib conversion mails I skipped over,
and decided to make my patches consistent with "current convention", and
just carefully examine each usage to assure that either the pointer
wasn't referenced after free, or that it was explicitly set to NULL.
I do recognize your concern that "some other people" (thanks for
explicitly, though incorrectly, excluding me! :-)) may not be as
diligent when doing a similar replacement though, and even after doing
it myself I have concern that I may have missed something.
And now you point out the new implementation to VIR_FREE() (*yet
another* change missed by me, as with so many other things that whiz by
on the mailing list) that uses g_clear_pointer (which, having not read
through the glib reference manual nor worked on other projects using
glib, I didn't know about until today)! This validates my original
apprehension (in the before before time) about replacing VIR_* with g_*
macros - when we use our own macros it may be slightly more difficult
for first-time readers of the code who *might* have already been
familiar with glib (or maybe not), but it allows us to easily change the
underlying implementation in the future without yet again churning
through all the code.
This convinces me that VIR_FREE shouldn't be replaced with g_free in
*any* circumstance. As a matter of fact, I would even go so far as to
say that use of g_free() should be .. er "prohibited" with a syntax
check (or would that be limiting free speech?).
(BTW, in case someone might bring up the argument of "g_free() should be
used in
Re: [PATCH 11/25] network: use g_free() in place of remaining VIR_FREE()
On Wed, Jun 24, 2020 at 23:34:00 -0400, Laine Stump wrote:
> Signed-off-by: Laine Stump
> ---
> src/network/bridge_driver.c | 59 +
> 1 file changed, 33 insertions(+), 26 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 668aa9ca88..a1b2f5b6c7 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
[...]
> @@ -706,7 +706,8 @@ networkStateInitialize(bool privileged,
>
> network_driver->lockFD = -1;
> if (virMutexInit(_driver->lock) < 0) {
> -VIR_FREE(network_driver);
> +g_free(network_driver);
> +network_driver = NULL;
> goto error;
In general I'm agains senseless replacement of VIR_FREE for g_free.
There is IMO no value to do so. VIR_FREE is now implemented via
g_clear_pointer(, g_free) so g_free is actually used.
Mass replacements are also substrate for adding bugs and need to be
approached carefully, so doing this en-mass might lead to others
attempting the same with possibly less care.
In general, mass replacements should be done only to
g_clear_pointer(, g_free)
and I'm not sure it's worth it.
[PATCH 11/25] network: use g_free() in place of remaining VIR_FREE()
Signed-off-by: Laine Stump
---
src/network/bridge_driver.c | 59 +
1 file changed, 33 insertions(+), 26 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 668aa9ca88..a1b2f5b6c7 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -148,7 +148,7 @@ networkDnsmasqDefNamespaceFree(void *nsdata)
virStringListFreeCount(def->options, def->noptions);
-VIR_FREE(def);
+g_free(def);
}
@@ -706,7 +706,8 @@ networkStateInitialize(bool privileged,
network_driver->lockFD = -1;
if (virMutexInit(_driver->lock) < 0) {
-VIR_FREE(network_driver);
+g_free(network_driver);
+network_driver = NULL;
goto error;
}
@@ -874,18 +875,19 @@ networkStateCleanup(void)
virPidFileRelease(network_driver->stateDir, "driver",
network_driver->lockFD);
-VIR_FREE(network_driver->networkConfigDir);
-VIR_FREE(network_driver->networkAutostartDir);
-VIR_FREE(network_driver->stateDir);
-VIR_FREE(network_driver->pidDir);
-VIR_FREE(network_driver->dnsmasqStateDir);
-VIR_FREE(network_driver->radvdStateDir);
+g_free(network_driver->networkConfigDir);
+g_free(network_driver->networkAutostartDir);
+g_free(network_driver->stateDir);
+g_free(network_driver->pidDir);
+g_free(network_driver->dnsmasqStateDir);
+g_free(network_driver->radvdStateDir);
virObjectUnref(network_driver->dnsmasqCaps);
virMutexDestroy(_driver->lock);
-VIR_FREE(network_driver);
+g_free(network_driver);
+network_driver = NULL;
return 0;
}
@@ -2192,7 +2194,7 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
/* Prevent guests from hijacking the host network by sending out
* their own router advertisements.
*/
-VIR_FREE(field);
+g_free(field);
field = g_strdup_printf(SYSCTL_PATH "/net/ipv6/conf/%s/accept_ra",
def->bridge);
@@ -2205,7 +2207,7 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
/* All interfaces used as a gateway (which is what this is, by
* definition), must always have autoconf=0.
*/
-VIR_FREE(field);
+g_free(field);
field = g_strdup_printf(SYSCTL_PATH "/net/ipv6/conf/%s/autoconf",
def->bridge);
if (virFileWriteStr(field, "0", 0) < 0) {
@@ -2713,19 +2715,19 @@ networkCreateInterfacePool(virNetworkDefPtr netdef)
for (i = 0; i < netdef->forward.nifs; i++) {
if (netdef->forward.ifs[i].type
== VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV)
-VIR_FREE(netdef->forward.ifs[i].device.dev);
+g_free(netdef->forward.ifs[i].device.dev);
}
netdef->forward.nifs = 0;
}
if (netdef->forward.nifs == 0)
-VIR_FREE(netdef->forward.ifs);
+g_free(netdef->forward.ifs);
for (i = 0; i < numVirtFns; i++) {
-VIR_FREE(vfNames[i]);
-VIR_FREE(virtFns[i]);
+g_free(vfNames[i]);
+g_free(virtFns[i]);
}
-VIR_FREE(vfNames);
-VIR_FREE(virtFns);
+g_free(vfNames);
+g_free(virtFns);
return ret;
}
@@ -3161,7 +3163,7 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets,
*/
if (!(virNetworkObjBridgeInUse(nets, newname, def->name) ||
virNetDevExists(newname) == 1)) {
-VIR_FREE(def->bridge); /*could contain template */
+g_free(def->bridge); /*could contain template */
def->bridge = g_steal_pointer();
return 0;
}
@@ -4256,7 +4258,8 @@ networkGetDHCPLeases(virNetworkPtr net,
nleases++;
}
-VIR_FREE(lease);
+g_free(lease);
+lease = NULL;
}
if (leases_ret) {
@@ -4269,7 +4272,7 @@ networkGetDHCPLeases(virNetworkPtr net,
rv = nleases;
cleanup:
-VIR_FREE(lease);
+g_free(lease);
virNetworkObjEndAPI();
return rv;
@@ -4278,7 +4281,7 @@ networkGetDHCPLeases(virNetworkPtr net,
if (leases_ret) {
for (i = 0; i < nleases; i++)
virNetworkDHCPLeaseFree(leases_ret[i]);
-VIR_FREE(leases_ret);
+g_free(leases_ret);
}
goto cleanup;
}
@@ -4402,7 +4405,7 @@ networkAllocatePort(virNetworkObjPtr obj,
return -1;
}
if (portprofile) {
-VIR_FREE(port->virtPortProfile);
+g_free(port->virtPortProfile);
port->virtPortProfile = portprofile;
}
@@ -5519,10 +5522,14 @@ networkPortSetParameters(virNetworkPortPtr port,
/* average or floor are mandatory, peak and burst are optional.
* So if no average or floor is given, we free inbound/outbound
* here which causes inbound/outbound to not be set. */
-if (!bandwidth->in->average && !bandwidth->in->floor)
-VIR_FREE(bandwidth->in);
-if (!bandwidth->out->average)
-VIR_FREE(bandwidth->out);
+if (!bandwidth->in->average &&
