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

On 2/12/21 11:07 AM, Erik Skultety wrote: > On Fri, Feb 12, 2021 at 10:43:56AM +0100, Martin Kletzander wrote: > > 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 > > You could always memset it explicitly, someone might find the code more > readable then. IMO I'd vote for an explicit memset just for the sake of better > security practice (since we'll have to wait a little while for something like > SGX to be convenient to deploy and develop with...). Anyhow, I'm not sure how > many cycles exactly would be wasted, but IIRC a recent discussion memset can be > optimized away (correct me if I don't remember it well!), so Dan P.B. > suggested to gradually convert to some platform-specific ways on how to > sanitize the memory safely - with that in mind, I'd say we use an explicit > memset in all the functions in question and convert them later? I think one can argue that if there's a memset() called inside a function that is supposed to clear out a member of a struct and later the member is tested against NULL, but compiler decides to "optimize" memset out - it's a compiler bug.