[PATCH 8/8] conf: replace VIR_FREE() with g_free() in vir*Free() functions

[Laine Stump]

On 2/1/21 5:23 AM, Daniel P. Berrangé wrote:
> On Mon, Feb 01, 2021 at 11:20:04AM +0100, Peter Krempa wrote:
>> On Mon, Feb 01, 2021 at 10:09:25 +0000, Daniel Berrange wrote:
>>> On Mon, Feb 01, 2021 at 11:05:15AM +0100, Peter Krempa wrote:
>>>> On Mon, Feb 01, 2021 at 09:59:57 +0000, Daniel Berrange wrote:
>> [...]
>>
>>>> In such case I'd strongly prefer if we replace all remaining usage of
>>>> VIR_FREE (even after this commit) right away to
>>>> g_clear_pointer(&ptr, g_free), whithout stretching the pain across
>>>> multiple spread-out refactoring steps.
>>>>
>>>> I don't have a problem getting rid of it, I just don't want it to be
>>>> dragging out forever.
>>> A bulk replacement isn't the right approach, because it will lead to
>>> greater code churn. Much usage of VIR_FREE is better replaced by use
>>> of g_autofree. What remains is better replaced by a simple g_free,
>>> only a relatively small amount needs g_clear_pointer. Bulking replacing
>>> everything with g_clear_pointer just means we'll need to replace it
>>> yet again with the desired final solution.
>> Well, that is extremely unlikely to be done soon there's a bit over 4k
>> VIR_FREE's left in various forgotten and unused parts of libvirt that
>> wasn't touched in a long time.
>>
>> Spending time converting the use of those to g_free and g_autofree
>> manually would be in many cases a waste of time.
>>
>> If we ever want to get rid of VIR_FREE in a timely manner it will IMO be
>> better to just replace it by the identical code, and let the cleanups
>> happen gradually and more localized in the parts people care about
>> actually.
>>
>>> Incrementally converting
>>> our codebase is just fine.
>> I'd agree with that, but approach in this commit is somewhere betwen
>> incremental and massive conversion. It picks the low hanging uses, but
>> leaves the ones which will likely stay there for a very long time.
> I'd say it picks the cases which are clearly correct to convert directly
> to g_free, and leaves the cases which are likely going to need to use
> either g_auto* or g_clear_pointer. IMHO this is the correct way to do
> the conversion.


Speaking of "other cases" - here are three classes of 
"non-g_autofreeable" VIR_FREE() that I see a lot of when looking in the 
conf directory (which unsurprisingly has 717 of the 4k+ uses of 
VIR_FREE(). If anyone has ideas/opinions on these, I wouldn't mind 
hearing it:


1) calling VIR_FREE() in a virBlahDispose() function to free subordinate 
objectspointed to by the objec being disposed of. Is there any reason to 
*not* convert those VIR_FREEs to g_free()? There's nothing that would 
ever look at the contents of an object after its vir*Dispose() function 
has been called is there?


2) calling VIR_FREE() in a virBlahClear() function. Technically these 
functions *do* need to set the pointer to NULL, because well, it says it 
right there in the name of the function! However, in several of the 
instances I checked, the only caller to the vir*Clear() function was a 
vir*Free() function that was cleaning up its subordinate objects prior 
to freeing itself. Usually those subordinate objects are contained in 
the larger object (rather than pointed to) in the name of efficiency 
(less calls to mallo... er I mean g_new0()). Do you think there's any 
point to, e.g. turning these "virBlah item" members into ",virBlahPtr 
item" so they are gotten rid of with virBlahFree() instead of 
virBlahClear()? Or is this one of the cases where it's definitely proper 
to use g_clear_pointer()


3) g_autofree pointers called "tmp", "str", "nodes", and probably some 
others that are re-used several times in a function, and are VIR_FREEd 
after each use. Aside from breaking the rule/guideline that you should 
never explicitly free an g_autofree pointer, they by definition won't 
simply go away by converting to g_autofree - they've already been 
converted! I can see two simple ways of eliminating these: 1) make 
multiple g_autofree char *'s in each function, once for each usage (will 
the compiler be smart enough to optimize these into a single pointer if 
the usage doesn't overlap?), or 2) switch to using g_clear_pointer() (is 
*that* also frowned upon for pointers that are already g_autofree?)


4) I know I said three, but there are also several examples of 
VIR_FREE() being called in a loop on the individual items in an array 
prior to freeing the array (see every example of calling 
virBlkioDeviceArrayClear(), for example). I don't think anyone has spent 
any time looking into converting things to use GArray and GPtrArray, but 
I suppose that's the best way to get rid of those...