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