On a Monday in 2021, Daniel P. Berrangé 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:
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@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.
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.
And dragging out the death of VIR_FREE makes it easier to search for places that have not been converted to g_autofree yet. Jano
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 :|