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

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