[libvirt PATCH v2 00/24] eliminate VIR_FREE in all *Free() functions

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

 



When I sent a patch to convert (what I thought was) all of the
VIR_FREEs in any *Free() function in the conf directory to g_free
(with the justification that it was trivial to verify that the
pointers weren't being referenced after they had been freed), there
were two responses that stuck in my mind:

1) Daniel said that this was a reasonable class of uses to change and
a reasonable way to arrive at the desired end result.

2) Peter justifiably expressing concern over the amount of churn in
the code, and also said that he'd rather not have things "halfway" for
an indeterminate time.

The combination of those two comments (ignoring the part about
"concern for churn" :-) let me to sit down last night and make this
patch series that eliminates all uses of VIR_FREE from any function
whose name ends in "Free" (or eliminate/rename the function, just for
completeness' sake)

In the vast majority of cases, this was done by replacing the
VIR_FREEs in the functions with g_free (while verifying that
everything being g_freed was actually something pointed to by the
parent object that was sent to the *Free() function, and that the
parent object itself was subsequently freed).

But there were a a couple of cases where this wasn't quite the right
thing to do:

  * in patch 20, two functions ending with Free() don't actually free
    the pointer they're sent. They instead behave like a *Clear()
    function, VIR_FREEing their components. Since we can see that
    these functions aren't actually be used as *Clear() functions (and
    never will be used that way), but don't want to promote confusion
    by leaving them misnamed, the patch renames them to *FreeContent()
    (more accurate name) and changes their VIR_FREEs to g_frees (it
    takes more than just looking at one function, but it's very easy
    to verify that the pointers are never referenced after they're
    freed).

  * In Patch 21-23, several *Free() functions were actually being
    passed a full copy of an object, and then VIR_FREEing all the
    pointers in the copied object. So the name was misleading, the
    function was inefficiently copying entire objects, and was
    unnecessarily NULLing the pointers that were then tossed out when
    the function returned. So I renamed the functions (21), changed
    the calling sequences to use pointers instead of copied objects
    (22), and (of course) changed the VIR_FREEs into g_free (23).


  * in Patch 24, a *Free() function actually appears to be used as a
    Clear() function. So I left the VIR_FREEs in place, but renamed
    the function to *Clear(). However, its one use as a *Clear() is
    when being called from a "Reset" function that (as far as I can
    see) is never used. If we could get rid of that Reset function, I
    could just fold the *Clear() function into its one remaining
    caller, and eliminate the VIR_FREEs. If someone knows more about
    the function virNetSSHSessionAuthReset() (which is in a section of
    the file labeled "Public API") please enlighten me.

**

The last time I ran wc over the diffs of these patches, it showed they
are eliminating 542 uses of VIR_FREE, which is > 13% of the 4k
remaining.


There is a companion series that eliminates VIR_FREE from all
*Dispose() functions, but I'm sending it separately since the two
series are completely independent, and I didn't want to scare anyone
off. (Really, most of these can be very mechanically reviewed - just
verify that all the items being g_freed are subordinates of the main
argument to the function, and that that object is freed before return)


Laine Stump (24):
  conf: replace remaining straggler VIR_FREE with g_free in vir*Free()
  util: replace VIR_FREE with g_free in all vir*Free() functions
  bhyve: replace VIR_FREE with g_free in all vir*Free() functions
  libxl: replace VIR_FREE with g_free in all vir*Free() functions
  qemu: replace VIR_FREE with g_free in all vir*Free() functions
  test_driver: replace VIR_FREE with g_free in all vir*Free() functions
  vbox: replace VIR_FREE with g_free in all vir*Free() functions
  vmx: replace VIR_FREE with g_free in all vir*Free() functions
  vz: replace VIR_FREE with g_free in all vir*Free() functions
  admin: replace VIR_FREE with g_free in all vir*Free() functions
  locking: replace VIR_FREE with g_free in all vir*Free() functions
  logging: replace VIR_FREE with g_free in all vir*Free() functions
  remote: replace VIR_FREE with g_free in all vir*Free() functions
  rpc: replace VIR_FREE with g_free in all vir*Free() functions
  security: replace VIR_FREE with g_free in all vir*Free() functions
  tools: replace VIR_FREE with g_free in all vir*Free() functions
  tests: replace VIR_FREE with g_free in all vir*Free() functions
  storage: replace VIR_FREE with g_free in all vir*Free() functions
  libvirtd: replace straggler VIR_FREE with g_free in all vir*Free()
    functions
  util: rename two *Free() functions while changing VIR_FREE to g_free
  qemu: rename virFirmware*Free() functions to have more accurate names
  qemu: pass pointers instead of copying objects for
    qemuFirmware*FreeContent()
  qemu: replace VIR_FREE with g_free in qemuFirmware*FreeContent()
  rpc: rename virNetSessionAuthMethodsFree to
    virNetSessionAuthMethodsClear

 src/admin/admin_server_dispatch.c   |  2 +-
 src/bhyve/bhyve_conf.c              |  6 +-
 src/bhyve/bhyve_domain.c            |  2 +-
 src/conf/domain_conf.c              |  6 +-
 src/conf/numa_conf.c                | 10 +--
 src/conf/storage_encryption_conf.c  |  2 +-
 src/conf/virchrdev.c                |  6 +-
 src/libvirt-domain.c                | 18 +++---
 src/libvirt-network.c               | 14 ++---
 src/libxl/libxl_domain.c            |  2 +-
 src/libxl/libxl_driver.c            |  2 +-
 src/libxl/libxl_migration.c         |  6 +-
 src/locking/lock_daemon.c           |  6 +-
 src/locking/lock_daemon_config.c    |  6 +-
 src/locking/lock_driver_lockd.c     | 10 +--
 src/locking/lock_driver_sanlock.c   |  6 +-
 src/locking/lock_manager.c          |  2 +-
 src/logging/log_daemon.c            |  4 +-
 src/logging/log_daemon_config.c     |  6 +-
 src/logging/log_handler.c           |  6 +-
 src/logging/log_manager.c           |  2 +-
 src/qemu/qemu_block.c               | 36 +++++------
 src/qemu/qemu_capabilities.c        |  8 +--
 src/qemu/qemu_cgroup.c              |  4 +-
 src/qemu/qemu_conf.c                |  6 +-
 src/qemu/qemu_domain.c              | 14 ++---
 src/qemu/qemu_firmware.c            | 42 ++++++-------
 src/qemu/qemu_migration_params.c    |  4 +-
 src/qemu/qemu_monitor.c             | 48 +++++++--------
 src/qemu/qemu_monitor_json.c        | 22 +++----
 src/qemu/qemu_process.c             | 24 ++++----
 src/qemu/qemu_saveimage.c           |  6 +-
 src/qemu/qemu_slirp.c               |  2 +-
 src/qemu/qemu_vhost_user.c          |  8 +--
 src/remote/remote_daemon_config.c   | 48 +++++++--------
 src/remote/remote_daemon_dispatch.c |  4 +-
 src/remote/remote_driver.c          |  2 +-
 src/rpc/virnetmessage.c             |  2 +-
 src/rpc/virnetsshsession.c          |  6 +-
 src/security/security_dac.c         |  8 +--
 src/security/security_selinux.c     | 10 +--
 src/storage/storage_backend_fs.c    |  6 +-
 src/storage/storage_backend_rbd.c   | 10 +--
 src/storage/storage_backend_scsi.c  |  4 +-
 src/storage/storage_driver.c        |  4 +-
 src/test/test_driver.c              |  6 +-
 src/util/virarptable.c              |  8 +--
 src/util/virauthconfig.c            |  4 +-
 src/util/virbitmap.c                |  4 +-
 src/util/vircgroup.c                | 12 ++--
 src/util/vircommand.c               | 24 ++++----
 src/util/virconf.c                  | 10 +--
 src/util/virdnsmasq.c               | 32 +++++-----
 src/util/virebtables.c              |  4 +-
 src/util/virfdstream.c              | 10 +--
 src/util/virfile.c                  |  4 +-
 src/util/virfirewall.c              | 16 ++---
 src/util/virfirmware.c              |  8 +--
 src/util/virjson.c                  | 12 ++--
 src/util/virlockspace.c             | 12 ++--
 src/util/virlog.c                   | 12 ++--
 src/util/virmacaddr.c               |  2 +-
 src/util/virmdev.c                  | 16 ++---
 src/util/virnetdev.c                | 12 ++--
 src/util/virnetdevbandwidth.c       |  6 +-
 src/util/virnetdevip.c              |  6 +-
 src/util/virnetdevmacvlan.c         |  8 +--
 src/util/virnetdevvlan.c            |  2 +-
 src/util/virnvme.c                  |  2 +-
 src/util/virobject.c                |  2 +-
 src/util/virpci.c                   | 18 +++---
 src/util/virperf.c                  |  2 +-
 src/util/virportallocator.c         |  4 +-
 src/util/virresctrl.c               |  6 +-
 src/util/virrotatingfile.c          | 14 ++---
 src/util/virscsi.c                  | 16 ++---
 src/util/virscsivhost.c             | 10 +--
 src/util/virseclabel.c              | 16 ++---
 src/util/virsocketaddr.c            |  2 +-
 src/util/virsysinfo.c               | 96 ++++++++++++++---------------
 src/util/virsystemd.c               |  6 +-
 src/util/virthreadpool.c            |  6 +-
 src/util/virtypedparam-public.c     |  2 +-
 src/util/virtypedparam.c            |  8 +--
 src/util/viruri.c                   | 20 +++---
 src/util/virusb.c                   |  8 +--
 src/util/virxml.c                   |  4 +-
 src/vbox/vbox_snapshot_conf.c       | 54 ++++++++--------
 src/vmx/vmx.c                       |  6 +-
 src/vz/vz_driver.c                  |  8 +--
 src/vz/vz_utils.c                   |  2 +-
 tests/nodedevmdevctltest.c          |  4 +-
 tests/nwfilterxml2firewalltest.c    |  2 +-
 tests/qemuhotplugtest.c             | 12 ++--
 tests/qemumonitortestutils.c        | 28 ++++-----
 tests/virnetdaemontest.c            |  2 +-
 tests/virnetserverclienttest.c      |  2 +-
 tools/virsh-checkpoint.c            |  6 +-
 tools/virsh-domain-monitor.c        |  4 +-
 tools/virsh-domain.c                |  2 +-
 tools/virsh-interface.c             |  4 +-
 tools/virsh-network.c               |  8 +--
 tools/virsh-nodedev.c               |  4 +-
 tools/virsh-nwfilter.c              |  8 +--
 tools/virsh-pool.c                  |  4 +-
 tools/virsh-secret.c                |  4 +-
 tools/virsh-snapshot.c              |  6 +-
 tools/virsh-volume.c                |  4 +-
 tools/vsh-table.c                   | 10 +--
 tools/vsh.c                         |  6 +-
 110 files changed, 557 insertions(+), 557 deletions(-)

-- 
2.29.2




[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