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-connection-call-sync > > Reported-by: Cole Robinson <crobinso@xxxxxxxxxx> > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> > --- > 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
Attachment:
signature.asc
Description: PGP signature
