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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux