[libvirt PATCH 00/15] eliminate VIR_FREE in all *Dispose() functions

Laine Stump posted 15 patches 3 years, 1 month ago
Failed in applying to current master (apply log)
src/access/viraccessmanager.c           |   2 +-
src/bhyve/bhyve_conf.c                  |   2 +-
src/conf/capabilities.c                 |  22 ++---
src/conf/checkpoint_conf.c              |   2 +-
src/conf/domain_capabilities.c          |  29 ++----
src/conf/domain_conf.c                  |   2 +-
src/conf/domain_event.c                 |  52 +++++-----
src/conf/moment_conf.c                  |   6 +-
src/conf/object_event.c                 |   4 +-
src/conf/snapshot_conf.c                |   4 +-
src/conf/virsecretobj.c                 |   6 +-
src/conf/virstorageobj.c                |   4 +-
src/datatypes.c                         |  34 +++----
src/hypervisor/virhostdev.c             |   2 +-
src/interface/interface_backend_netcf.c |   2 +-
src/libxl/libxl_conf.c                  |  20 ++--
src/libxl/libxl_migration.c             |   2 +-
src/logging/log_handler.c               |   2 +-
src/qemu/qemu_agent.c                   |   2 +-
src/qemu/qemu_capabilities.c            |  10 +-
src/qemu/qemu_conf.c                    | 122 ++++++++++++------------
src/qemu/qemu_domain.c                  |  10 +-
src/qemu/qemu_monitor.c                 |   4 +-
src/rpc/virnetclient.c                  |   4 +-
src/rpc/virnetdaemon.c                  |   6 +-
src/rpc/virnetlibsshsession.c           |  37 +++----
src/rpc/virnetsaslcontext.c             |   2 +-
src/rpc/virnetserver.c                  |   8 +-
src/rpc/virnetserverservice.c           |   2 +-
src/rpc/virnetsocket.c                  |   6 +-
src/rpc/virnetsshsession.c              |   8 +-
src/rpc/virnettlscontext.c              |   6 +-
src/security/security_manager.c         |   2 +-
src/util/virdnsmasq.c                   |   2 +-
src/util/virfilecache.c                 |   4 +-
src/util/virmdev.c                      |   2 +-
src/util/virnvme.c                      |   2 +-
src/util/virpci.c                       |   2 +-
src/util/virresctrl.c                   |  40 ++++----
src/util/virscsi.c                      |   2 +-
src/util/virscsivhost.c                 |   2 +-
src/util/virusb.c                       |   2 +-
tests/virfilecachetest.c                |   2 +-
43 files changed, 235 insertions(+), 251 deletions(-)
[libvirt PATCH 00/15] eliminate VIR_FREE in all *Dispose() functions
Posted by Laine Stump 3 years, 1 month ago
A *Dispose() function is similar to a *Free() function, except that 1)
the object is always sent as a void* so it has to be typecast into
some other object-specific pointer at the top of the function, and 2)
it frees all the resources inside the object, but never frees the
object itself (this is done by the caller, somewhere deep in the
bowels of virObjectDispose or something I guess; frankly I've always
ignored the details simply "because I could").

The important point is that the contents of the object are never
referenced in any way after return from the Dispose function, so it is
unnecessary to clear any pointers, ergo (always wanted to use that
word!) it's completely safe to replace all VIR_FREEs in a *Dispose()
function with g_free (as long as there's nothing within the Dispose
function itself that depends on the pointers being cleared).

After this series is applied, there will be exactly 0 instances of
VIR_FREE in any *Dispose() (or *Free()) function. As with the *Free()
series, almost all were accomplished by directly replacing VIR_FREE
with g_free, but there were a couple oddities that needed separate
patches just to call them out:

 * Patch 1 & 2 - in both cases a Dispose() function was the only
   caller to a *Free() function that didn't fit the normal pattern of
   a *Free() function. Since each of the Free()s had only one caller
   (their respective *Dispose() parents), their contents were just
   moved into the caller, clearing the way for their VIR_FREEs to be
   g_free-ified (in later patches, along with the other *Dispose()
   functions in the same directories).

220 VIR_FREE uses eliminated in this series, so a total of 762 for the
two series combined (nearly 20% of all remaining VIR_FREEs).

Laine Stump (15):
  conf: simplify virDomainCapsDispose()
  rpc: eliminate static function virNetLibsshSessionAuthMethodsFree()
  bhyve: replace VIR_FREE with g_free in all *Dispose() functions
  libxl: replace VIR_FREE with g_free in all *Dispose() functions
  qemu: replace VIR_FREE with g_free in all *Dispose() functions
  interface: replace VIR_FREE with g_free in all *Dispose() functions
  access: replace VIR_FREE with g_free in all *Dispose() functions
  hypervisor: replace VIR_FREE with g_free in all *Dispose() functions
  logging: replace VIR_FREE with g_free in all *Dispose() functions
  rpc: replace VIR_FREE with g_free in all *Dispose() functions
  security: replace VIR_FREE with g_free in all *Dispose() functions
  util: replace VIR_FREE with g_free in all *Dispose() functions
  conf: replace VIR_FREE with g_free in all *Dispose() functions
  tests: replace VIR_FREE with g_free in all *Dispose() functions
  datatypes: replace VIR_FREE with g_free in all *Dispose() functions

 src/access/viraccessmanager.c           |   2 +-
 src/bhyve/bhyve_conf.c                  |   2 +-
 src/conf/capabilities.c                 |  22 ++---
 src/conf/checkpoint_conf.c              |   2 +-
 src/conf/domain_capabilities.c          |  29 ++----
 src/conf/domain_conf.c                  |   2 +-
 src/conf/domain_event.c                 |  52 +++++-----
 src/conf/moment_conf.c                  |   6 +-
 src/conf/object_event.c                 |   4 +-
 src/conf/snapshot_conf.c                |   4 +-
 src/conf/virsecretobj.c                 |   6 +-
 src/conf/virstorageobj.c                |   4 +-
 src/datatypes.c                         |  34 +++----
 src/hypervisor/virhostdev.c             |   2 +-
 src/interface/interface_backend_netcf.c |   2 +-
 src/libxl/libxl_conf.c                  |  20 ++--
 src/libxl/libxl_migration.c             |   2 +-
 src/logging/log_handler.c               |   2 +-
 src/qemu/qemu_agent.c                   |   2 +-
 src/qemu/qemu_capabilities.c            |  10 +-
 src/qemu/qemu_conf.c                    | 122 ++++++++++++------------
 src/qemu/qemu_domain.c                  |  10 +-
 src/qemu/qemu_monitor.c                 |   4 +-
 src/rpc/virnetclient.c                  |   4 +-
 src/rpc/virnetdaemon.c                  |   6 +-
 src/rpc/virnetlibsshsession.c           |  37 +++----
 src/rpc/virnetsaslcontext.c             |   2 +-
 src/rpc/virnetserver.c                  |   8 +-
 src/rpc/virnetserverservice.c           |   2 +-
 src/rpc/virnetsocket.c                  |   6 +-
 src/rpc/virnetsshsession.c              |   8 +-
 src/rpc/virnettlscontext.c              |   6 +-
 src/security/security_manager.c         |   2 +-
 src/util/virdnsmasq.c                   |   2 +-
 src/util/virfilecache.c                 |   4 +-
 src/util/virmdev.c                      |   2 +-
 src/util/virnvme.c                      |   2 +-
 src/util/virpci.c                       |   2 +-
 src/util/virresctrl.c                   |  40 ++++----
 src/util/virscsi.c                      |   2 +-
 src/util/virscsivhost.c                 |   2 +-
 src/util/virusb.c                       |   2 +-
 tests/virfilecachetest.c                |   2 +-
 43 files changed, 235 insertions(+), 251 deletions(-)

-- 
2.29.2

Re: [libvirt PATCH 00/15] eliminate VIR_FREE in all *Dispose() functions
Posted by Daniel Henrique Barboza 3 years, 1 month ago

On 2/4/21 1:38 AM, Laine Stump wrote:
> A *Dispose() function is similar to a *Free() function, except that 1)
> the object is always sent as a void* so it has to be typecast into
> some other object-specific pointer at the top of the function, and 2)
> it frees all the resources inside the object, but never frees the
> object itself (this is done by the caller, somewhere deep in the
> bowels of virObjectDispose or something I guess; frankly I've always
> ignored the details simply "because I could").
> 
> The important point is that the contents of the object are never
> referenced in any way after return from the Dispose function, so it is
> unnecessary to clear any pointers, ergo (always wanted to use that
> word!) it's completely safe to replace all VIR_FREEs in a *Dispose()
> function with g_free (as long as there's nothing within the Dispose
> function itself that depends on the pointers being cleared).
> 
> After this series is applied, there will be exactly 0 instances of
> VIR_FREE in any *Dispose() (or *Free()) function. As with the *Free()
> series, almost all were accomplished by directly replacing VIR_FREE
> with g_free, but there were a couple oddities that needed separate
> patches just to call them out:
> 
>   * Patch 1 & 2 - in both cases a Dispose() function was the only
>     caller to a *Free() function that didn't fit the normal pattern of
>     a *Free() function. Since each of the Free()s had only one caller
>     (their respective *Dispose() parents), their contents were just
>     moved into the caller, clearing the way for their VIR_FREEs to be
>     g_free-ified (in later patches, along with the other *Dispose()
>     functions in the same directories).
> 
> 220 VIR_FREE uses eliminated in this series, so a total of 762 for the
> two series combined (nearly 20% of all remaining VIR_FREEs).
> 
> Laine Stump (15):
>    conf: simplify virDomainCapsDispose()
>    rpc: eliminate static function virNetLibsshSessionAuthMethodsFree()
>    bhyve: replace VIR_FREE with g_free in all *Dispose() functions
>    libxl: replace VIR_FREE with g_free in all *Dispose() functions
>    qemu: replace VIR_FREE with g_free in all *Dispose() functions
>    interface: replace VIR_FREE with g_free in all *Dispose() functions
>    access: replace VIR_FREE with g_free in all *Dispose() functions
>    hypervisor: replace VIR_FREE with g_free in all *Dispose() functions
>    logging: replace VIR_FREE with g_free in all *Dispose() functions
>    rpc: replace VIR_FREE with g_free in all *Dispose() functions
>    security: replace VIR_FREE with g_free in all *Dispose() functions
>    util: replace VIR_FREE with g_free in all *Dispose() functions
>    conf: replace VIR_FREE with g_free in all *Dispose() functions
>    tests: replace VIR_FREE with g_free in all *Dispose() functions
>    datatypes: replace VIR_FREE with g_free in all *Dispose() functions


Series LGTM, just be careful with what I mentioned in patch 10.


All patches:


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

> 
>   src/access/viraccessmanager.c           |   2 +-
>   src/bhyve/bhyve_conf.c                  |   2 +-
>   src/conf/capabilities.c                 |  22 ++---
>   src/conf/checkpoint_conf.c              |   2 +-
>   src/conf/domain_capabilities.c          |  29 ++----
>   src/conf/domain_conf.c                  |   2 +-
>   src/conf/domain_event.c                 |  52 +++++-----
>   src/conf/moment_conf.c                  |   6 +-
>   src/conf/object_event.c                 |   4 +-
>   src/conf/snapshot_conf.c                |   4 +-
>   src/conf/virsecretobj.c                 |   6 +-
>   src/conf/virstorageobj.c                |   4 +-
>   src/datatypes.c                         |  34 +++----
>   src/hypervisor/virhostdev.c             |   2 +-
>   src/interface/interface_backend_netcf.c |   2 +-
>   src/libxl/libxl_conf.c                  |  20 ++--
>   src/libxl/libxl_migration.c             |   2 +-
>   src/logging/log_handler.c               |   2 +-
>   src/qemu/qemu_agent.c                   |   2 +-
>   src/qemu/qemu_capabilities.c            |  10 +-
>   src/qemu/qemu_conf.c                    | 122 ++++++++++++------------
>   src/qemu/qemu_domain.c                  |  10 +-
>   src/qemu/qemu_monitor.c                 |   4 +-
>   src/rpc/virnetclient.c                  |   4 +-
>   src/rpc/virnetdaemon.c                  |   6 +-
>   src/rpc/virnetlibsshsession.c           |  37 +++----
>   src/rpc/virnetsaslcontext.c             |   2 +-
>   src/rpc/virnetserver.c                  |   8 +-
>   src/rpc/virnetserverservice.c           |   2 +-
>   src/rpc/virnetsocket.c                  |   6 +-
>   src/rpc/virnetsshsession.c              |   8 +-
>   src/rpc/virnettlscontext.c              |   6 +-
>   src/security/security_manager.c         |   2 +-
>   src/util/virdnsmasq.c                   |   2 +-
>   src/util/virfilecache.c                 |   4 +-
>   src/util/virmdev.c                      |   2 +-
>   src/util/virnvme.c                      |   2 +-
>   src/util/virpci.c                       |   2 +-
>   src/util/virresctrl.c                   |  40 ++++----
>   src/util/virscsi.c                      |   2 +-
>   src/util/virscsivhost.c                 |   2 +-
>   src/util/virusb.c                       |   2 +-
>   tests/virfilecachetest.c                |   2 +-
>   43 files changed, 235 insertions(+), 251 deletions(-)
>