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

[Daniel P. Berrangé]

On Mon, Feb 01, 2021 at 10:54:26AM +0100, Peter Krempa wrote:
> On Mon, Feb 01, 2021 at 01:27:20 -0500, Laine Stump wrote:
> > This patch takes on one set of examples of unnecessary use of
> > VIR_FREE() when g_free() is adequate - it modifies only vir*Free()
> > functions within the conf directory that take a single pointer and
> > free the object pointed to by that argument before returning. The
> > modification is to replace VIR_FREE() with g_free() for the object
> > itself *and* for all subordinate chunks of memory pointed to by that
> > object.
> > 
> > (NB: there are other functions that VIR_FREE subordinate memory of
> > objects that end up being freed before return (also sometimes with
> > VIR_FREE); I am purposefully ignoring those to reduce scope and focus
> > on a sub class where the pointlessness is obvious.)
> > 
> > Signed-off-by: Laine Stump <laine at redhat.com>
> > ---
> > 
> > NB: After going through the experience a few days ago of needing to
> > more or less manually backport a patch due to a different patch
> > similar to this one touching the one function relevant to the desired
> > patch as well as many dozens of other functions (thus making it
> > impractical to backport that patch as a prerequisite), I contemplated
> > splitting this patch up all the way to 1 patch for every
> > function. That seemed extreme though, so I've left it as is. An
> > alternative, of course, is to just do nothing and leave everything as
> > VIR_FREE() (and I know there is sentiment in that direction, a bit
> > from me even :-P). I wonder though if we'll ever live up to the goals
> > listed in docs/glib-adoption.txt...
> 
> I reckon that there isn't much value in replacing VIR_FREE by g_free as
> a separate cleanup step, unless we are about to remove VIR_FREE
> altoghether and open-code it in places where it's required.

Removing entire of viralloc.h is the long term goal.

In most methods we remove alot of VIR_FREE usage by switching to
g_autofree.  The virXXXXFree() methods though can't use g_autofree,
so ultimately they are were most remaining VIR_FREEs are going to
be. IOW, this change is a good step towards full elimination
of VIR_FREE.

> As of such I'm not in favor of such cleanup patch. There's quite lot of
> churn and the technical justification is rather weak.

The justification is to get further towards fully eliminating VIR_FREE.

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