[libvirt RFC PATCH 0/5] eliminating VIR_FREE in the *Clear() functions

[Michal Privoznik]

On 2/12/21 6:54 AM, Laine Stump wrote:
> I've looked at a few of these, and one thing I've found is that very
> often we have a function called somethingSomethingClear(), and:
> 
> 1) The only places it is ever called will immediately free the memory
> of the object as soon as they clear it.
> 
> and very possibly
> 
> 2) It doesn't actually *clear* everything. Some items are cleared via VIR_FREE(), but then some of the other pointers call
> 
>      bobLoblawFree(def->bobloblaw)
> 
> and then don't actually set def->bobloblaw to NULL - so the functions
> aren't actually "Clearing", they're "Freeing and then clearing a few
> things, but not everything".
> 
> So I'm wondering if it is worthwhile to
> 
> A) audit all the *Clear() functions and rename the functions that
> don't actually need to clear the contents to be, e.g.
> bobLobLawFreeContents(), while also replacing VIR_FREE with g_free().
> (this is what I've done in these 5 patches)
> 
> or if we should
> 
> B) just do the wholesale approach and (as danpb suggested last week)
> change all VIR_FREE in *Clear() functions to g_free(), and put a
> "memset(obj, 0, sizeof(*obj))" at the end of each function, ignoring
> whether or not we actually need that.
> 
> (B) would obviously be much faster to get done, and simpler for
> everyone to keep track of what's happening, but of course it is less
> efficient. Very likely this efficiency is completely meaningless in
> the large scheme (even in the medium or small scheme).
> 
> (I actually like the idea of 0'ing out *everything*[*] when we're done
> with it, extra cycles be damned! I think of the two choices above,
> after going through this exercise, I'd say (B) is a more reasonable
> choice, but since I tried this, I thought I'd send it and see if
> anyone else has an opinion (or different idea)
> 
> [*](including all those places I've replaced VIR_FREE with g_free in
> the name of "progress?")
> 
> Laine Stump (5):
>    conf: rename and narrow scope of virDomainHostdevDefClear()
>    conf: rename virDomainHostdevSubsysSCSIClear
>    conf: replace pointless VIR_FREEs with g_free in
>      virDomainVideoDefClear()
>    conf: don't call virDomainDeviceInfoClear from
>      virDomainDeviceInfoParseXML
>    conf: replace virDomainDeviceInfoClear with
>      virDomainDeviceInfoFreeContents
> 
>   src/conf/device_conf.c   | 15 +++-----
>   src/conf/device_conf.h   |  2 +-
>   src/conf/domain_conf.c   | 81 ++++++++++++++++++++--------------------
>   src/conf/domain_conf.h   |  1 -
>   src/libvirt_private.syms |  1 -
>   5 files changed, 47 insertions(+), 53 deletions(-)
> 

I don't like change and thus prefer keeping *Clear() with explicit 
memset(0) - if needed, but don't want to stop progress.

Michal