[PATCH 11/25] network: use g_free() in place of remaining VIR_FREE()

[Daniel P. Berrangé]

On Thu, Jun 25, 2020 at 11:01:48AM -0400, Laine Stump wrote:
> On 6/25/20 3:55 AM, Peter Krempa wrote:
> > On Wed, Jun 24, 2020 at 23:34:00 -0400, Laine Stump wrote:
> > > Signed-off-by: Laine Stump <laine at redhat.com>
> > > ---
> > >   src/network/bridge_driver.c | 59 +++++++++++++++++++++----------------
> > >   1 file changed, 33 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > > index 668aa9ca88..a1b2f5b6c7 100644
> > > --- a/src/network/bridge_driver.c
> > > +++ b/src/network/bridge_driver.c
> > [...]
> > 
> > > @@ -706,7 +706,8 @@ networkStateInitialize(bool privileged,
> > >       network_driver->lockFD = -1;
> > >       if (virMutexInit(&network_driver->lock) < 0) {
> > > -        VIR_FREE(network_driver);
> > > +        g_free(network_driver);
> > > +        network_driver = NULL;
> > >           goto error;
> > In general I'm agains senseless replacement of VIR_FREE for g_free.
> > There is IMO no value to do so. VIR_FREE is now implemented via
> > g_clear_pointer(&ptr, g_free) so g_free is actually used.
> > 
> > Mass replacements are also substrate for adding bugs and need to be
> > approached carefully, so doing this en-mass might lead to others
> > attempting the same with possibly less care.


> > In general, mass replacements should be done only to
> > 
> > g_clear_pointer(&ptr, g_free)
> > 
> > and I'm not sure it's worth it.
> > 
> 
> There's no getting around it - that looks ugly. And who wants to replace
> 5506 occurences of one simple-looking thing with something else that's
> functionally equivalent but more painful to look at?
> 
> 
> I would vote for just documenting that, for safety and consistency reasons,
> VIR_FREE() should always be used instead of g_free(), and eliminating all
> direct use of g_free() (along with the aforementioned syntax check). (BTW, I
> had assumed there had been more changes to g_free(), but when I looked at my
> current tree just now, there were only 228 occurences, including the changes
> in this patch)

The point in getting rid of VIR_FREE is so that we reduce the libvirt
specific wrappers in favour of standard APIs.

A large portion of the VIR_FREE's will be eliminated by g_autoptr.

Another large set of them are used in the virFooStructFree() methods.
Those can all be converted to g_free safely, as all the methods do
is free stuff.

Most VIR_FREEs that occur at the exit of functions can also be
safely converted to g_free, if g_autoptr  isnt applicable. Sometimes
needs a little care if you have multiple goto jumps between labels.

The big danger cases are the VIR_FREE()s that occur in the middle
of methods, especially in loop bodies. Those the ones that must
use the g_clear_pointer, and that's not very many of those, so the
ugly syntax isn't an issue.

So I see no reason to keep VIR_FREE around long term.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|