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 <laine(a)redhat.com&gt; > > --- > > 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)