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

Laine Stump laine at redhat.com
Fri Jun 26 17:52:00 UTC 2020


On 6/25/20 11:12 AM, Daniel P. Berrangé wrote:
> 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.

Is this just to make the code more accessible/understandable to new 
contributors? Or is there some other reason that I missed due to being 
incapable of reading all the messages on all the lists? (I guess there's 
also the issue of reducing support burden by reproducing identical code 
to something that someone else is already maintaining in a different 
library. But in this case we're just talking about a few lines that 
enforces good behavior.)

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

It still requires thought + diligence = time. And what if new code is 
added to the end of a function, thus making those things that had been 
"at the end" now in the middle. The more thought and decision making is 
needed to get something right, the more likely it is that someone will 
get it wrong.

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

1) Maybe I'll feel differently after more of the code has been converted 
to use g_auto* and eliminated more of the existing explicit frees, but 
with currently > 5000 uses of VIR_FREE still in the code, I fear that 
"not many of those" may be more than we're expecting, and especially 
with many of them left, it would give me more warm fuzzies to be able to 
say

  "We can verifiably state that no pointers will be used
   after free , because their values have been NULLed,
   and any access will either be a NOP, or cause an
   immediate segfault"

rather than

  "We believe that the contributors to libvirt have been
   diligent in their manual auditing of all cases of
   free'ing memory to assure that none of the freed
   pointers are ever used at any later point,
   because.... well, just *because*".

(on the other hand, admittedly any pointer to something with its own 
vir*Free() function already requires diligence on the part of the 
programmer, since vir*Free() doesn't NULL the pointer. In that case, 
what's a little extra burden?)


2) Speaking from my experience with the occurrences I converted here, 
the worst offenders were the places where someone re-used a local 
pointer multiple times in a function (sometimes naming the multiply-used 
variable something generic like "tmp", other times naming it 
specifically (e.g. "flags", then using it once for a matching purpose 
(e.g. a string containing the flags arg for an ebtables command option), 
and again for something only tangentially related (e.g. the *mask* arg 
for an ebtables command option). I used to think that re-using an 
automatic was clever and efficient because it conserved stack usage, but 
now I think it's just another source of making the code confusing, error 
prone, and time consuming to modify.


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

Now I'm torn. I agree with Peter's assertion that moving to g_free could 
introduce new bugs in spite of our diligence, and that in general it's 
safer to rely on the programmatically verifiable safety of VIR_FREE 
rather than said diligence. But if that's the way we're going, I also 
don't want my patches to leave the conversion "half done", and pawn the 
last bit off on someone else.

(actually, I already noticed that, of all the things to leave out (!), I 
didn't even fully convert the one file that started this all - 
domain_conf.c; I failed to even include a patch to eliminate 
VIR_ALLOC/VIR_REALLOC (much less VIR_FREE). I guess that just shows how 
Kerouac-esque stream-of-consciousness the patches in this series were :-))

I guess I'll separate out all the g_free() conversions and deal with 
them separately after all the rest is finished.




More information about the libvir-list mailing list