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@xxxxxxxxxx>
> > > ---
> > > 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 :|
