[PATCH 1/5] lib: Don't unref message passed to virGDBusCallMethod{WithFD}()

[Pavel Hrdina]

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 at redhat.com>
> Signed-off-by: Michal Privoznik <mprivozn at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201002/762c20da/attachment-0001.sig>