11:25 p.m.
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...