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
