Re: [PATCH 11/25] network: use g_free() in place of remaining VIR_FREE()

On 6/25/20 11:12 AM, Daniel P. Berrangé wrote: Is this just to make the code more accessible/understandable to new contributors? Or is there some other reason that I missed due to being incapable of reading all the messages on all the lists? (I guess there's also the issue of reducing support burden by reproducing identical code to something that someone else is already maintaining in a different library. But in this case we're just talking about a few lines that enforces good behavior.) It still requires thought + diligence = time. And what if new code is added to the end of a function, thus making those things that had been "at the end" now in the middle. The more thought and decision making is needed to get something right, the more likely it is that someone will get it wrong. 1) Maybe I'll feel differently after more of the code has been converted to use g_auto* and eliminated more of the existing explicit frees, but with currently > 5000 uses of VIR_FREE still in the code, I fear that "not many of those" may be more than we're expecting, and especially with many of them left, it would give me more warm fuzzies to be able to say "We can verifiably state that no pointers will be used after free , because their values have been NULLed, and any access will either be a NOP, or cause an immediate segfault" rather than "We believe that the contributors to libvirt have been diligent in their manual auditing of all cases of free'ing memory to assure that none of the freed pointers are ever used at any later point, because.... well, just *because*". (on the other hand, admittedly any pointer to something with its own vir*Free() function already requires diligence on the part of the programmer, since vir*Free() doesn't NULL the pointer. In that case, what's a little extra burden?) 2) Speaking from my experience with the occurrences I converted here, the worst offenders were the places where someone re-used a local pointer multiple times in a function (sometimes naming the multiply-used variable something generic like "tmp", other times naming it specifically (e.g. "flags", then using it once for a matching purpose (e.g. a string containing the flags arg for an ebtables command option), and again for something only tangentially related (e.g. the *mask* arg for an ebtables command option). I used to think that re-using an automatic was clever and efficient because it conserved stack usage, but now I think it's just another source of making the code confusing, error prone, and time consuming to modify. Now I'm torn. I agree with Peter's assertion that moving to g_free could introduce new bugs in spite of our diligence, and that in general it's safer to rely on the programmatically verifiable safety of VIR_FREE rather than said diligence. But if that's the way we're going, I also don't want my patches to leave the conversion "half done", and pawn the last bit off on someone else. (actually, I already noticed that, of all the things to leave out (!), I didn't even fully convert the one file that started this all - domain_conf.c; I failed to even include a patch to eliminate VIR_ALLOC/VIR_REALLOC (much less VIR_FREE). I guess that just shows how Kerouac-esque stream-of-consciousness the patches in this series were :-)) I guess I'll separate out all the g_free() conversions and deal with them separately after all the rest is finished.