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

[Daniel P. Berrangé]

On Tue, Feb 02, 2021 at 12:25:40AM -0500, Laine Stump wrote:
> 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?

Yep, a virBlahDispose function should be considered the same as
a virBlahFree function. The only difference is that in the former
case the final  g_free(blah) of the struct itself is done for you.

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

A simpler option might be to just put a "memset(blah, 0, sizeof(blah))"
at the end of the virBlahClear function so that we splatter the entire
struct at once.

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

I think in general we probably prefer to *not* refuse variables multiple
times. Where we do though, I'd say  g_clear_pointer is the semantically
correct approach.

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

Yep, but using g_free directly is fine here in the meanwhile.


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