10:40 p.m.
Like most other things I do, this started as a distraction from
something else. I noticed that a function that took a virBufferPtr as
an argument was clearing that buffer on error, but most (but *not
all*) of its callers (which were the creators/owners of said
virBuffer) were already clearing the buffer object on error
anyway. Here's the original patch resulting from my seeing this in one
place:
https://www.redhat.com/archives/libvir-list/2020-June/msg01163.html
In the example I saw, eliminating the extra clearing of the virBuffer
in the subordinate function would simplify the error-cleanup code for
that function, but it turned out the calling function *wasn't*
clearing the virBuffer when an error occurred.
Looking further, I saw this same pattern occurred in other places in
the code - a function would create a new buffer (with "virBuffer buf =
VIR_BUFFER_INITIALIZER;"), and clear/free that buffer when it was
finished *unless there was an error*, in which case some functions
would properly clear the buffer, and some would just return, I guess
assuming the caller that generated the error had cleared the buffer.
This not only makes the error cleanup logic in subordinate functions
messier, it seems philosophically wrong to me, it also sounds like a
memory leak just waiting to happen.
So I decided that the way it should more properly work is this:
1) all virBuffers should be declared with g_auto(virBuffer), so that
they are automatically cleared (with virBufferFreeAndReset()) when
that toplevel function declaring/initializing the buffer returns to
its caller.
2) subordinate functions called with the virBuffer object as an arg
should just leave the buffer in whatever state it was when the error
occurred. Since those functions don't use the virBuffer after the
error happens, and the caller doesn't look at anything in the
virBuffer to determine an error has occurred, this is completely safe.
(obvious exceptions to (2) are of course those functions whose main
intent is in fact to consume the virBuffer,
e.g. virBufferContentAndReset(), and virCommandAddArgBuffer())
Patches 01 - 15 handle part (1) - *all* declarations of virBuffer as
an automatic are changed to g_auto(virBuffer), and any
virBufferFreeAndReset() calls in those same functions are removed.
(I have it split into so many patches because virBuffer is used all
over the place, and I figured it would make it easier to backport just
one part of the entire set if needed when backported some unrelated
bugfix that only touched one of the many directories represented
here. I would happily squash together any group of patches anyone
wants, however).
Patches 16 and 17 fix a couple "one-off" anomolies in the code related
to virBuffers.
Patch 18 then takes care of point (2) above, by removing any
extraneous virBufferFreeAndReset() calls in subordinate functions that
are no longer necessary due to the guarantee that the toplevel will
cleanup after error.
Patches 19-30 just eliminate any labels (and associated "ret"
variables and "goto's) that have been rendered pointless by removal of
virBufferFreeAndReset().
Finally Patches 31 and 32 convert all usages of virFirewall to use
g_autoptr(). They are included in this same set because virFirewall
objects are often declared right next to virBuffer objects, so doing
those patches in a different order would have caused merge conflicts..
NB: In many of the cases where "virBuffer" was changed to
"g_auto(virBuffer)", it doesn't actually eliminate any code from the
function, due to the function *always* calling
virBufferContentAndReset() anyway. I considered not changing those
cases, but in the ended decided it was better to add g_auto() even in
those cases for two reasons:
1) consistency makes verification easier, and means someone a year
from now won't come up with the same idea and waste time verifying all
those cases of virBuffer don't need g_auto().
2) if those functions ever change to have an alternate return that
doesn't explicitly call virBufferContentAndReset(), they will still be
safe.
3) the extra overhead is very minimal; a small price to pay for (1)
and (2)
NB2: these patches aren't just academic code churning; they have fixed
some actual (well, theoretically actual) memory leaks, I just haven't
taken the time to try and track all of them down or document them,
because they only occur in error cases which will likely never
happen. One example from my notes:
virStoragePoolSaveState calls
virStoragePoolDefFormatBuf which calls
virStoragePoolSourceFormat, which errors, returns
virStoragePoolDefFormatBuf, returns
virStoragePoolSaveState returns without freeing virBuffer
Laine Stump (32):
bhyve: use g_auto() for all virBuffers
esx: use g_auto() for all virBuffers
hyperv: use g_auto() for all virBuffers
libxl: use g_auto() for all virBuffers
lxc: use g_auto() for all virBuffers
qemu: use g_auto() for all virBuffers
tests: use g_auto for all virBuffers
tools: use g_auto() for all virBuffers
conf: use g_auto() for all virBuffers
util: use g_auto() for all virBuffers
cpu: use g_auto() for all virBuffers
rpc: use g_auto() for all virBuffers
nwfilter: use g_auto() for all virBuffers
network: use g_auto() for all virBuffers
use g_auto() for all remaining non-g_auto() virBuffers
qemu: remove unnecessary virBufferFreeAndReset() after
virCommandAddArgBuffer()
conf: consistently check for error when calling virSysinfoFormat()
remove redundant calls to virBufferFreeAndReset()
libxml: eliminate extra copy of string
bhyve: eliminate unnecessary labels
conf: eliminate unnecessary labels
libxl: eliminate unnecessary labels
util: eliminate unnecessary labels
tests: eliminate unnecessary labels
tools: eliminate unnecessary labels
network: eliminate unnecessary labels
lxc: eliminate unnecessary labels
esx: eliminate unnecessary labels
nwfilter: eliminate unnecessary labels
storage: eliminate unnecessary labels
use g_autoptr() for all usages of virFirewallNew/Free
eliminate unnecessary labels and ret variables
src/bhyve/bhyve_command.c | 40 +++---
src/bhyve/bhyve_driver.c | 4 +-
src/conf/capabilities.c | 13 +-
src/conf/checkpoint_conf.c | 11 +-
src/conf/cpu_conf.c | 27 ++--
src/conf/domain_addr.c | 2 +-
src/conf/domain_capabilities.c | 2 +-
src/conf/domain_conf.c | 105 +++++++-------
src/conf/interface_conf.c | 7 +-
src/conf/network_conf.c | 8 +-
src/conf/node_device_conf.c | 2 +-
src/conf/nwfilter_conf.c | 12 +-
src/conf/secret_conf.c | 8 +-
src/conf/snapshot_conf.c | 14 +-
src/conf/storage_capabilities.c | 6 +-
src/conf/storage_conf.c | 28 ++--
src/conf/virnetworkobj.c | 10 +-
src/conf/virnetworkportdef.c | 6 +-
src/conf/virnwfilterbindingdef.c | 6 +-
src/conf/virnwfilterbindingobj.c | 6 +-
src/conf/virsavecookie.c | 8 +-
src/cpu/cpu_map.c | 2 +-
src/cpu/cpu_x86.c | 6 +-
src/esx/esx_driver.c | 19 +--
src/esx/esx_util.c | 4 +-
src/esx/esx_vi.c | 28 ++--
src/esx/esx_vi_methods.c | 6 +-
src/hyperv/hyperv_driver.c | 34 +++--
src/hyperv/hyperv_wmi.c | 11 +-
src/hypervisor/domain_driver.c | 7 +-
src/libxl/libxl_conf.c | 19 +--
src/libxl/libxl_driver.c | 2 +-
src/libxl/libxl_migration.c | 2 +-
src/libxl/xen_common.c | 28 ++--
src/libxl/xen_xl.c | 45 ++----
src/libxl/xen_xm.c | 10 +-
src/locking/lock_driver_sanlock.c | 2 +-
src/lxc/lxc_container.c | 4 +-
src/lxc/lxc_controller.c | 12 +-
src/lxc/lxc_driver.c | 2 +-
src/lxc/lxc_fuse.c | 3 +-
src/network/bridge_driver.c | 25 ++--
src/network/bridge_driver_linux.c | 36 ++---
src/node_device/node_device_udev.c | 2 +-
src/nwfilter/nwfilter_ebiptables_driver.c | 160 +++++++++-------------
src/nwfilter/nwfilter_gentech_driver.c | 6 +-
src/nwfilter/nwfilter_learnipaddr.c | 2 +-
src/openvz/openvz_driver.c | 5 +-
src/qemu/qemu_agent.c | 2 +-
src/qemu/qemu_block.c | 2 +-
src/qemu/qemu_capabilities.c | 2 +-
src/qemu/qemu_command.c | 5 +-
src/qemu/qemu_dbus.c | 2 +-
src/qemu/qemu_domain.c | 4 +-
src/qemu/qemu_driver.c | 5 +-
src/qemu/qemu_migration.c | 2 +-
src/qemu/qemu_migration_cookie.c | 6 +-
src/qemu/qemu_monitor.c | 2 +-
src/rpc/virnetclient.c | 4 +-
src/rpc/virnetlibsshsession.c | 7 +-
src/rpc/virnetsocket.c | 2 +-
src/rpc/virnetsshsession.c | 2 +-
src/security/virt-aa-helper.c | 4 +-
src/storage/storage_backend_rbd.c | 7 +-
src/storage/storage_util.c | 14 +-
src/util/virbitmap.c | 4 +-
src/util/vircommand.c | 3 +-
src/util/virconf.c | 5 +-
src/util/virdnsmasq.c | 6 +-
src/util/virebtables.c | 24 +---
src/util/virfile.c | 2 +-
src/util/virfilecache.c | 2 +-
src/util/virfirewall.c | 2 +-
src/util/viriptables.c | 14 +-
src/util/virlog.c | 5 +-
src/util/virnetdevip.c | 3 +-
src/util/virpidfile.c | 2 +-
src/util/virqemu.c | 11 +-
src/util/virresctrl.c | 10 +-
src/util/virsocketaddr.c | 25 ++--
src/util/virstoragefile.c | 2 +-
src/util/virstring.c | 4 +-
src/util/virsysinfo.c | 5 +-
src/util/virsystemd.c | 4 +-
src/util/viruri.c | 2 +-
src/util/virxml.c | 12 +-
src/vmx/vmx.c | 5 +-
src/vz/vz_driver.c | 4 +-
tests/commandtest.c | 3 +-
tests/cputest.c | 2 +-
tests/networkxml2firewalltest.c | 3 +-
tests/nodedevmdevctltest.c | 6 +-
tests/nwfilterebiptablestest.c | 21 +--
tests/nwfilterxml2firewalltest.c | 3 +-
tests/qemublocktest.c | 2 +-
tests/qemucommandutiltest.c | 2 +-
tests/qemumigparamstest.c | 6 +-
tests/qemumonitorjsontest.c | 9 +-
tests/qemumonitortestutils.c | 2 +-
tests/testutils.c | 2 +-
tests/vboxsnapshotxmltest.c | 3 +-
tests/virbuftest.c | 58 ++++----
tests/vircgrouptest.c | 3 +-
tests/virfirewalltest.c | 80 +++--------
tests/virhostcputest.c | 3 +-
tests/virkmodtest.c | 4 +-
tests/virnetdevbandwidthtest.c | 3 +-
tools/virsh-checkpoint.c | 3 +-
tools/virsh-domain-monitor.c | 3 +-
tools/virsh-domain.c | 58 ++++----
tools/virsh-pool.c | 19 ++-
tools/virsh-secret.c | 2 +-
tools/virsh-snapshot.c | 3 +-
tools/virsh-volume.c | 3 +-
tools/vsh-table.c | 2 +-
tools/vsh.c | 15 +-
116 files changed, 505 insertions(+), 853 deletions(-)
--
2.25.4