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

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: > > 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(a)redhat.com&gt; > > > > --- > > > > > > > > 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. > > 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. Incrementally converting our codebase is just fine. 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 :|