Re: glib dbus bindings notes

[Havoc Pennington <hp pobox com>]

Hi,

Finally, maybe it's useful to go more concrete looking at eggdbus and
mentioning some of the specific stuff in there. I know it's a lot of
comments but I'm just giving my opinion, take it for whatever it's
worth.

General stuff
===

* Maybe this should have some use of gobject-introspection. In
particular I think it might be nice if I have any introspectable
object, to be able to just stick it on the bus, and have incoming
calls mapped to it. Then I can just write a normal gobject, stick it
on the bus, and have it work, much of the time. In fact for
*implementing* objects I'm not sure there's much point in generating
anything statically beyond the g-i typelib ... a couple
gobject-introspection annotations could specify that some char* are
really object paths, or whatever.

* the code generator can probably generate smaller code; it's worth
factoring out pretty much everything except what actually varies
per-method into a generic function, to get the size on this generated
stuff down

* the library is missing what I called pieces 4 and 5 for non-C
bindings, because it tangles that functionality into object proxies
and thus the particular GType object mapping.

* of the files here, only parts of eggdbusconnection and
eggdbusbusnametracker are useful for other bindings. *everything* else
is the C object mapping, or pretty close to everything else. Of
course, bindings also need the dbus-glib main loop glue which is being
used here.

* despite all the comments the object mapping looks nice and that's
most of the code. Most of my comments are about the "generic" plumbing
parts.

eggdbusconnection
===

Like:

 * wrapping DBusConnection in GObject makes memory management less
annoying for bindings, more gobject-introspection friendly
 * exports unique name and bus type properties
 * maps pending call to GAsyncResult

Suggestions:

 * object proxy and interface registration is part of object mapping;
should really be split out
 * egg_dbus_connection_get_bus() is blocking rather than async; imo
apps should never get the bus anyway, they should always instead
either ask to acquire a name or ask to watch a name, and the bus
connection is dealt with behind scenes
 * because apps should not touch connection much if ever, that's all
the more reason to make it an API for use by bindings, and thus not
have object-mapping-specific stuff in it and not bother to try to hide
libdbus
 * egg_dbus_connection_send_message_with_reply_sync(): imo it's worth
strongly considering flat-out disallowing synchronous calls. Yes async
is painful in C. No you should not block in a GUI app or a daemon,
which means almost all apps. Remove the temptation!
 * EGG_DBUS_CALL_FLAGS_BLOCK_IN_MAINLOOP nooooooooooooooooooo!
Seriously, you have no idea how much time was wasted chasing
heisenbugs from people doing this with bonobo. You simply cannot write
a correct program of any size that uses this. The warning in the docs
is "be careful", but the problem is, it's almost impossible to be
careful enough to actually handle this in any real app. It should say
"this will crash your app." If people are going to be lazy and block,
they need to block, not use some mainloop hack that kinda-blocks.
 * egg_dbus_connection_register_interface() seems to register an
object with interfaces, not an interface, seems like it should be
called register_object

eggdbusnametracker
===

Like:

 * essential functionality, "piece 3"
 * clean gobject interface beats hippo-dbus-helper type of hackiness

Suggestions:

 * egg_dbus_bus_name_tracker_get_owner_for_bus_name blocks in
recursive main loop; nooooooooooooooo again.
 * aside from re-entrancy hell, any app using this
egg_dbus_bus_name_tracker_get_owner_for_bus_name will be broken
because it probably won't handle the name reappearing when the other
side restarts. For a _correct_ app this API is not convenient. It's
only convenient if your app is buggy and gets wedged when the other
end restarts.
 * API should be async by default; consider C convenience version
where you hide the tracker object, and developer can provide two
callbacks statically instead of having to alloc/get object, get
current state, connect to two signals, worry about freeing object.
 * egg_dbus_bus_name_tracker_get_known_well_known_bus_names_for_unique_bus_name
(longest function name ever?) this should be an internal API probably,
if it exists, but I don't think it should. I would recode the use of
this in eggdbusconnection such that at any given time a proxy is bound
only to a unique bus name. Proxies could all watch the well-known name
they are bound to, and just update the unique bus name they use when
it changes. Then just route signals to proxies currently bound to a
given unique bus name. This will be more efficient and less work.
 * because it's 1 global tracker instead of a separate tracker per
watch of a name, to watch a name someone has to first get the current
state, then set up the two signal connections. It's more convenient if
you can just say "I care about this name; here are my two callbacks.
Guarantee you will call 1 of them next time I hit main loop." If the
name  already has a known owner, then just queue an idle and invoke
the "name gained owner" callback in there.
 * might work out better to make this entire "track all names" object
private, and make the public API be lightweight objects allocated each
time someone wants to watch a specific name
 * an advantage of that approach is that signal handlers don't need a
strcmp(bus_name_owner, "NameICareAbout") in them, much more efficient
if name watchers only get a callback for the name they want to watch

eggdbuserror, eggdbushashmap, eggdbusmessage, eggdbusstructure, eggdbusvariant
===

This is the type-mapping stuff that's only useful for the particular C
object mapping. This is one of the big areas I think is pretty much
bloat for other language bindings. Aside from just amount of code,
eggdbusmessage imposes an extra copy on all data; JavaScript can't use
a g_malloc'd string or a GHashTable or anything, so they are created
and live for a millisecond then nuked, if you were to bind JavaScript
or Python or something to this. It's more appropriate IMO to go
straight from the DBusMessage into the native language type.

eggdbusobject/ifaceproxy
===

Like:

* maps props to GObject properties
* keeps proxy objects separate from implementation objects

Suggestions:

 * would love a thing like dbus_g_proxy_call that lets you skip the
introspection and static binding stuff
 * add a got-properties signal perhaps so it's at least more possible
to deal with properties asynchronously; consider an API that actively
encourages it, e.g. an async 'get proxy' call that returns the proxy
with properties loaded
 * I don't think proxies bound to a well-known name are very good.
They assume all methods are completely independent and remote objects
are stateless, or else they have races. I don't think many methods and
objects are independent and stateless. Pretty much *nothing* in the
NetworkManager dbus API is, to use an example I was just working with.
To use NetworkManager API correctly, you would need to regenerate all
proxies whenever the NM daemon reappeared on the bus. IMO if an app is
written nicely with good dbus binding support, this is just about as
easy as doing things wrong; you just have to make it so that in the
well-known-name-appeared callback you create your proxies, and in the
well-known-name-vanished callback you destroy them. Not really harder
than creating the proxies in a one-time init function. Yes apps can
connect to notify::name-owner, which is convenient for one singleton
root object, but I think kind of fails for NetworkManager where you
have a root object containing devices containing access points ... and
the same object path across restarts might not even be the same
logical object. So the well-known-name proxy here makes sense only for
the root singleton object, and all other proxies should be created
with unique name inside the notify::name-owner on the root singleton,
or something. Anyway, bottom line, to me a separate watch_name
operation plus unique-name proxies makes everything a lot more clear
and easier to keep correct.
 * I know sync method calls are convenient, but they are just 99% of
the time wrong.... not convinced it's doing anyone any good to let
them write code they are going to have to rewrite. Just saying.
 * I'm not sure it's strictly needed to allow object proxies to
represent multiple interfaces - for me, just having two proxies, 1 per
interface, would generally be fine. Or just have a proxy with no
interface in it and provide the interface at method call time.
Harmless but not sure it's useful / worth the effort.
 * comment says "TODO: Right now we block in the mainloop. This is
*probably* wrong." - yes it is wrong!


Well there is my couple hundred pennies.

Havoc