Re: [PATCH 1/5] lib: Don't unref message passed to virGDBusCallMethod{WithFD}()
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-connec...
Reported-by: Cole Robinson <crobinso(a)redhat.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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
Attachments:
- signature.asc (application/pgp-signature — 833 bytes)