On Fri, Oct 02, 2020 at 11:22:06AM +0200, Michal Privoznik wrote:
The virGDBusCallMethod() and virGDBusCallMethodWithFD() are simple wrappers over g_dbus_connection_call_sync() and g_dbus_connection_call_with_unix_fd_list_sync() respectively. The documentation to these function states that passed parameters (@message in our case) is consumed for 'convenient' inline use of g_variant_new() [1]. But that means we must not unref the message afterwards. To make it explicit that the message is consumed the signature of our wrappers is changed too.
1: https://developer.gnome.org/gio/stable/GDBusConnection.html#g-dbus-connectio...
Reported-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetdaemon.c | 4 ++-- src/util/virfirewalld.c | 16 ++++++++-------- src/util/virgdbus.c | 29 ++++++++++++++++++++--------- src/util/virgdbus.h | 4 ++-- src/util/virpolkit.c | 4 ++-- src/util/virsystemd.c | 23 ++++++++--------------- 6 files changed, 42 insertions(+), 38 deletions(-)
This doesn't look right, I know about the limitation and was counting with it when introducing virgdbus.c where we call g_variant_ref_sink() on the passed message. g_variant_new() returns floating reference which makes it possible to us it as described in the g_dbus_connection_call_sync() documentation but for our convenience I changed the behavior in virGDBusCallMethod() to make it a normal reference so it is not consumed. The motivation was to not introduce a new reference concept into libvirt code. I'll look into the issue but this should not happen. If you look into g_dbus_connection_call_sync() code in GLib they call g_variant_ref_sink() on the passed data as well and if they receive a normal reference they will simply add a new normal reference instead of consuming it. In addition the GLib g_dbus_connection_call_sync() should not be called from our tests because we mock this call. I'll look again into the issue to figure out what's wrong. For now NACK to this patch. Pavel