[Bug 28782] Migrate from dbus-glib to glib's GDBus

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=28782

--- Comment #24 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
So I know: what's the last commit you reviewed? Did you get as far as 03a5f5d4
"TpProxy: strip detailed error name..." on the gdbus2 branch, which is my
latest?

(In reply to comment #23)
>  - You can remove tp_proxy_get_interface_by_id() completely, it's not used
> any more.

Yeah, I did think about that, but it isn't critical-path. I want to get the
whole stack ported first.

[ACTION smcv] Do this later

>  - tp_svc_interface_set_dbus_interface_info(): doc says there is a vtable
> arg, but there isn't.

Fixed in af9d506f, I think.

[ACTION xclaesse] Please confirm

>  - "tp_dbus_daemon_register_object: rewrite in terms of
> TpSvcInterfaceSkeleton":
>    1) I think GSList's only purpose is making your app crash if you use
> g_list_free on it. I see no other reason anyone will ever use it. We are not
> going to export millions of objects, so a GList is perfectly good.

Fair enough, we can use a GSList (but whole-stack porting > this).

[ACTION smcv] Do this later

>    2) I would add a comment in tp_dbus_daemon_unregister_object() telling
> that setting the qdata to NULL will call tp_dbus_daemon_registration_free()
> that unexport stuff.

Reasonable.

[ACTION smcv] Do this later

>    3) g_object_replace_qdata() seems complex, had to read that if condition
> 3 times. I would just have classic r=g_object_get_qdata(); if(r!=NULL){...;
> return;} g_object_set_qdata(); We are not threadsafe so atomic operation
> isn't needed and I don't think the perf gain it worth it.

Now that we're setting fire to dbus-glib, there is at least the possibility
that we can become thread-safe in future.

I understand your clarity concerns, though. I could go either way.

Would a better comment help?

[ACTION smcv] Write a better comment, or split it, later

>  - "tp_dbus_daemon_dup: rewrite in terms of GDBus, and use session bus":
> dbus_g_bus_get() was doing blocking operation as well? Probably not a big
> deal, but I'm wondering what would be our API if we want to be fully async.

Yes, the underlying dbus_connection_open() blocks, because writing async API in
libdbus (without even GMainContext, let alone something like GAsyncResult) is
pretty horrible.

Note that g_bus_get_sync() returns immediately if you already have the relevant
singleton, so one easy API for fully-async is:

    g_bus_get (...);

    ...later, in the callback...

    g_bus_get_finish (...);
    if (error)
      {
        ... handle it ...;
        return;
      }
    my_dbus_daemon = tp_dbus_daemon_dup (NULL); /* can't fail now */

When we've swapped the hierarchy around so that TpClientFactory is at the top,
we could add a tp_client_factory_get_async(), if you really want to.

[ACTION xclaesse] Will this do for now?

>  - "tp_dbus_daemon_watch_name_owner: rewrite in terms of
> g_bus_watch_name_on_conn...": I think it does have a behaviour difference:
> g_dbus_watch_name() will call vanished or appeared for the initial status,
> not only when it changes later.

So does the old tp_dbus_daemon_watch_name_owner() - they are both implemented
as a combination of GetNameOwner() and NameOwnerChanged. I removed the explicit
call to GetNameOwner(), relying on the implicit call done by
g_dbus_watch_name().

[ACTION xclaesse] Please re-read and confirm that it's OK

> Also you add in the doc "New code should use
> g_bus_watch_name() or similar instead." that sounds like deprecated
> function, do you consider 1.0-blocker to actually remove that function?

No, I don't think I want to put "remove all uses of this function" on the to-do
list (and that's why I didn't formally deprecate it). We can have a closer look
at how much it's called after I get all the CMs, MC, Empathy and Folks ported,
but for now, my plan is "deprecate in 1.2, don't remove in the forseeable
future".

One possibility would be to internalize it, if MC turns out to be the only
caller.

[ACTION xclaesse] Is my plan OK?

[ACTION smcv] Later, do a git grep for it to assess feasibility of making it
private

>  - "tp_dbus_daemon_request_name: rewrite using GDBus": Hm, it still uses
> dbus-glib's enum for the reply.

Actually libdbus' enum. I don't consider this to be a big problem: the numbers
are fixed by the D-Bus Specification, and if/when we finally get rid of
libdbus, we can just have our own #define.

[ACTION xclaesse] is that OK?

> Maybe we want to write our own if GDBus does
> not have helper for that. Maybe open a glib bug to add the helper?

It blocks. I don't think GDBus wants that. It's also really quite simple.

I don't want to put removal of this function on the critical path :-)

[ACTION xclaesse] is that OK?

>  - "tp_run_connection_manager: use GDBus to watch for disconnection": commit
> msg has a FIXME. You didn't add the g_dbus_connection_set_exit_on_close()
> and I think it's fine for the CM-side.

Thanks for the reminder. I'm vaguely inclined to reinstate turning off
exit-on-close to preserve existing behaviour, and so CMs can be valgrinded
(valground?).

On one hand I want to get a minimum viable port of the whole stack working
first; on the other hand, getting closer to how it used to work is likely to
make that go quicker.

[ACTION smcv] reinstate existing behaviour

>  - "TpProxy: rewrite async and reentrant calls": We still have reentrant
> calls in 1.0?

We have them in regression tests. They're private.

>  I would consider blocker to remove them, or hand-write the few
> that we really want and drop the codegen for them, using
> g_dbus_connection_call_sync() probably?

They're only in regression tests, where there are are a lot of them. I don't
want things that don't affect our API to be on the critical path.

[ACTION xclaesse] Is that OK?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.