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