On 2/4/21 12:57 AM, 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-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(-)
All patches:
Reviewed-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>
