SELF NACK (Re: [libvirt PATCH 00/21] eliminate VIR_FREE in all *Free() functions)

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

 



Sigh.

Self-NACK to this entire series. I accidentally took the starting commit ID for the starting patch in the git send-email command from a stale instance of gitk that was sitting on my desktop, so these are *not* the final versions or ordering of the patches :-(

I'm now sending the correct versions as "PATCH v2".


On 2/3/21 10:28 PM, Laine Stump wrote:
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, a *Free() function actually appears to be used as a
     *Clear() function, 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.

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.

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

Laine Stump (21):
   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
   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            | 10 +--
  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, 541 insertions(+), 541 deletions(-)





[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