[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 #25 from Xavier Claessens <xclaesse at gmail.com> ---
(In reply to comment #24)
> 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?

I reviewed smcv/gdbus at commit 902e566ba9f566c8015d6d9aebc54cee2dceaf6b

> (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

Ok.

> >  - 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

Right.

> >  - "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

I didn't know we were using GSList in other places. Opening a bug is good
enough for now.

> >    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

Ok

> >    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

Actually you commented it well already, maybe I'm just not familiar enough with
compare-and-exchange syntax because I don't do threading a lot. It just felt
uselesly complex to me because we are miles away at being thread safe anyway.
But I'm ok to keep it like that if you feel it's valuable :)

> >  - "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?

Good enough. In GNOME world you would use GApplication and get the
GDBusConnection from there.

> >  - "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

Oh right, previous code was notifying about initial state as well. I'm fine
then.

> > 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

Ok, please internalize if possible, otherwise we'll deprecated it post-1.0.

> >  - "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?

Ok, fine while we are still depending on libdbus anyway.

> > 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.

fair enough.

> I don't want to put removal of this function on the critical path :-)
> [ACTION xclaesse] is that OK?

sure.

> >  - "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?).

Why would valgrind close the connection? I don't understand why it would affect
it. Note that g_test_dbus_down() will disable exit-on-close before stopping the
test dbus-daemon.

> 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

Thinking about it, if dbus-daemon crash the CM probably want to first close the
server connection before leaving. I don't know if our CMs does that, but could
be safer to reinstate that, yes.

> >  - "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?

Ok if it's only private usage. Would be nice to have gio-like _sync() methods
instead of spinning our mainloop, but it doesn't matter for our unit tests I
guess.


Please open bugs for all the "later" points, or keep this bug open to handle
them, but I'm fine with merging that branch as-is already. Good job!

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