[libvirt PATCH v2 00/24] eliminate VIR_FREE in all *Free() functions
Daniel Henrique Barboza
danielhb413 at gmail.com
Thu Feb 4 19:31:48 UTC 2021
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 at gmail.com>
>
More information about the libvir-list
mailing list