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

Laine Stump laine at redhat.com
Thu Jun 25 15:01:48 UTC 2020


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.


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 cases when it's not important to null out the pointer, because 
it's more efficient!", my response is that 1) nobody should have to be 
burdened with manually verifying that their pointer isn't referenced 
after its freed just to determine which function to call, and 2) 
consistency is more important than an undetectably tiny amount of extra 
efficiency (and this comes from someone who spent a significant amount 
of time in his early career writing device drivers and even simple 
application programs in assembly language, and internally shudders 
whenever automatic garbage collection is mentioned))


> 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)


So what about the rest of these patches? Should I repost the entire 
series without any g_free() additions? Or will you (or someone else) ACK 
the ones here that don't include g_free()? (Maybe there's something else 
I've done that, despite my purist intentions, doesn't fit with current 
sentiment?)





More information about the libvir-list mailing list