On Fri, Feb 12, 2021 at 12:54:02AM -0500, 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".
One thing I am wondering is whether this is really only used where it makes
sense. As far as I understand, and please correct me if I am way off, the
purpose of the Clear functions is to:
a) provide a way to remove everything from a structure that the current
function cannot recreate (there is a pointer to it somewhere else which
would not be updated) and
b) provide a way to reset a structure so that it can be filled again without
needless reallocation.
I think (b) is obviously pointless, especially lately, so the only reasonable
usage would be for the scenario (a). However, I think I remember this also
being used in places where it would be perfectly fine to free the variable and
recreate it. Maybe it could ease up the decision, at least by eliminating some
of the code, if my hunch is correct.
In my quick search I only found virDomainVideoDefClear to be used in this manner
and I am not convinced that it is worth having this extra function with extra
memset(). Just food for thought.
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(-) -- 2.29.2
Attachment:
signature.asc
Description: PGP signature
