GNOME Bugzilla – Bug 579571
D-Bus library
Last modified: 2010-06-17 16:42:44 UTC
Hey, There's recently been a couple of threads on gtk-devel-list about getting proper D-Bus support at the GLib level. It seems that one conclusion is that it would be good to separate language specific object mappings and goo from generic low-level routines and also provide some useful helper classes that all languages can benefit from. Havoc wrote a good list of recommendations, see http://mail.gnome.org/archives/gtk-devel-list/2009-February/msg00121.html and surrounding messages. I've been working on implementing pieces one through five and will attach a patch with what I've got so far. One assumption with this effort is that libdbus-1 will be used and we will not try to hide this fact. The idea is that different object mappings for various languages can access the underlying DBusConnection directly to avoid paying a penalty for going through a C/GObject mapping. With the right changes to libdbus-1 (specifically the DBusConnection type), this also means that things like the proposed GVariant or other serialization APIs can be used to send D-Bus messages. When the low-level bits are done, I expect to work on a object mapping for C/GObject very similar to how EggDBus works.
Created attachment 132942 [details] [review] Initial stab This should solve most of the first three pieces 1. Main Loop Integration 2. Asynchronous DBusConnection 3. Asynchronous Name Ownership with a few caveats and TODO items. There are also test cases and docs for all this. http://people.freedesktop.org/~david/gdbus-first-stab/ The most important function in this patch is this one http://people.freedesktop.org/~david/gdbus-first-stab/GBusNameOwner.html#g-bus-name-owner-new since it makes it very simple to implement D-Bus service. $ diffstat 0001-Initial-commit-of-GDBus-work.patch Makefile.am | 2 configure.in | 16 docs/reference/Makefile.am | 2 docs/reference/gdbus/Makefile.am | 72 + docs/reference/gdbus/gdbus-docs.xml | 42 + docs/reference/gdbus/gdbus-sections.txt | 70 + docs/reference/gdbus/gdbus.types | 5 docs/reference/gdbus/version.xml.in | 1 gdbus-2.0-uninstalled.pc.in | 6 gdbus-2.0.pc.in | 11 gdbus/Makefile.am | 195 +++++ gdbus/abicheck.sh | 13 gdbus/example-gbusnameowner.c | 90 ++ gdbus/gbusnameowner.c | 1244 +++++++++++++++++++++++++++++ gdbus/gbusnameowner.h | 114 ++ gdbus/gdbus-marshal.list | 1 gdbus/gdbus.h | 37 gdbus/gdbus.symbols | 73 + gdbus/gdbusconnection.c | 964 ++++++++++++++++++++++++ gdbus/gdbusconnection.h | 116 ++ gdbus/gdbusenums.h | 225 +++++ gdbus/gdbusenumtypes.c.template | 42 + gdbus/gdbusenumtypes.h.template | 24 gdbus/gdbuserror.c | 507 +++++++++++++ gdbus/gdbuserror.h | 80 ++ gdbus/gdbusmainloop.c | 782 ++++++++++++++++++++ gdbus/gdbusmainloop.h | 45 + gdbus/gdbusprivate.c | 37 gdbus/gdbusprivate.h | 38 gdbus/gdbustypes.h | 39 + gdbus/makegdbusalias.pl | 137 +++ gdbus/pltcheck.sh | 19 gdbus/tests/Makefile.am | 30 gdbus/tests/connection.c | 905 +++++++++++++++++++++++ 34 files changed, 5980 insertions(+), 4 deletions(-)
Also, one thing that is perhaps missing from Havoc's list is mapping DBusError to a language / runtime specific error system (i.e. GError). I did this already in EggDBus so that's what in gdbuserror.[ch] in the patch in the previous comment. The idea here is twofold - We can get the D-Bus error name from the nick of the error code in an error domain enumeration (so it needs to be registered with GType) - Otherwise we can encode a GError in a special way so the receiver can recover the error domain name and error code. Obviously this only works for GDBus <-> GDBus apps but often that's the case. (This is how GVfs works) - Unmapped error codes map to G_DBUS_ERROR_REMOTE_EXCEPTION and we provide utility API g_dbus_error_get_remote_exception() to recover the D-Bus error name and error message. (This is similar to how dbus-glib works except that the include-nul-byte- in-the-error-message trick isn't used since it breaks when copying the error around cf. g_simple_async_result_set_from_error()) (Disclaimer: it might need some work to work properly; for example there's a nasty assumption that the type name for an error domain is similar to the error quark string. It's not super performant either but then again error conditions are supposed to be rare. This is all fixable.)
Btw, one obvious API omission is g_dbus_connection_new_for_address (const gchar *address, <other stuff>); that takes a D-Bus specific address string, as described in the D-Bus specification http://dbus.freedesktop.org/doc/dbus-specification.html#addresses The alternative to g_dbus_connection_new_for_address() is to attempt to provide deep integration with the upcoming socket classes in GIO as described in bug 515973 and here http://mail.gnome.org/archives/gtk-devel-list/2009-February/msg00067.html and this would also affect whether we'd need convenience API around DBusServer. It's not super important to resolve this as it's only needed for peer-to-peer connections and they are not used in a lot of places least of all typical desktop apps. Also, no one is preventing us from doing both assuming we can get the requisite changes into libdbus-1 e.g. a way to create a DBusConnection from a set of file descriptors or something.
Quick discussion of name owner flags (came up in email), the basic issue is that the flags on the dbus level are kind of low-level and people find them confusing. It's also possible that the "queue" thing is just not useful. My attempt to simplify this is based on two usage patterns describing whether to queue: 1) SINGLE_INSTANCE: you want to either own the name or not, and probably want to exit if you don't get it. Sets DO_NOT_QUEUE so you get back a failure if you fail to own. 2) MANY_INSTANCES: you can own the name, but don't care that much. For example, you are one of many apps that might provide the service. So you want to be queued as an owner but aren't particularly going to do anything different if you are an owner or not; if you become owner you'll just start getting messages via the well-known name. I've never really used MANY_INSTANCES mode. Other flags: REPLACE_EXISTING is an orthogonal thing, and I think should usually go with a --replace command line option and be off by default. ALLOW_REPLACEMENT I think should probably always be set. Reasons not to set it could include: app is broken and can't deal with losing name; some kind of security consideration on system bus (don't know specifically what it might be). Conclusion: it might be fine to set ALLOW_REPLACEMENT and DO_NOT_QUEUE by default (yay dbus for getting the defaults totally wrong!) and have the only flag be whether to REPLACE_EXISTING or not. More flags can always be added later.
OK, thanks for explaining the intent behind SINGLE_INSTANCE and MANY_INSTANCES - it sounds like we can use something like this to improve the API in GBusNameOwner, in particular it would be good to get rid of the GAsyncReadyCallback and just make the caller set up signal handlers. Btw, right now with GBusNameOwner, if you want SINGLE_INSTANCE you simply exit() in the GAsyncReadyCallback on failure. If you want MANY_INSTANCES, well, then you don't care too much and you would just pass NULL for the GAsyncReadyCallback and only handle ::name-acquired and ::name-lost. About queuing, this is implicit in how GBusNameOwner works. The request for the name is queued by default and if the user don't want that, he can just destroy the object (the finalizer will call ReleaseName()). So I don't think we need to express any flags for this. So maybe we want the GBusNameOwner API to look like this instead Flags G_BUS_NAME_OWNER_FLAGS_NONE, G_BUS_NAME_OWNER_FLAGS_REPLACE, G_BUS_NAME_OWNER_FLAGS_DO_NOT_ALLOW_REPLACEMENT, G_BUS_NAME_OWNER_FLAGS_EXIT_ON_LOSING_NAME, where G_BUS_NAME_OWNER_FLAGS_EXIT_ON_LOSING_NAME also means "exit with exit code 1 and a message on stderr if you can't acquire the name" (in addition to also exiting with exit code 0 and no error message when the name is lost). Maybe there's a better name than G_BUS_NAME_OWNER_FLAGS_EXIT_ON_LOSING_NAME but this is the intent anyway... Then the canonical example is like this for a single-instance app owner = g_bus_name_owner_new (G_MESSAGE_BUS_TYPE_SESSION, "org.foo.Name", G_BUS_NAME_OWNER_FLAGS_EXIT_ON_LOSING_NAME | (opt_replace ? G_BUS_NAME_OWNER_FLAGS_REPLACE : 0)); g_signal_connect (owner, "name-acquired", G_CALLBACK (on_name_acquired), data); g_main_loop_run (); and all the user has to do is to set up stuff in on_name_acquired(). If he loses the name then we call exit() for him. Bonus points if we can automatically (or easily) parse --replace for him ;-) For a many-instance apps it is equally simple owner = g_bus_name_owner_new (G_MESSAGE_BUS_TYPE_SESSION, "org.foo.Name2", G_BuS_NAME_OWNER_FLAGS_NONE); g_signal_connect (owner, "name-acquired", G_CALLBACK (on_name_acquired), data); g_signal_connect (owner, "name-lose", G_CALLBACK (on_name_lost), data); g_main_loop_run (); and the user can do his thing when acquiring and losing the name (as long as the owner object is alive, the app is queued to own the name and will own the name when it becomes available). Hmm...
(In reply to comment #5) > Maybe there's a better name than G_BUS_NAME_OWNER_FLAGS_EXIT_ON_LOSING_NAME but > this is the intent anyway... Duh, it was right there in front of me all the time! Maybe G_BUS_NAME_OWNER_FLAGS_SINGLE_INSTANCE is in fact a better name...
(In reply to comment #5) > Then the canonical example is like this for a single-instance app > > owner = g_bus_name_owner_new (G_MESSAGE_BUS_TYPE_SESSION, > "org.foo.Name", > G_BUS_NAME_OWNER_FLAGS_EXIT_ON_LOSING_NAME | > (opt_replace ? G_BUS_NAME_OWNER_FLAGS_REPLACE : > 0)); For a single instance background service, you might want that, but for a single-instance *application*, you generally need some amount of command-line forwarding magic too. So libunique wouldn't be able to use the proposed EXIT_ON_LOSING_NAME flag. Not sure how much of the use case that kills.
(In reply to comment #7) > (In reply to comment #5) > > Then the canonical example is like this for a single-instance app > > > > owner = g_bus_name_owner_new (G_MESSAGE_BUS_TYPE_SESSION, > > "org.foo.Name", > > G_BUS_NAME_OWNER_FLAGS_EXIT_ON_LOSING_NAME | > > (opt_replace ? G_BUS_NAME_OWNER_FLAGS_REPLACE : > > 0)); > > For a single instance background service, you might want that, but for a > single-instance *application*, you generally need some amount of command-line > forwarding magic too. So libunique wouldn't be able to use the proposed > EXIT_ON_LOSING_NAME flag. Not sure how much of the use case that kills. Good point, so this is an example of where you don't immediately want to exit when failing to acquire the name - e.g. you want to know who owns the name and then call a method on the name owner before you exit. So maybe the way it works right now is fine since you can do this in the GAsyncReadyCallback. Thoughts? (FWIW, an earlier version of GBusNameOwner I had a ::failed-to-acquire-name signal instead of the GAsyncReadyCallback stuff. But that's mostly a style issue, I don't know what's better.)
Hmm, I thought a bit more about it and read all the threads again. This message has a lot of good points http://mail.gnome.org/archives/gtk-devel-list/2009-February/msg00124.html In particular the points about *forcing* users to get it right is good. This reminded me this blog entry http://www.pointy-stick.com/blog/2008/01/09/api-design-rusty-levels/ which is always a good read when it comes to API design. So I think I'd like the API to be something like this void g_bus_name_watcher_new (const gchar *name, GBusNameWatcherCallback name_appeared_callback, GBusNameWatcherCallback name_vanished_callback, GCancellable *cancellable, gpointer user_data); where (I expect this information to go into the function docs) - The callback functions are bound to the signals - We _also_ invoke the callback functions when the initialization bit is done, e.g. when we have the result from GetNameOwner() Sure it's a little bit weird to have on_name_appeared() invoked since the name didn't really *appear* we only found out that it exists. But I think that's fine. - User is guaranteed that name_appeared_callback and name_vanished_callback will be alternating - We assert that both name_appeared_callback != NULL and name_vanished_callback != NULL - The user can use @cancellable to cancel construction of a watcher; this is nifty when using a GBusNameWatcher as a class instance variable, e.g. static void my_class_finalize (MyClass *instance) { g_cancellable_cancel (instance->init_cancellable); g_object_unref (instance->init_cancellable); g_object_unref (instance->watcher); .. } static void on_name_appeared (GBusNameWatcher *watcher, MyClass *instance) { if (instance->watcher != NULL) instance->watcher = watcher; // Do stuff } static void on_name_vanished (GBusNameWatcher *watcher, MyClass *instance) { if (instance->watcher != NULL) instance->watcher = watcher; // Do stuff } static void my_class_init (MyClass *instance) { instance->init_cancellable = g_cancellable_new (); g_bus_name_watcher_new ("org.blah.name", on_name_appeared, on_name_vanished, instance->init_cancellable, instance); } One caveat with this is that there's no error reporting. I don't think that's a problem. Also, I expect the object mappings to provide a full-blown proxies like http://people.freedesktop.org/~david/eggdbus-0.1-docs/eggdbus-eggdbusbus.html so if people are missing something they can always just do things manually. I'd expect we do something similar for GBusNameOwner and GDBusConnection. Thoughts?
Hmm... I tried thinking about what the actual code and docs would look like if we did what is suggested in comment 9. See below. It's really simple (easy to get right and impossible to get wrong) for global bus name watchers... but it's little icky for transient watchers (easy to get right but also easy to fuck up by e.g. forgetting to disconnect handlers, leaking watchers). I don't really know if this is the API we want, I think it is.. haven't actually changed the code, am just trying to get a sense of fine-tuning the API so feedback is appreciated. Code follows /** * g_bus_name_watcher_new: * @bus_type: A #GMessageBusType specifying what message bus to use for watching @name. * @name: A bus name (well-known or unique). * @cancellable: A #GCancellable or %NULL. * @bus_name_appeared_handler: Handler for when the name being watched * is known to have an owner. * @bus_name_vanished_handler: Handler for when the name being watched * loses its owner. * @user_data: User data to pass to @bus_name_appeared_handler and * @bus_name_vanished_handler. * * Asynchronously creates a new #GBusNameWatcher for watching @name on * the bus specified by @bus_type and calls @bus_name_appeared_handler * and @bus_name_vanished_handler when the name is known to have a * owner respectively known to lose its owner. You are guaranteed that * either of the handlers will be invoked (on the main thread) after * calling this function. * * This allows you to write code like this: * <programlisting> * static void * on_bus_name_vanished (GBusNameWatcher *watcher, * gpointer user_data) * { * /<!-- -->* destroy proxies already created, if any *<!-- -->/ * } * * static void * on_bus_name_appeared (GBusNameWatcher *watcher, * const gchar *name_owner, * gpointer user_data) * { * /<!-- -->* create proxies for name_owner *<!-- -->/ * } * * int * main (int argc, char *argv[]) * { * GMainLoop *loop; * * g_type_init (); * g_bus_name_watcher_new (G_MESSAGE_BUS_TYPE_SESSION, * "org.someproject.SomeName", * NULL, * on_bus_name_vanished, * on_bus_name_appeared, * NULL); * * loop = g_main_loop_new (NULL, FALSE); * g_main_loop_run (loop); * * return 0; * } * </programlisting> * to watch a name. * * Note that the returned object may be shared with other callers if * @bus_type and @name matches. As such you need to always disconnect * @bus_name_appeared_handler and @bus_name_vanished_handler when * giving up the reference you obtained from the first call to either * of these handlers. * * If @cancellable is not %NULL, then this function is a no-op if * @cancellable is already cancelled. If @cancellable is cancelled * after this function is invoked but before either of the handlers * are invoked for the first time, then the handlers will never be * invoked and no object will be returned. * * To use #GBusNameWatcher as a class instance variable, the code * would look like this: * <programlisting> * struct _FooBarPrivate * { * GBusNameWatcher *name_watcher; * GCancellable *init_cancellable; * }; * * static void * on_bus_name_vanished (GBusNameWatcher *watcher, * gpointer user_data) * { * FooBar *foobar = FOO_BAR (user_data); * * if (foobar->priv->name_watcher == NULL) * foobar->priv->name_watcher = watcher; * * /<!-- -->* destroy proxies already created, if any *<!-- -->/ * } * * static void * on_bus_name_appeared (GBusNameWatcher *watcher, * const gchar *name_owner, * gpointer user_data) * { * FooBar *foobar = FOO_BAR (user_data); * * if (foobar->priv->name_watcher == NULL) * foobar->priv->name_watcher = watcher; * * /<!-- -->* create proxies for name_owner *<!-- -->/ * } * * static void * foo_bar_finalize (GObject *object) * { * FooBar *foobar = FOO_BAR (object); * * g_cancellable_cancel (foobar->priv->init_cancellable); * g_object_unref (foobar->priv->init_cancellable); * if (foobar->priv->name_watcher != NULL) * { * g_signal_handlers_disconnect_by_func (foobar->priv->name_watcher, * on_bus_name_vanished, * foobar); * g_signal_handlers_disconnect_by_func (foobar->priv->name_watcher, * on_bus_name_appeared, * foobar); * g_object_unref (foobar->priv->name_watcher); * } * * if (G_OBJECT_CLASS (foo_bar_parent_class)->finalize != NULL) * G_OBJECT_CLASS (foo_bar_parent_class)->finalize (object); * } * * static void * foo_bar_constructed (GObject *object) * { * FooBar *foobar = FOO_BAR (object); * * foobar->priv->init_cancellable = g_cancellable_new (); * * g_bus_name_watcher_new (G_MESSAGE_BUS_TYPE_SESSION, * "org.someproject.SomeName", * NULL, * on_bus_name_vanished, * on_bus_name_appeared, * foobar); * * if (G_OBJECT_CLASS (foo_bar_parent_class)->constructed != NULL) * G_OBJECT_CLASS (foo_bar_parent_class)->constructed (object); * } * </programlisting> **/ void g_bus_name_watcher_new (GMessageBusType bus_type, const gchar *name, GCancellable *cancellable, GBusNameAppearedCallback bus_name_appeared_callback, GBusNameVanishedCallback bus_name_vanished_callback, gpointer user_data);
Here's a better and easier to read version of what I proposed in comment 10 http://people.freedesktop.org/~david/gdbus-second-stab/GBusNameWatcher.html#g-bus-name-watcher-new
it feels backwards to me to return the object via the implicitly connected signal handlers. why not just return the object from g_bus_name_watcher_new ?
At least, the function cannot be called _new() if it doesn't return an object. Maybe g_bus_start_watching_name (...) ? If going that route, I'd prefer to keep the object totally out of the api, and maybe add a g_bus_stop_watching_name (name, appeared_cb, vanished_cb, data) that does the object lookup, signal disconnection, and unref.
Hey Matthias, Thanks for your feedback. (In reply to comment #12) > it feels backwards to me to return the object via the implicitly connected > signal handlers. why not just return the object from g_bus_name_watcher_new ? We don't return the object simply because it's not ready yet (it's an async operation).. if we returned the object from _new() it's like asking the user to use the object. We don't want to make it easy to misuse the API so that's why I it's like that... (In reply to comment #13) > At least, the function cannot be called _new() if it doesn't return an object. (Will it break language bindings if it's called _new() and doesn't return an object?) FWIW, it does return an object, it's just non-standard how it's returned. The only difference is that it's async, not sync.. we could call it new_async() but I don't think that buys us anything (and we won't have sync versions of this API either). We could also call it _get() instead of _new() but that sounds wrong since _get() normally refers to something well-known (e.g. "get the session bus", "get the volume monitor", "get the singleton app settings object"). > Maybe g_bus_start_watching_name (...) ? > If going that route, I'd prefer to keep the object totally out of the api, and > maybe add a g_bus_stop_watching_name (name, appeared_cb, vanished_cb, data) > that does the object lookup, signal disconnection, and unref. It's nice to have the object at hand so you can do g_bus_name_watcher_get_name_owner(); // returns unique name or NULL if no owner g_bus_name_watcher_get_connection(); which you want to use for object mapping stuff. And one point about this stuff is that you don't need to create a connection object when using it; e.g. it makes the code very simple cf. http://people.freedesktop.org/~david/gdbus-second-stab/GBusNameWatcher.html#gbusnamewatcher-from-main Of course we could have lookup functions for getting these things.. but wouldn't that be backwards? It's true this API is more non-standard (see the initial stab for a more standard API) but I think that's OK because it's a lot easier to use and a lot harder to misuse.... But I'm totally open for suggestions apart from that ;-)
Matthias, did you mean API like this instead? void g_bus_name_owner_start_owning (GMessageBusType bus_type, const gchar *name, GBusNameOwnerFlags flags, GBusNameAcquiredCallback name_acquired_handler, GBusNameLostCallback name_lost_handler, gpointer user_data); void g_bus_name_owner_stop_owning (GMessageBusType bus_type, const gchar *name, GBusNameAcquiredCallback name_acquired_handler, GBusNameLostCallback name_lost_handler, gpointer user_data); GDBusConnection * g_bus_name_owner_get_connection (GMessageBusType bus_type, const gchar *name, GBusNameAcquiredCallback name_acquired_handler, GBusNameLostCallback name_lost_handler, gpointer user_data); gboolean g_bus_name_owner_get_owns_name (GMessageBusType bus_type, const gchar *name, GBusNameAcquiredCallback name_acquired_handler, GBusNameLostCallback name_lost_handler, gpointer user_data); void g_bus_name_watcher_start_watching (GMessageBusType bus_type, const gchar *name, GBusNameAppearedCallback name_appeared_handler, GBusNameVanishedCallback name_vanished_handler, gpointer user_data); void g_bus_name_watcher_stop_watching (GMessageBusType bus_type, const gchar *name, GBusNameAppearedCallback name_appeared_handler, GBusNameVanishedCallback name_vanished_handler, gpointer user_data); GDBusConnection * g_bus_name_watcher_get_connection (GMessageBusType bus_type, const gchar *name, GBusNameAppearedCallback name_appeared_handler, GBusNameVanishedCallback name_vanished_handler, gpointer user_data); gboolean g_bus_name_watcher_get_owns_name (GMessageBusType bus_type, const gchar *name, GBusNameAppearedCallback name_appeared_handler, GBusNameVanishedCallback name_vanished_handler, gpointer user_data); I guess, actually, that it's easier to deal with this kind of API...
Yay for bugzilla breaking lines. It might be easier to read here http://people.freedesktop.org/~david/non-oo-api.txt
would like to look at all this code more generally soon, but on the one topic here, it seems a bit odd to mix the gobject-ish API with the hippo-helper-ish API. It's kind of a leaky abstraction, because some stuff is done for you, but then you also have to deal with the GBusNameWatcher object sometimes. Some thoughts: * feel like the "base" API should be the GObject GBusNameWatcher * if there's a higher-level API that lets you just provide the two callbacks hippo-helper style, it'd be good if you never saw the GBusNameWatcher in that case. Something like: unsigned int g_bus_watch_name(GMessageBusType bus_type, const char *name, GBusNameAppearedCallback appeared_handler, GBusNameAppearedCallback vanished_handler, void *data); void g_bus_unwatch_name(unsigned int id); Basically this would just keep an internal hash table of GBusNameWatcher GObject, by id. It's a C convenience / correctness API. Why have the GBusNameWatcher at all? Well, it's a nice clean implementation, and it will map nicely to gobject-introspection ... a language binding may well find it nice to use GBusNameWatcher directly instead of letting the convenience/correctness API here create name watcher objects implicitly. Another approach could be: GBusNameWatcher* g_bus_name_watcher_new_with_handlers(GMessageBusType bus_type, /* other args as above*/) Just a convenience constructor. Here the GBusNameWatcher would be in "indeterminate name owner" state, and then asynchronously become determinate after the connection is established and the GetNameOwner reply arrives. Not sure there's a tradeoff here. I'd say you want two clean standalone APIs, one is GBusNameWatcher GObject, and one is just providing the name and two callbacks hippo-helper style... the header files of those two APIs should not mix, should be cleanly distinct APIs... but the question is which of the two APIs should be public. I'd lean toward both, but can see the case for only one or the other.
btw there's a tradeoff with GMessageBusType; ideally you would allow arbitrary GDBusConnection there, but that gets super annoying to type g_bus_get(BUS_SESSION) all the time, plus g_bus_get(BUS_SESSION) forces synchronously connecting to the bus and not reconnecting ... as long as we're being automagic, we could go ahead and support the utopia of automatically handling system bus restart and/or doing the bus connect async ... So I tend to think GMessageBusType is a nicer API all around. But it could be that GBusNameWatcher object supports providing a connection pointer to support custom buses or private connections to the well-known buses, and the convenience API only works with shared connection to well-known buses. In short suggest GMessageBusType (or GBusBusType?) on the convenience layer and connection pointer on the versatile layer.
Yeah, having a split between the versatile layer (e.g. GObject types) and the convenience layer (e.g. based on id's) sounds like a good idea. And maybe we won't end up exporting the versatile layer at all but separating the two sounds useful in either case not at least to keep the code easier to read, review, test and audit. Looks like the versatile layer we want is pretty close to the GBusNameOwner in the first patch isn't? E.g. http://people.freedesktop.org/~david/gdbus-first-stab/GBusNameOwner.html#g-bus-name-owner-new-for-connection where signals are not auto-connected and you have to deal with a GAsyncReadyCallback as well as signal handlers. So the changes I see here would be - only take a GDBusConnection, not GMessageBusType - a property specifying if the object is in a indeterminate state Also, I'll rename GMessageBusType to GBusType (not GBusBusType as I'm not sure we need to namespace everything with g_bus - as a data point libgio has GAsyncResult so I think we can just use g_) FWIW, I've also got a GBusNameWatcher class that is very similar to this. Matthias, do you mind if I push my branch to git.gnome.org? (it's called gdbus)
Talked to Matthias on IRC and got permission to push the branch, it's here http://git.gnome.org/cgit/glib/log/?h=gdbus
OK, added the convenience API; just pushed my changes to the gdbus branch http://people.freedesktop.org/~david/gdbus-third-stab/ Looks like the only thing missing now (apart from the object mappings of course) is an easy way to deal with signals ("Fifth Piece: Asynchronous Signal Watching ").
Starting to look at the code, is there any real need to refcount the Client objects ? I see that you take an extra ref for the async_ready callback, but even that doesn't really seem to be necessary...
Not that there is anything wrong with refcounting, just wondering if this is a leftover from when you planned to share these objects between multiple callers.
(In reply to comment #22) > Starting to look at the code, is there any real need to refcount the Client > objects ? I see that you take an extra ref for the async_ready callback, but > even that doesn't really seem to be necessary... We need the Client object around when processing the GAsyncCallback in order to avoid invoking the handlers if client->cancelled is TRUE. We could also use a GCancellable and just check if we get G_DBUS_ERROR_CANCELLED in the GAsyncReadyCallback but that seems like more overhead (create a new object, connect to signals, that whole thing). (And, hey, I haven't implemented support for GCancellable in GBusNameOwner or GBusNameWatcher just yet. Shouldn't be hard but I'm tired of writing test cases...) My understanding is that GCancellable is typically only something you only use if you have a long series of operations, not for one-offs like this. (In reply to comment #23) > Not that there is anything wrong with refcounting, just wondering if this is a > leftover from when you planned to share these objects between multiple callers. The Client objects can't be shared (need distinct handlers/user_data/ids) but the underlying GBusNameOwner and GBusNameWatcher objects are shared and that's kind of transparent (on purpose); e.g. from the users point of view he doesn't really need to know they are shared. Anyway, sharing is generally what you want both for performance (don't want multiple identical AddMatch rules taxing the bus) but also for things to work if two parts of your app wants to own the same name. (In fact, the latter is a really nice property; suppose Nautilus owns the name org.gnome.Nautilus and I write a Nautilus extension. Then my extension can just do g_bus_name_own() on org.gnome.Nautilus and export objects in /org/gnome/Nautilus/my_extension/ without anything breaking.)
Do you think the duplication between bustype and connection on the 'versatile' layer is worth it ? Whats the rationale for gbus- vs gdbus- filenames ?
(In reply to comment #25) > Do you think the duplication between bustype and connection on the 'versatile' > layer is worth it ? Implementation-wise the cost is almost zero (~5 lines of code). API-wise it's a set of extra functions. Yeah, sometimes you may need a private connection e.g. one that is not shared with other callers (though I can't really come up with good reasons); at least it makes writing the test cases easier (for name owner checking etc.) > Whats the rationale for gbus- vs gdbus- filenames ? If the files are for a class, the filenames are the classname in all lowercase so answering that is answering why it's called GDBusConnection and GBusNameOwner I guess. Isn't this somewhat consistent with the rest of GLib? (For files with non-class functions, it's more random, I just picked gdbusnameowning.[ch] and gdbusnamewatching.[ch] mostly because I suck at naming and just wanted to write the code. But I think this is consistent with the rest of GLib too.) To answer why the class are like they are, I'd say that, yes, we could prefix everything with GDBus but I don't think that makes sense, e.g. GDBusBusNameOwner is just more typing and it looks awkward. Similarly, we don't call it GIOFile in GIO, we just call it GFile. Of course, one consequence is that libgdbus then claims ownership of the word "Bus" in the whole GLib stack but I think that's fine.
Do we want to offer synchronous versions of anything? I'm specifically thinking of grabbing a bus name, as at least in all my apps that's something that basically needs to be done sync.
colin does it need to be done sync for some inherent reason, or are you just saying you did it sync already, so porting would be easier if you had a sync api? One way to support sync while still making the async API "primary" is kind of like dbus_pending_call_block, where you have to use the async API, but there's a call to wait for the async answer. So GBusNameOwner has an indeterminate state, you could have basically: g_bus_name_owner_block_until_determined(GBusNameOwner*) Though, it is possibly annoying to implement this without starting a main loop.
I don't know what my app would do while waiting for the reply, other than starting a mainloop just to do that. This is for the single-instance case where if running you want to open a document, if not then you go on creating the window, loading state etc.
Or maybe this gets done in libunique (is that planned to go into glib too?)
you don't necessarily have to do anything while waiting, can just move the rest of main (except mainloop.run()) into the I-own-the-name-now handler. Should just be a matter of moving the code around in a minor way. But agree there's no "point" to async in this case, other than the async API should exist for other cases, and it seems clean and elegant somehow to have a callback for "got name" since there's one for "lost name" Perhaps there's some consistency argument as well; for watching a name, or watching signals, using a sync API would be more clearly a bad idea in most cases, and so the idea would be to make the "own a name" API just look and work similarly to those other two. In theory something _could_ be done in parallel with asking for the name, esp. if we used threads, but yeah in practice not that likely. For single-instance, I had a couple of old gtk-devel-list posts on that... my thought is we should go one notch further abstracting it, and use the same codepath for "new document" and "first startup" ... basically have an "open a new window" callback which gets called either from an idle on startup of first instance, or from a dbus method call when an existing instance gets a new document. So first instance and second instance both end up in the same "new window" callback. This would use the NameOwner thing underneath but to me a nice libunique equivalent does not allow the app to mess up, it just has the app provide the bus name for singletoning, and a "new window" callback, and then it takes care of everything else.
I'll look through the gdbus branch to assess whether it fixes the dbus-glib issues we currently have to work around in telepathy-glib, and respond here soon.
(In reply to comment #32) > I'll look through the gdbus branch to assess whether it fixes the dbus-glib > issues we currently have to work around in telepathy-glib, and respond here > soon. > Cool, thanks a lot for doing this. The only thing on that branch that is somewhat baked is the name watching/owning bits. There's really no equivalent of this in dbus-glib (nor EggDBus) so any feedback is welcome. FWIW, I just reworked the "versatile layer" (see above) this weekend so it's simpler, more robust and more bindings friendly. The other bit I've been working on is mapping D-Bus types to GType (the same way EggDBus does it). This is almost done, see get_value_from_iter() and append_value_to_iter() in gdbusproxy.c and also see the test cases http://git.gnome.org/cgit/glib/tree/gdbus/tests/proxy.c?h=gdbus This mapping is very different from dbus-glib insofar that the user is never subjected to GValue at all. It does this at the cost of introducing GDBusStructure and GDBusVariant classes. It's a matter of taste whether you like this or not but my experience is that it's really easy to work with (and not inefficient either). Anyway, this bit isn't baked at all (g_dbus_proxy_new() should not take a GAsyncReadyCallback, we should have g_bus_watch_name_with_proxy() that gives you a proxy loaded with properties, append_value_to_iter() should be public, we should (optionally?) check what the user passes in etc.) but I hope it shows what I'm trying to achieve here. I was planning to post an update once I think the API is baked. Also, be advised the test cases sometimes fail (especially the GDBusConnection and GBusNameWatcher and GBusNameOwner ones) - I think this is related to how we bring session buses up and down - a race of some sort. Either way, this should be fixable. Anyway, any feedback is more than welcome. Thanks for looking at this!
(In reply to comment #33) > The other bit I've been working on is mapping D-Bus types to GType (the same > way EggDBus does it). This is almost done, see get_value_from_iter() and > append_value_to_iter() in gdbusproxy.c and also see the test cases > > http://git.gnome.org/cgit/glib/tree/gdbus/tests/proxy.c?h=gdbus > > This mapping is very different from dbus-glib insofar that the user is never > subjected to GValue at all. It does this at the cost of introducing > GDBusStructure and GDBusVariant classes. It's a matter of taste whether you > like this or not but my experience is that it's really easy to work with (and > not inefficient either). Btw, to expand on this, the plan is to have a code generator that takes D-Bus introspection XML (and in the future some sort of IDL) ("the IDL") that - Generates GInterfaces for each D-Bus interface. Each GInterface will have a way to get a GDBusProxy derived class that we hand out to the application. - For each struct declared in the IDL, generates functions _new(), _get_XXX(), _set_XXX() that operates on a GDBusStructure [1] - For each error domain, flags enumeration and enumeration in the IDL, generates these (and registers them with GType) This code generator should be written in a way so you can pass --target=c, --target=gjs, --target=seed, --target=gtkmm, --target=python etc. and it will generate natural looking and efficient (e.g. --target=gjs would bypass the DBus<->GType mapping) code for proxies. Of course if you are a generated code hater, you are free to use GDBusProxy (which is useful given things like g_dbus_proxy_invoke_method() and g_dbus_proxy_get_cached_property()) instead of generating code. [1] : In EggDBus the generated structs are actually subclasses of EggDBusStructure and we play games pretending that abusing GType as a structural type system is a good idea, see http://people.freedesktop.org/~david/eggdbus-HEAD/EggDBusStructure.html#EggDBusStructure.description for more details. While this is cute, for example properties on a GObject will have the type TestPair instead of EggDBusStructure, see http://people.freedesktop.org/~david/eggdbus-HEAD/tests-testtweak.html#TestTweak--baz-forced-to-use-pair I don't think it's ultimately worth the effort to do this.
The way the gjs bindings work is that you convert the introspection xml to "json" (not really json, really just JS source code) so at runtime there's a javascript interface description object. The proxies are then generated dynamically from that JS "idl" object at runtime. For lots of simple cases (app-internal/no-interop-needed) you can just type the javascript interface description in JS to begin with and not bother having an xml file. // this could be generated from xml, or just typed inline in JS var MyIface = { name: 'com.example.Whatever', methods: [{ name: 'frobate', outSignature: 's', inSignature: 'i' }, ...], signals: [ { name: 'signalFoo', inSignature: 's' } ], properties: [ { name: 'PropReadOnly', signature: 'b', access: 'read' }, { name: 'PropWriteOnly', signature: 's', access: 'write' }, { name: 'PropReadWrite', signature: 'v', access: 'readwrite' } ] }; function MyProxy() { this._init(); } MyProxy.prototype = { _init: function() { DBus.session.proxifyObject(this, 'com.example.Whatever', '/com/example/wahtever'); } }; DBus.proxifyPrototype(MyProxy.prototype, MyIface); let proxy = new MyProxy(); proxy.frobateRemote(42, function(result, error) { });
(In reply to comment #35) > The way the gjs bindings work is that you convert the introspection xml to > "json" (not really json, really just JS source code) so at runtime there's a > javascript interface description object. The proxies are then generated > dynamically from that JS "idl" object at runtime. For lots of simple cases > (app-internal/no-interop-needed) you can just type the javascript interface > description in JS to begin with and not bother having an xml file. Right, this looks nice and all (and much simpler than the generated C goo) but I think having the binding tool generate it regardless is preferable (e.g. less chance of errors) at least for non-trivial services like e.g. DeviceKit-disks or whatever. Not sure about mapping structs and errors is necessary for JS, but I guess you want at least enumerations generated too, right? I forgot to mention that the binding tool should also support --target=docbook for generating nice docs like http://people.freedesktop.org/~david/eggdbus-HEAD/example-dbus.html http://people.freedesktop.org/~david/eggdbus-HEAD/eggdbus-interface-org.freedesktop.DBus.html which is generated from the same XML/IDL. The EggDBus bindings tool already does this so it shouldn't be a big deal. Of course generating code royally sucks when using autotools but that's more a problem with autotools (other developer environments (such as Eclipse) have first-class support for this stuff)... doesn't change the fact that autotools won't go away though but not something I want to worry about right now.
(In reply to comment #0) > One assumption with this effort is that libdbus-1 will be used and we will not > try to hide this fact. The idea is that different object mappings for various > languages can access the underlying DBusConnection directly to avoid paying a > penalty for going through a C/GObject mapping. This is one thing I picked up while reviewing the proposed code. By G* here I mean the stuff that lives in /usr/include/glib-2.0, including GObject, GIO and the proposed GDBus. It seems that this code adds libdbus' API/ABI into GLib's API by reference, and does so in the main GDBus header. Is this really desirable? This means that if libdbus ever undergoes an ABI break (or indeed if GLib's use of libdbus is dropped), so does GLib (as a result of the stated policy that GLib, GObject, GIO and GDBus all break ABI if any of them does). I would recommend that instead, there could be a "low level" section of the API which explicitly references libdbus-1 and does not have the same guarantees as the rest, similar to dbus-glib's <dbus/dbus-glib-lowlevel.h>. If this was a separate shared library in GDBus' Requires.private, with headers in a separate directory (perhaps /usr/include/dbus-1.0 like libdbus, dbus-glib and dbus-python use), then G* itself would have some insulation from the implementation detail that it currently uses libdbus-1, and only applications that actively use the low-level interface (with a separate .pc and a separate header directory) would have to undergo an ABI transition if this changed. I would expect such applications to be relatively rare (in Telepathy, a rather elaborate user of dbus-glib, we only bypass dbus-glib for Tubes and to handle disconnection, and hopefully any new D-Bus binding will eventually cope with both of these better anyway). For instance, g_dbus_connection_get_unique_name() is certainly suitable for the G* stack - any D-Bus binding needs this functionality - but g_dbus_connection_get_dbus_1_connection() leaks libdbus' API through the abstraction and is an example of a function that should be in the low-level section. I see your point about language bindings - dbus-python, in its current form, would want to use GDBus' main loop integration instead of dbus-glib's, so it would be using the dbus-1 section of the API explicitly - but I would hope that for relatively GLib-specific bindings like gjs or Vala, GDBus is at least as easy to use as libdbus. I don't think going out of your way to let dbus-python share your DBusConnection is very useful, given that I'm not likely to make use of that ability in dbus-python :-)
Main loop: licensing ==================== This pulls the Codefactory licensing issue into the main GLib tree as a result of verbatim copying from dbus-glib, which I doubt is what you intended to happen - Rob and I have already raised our concerns about linking against libdbus and you've explained why you don't consider them to be a problem, but there's an important difference between linking against libdbus, and pasting Codefactory's un-relicensed code in-tree. Suggested resolution: we're trying to obtain the copyright. If that fails, a rewrite might be needed.
Main loop: re-entrancy (fd.o #14581) ==================================== This code seems to inherit the bug <http://bugs.freedesktop.org/show_bug.cgi?id=14581> from dbus-glib - the GSource is not marked as safe for re-entrant calls, presumably due to doubts about libdbus' reentrancy, meaning that the source is blocked in recursive main loop invocations. This is one of the dbus-glib deficiencies that TpProxy in telepathy-glib works around (although we don't work around everything - you still can't receive D-Bus messages in a recursive main loop invoked inside a D-Bus method implementation). dbus-python's use of the dbus-glib main loop integration gives it the same bug. Suggested resolution: * assess whether libdbus is in fact re-entrant (in particular, whether it calls user code with locks held) * if libdbus is re-entrant, mark the GSource as such (and as a bonus, close fd.o #14581) * if not, do as little as possible in the libdbus callback: instead, shuffle D-Bus messages from the libdbus queue to a second queue which *is* re-entrant, and return; in a separate GSource, pop messages from that second queue, and invoke user callbacks from there
GDBusError ========== This looks a lot less terrifying than dbus-glib, so that's good :-) It seems strange that unmapped GErrors are encoded in a way that'd be so difficult to read in debug output (like dbus-monitor). telepathy-glib has a function tp_escape_as_identifier() which I would be happy to donate. It escapes "" as "_", and otherwise, encodes any byte except A-Za-z0-9 (and also 0-9 if they appear at the beginning) as "_xx" for hex digits xx, resulting in a valid C identifier fragment for any input. In practice you'd generally see something like my-errors -> my_2derrors. This is sufficiently strict that the output can be used as a D-Bus object path component, interface/error name component, bus name component or member (method/signal) name (where by "component" I mean one of the dot- or slash-separated parts). We also use this function when generating C function names that need to embed a D-Bus type signature, to guarantee collision-free naming. tp_unescape_identifier() has not yet been written, but would be pretty easy (the encoding is fully reversible).
GDBusConnection - behaviour on disconnection ============================================ > upon disconnection, GDBusConnection instances will attempt to reconnect to the remote end This looks uncomfortably like policy encoded in a library (again - libdbus' automatic _exit() on disconnection is already policy-in-a-library). I'm concerned about this "retry connecting every second" idea. On the system bus, that might be appropriate, while on the session bus, once the bus daemon has exited, it's highly unlikely to come back at the same address (or indeed at all) so there's no point in retrying. Could we have some sort of tristate? Behaviour on disconnection: exit (appropriate for simplistic session things) / retry (appropriate for some system bus things, like nm-applet) / just signal it to the app (appropriate for processes that need to do teardown or have other special requirements). I'd prefer "just signal" to be the default, although people's opinions differ here.
GDBusConnection - sharing ========================= I notice the libdbus shared instances are never used at the moment. That seems wise - dbus-python and QtDBus don't use them either, and the other "bindings" are reimplementations, so the only real sharing is between libdbus and dbus-glib. As a result, at this point there's no real cost in having no attempt to share buses between "stacks". It also means that "we use the dbus-1 shared instance" is not part of the ABI, which is another piece of ABI that won't get broken if GLib moves to using a reimplementation of libdbus on some or all platforms. g_dbus_connection_bus_get says "Note that the returned object is a singleton" but that's not really true - there's 0-1 per bus type. g_dbus_connection_get_bus_type says "If G_BUS_TYPE_STARTER was passed to g_dbus_connection_get() then the return value will be either G_BUS_TYPE_SESSION or G_BUS_TYPE_SYSTEM depending on what bus started the process", but what happens if the starter bus is neither the session bus nor the system bus? bus-type says "type of the message bus the connection is for or G_BUS_TYPE_NONE if the connection is not to a message bus". Do you really mean "... if the connection is not to one of the well-known message bus instances"? There is a semantic difference between a connection to an unspecified endpoint (dbus-python: dbus.connection.Connection), and a connection to a message bus (dbus-python: dbus.bus_connection.BusConnection, although in practice everyone uses the shareable subclasses). A plain connection can be to any process, and is just a pipe for message-passing. Telepathy Tubes use a plain connection (when used in an XMPP or local-XMPP Multi-User Chat room, they provide vaguely bus-like semantics, but without the org.freedesktop.DBus pseudo-service), so I have an interest in having some sort of API support for such a connection. dbus-python supports this via dbus.connection.Connection; QtDBus does not support this usage yet. A bus connection is to a process such as dbus-daemon that implements the special semantics of the org.freedesktop.DBus pseudo-service, including the initial Hello message, unique name allocation, well-known name owners, and message routing. This could be the session bus, the system bus, an as-yet uninvented bus type (a one-per-uid-per-kernel "user bus" has been proposed several times), or (in principle) any dbus-daemon whose address string has been communicated to you somehow. dbus-python and QtDBus both support passing an arbitrary address to the BusConnection/QDBusConnection constructor for this purpose.
GDBusConnection - the starter bus ================================= It is technically possible for the starter bus to be something other than the system or session buses (you can be service-activated on the hypothetical per-user bus, or on some custom bus). Look for DBUS_STARTER_ADDRESS in libdbus source. GDBus doesn't handle this (and I think its use of g_critical to deal with this circumstance is inappropriate).
GDBusConnection - failing ========================= I think g_dbus_connection_send_dbus_1_message_with_reply should use an error more specific than G_DBUS_ERROR_FAILED to represent the connection having closed, unless this is that error's only possible meaning.
g_bus_own_name ============== I'm not a GLib style guru, but can't g_bus_own_name() return an opaque pointer, perhaps GBusOwnedName*? It'd be fine if the pointer was secretly GUINT_TO_POINTER(id) in the .c, but we have compiler type-checking for library users, so we might as well benefit from it.
GDBusNameWatcher ================ "TODO: don't hold lock when doing callbacks" looks scary. I'm pleased to see that this class uses arg0 matching right from the beginning. Implementation detail: should there really be a filter function per name owner?! Surely a map { name => interested parties } would avoid duplicated work? I'd be happy to donate telepathy-glib's code to do this (see TpDBusDaemon in telepathy-glib/dbus-daemon.c).
g_bus_watch_name ================ As for g_bus_own_name, can't we use opaque pointers? The address of the Client looks suitable, for instance? You can still look it up in a hash table to be able to make programming errors non-crashy, if you want.
That's all for now (I covered the stuff documented in Comment #21 in my initial review). I'll look at type mapping, signal receiving and proxies next. Can I assume that the stuff in EggDBus accurately reflects the direction you want GDBus to go in? Adding desrt to Cc since he's implemented another GLib D-Bus binding, so probably has some useful points of view here.
* I agree it makes sense to have a -lowlevel or -libdbus header that actually pulls in dbus.h (also, you could omit -ldbus from the .pc file) * I think libdbus does use a recursive mutex now and a recursive main loop source would work, but testing it would not be a bad idea. http://lists.freedesktop.org/archives/dbus/2006-October/006171.html A second message queue would clearly be a hackaround for something we should fix in DBusConnection. * tristate on disconnect makes sense. I strongly feel the default should be exit, because the other two (reconnect, do nothing and let app handle) require extra code in the app. exit is the only one that can _possibly_ be right if the app does nothing. If the app is writing code to handle disconnect, then it can set the tristate. exit is the right default for pretty much all session apps anyhow, and matches xlib. Another option is to default to reconnect for system bus and default to exit for session bus; those are the most-commonly-correct options for those two cases. But, it's probably confusing to have two different defaults. * I disagree on connection sharing. The shared DBusConnection should be used. The reasons for avoiding this in other bindings have been that people did not understand how to use the sharing or why it mattered. Here are some posts (entire thread probably useful): http://lists.freedesktop.org/archives/dbus/2007-August/008236.html http://lists.freedesktop.org/archives/dbus/2007-August/008255.html Thiago is sold ;-) http://lists.freedesktop.org/archives/dbus/2007-August/008257.html Specifically for gjs, e.g. we do not want gjs apps to have two bus connections as soon as anything in the underlying gnome library stack uses dbus. That bloat is not necessary.
(In reply to comment #48) > That's all for now (I covered the stuff documented in Comment #21 in my initial > review). Great, thanks, this looks very useful, from a cursory look all these suggestions are good; I'll follow up on them later when I've fixed things. > I'll look at type mapping, signal receiving and proxies next. Can I > assume that the stuff in EggDBus accurately reflects the direction you want > GDBus to go in? Yeah, pretty much. The type mapping (append_value_to_iter(), get_value_to_iter()) is done modulo the disclaimers in comment 33 and 34 and just general niceties / sanity checking like erroring out if the user passes malformed signature or object paths or something (if performance turns out to be a concern we need to profile / rethink things). There's the question if we want to push for G_TYPE_INT16 and stuff in GObject, it would be nicer to be able to do this but since we require the signature this is not a biggie. Signal receiving isn't done at all, I expect GDBusProxy to have something like this signal g-dbus-proxy-dbus-signal (const gchar *name, guint num_params, const GValue *params); where generated proxies can hook into the signal class handler and dispatch their own signals after mapping @name to the suitable G-name etc. (ideally we'd use libffi for this). GDBusConnection will also need some smarts to make it easy for GDBusProxy to subscribe/unsubscribe from signals (so the "add/remove match rules" logic, Havoc's fourth point, will be in GDBusConnection - want it there since non-C proxies wants to use this too).
(In reply to comment #39) > Main loop: re-entrancy (fd.o #14581) > ==================================== > > This code seems to inherit the bug > <http://bugs.freedesktop.org/show_bug.cgi?id=14581> from dbus-glib - the > GSource is not marked as safe for re-entrant calls, presumably due to doubts > about libdbus' reentrancy, meaning that the source is blocked in recursive main > loop invocations. This is one of the dbus-glib deficiencies that TpProxy in > telepathy-glib works around (although we don't work around everything - you > still can't receive D-Bus messages in a recursive main loop invoked inside a > D-Bus method implementation). > > dbus-python's use of the dbus-glib main loop integration gives it the same bug. > > Suggested resolution: > > * assess whether libdbus is in fact re-entrant (in particular, whether it calls > user code with locks held) > * if libdbus is re-entrant, mark the GSource as such (and as a bonus, close > fd.o #14581) > * if not, do as little as possible in the libdbus callback: instead, shuffle > D-Bus messages from the libdbus queue to a second queue which *is* re-entrant, > and return; in a separate GSource, pop messages from that second queue, and > invoke user callbacks from there Funny enough I just ran into this bug last night before leaving; for now I'm queuing up messages http://git.gnome.org/cgit/glib/commit/?h=gdbus&id=ccb19719e7206b5578aa9d34af9775aaa938be42 but I agree with you and Havoc (comment 49) that we should fix this. Fortunately some of the test cases I'm adding now is triggering it so it should be straightforward to remove this hack, watch the tests fail, fix it and see the tests pass.
Fixing the main loop source to be recursive should be a 1-line patch (g_source_set_can_recurse). You should not need that huge patch. The workaround patch has various problems, but rather than fix them, I would just revert the patch.
(In reply to comment #52) > Fixing the main loop source to be recursive should be a 1-line patch > (g_source_set_can_recurse). Hmm, I'm not very familiar with the mainloop integration code; this is the patch I'm using $ git diff gdbusmainloop.c diff --git a/gdbus/gdbusmainloop.c b/gdbus/gdbusmainloop.c index 5347585..96f5c7c 100644 --- a/gdbus/gdbusmainloop.c +++ b/gdbus/gdbusmainloop.c @@ -156,6 +156,7 @@ connection_setup_new (GMainContext *context, cs->message_queue_source = g_source_new ((GSourceFuncs *) &message_queue_ sizeof (DBusGMessageQueue)); + g_source_set_can_recurse (cs->message_queue_source, TRUE); ((DBusGMessageQueue*)cs->message_queue_source)->connection = connection; g_source_attach (cs->message_queue_source, cs->context); } @@ -272,6 +273,7 @@ connection_setup_add_watch (ConnectionSetup *cs, channel = g_io_channel_unix_new (dbus_watch_get_unix_fd (watch)); handler->source = g_io_create_watch (channel, condition); + g_source_set_can_recurse (handler->source, TRUE); g_source_set_callback (handler->source, (GSourceFunc) io_handler_dispatch, ha io_handler_source_finalized); g_source_attach (handler->source, cs->context); @@ -366,6 +368,7 @@ connection_setup_add_timeout (ConnectionSetup *cs, handler->timeout = timeout; handler->source = g_timeout_source_new (dbus_timeout_get_interval (timeout)); + g_source_set_can_recurse (handler->source, TRUE); g_source_set_callback (handler->source, timeout_handler_dispatch, handler, timeout_handler_source_finalized); g_source_attach (handler->source, handler->cs->context); > You should not need that huge patch. Doing what you suggest causes a deadlock, see [1].. I'm running this against HEAD of dbus's master branch. Sure, this is a recursive mainloop, that's how my test suite is set up (for better or worse).... So, this looks like a bug in libdbus does it not, I mean according to your Oct 2006 message this is supposed to work, yes? [1] :
+ Trace 214902
Oh, the mutexes are recursive, but the dispatch condvar is not. Step 1, make _dbus_connection_acquire_dispatch() not wait to get condition from itself. Anyone who is actually smart about threads probably knows the best way to do this. Step 2, audit that dbus_connection_dispatch() does something sane-ish if called recursively in the same thread. I think it's probably as sane as the workaround (keeping your own queue), whether it's sane in absolute terms I don't know. As discussed on irc, but for record, keeping own queue has conceptual problems (aside from any implementation headaches with re-entrancy), like not knowing whether to return HANDLED.
I added a mechanism to subscribe to signals, see http://git.gnome.org/cgit/glib/tree/gdbus/gdbusconnection.c?h=gdbus#n1591 (this also addresses the request in comment 46 - we only have a single D-Bus filter function now that BusNameOwner and BusNameWatcher uses this) Anyway, am unsure about what level of error handling we want. My understanding is that AddMatch() can only fail if the bus is in a OOM condition. I'm not sure how useful or meaningful it is to handle errors here, the alternative to doing abort() upon receiving an OOM error from the bus (as I'm doing now) is something like [2] which complicates all call sites (e.g. GDBusProxy, GBusNameWatcher, GBusNameOwner) and I'm not really sure what their behavior should be if AddMatch() fails. It seems like the only reasonable thing these higher level classes can do is a. punt this error to the app programmer .. meaning more codepaths and API and opportunities to get things wrong; or b. silently break the contract that signals are guaranteed to be delivered; or c. abort() So let's review: b. is not really an option, a. is not really meaningful.. so I went for c. (at the lowest level). [2] : void g_dbus_connection_dbus_1_signal_subscribe (GDBusConnection *connection, const gchar *sender, const gchar *interface_name, const gchar *member, const gchar *object_path, const gchar *arg0, GDBusSignalCallback1 callback, gpointer user_data, GAsyncReadyCallback async_callback, gpointer async_user_data); guint g_dbus_connection_dbus_1_signal_subscribe_finish (GDBusConnection *connection, GAsyncResult *res, GError **error);
(In reply to comment #49) > * tristate on disconnect makes sense. I strongly feel the default should be > exit, because the other two (reconnect, do nothing and let app handle) require > extra code in the app. exit is the only one that can _possibly_ be right if the > app does nothing. If the app is writing code to handle disconnect, then it can > set the tristate. exit is the right default for pretty much all session apps > anyhow, and matches xlib. Right, I thought you'd say that :-) We've discussed this before, and I still disagree. Perhaps I'd agree with you if every user of D-Bus was an application, but libraries and plugins use D-Bus too, not always with the application's knowledge. Consider Quod Libet, a PyGtk media player. Quod Libet the application (the quodlibet source package in Debian) has no relationship at all with D-Bus, so naturally it doesn't have any code for handling a D-Bus disconnection and shouldn't be required to to add such code. However, an externally-distributed plugin (in a separate tarball with a separate release schedule - the quodlibet-plugins source package in Debian) supports "D-Bus multimedia keys" using dbus-python. This puts Quod Libet on the session bus, without the application's knowledge. Should this plugin really be manipulating app-global state (the exit-on-disconnect flag) in order to prevent a session bus exit from bringing down the application abruptly? Quod Libet can play music fine without D-Bus, it just loses that plugin-provided feature; even if your position is that the session bus defines the lifetime of the session (does it really?), the application should get a chance to exit gracefully. If a second plugin uses the session bus for something non-essential (perhaps it pushes "now playing..." information into Telepathy or Pidgin IM connections), are *both* plugins now responsible for switching off exit-on-disconnect and translating the Disconnected signal into a no-op or a graceful shutdown? > Another option is to default to reconnect for system bus and default to exit > for session bus; those are the most-commonly-correct options for those two > cases. But, it's probably confusing to have two different defaults. The correct default for unrecognised buses (when GDBus supports them, which I think is necessary) is impossible to determine. I think the only reasonable thing for these would be to report the disconnection and continue (who are we to say that an as-yet uninvented bus has "defines the lifetime of..." semantics?) but if we're special-casing the well-known buses, we'd either end up changing the behaviour incompatibly in versions of GDBus new enough to treat those buses specially (for the hypothetical per-user bus you probably want to reconnect, for instance), or continue with a default that's now known to be inappropriate. Having no special behaviour by default (disconnection is reported, the library user can handle it if it wants to, and minimal correct handling is to stop trying to use that connection) could be consistent across all bus types, and also consistent with a general principle that libraries don't do anything you don't tell them to.
(In reply to comment #55) > Anyway, am unsure about what level of error handling we want. My understanding > is that AddMatch() can only fail if the bus is in a OOM condition. I believe it can also fail because the bus refuses to add any more match rules *for you* (the system bus wants to limit CPU and memory consumption per client, so there's a limit on the number of match rules it's willing to process - the session bus still has this limit but sets it to a very large number). I don't like the idea of abort()ing because a resource limit in "some process over there" (the bus daemon) has been hit. Couldn't we avoid violating the invariant mentioned as (b) by altering the bus daemon's failure mode, so instead of refusing to add the match rule, it sets a flag in the bus-client struct (which is already allocated so cannot fail due to OOM), that means "this client has run out of space for match rules, so recover by assuming that everything matches"? Obviously, having done that, the bus daemon code can then layer on arbitrarily clever performance improvements (perhaps consolidating sufficiently similar match rules instead of just adding an implicit catch-all rule, if it can) if the excess signals turn out to be a practical problem. A refinement that would probably be worthwhile would be to have a second flag in the struct indicating "assume every *signal* matches", for instance, since basically nobody except dbus-monitor sets a match rule for non-signals. Sending excess signals is obviously non-ideal for performance, but IMO it's better than aborting. The additive nature of AddMatch means that in the presence of plugins, other bindings etc. sharing a connection, you can never assert that you will not receive signals you didn't ask for (indeed, I believe unicast signals are always delivered too).
(In reply to comment #56) > > if your position is that the session bus defines the lifetime of the session > (does it really?), Yes, that's the idea with the session bus. > the application should get a chance to exit gracefully. My take on this is that generally apps should be automatically saving e.g. any relevant user data or state automatically and not only on clean app shutdown. http://en.wikipedia.org/wiki/Crash-only_software http://lwn.net/Articles/191059/ Now you do generally want to take application shutdown as a good time to save state again if you haven't already done it. For new applications maybe the right thing there is to listen to the session dbus signal? Or XSMP I guess. Possibly atexit().
(In reply to comment #58) > My take on this is that generally apps should be automatically saving e.g. any > relevant user data or state automatically and not only on clean app shutdown. A worthy goal, but there's a difference between "I want this to happen" and "anyone who doesn't do this, and might have a plugin that uses D-Bus, loses". It seems presumptuous for an IPC system to enforce this behaviour. > Now you do generally want to take application shutdown as a good time to save > state again if you haven't already done it. For new applications maybe the > right thing there is to listen to the session dbus signal? Or XSMP I guess. > Possibly atexit(). The problem with that is: * to listen for Disconnected, you have to know about the session bus and D-Bus, be on the session bus, turn off exit-on-disconnect, and register your callback * if you're on the bus (perhaps because of a plugin, e.g. in the Quod Libet case above), by the time you get a chance to process the XSMP message, libdbus may already have killed you (I doubt the event ordering is guaranteed to be either way round, since it's a race between libdbus reading the D-Bus socket and xlib reading the X socket?) * if you're on the bus (perhaps because of a plugin) atexit() won't work, because libdbus calls _dbus_exit(), which (on Unix) is _exit(), which bypasses atexit() and on_exit() (I assume there's a rationale for this) I'd rather not have D-Bus be something that can be substituted for $X in: Well-written applications have to care about $X even if they don't, themselves, take advantage of $X, just in case one of their plugins, or one of the libraries they use, does need $X (Enabling locks needed for multi-threaded operation, e.g. g_thread_init() and dbus_g_thread_init(), seems to be the main value for $X at the moment.)
Switching subject, before I start reviewing type mapping and object mapping: I'm a bit concerned about the fact that the medium- to low-level message sending and receiving functions use DBusMessage and so cause dbus-1 to be part of the glib-2.0 API. I have a suspicion that you're eventually going to want a GDBusMessage structure anyway, although it might be disguised as something else, like DBusGMethodInvocation (which is basically a received method call message in disguise). If this is the case, then it might be worth having methods on GDBusConnection that can use this structure to emit a signal, call a method with an asynchronous reply through a callback, and call a method with reply ignored (but that could just be the asynchronous-reply one with callback == NULL, like in telepathy-glib). Some of the higher-level (object mapping) constructs could then be implemented in terms of these. Alternatively, you could consider having an API like (pseudocode): g_dbus_connection_emit_signal (GDBusConnection, object_path, interface, signal_name, array<GDBusVariant>) g_dbus_connection_call_method (GDBusConnection, bus_name, object_path, interface, method_name, array<GDBusVariant>, [insert async stuff here]) (This would be especially good if GVariant ends up in into GLib, because your array of GVariant would essentially already be the serialized payload of a message.)
(This type mapping stuff is presumably irrelevant if GVariant lands in GLib, since it would seem perverse to have a variant type whose data model is exactly that of D-Bus, but then not use it in the D-Bus bindings.) I see you plan to introduce GTypes for gint64, guint16, D-Bus object path and D-Bus signature. That's good. I also observe that you plan to diverge from dbus-glib by using a gchar ** to represent object path arrays and the rarely-seen signature arrays. It's good to have consistency between strings and object paths, although in some ways I'd prefer a GPtrArray of strings. The duality that NULL and { NULL } are both an empty GStrv makes it unclear whether one or the other is guaranteed, or whether you'll get one or the other at random. I suspect GStrv is sufficiently entrenched in GLib that this isn't going to change, though. g_dbus_variant_set_array and _set_ptr_array worry me: it's not at all clear which types are accepted by which function. While setting an inappropriate type is clearly programmer error here, I think they should specifically g_critical() if the type is not one that they support, rather than continuing and failing later. I think it would be worthwhile having g_dbus_variant_get_*int* being somewhat permissive with types - in dbus-glib and dbus-python, particularly, it's tricky to get the right type into a variant - and having them consider calling the wrong accessor to be a recoverable runtime error rather than a programmer error. It's very annoying when dealing with input from some other process to have to check that the type is what you think it ought to be, then retrieve it (which would assert if the type was wrong and the check had not been done), then use it, rather than having the retrieval + check happen at the same time in the case where the type is right. In telepathy-glib we have a family of functions for dealing with a{sv} (string-to-variant map) which consider all the integers to be "basically the same", so tp_asv_get_uint32() will return successfully regardless of whether the value is actually a guchar or a gint64, as long as its numeric value would fit in 32 bits. This is kind to Python programmers who accidentally put a signed 32-bit integer (the default mapping for Python 'int' objects, which are really a C long, in dbus-python) into a dict entry that's meant to represent a port number. Further, our a{sv} handling functions return 0/FALSE/NULL on inappropriate types or missing values, with an error indicator (a gboolean* argument) for the 10% of cases where you actually want to distinguish between 0 and missing, or 0 and "actually it's a byte array, wtf?" - in many cases in Telepathy, though, we want missing things to be 0, FALSE, NULL or "" anyway. It may be worth having get_double return successfully for any integer, too, but not vice versa (we don't do this yet, iirc). Typical usage is something like this: guint handle_type; handle_type = tp_asv_get_uint32 (dict, "TargetHandleType", NULL); if (handle_type != TP_HANDLE_TYPE_CONTACT && handle_type != TP_HANDLE_TYPE_ROOM) { /* raise GError: we support contacts and chatrooms, not whatever you gave us */ } or in the case where you do care about the distinction between 0, missing and wrong-type: gboolean valid; guint handle_type; handle_type = tp_asv_get_uint32 (dict, "TargetHandleType", &valid); if (!valid) { /* raise GError (and you can use tp_asv_lookup() to look at the raw GValue * here, if you care about the difference between missing and * wrong-type) */ } > GArray * > g_dbus_variant_get_array (GDBusVariant *variant) This should be a const GArray *. > GPtrArray * > g_dbus_variant_get_ptr_array (GDBusVariant *variant) This should be a const GPtrArray *. > GHashTable * > g_dbus_variant_get_hash_table (GDBusVariant *variant) Sadly, this has to remain a mutable GHashTable * with documentation, unless various GHashTable APIs change to take a const parameter. Regarding GDBusStructure: > gchar **element_signatures; >... > dbus_free (structure->priv->element_signatures[n]); /* allocated by libdbus */ In Telepathy we've found that a useful convention is that every gchar * was allocated by GLib, and every char * was allocated by "something else" like libdbus or Avahi (there's rarely more than one "something else" in each module). > GDBusStructure * > g_dbus_structure_new (const gchar *signature, > GType first_element_type, > ...) > { >... > gchar *signature_with_parenthesis; >... > signature_with_parenthesis = g_strdup (signature + 1); > signature_with_parenthesis[strlen (signature_with_parenthesis) - 1] = '\0'; Er, isn't this the signature *without* the parentheses (plural)? Calling it contents_sig or inner_sig or something would seem to be sufficient without being so long, too. > a = g_array_new (FALSE, > TRUE, > sizeof (GValue)); Isn't this a reimplementation of GValueArray?
(In reply to comment #61) > I see you plan to introduce GTypes for gint64, guint16, D-Bus object path and > D-Bus signature. That's good. That should have been gint16, of course; gint64 already has a perfectly good GType.
(In reply to comment #60) > Switching subject, before I start reviewing type mapping and object mapping: > > I'm a bit concerned about the fact that the medium- to low-level message > sending and receiving functions use DBusMessage and so cause dbus-1 to be part > of the glib-2.0 API. > > I have a suspicion that you're eventually going to want a GDBusMessage > structure anyway, although it might be disguised as something else, like > DBusGMethodInvocation (which is basically a received method call message in > disguise). > > If this is the case, then it might be worth having methods on GDBusConnection > that can use this structure to emit a signal, call a method with an > asynchronous reply through a callback, and call a method with reply ignored > (but that could just be the asynchronous-reply one with callback == NULL, like > in telepathy-glib). Some of the higher-level (object mapping) constructs could > then be implemented in terms of these. > > Alternatively, you could consider having an API like (pseudocode): > > g_dbus_connection_emit_signal (GDBusConnection, object_path, interface, > signal_name, array<GDBusVariant>) > > g_dbus_connection_call_method (GDBusConnection, bus_name, object_path, > interface, method_name, array<GDBusVariant>, [insert async stuff here]) > > (This would be especially good if GVariant ends up in into GLib, because your > array of GVariant would essentially already be the serialized payload of a > message.) > The idea is that GDBusProxy has methods g_dbus_proxy_invoke_method() g_dbus_proxy_invoke_method_finish() g_dbus_proxy_invoke_method_sync() g_dbus_proxy_emit_signal() and a signal ::g-dbus-proxy-signal that is built around GType (everything but g_dbus_proxy_emit_signal() is already in the branch, semi-baked and awaiting comments). So there are no leaks of libdbus-1 here because the type mapping happens internally. Of course, this class uses stuff on GDBusConnection that will be in the gdbus-dbus-1 headers and if libdbus ever breaks ABI we have a transition story. Bottom line: no leaking of libdbus if you just stick to GDBusProxy. I expect other languages to do similar things. With generated code most people will never ever need to see GDBusProxy; they will only use sub classes. So I am not convinced there is any need to hide DBusConnection, DBusMessage and other libdbus-1 concepts except for sticking them in a separate header (protected by G_DBUS_I_KNOW_LOW_LEVEL_STUFF_IS_PRIVATE_ABI_SUBJECT_TO_CHANGE etc. etc.) so we have an ABI transition story should we ever need one. There are precedents for marking both API (gtktextlayout.h) and ABI (gtk+ file chooser) as unstable in the G* stack so this is not really something new.
(In reply to comment #61) > (This type mapping stuff is presumably irrelevant if GVariant lands in GLib, > since it would seem perverse to have a variant type whose data model is exactly > that of D-Bus, but then not use it in the D-Bus bindings.) > > I see you plan to introduce GTypes for gint64, guint16, D-Bus object path and > D-Bus signature. That's good. I was planning to, strictly it's not needed since we can coerce types to fit the signature. It's bug 562498, for the record. I think we might want to have this, Matthias certainly indicated it wasn't worth fighting against http://mail.gnome.org/archives/gtk-devel-list/2009-February/msg00039.html I've left TODO:16 everywhere in the code if we do this. I think we should... > I also observe that you plan to diverge from dbus-glib by using a gchar ** to > represent object path arrays and the rarely-seen signature arrays. It's good to > have consistency between strings and object paths, although in some ways I'd > prefer a GPtrArray of strings. The duality that NULL and { NULL } are both an > empty GStrv makes it unclear whether one or the other is guaranteed, or whether > you'll get one or the other at random. I suspect GStrv is sufficiently > entrenched in GLib that this isn't going to change, though. As a general rule, I think we should always g_return_val_if_fail() if any non-integral type being passed in is NULL. Similarly, we should never return NULL types. While it's tempting to allow NULL strings to mean "" and NULL arrays to mean empty arrays, we _will_ get screwed if a future extension to the D-Bus protocol allow nullable types (an extension, which I would welcome [1] - we'd need to figure out how to handle nullable integral types though - don't think the extension should cover integral types though since it would screw over C/C++ at the very least). [1] : for example it's not much fun to have an object path property that isn't set - you have to cheat and make it return "/" with the meaning "is not set". > g_dbus_variant_set_array and _set_ptr_array worry me: it's not at all clear > which types are accepted by which function. While setting an inappropriate type > is clearly programmer error here, I think they should specifically g_critical() > if the type is not one that they support, rather than continuing and failing > later. Sure thing, I'm a big fan of checking incoming args etc. Also, docs will help out here. In general, it should be clear e.g. GArray is for integral types, GPtrArray for pointer types. > I think it would be worthwhile having g_dbus_variant_get_*int* being somewhat > permissive with types - in dbus-glib and dbus-python, particularly, it's tricky > to get the right type into a variant - and having them consider calling the > wrong accessor to be a recoverable runtime error rather than a programmer > error. It's very annoying when dealing with input from some other process to > have to check that the type is what you think it ought to be, then retrieve it > (which would assert if the type was wrong and the check had not been done), > then use it, rather than having the retrieval + check happen at the same time > in the case where the type is right. This is probably not a bad idea. > In telepathy-glib we have a family of functions for dealing with a{sv} > (string-to-variant map) which consider all the integers to be "basically the > same", so tp_asv_get_uint32() will return successfully regardless of whether > the value is actually a guchar or a gint64, as long as its numeric value would > fit in 32 bits. This is kind to Python programmers who accidentally put a > signed 32-bit integer (the default mapping for Python 'int' objects, which are > really a C long, in dbus-python) into a dict entry that's meant to represent a > port number. > > Further, our a{sv} handling functions return 0/FALSE/NULL on inappropriate > types or missing values, with an error indicator (a gboolean* argument) for the > 10% of cases where you actually want to distinguish between 0 and missing, or 0 > and "actually it's a byte array, wtf?" - in many cases in Telepathy, though, we > want missing things to be 0, FALSE, NULL or "" anyway. Right, so all this is possible to do on GHashTable itself. I don't think the GLib maintainers in general are happy about taking patches with convenience functions though. FWIW, in EggDBus I added a ton of convenience stuff http://people.freedesktop.org/~david/eggdbus-HEAD/EggDBusHashMap.html e.g. egg_dbus_hash_map_{contains|lookup|insert}_{float|fixed} just to make it easier to deal with. Not sure it's worth the effort though. > In Telepathy we've found that a useful convention is that every gchar * was > allocated by GLib, and every char * was allocated by "something else" like > libdbus or Avahi (there's rarely more than one "something else" in each > module). Good point, actually I'm normally doing this myself, not sure why I failed to do it here ;-) > > GDBusStructure * > > g_dbus_structure_new (const gchar *signature, > > GType first_element_type, > > ...) > > { > >... > > gchar *signature_with_parenthesis; > >... > > signature_with_parenthesis = g_strdup (signature + 1); > > signature_with_parenthesis[strlen (signature_with_parenthesis) - 1] = '\0'; > > Er, isn't this the signature *without* the parentheses (plural)? Calling it > contents_sig or inner_sig or something would seem to be sufficient without > being so long, too. Good catch, thanks. > > a = g_array_new (FALSE, > > TRUE, > > sizeof (GValue)); > > Isn't this a reimplementation of GValueArray? Actually GValueArray copies the GValue on insertion and we want to avoid the extra copy. In addition, g_boxed_ref() and g_boxed_unref() for GValueArray performs a deep copy and that will hurt performance even more. In general we try hard not to copy things around especially since some types like GStrv and gchararray performs deep copies. For example, in get_value_from_iter() and extract_value_from_iter() we use G_VALUE_NOCOPY_CONTENTS. (yes, the usage of GValueArray in the GDBusProxy::g-dbus-proxy-signal signal suffers from this too; I will change this to avoid copies.)
(In reply to comment #61) > > GArray * > > g_dbus_variant_get_array (GDBusVariant *variant) > > This should be a const GArray *. > > > GPtrArray * > > g_dbus_variant_get_ptr_array (GDBusVariant *variant) > > This should be a const GPtrArray *. > > > GHashTable * > > g_dbus_variant_get_hash_table (GDBusVariant *variant) > > Sadly, this has to remain a mutable GHashTable * with documentation, unless > various GHashTable APIs change to take a const parameter. First of all, const in C is kind of a mess (it's slightly better in C++). Anyway, actually the intention here is that the user is free to change the value. One example is actually the ChatRoom example we talked about on dbus-list some time ago. I'll recap the general idea here. The idea here is that you have a remote object that implements the org.ChatRoom interface. Now, the participants in the chat room are available as a property, :Participants, say, with signature a{ss}. For performance reasons you don't really want to emit PropertiesChanged() every time this happens because users part and join every few seconds and the participant list can get huge so it's a lot of data to transfer all the time. So you have other signals ParticipantJoined(ss) and ParticipantParted(ss). The idea here is that you can subclass the generated proxy class for org.ChatRoom like [1] to synthesize signals for properties changed. [1] : static void foo_class_init(FooClass *klass) { GDBusProxyClass *proxy_class = G_DBUS_PROXY_GET_CLASS (klass); ... proxy_class->signal = on_dbus_signal; } static void on_dbus_signal (GDBusProxy *proxy, const gchar *signal_name, const gchar *signature, GValueArray *args) { GDBusVariant *value; GPtrArray *p; value = g_dbus_proxy_get_cached_property (proxy, "Participants", NULL); g_assert (g_dbus_variant_is_ptr_array (value)); p = g_dbus_variant_get_ptr_array (value); // modify p here using @args g_value_unref (value); g_signal_emit_by_name (proxy, "g-dbus-proxy-properties-changed"); }
(In reply to comment #61) > (This type mapping stuff is presumably irrelevant if GVariant lands in GLib, > since it would seem perverse to have a variant type whose data model is exactly > that of D-Bus, but then not use it in the D-Bus bindings.) Btw, strongly disagree that a D-Bus<->GType mapping it's irrelevant in the light of something like GVariant. GVariant is on the level on DBusMessage, e.g. serialization, and I think 5+ years of collective experience has shown this kind of API is not useful for exporting or consuming non-trivial D-Bus services. You basically want something type-safe and something that maps to well-known robust data types that you are already familiar with. That's my opinion anyway. YMMV.
(In reply to comment #59) > (In reply to comment #58) > > My take on this is that generally apps should be automatically saving e.g. any > > relevant user data or state automatically and not only on clean app shutdown. > > A worthy goal, but there's a difference between "I want this to happen" and > "anyone who doesn't do this, and might have a plugin that uses D-Bus, loses". > It seems presumptuous for an IPC system to enforce this behaviour. Theory aside I'm not seeing a real-world problem here because the only time the session bus should go away is after the X connection dies, in which case xlib will have already killed most applications. The reason we also exit on session bus termination is that it's convenient for desktop services which don't have an X connection.
Red Hat used to get tons of bugs about stuff persisting after logout (gconfd and such), drives sysadmins insane. The plugin argument is interesting, but I'd rather deal with that in some special way, instead of breaking the default behavior. Maybe brainstorm this out: If you read that thread I linked about the shared connection, there was a similar plugin issue with the main loop integration; how does a plugin know the bus connection has been attached to a main loop? Should it mainloopize the dbus connection itself? So one solution is private connections for plugins, then the plugin can set the no-exit-on-disconnect flag and set up the main loop. I don't think this solution is all that bad if it's a rare case only for sticking dbus into processes that don't otherwise use dbus. If a plugin sets up a main loop different from that of the app, it isn't going to work anyway. At some point, plugins and apps do have to make some assumptions about each other. I think I had an idea in some old thread, maybe the shared connection thread, about an API where basically some part of a process could claim the connection - become the "connection manager"? - and would be responsible for mainloop integration, exit-on-disconnect policy, etc. This could either be a runtime negotiation, or it could amount to an assertion (print warnings if two things try to both set up a connection). In the big picture, the case of a plugin that uses dbus and an app that doesn't should be very uncommon, we shouldn't optimize defaults for this. But we could handle it nicely perhaps. Surely there's a pretty elegant solution without breaking the normal default case.
(In reply to comment #37) > (In reply to comment #0) > > One assumption with this effort is that libdbus-1 will be used and we will not > > try to hide this fact. The idea is that different object mappings for various > > languages can access the underlying DBusConnection directly to avoid paying a > > penalty for going through a C/GObject mapping. > > This is one thing I picked up while reviewing the proposed code. By G* here I > mean the stuff that lives in /usr/include/glib-2.0, including GObject, GIO and > the proposed GDBus. > > It seems that this code adds libdbus' API/ABI into GLib's API by reference, and > does so in the main GDBus header. Is this really desirable? This means that if > libdbus ever undergoes an ABI break (or indeed if GLib's use of libdbus is > dropped), so does GLib (as a result of the stated policy that GLib, GObject, > GIO and GDBus all break ABI if any of them does). > > I would recommend that instead, there could be a "low level" section of the API > which explicitly references libdbus-1 and does not have the same guarantees as > the rest, similar to dbus-glib's <dbus/dbus-glib-lowlevel.h>. > > If this was a separate shared library in GDBus' Requires.private, with headers > in a separate directory (perhaps /usr/include/dbus-1.0 like libdbus, dbus-glib > and dbus-python use), then G* itself would have some insulation from the > implementation detail that it currently uses libdbus-1, and only applications > that actively use the low-level interface (with a separate .pc and a separate > header directory) would have to undergo an ABI transition if this changed. Done in this commit http://git.gnome.org/cgit/glib/commit/?h=gdbus&id=562449f8432456cc1a1cb4646995a1ea4df3a169 but I only split out the headers. Not sure it makes sense to split the library into two pieces as this causes unreasonable linker overhead and I haven't thought about whether it even makes sense to split the library in two. Anyway, getting to the functions exposing low-level stuff now requires - that gdbus-lowlevel-2.0.pc is used - that gdbus/gdbus-lowlevel.h is included - that G_DBUS_I_UNDERSTAND_THAT_ABI_AND_API_IS_UNSTABLE is defined and D-Bus's include path is no longer pulled in by using gdbus-2.0.pc. This allows apps not relying on symbols explicitly marked as unstable to gracefully transition if GDBus itself were to switch to e.g. libdbus-2 or something else.
(In reply to comment #64) > (In reply to comment #61) > > The duality that NULL and { NULL } are both an > > empty GStrv makes it unclear whether one or the other is guaranteed, or whether > > you'll get one or the other at random. I suspect GStrv is sufficiently > > entrenched in GLib that this isn't going to change, though. > > As a general rule, I think we should always g_return_val_if_fail() if any > non-integral type being passed in is NULL. Similarly, we should never return > NULL types. While it's tempting to allow NULL strings to mean "" and NULL > arrays to mean empty arrays... I'm fairly sure that some of the GLib functions consider NULL to be a valid GStrv already, and indeed can return it? I hope I'm wrong, though. dbus-glib certainly does accept NULL as an empty string on input, and existing practice seems to be to take advantage of that, so I can see porting being painful... but rejecting NULL here is clearly correct.
(In reply to comment #69) > Done in this commit > > http://git.gnome.org/cgit/glib/commit/?h=gdbus&id=562449f8432456cc1a1cb4646995a1ea4df3a169 > > but I only split out the headers. Not sure it makes sense to split the library > into two pieces as this causes unreasonable linker overhead and I haven't > thought about whether it even makes sense to split the library in two. The point of possibly splitting the library is that in the event of an ABI break, the automatically-gathered dependencies in e.g. Debian and Red Hat packages immediately tell distro maintainers what needs porting and what is unaffected. I suppose putting libdbus-1 in Requires rather than Requires.private might have a similar effect, but it relies on users of gdbus-lowlevel not using -Wl,-as-needed (if it's possible to access libdbus structs without calling libdbus functions, which it is for at least DBusError). > Anyway, getting to the functions exposing low-level stuff now requires > > - that gdbus-lowlevel-2.0.pc is used > - that gdbus/gdbus-lowlevel.h is included Given this... > - that G_DBUS_I_UNDERSTAND_THAT_ABI_AND_API_IS_UNSTABLE is defined ... I don't see what added value this provides (it's two pieces of boilerplate in each .h/.c instead of one). The point isn't really that the API/ABI is *unstable* as such, it's that it adds a dependency on an external library. Perhaps gdbus/libdbus-1.h would be a better name, as it neatly explains exactly what is being depended on (that gdbus currently uses libdbus-1).
(In reply to comment #63) > The idea is that GDBusProxy has methods > > g_dbus_proxy_invoke_method() > g_dbus_proxy_invoke_method_finish() > g_dbus_proxy_invoke_method_sync() > g_dbus_proxy_emit_signal() > > and a signal > > ::g-dbus-proxy-signal > > that is built around GType (everything but g_dbus_proxy_emit_signal() is > already in the branch, semi-baked and awaiting comments). I don't think this is sufficient for signals. The fact that dbus-glib can only bind to signals per-proxy is awkward, making some D-Bus APIs impossible (<https://bugs.freedesktop.org/show_bug.cgi?id=12773>, which for once wasn't filed by me :-) For instance, I don't think it should be necessary to dive into libdbus to get functionality like "call this callback whenever any process on my session bus emits org.freedesktop.Telepathy.ConnectionManager.NewConnection". dbus-python has a method on the BusConnection to bind to signals in arbitrary ways (you basically write a D-Bus match rule, but in Python rather than D-Bus syntax). Perhaps the nicest way in C would be to have an argument for each of sender, object path, interface, signal name etc., with each using NULL as a wildcard; that still doesn't give you arg0 matching support, which would need a more elaborate "extended" or "full" API. Because of C, you'd need to pass in a D-Bus signature and/or an array/valist of GTypes to tell GDBus what the signature of your callback is, and possibly also an unmarshaller. Language bindings might prefer a variation of this API that gives their callback a GValueArray, or an array of GVariant, or something. Having implemented this, it would make sense for the signal-receiving part of proxies to be a thin wrapper around it rather than reinventing it. This is how dbus-python works, if I remember correctly. I don't know why you've put signal emission here: my view is that "proxies" receive signals from D-Bus and emit method calls onto it, while "exported objects" emit signals and receive method calls (see dbus-python, dbus-glib, QtDBus etc.). There may be some value in having a method on the GDBus*Connection* to emit a signal with varargs (or a valist or a GValueArray or...), so you don't have to create an exported GObject just to emit a signal. APIs where an object's only reason for existence is to emit a signal and disappear again are quite a small use-case; on the other hand, it may turn out to make it easier to implement exported objects if you do it like this. For exported objects I think the approach we have in telepathy-glib is good - I haven't looked at the one in EggDBus yet. In telepathy-glib we generate one magical D-Bus-integrated GInterface per D-Bus interface, which only has what dbus-glib calls the "async" calling convention for all method implementations (even if we expect the method to return instantly, because it's just as easy to use in async mode, and more flexible). It'd be well worth using the same tricks as in telepathy-glib, so the GInterface generated by a D-Bus interface implements all unimplemented methods as "raise a D-Bus error" and is robust against method re-ordering (if this isn't done, we won't be able to use it in telepathy-glib in any case, since Telepathy's evolution relies on the ability to add methods that might fail). See http://smcv.pseudorandom.co.uk/2008/04/16/tp-svc/ (I should really blog about TpProxy at some point, too). While I'm on the subject of exporting objects, I think it may be worth having exported objects emit D-Bus signals by some mechanism that isn't a GObject signal; entangling the two is tempting, but can lead to problems. The current Telepathy C code relies on the fact that when a Channel emits Closed on D-Bus, it also emits 'closed' in GObject; this turns out not to be ideal, as seen in <http://bugs.freedesktop.org/show_bug.cgi?id=20946>. In telepathy-glib we generate a wrapper function to emit each D-Bus signal anyway (see blog post linked above), so we could just as easily do GDBus method call that isn't emission of a GObject signal.
(In reply to comment #70) > (In reply to comment #64) > > (In reply to comment #61) > > > The duality that NULL and { NULL } are both an > > > empty GStrv makes it unclear whether one or the other is guaranteed, or whether > > > you'll get one or the other at random. I suspect GStrv is sufficiently > > > entrenched in GLib that this isn't going to change, though. > > > > As a general rule, I think we should always g_return_val_if_fail() if any > > non-integral type being passed in is NULL. Similarly, we should never return > > NULL types. While it's tempting to allow NULL strings to mean "" and NULL > > arrays to mean empty arrays... > > I'm fairly sure that some of the GLib functions consider NULL to be a valid > GStrv already Yup. > , and indeed can return it? I hope I'm wrong, though. But.. the idiom for building a GStrv is something like this gchar **strv; GPtrArray *p; p = g_ptr_array_new (); while (<stuff>) g_ptr_array_add (p, <string>); g_ptr_array_new (p, NULL); strv = g_ptr_array_free (p, FALSE); which is compatible with what we want to do. Of the functions that return a GStrv, e.g. g_strsplit() g_strdupv() you always get a non-NULL value (OK, g_strdupv(NULL) gives you NULL which is expected; g_strdupv(strv) with non-NULL strv of length 0 returns non-NULL). So I think we're good here. > dbus-glib certainly does accept NULL as an empty string on input, and existing > practice seems to be to take advantage of that, so I can see porting being > painful... but rejecting NULL here is clearly correct. Yeah, I expect we're going to just warn on stderr etc. if people pass NULL. Should be easy for folks to just use G_DEBUG=fatal-warnings and then get a backtrace in gdb and fix up the code.
(In reply to comment #72) > (In reply to comment #63) > > The idea is that GDBusProxy has methods > > > > g_dbus_proxy_invoke_method() > > g_dbus_proxy_invoke_method_finish() > > g_dbus_proxy_invoke_method_sync() > > g_dbus_proxy_emit_signal() > > > > and a signal > > > > ::g-dbus-proxy-signal > > > > that is built around GType (everything but g_dbus_proxy_emit_signal() is > > already in the branch, semi-baked and awaiting comments). > > I don't think this is sufficient for signals. The fact that dbus-glib can only > bind to signals per-proxy is awkward, making some D-Bus APIs impossible > (<https://bugs.freedesktop.org/show_bug.cgi?id=12773>, which for once wasn't > filed by me :-) > > For instance, I don't think it should be necessary to dive into libdbus to get > functionality like "call this callback whenever any process on my session bus > emits org.freedesktop.Telepathy.ConnectionManager.NewConnection". > > dbus-python has a method on the BusConnection to bind to signals in arbitrary > ways (you basically write a D-Bus match rule, but in Python rather than D-Bus > syntax). Perhaps the nicest way in C would be to have an argument for each of > sender, object path, interface, signal name etc., with each using NULL as a > wildcard; that still doesn't give you arg0 matching support, which would need a > more elaborate "extended" or "full" API. > > Because of C, you'd need to pass in a D-Bus signature and/or an array/valist of > GTypes to tell GDBus what the signature of your callback is, and possibly also > an unmarshaller. Language bindings might prefer a variation of this API that > gives their callback a GValueArray, or an array of GVariant, or something. > > Having implemented this, it would make sense for the signal-receiving part of > proxies to be a thin wrapper around it rather than reinventing it. This is how > dbus-python works, if I remember correctly. There's g_dbus_connection_dbus_1_signal_subscribe() which should do this (commit 1a0a8700d6de0f36818ba4b3923330f28f6dc87f makes it possible to use pass NULL for the sender) but note this is low-level API, e.g. you need the separate .pc file etc. I guess we can add the C type mapping variants for this on GDBusConnection too though it is not really interesting for non-C languages. I guess there's a way to mark these methods as "not to be included in the introspection data" - Colin, Havoc?
(In reply to comment #74) > (In reply to comment #72) > > Having implemented this, it would make sense for the signal-receiving part of > > proxies to be a thin wrapper around it rather than reinventing it. This is how > > dbus-python works, if I remember correctly. > > There's g_dbus_connection_dbus_1_signal_subscribe() which should do this > (commit 1a0a8700d6de0f36818ba4b3923330f28f6dc87f makes it possible to use pass > NULL for the sender) but note this is low-level API, e.g. you need the separate > .pc file etc. Right, hence me saying "you shouldn't have do delve into libdbus in order to do this". :-) > I guess we can add the C type mapping variants for this on GDBusConnection too > though it is not really interesting for non-C languages. I guess there's a way > to mark these methods as "not to be included in the introspection data" - > Colin, Havoc? A fully type-mapped varargs version would indeed be uninteresting for non-C languages; if there isn't an annotation to represent "C bindings" like this, then there ought to be. :-P I'd rather not have all of GLib's numerous bindings into not-C required to use the low-level library either, hence my request for a "binding-friendly" version with a sequence of GVariant, or a sequence of EggDBusVariant, or a GValueArray, or something, that those bindings could use to extract the arguments and convert into whatever they wanted in the first place.
(In reply to comment #73) > Yeah, I expect we're going to just warn on stderr etc. if people pass NULL. > Should be easy for folks to just use G_DEBUG=fatal-warnings and then get a > backtrace in gdb and fix up the code. Aren't library usage errors meant to be a critical warning (return_if_fail)?
(In reply to comment #75) > I'd rather not have all of GLib's numerous bindings into not-C required to use > the low-level library either, hence my request for a "binding-friendly" version > with a sequence of GVariant, or a sequence of EggDBusVariant, or a GValueArray, > or something, that those bindings could use to extract the arguments and > convert into whatever they wanted in the first place. Presumably non-C bindings use their own type-mapping - for example for standard C++ you want to map e.g. a{ss} into std::map<std::string, std::string> and the point is that you want to do this without the overhead of going through GType. You also want to map variants and structures in different ways. Sure, you can say that since every language binding will need some kind of mapping from their native type system to GType _anyway_ (for introspection) you can do it like this: native <-> GType <-> D-Bus. But that's going to add complexity, heisenbugs and just generally make things less performant. So the thinking here really is that GDBusProxy GDBusStructure GDBusVariant and intended _only_ for the C type and object mapping and that's why they are in the "C Object Mapping" section of the GDBus API reference docs. So, basically, language bindings are *encouraged* to provide their own mapping but may use the native <-> GType <-> DBus shortcut if they so desire (for example Vala might want to do this). Anyway, what you are asking for makes sense - e.g. it is reasonable that C apps can subscribe to signals that are not tied to a proxy or name so I'm going to add the what's in [1] to gdbusconnection.h. Does this sound OK? [1] : /* The following is only for the C object mapping and should not be bound to other languages */ /** * GDBusSignalCallback: * @connection: A #GDBusConnection. * @name: The name of the signal. * @signature: The signature of the signal. * @args: A #GValueArray containing the arguments for the signal. * @user_data: User data passed when subscribing to the signal. * * Signature for callback function used in g_dbus_connection_signal_subscribe(). */ typedef void (*GDBusSignalCallback) (GDBusConnection *connection, const gchar *name, const gchar *signature, GValueArray *args, gpointer user_data); guint g_dbus_connection_signal_subscribe ( GDBusConnection *connection, const gchar *sender, const gchar *interface_name, const gchar *member, const gchar *object_path, const gchar *arg0, GDBusSignalCallback callback, gpointer user_data); void g_dbus_connection_signal_unsubscribe ( GDBusConnection *connection, guint subscription_id);
(I'm finding myself writing quite a lot per comment, so I'm going to split them up - my X server/intel driver has been unstable lately and I keep losing text in unscheduled reboots when X locks up :-( ) (In reply to comment #66) > (In reply to comment #61) > > (This type mapping stuff is presumably irrelevant if GVariant lands in GLib, > > since it would seem perverse to have a variant type whose data model is exactly > > that of D-Bus, but then not use it in the D-Bus bindings.) > > Btw, strongly disagree that a D-Bus<->GType mapping it's irrelevant in the > light of something like GVariant. GVariant is on the level on DBusMessage, e.g. > serialization, and I think 5+ years of collective experience has shown this > kind of API is not useful for exporting or consuming non-trivial D-Bus > services. You basically want something type-safe and something that maps to > well-known robust data types that you are already familiar with. That's my > opinion anyway. YMMV. As a D-Bus binding maintainer, I would disagree that GVariant is as hard to work with as DBusMessage. For a start, it has g_variant_n_children/g_variant_iter_init (rather than requiring you to use an iterator/sub-iterator), and you can easily pass around GVariants between functions (without copying - since they're ref-counted - or requiring that they're at the top level). It looks like rather a nice data structure actually, and I think I might recognise some of its ideas from conversations I had with Alp about ndesk-dbus :-) Admittedly, some convenience API for dealing with maps would be nice (desrt, are you listening? :-) Since each GVariant is immutable, if we want map lookups to be better than O(n), it could lazily allocate an index (a map with keys borrowed from the data and values pointing into the data) the first time g_variant_map_lookup() was called; so the first lookup would be O(n) (the same complexity as unmarshalling into a GHashTable), and subsequent lookups would be O(log n) or better. I agree that there could be some value in using marshallers or libffi to marshal/unmarshal the "first level" of arguments, and make "simple" D-Bus method calls look like "simple" C function calls: void tp_badger_call_add_mushrooms (TpBadger *proxy, const gchar *species, guint n_mushrooms, [the usual async-call rubbish]); but once you get into complex data structures I don't really see the advantage of using GPtrArray, GArray, and GValueArray. Suppose we add an a(ssss) argument to that function (all onomatopoeia intended). We could have something like: (Excuse my C++ template notation, I've been doing Telepathy Qt bindings recently) void tp_badger_call_add_mushrooms (TpBadger *proxy, const gchar *species, guint n_mushrooms, GVariant<a(ssss)> *snake_descriptions, ...) just as easily as dbus-glib's void tp_badger_call_add_mushrooms (TpBadger *proxy, const gchar *species, guint n_mushrooms, GPtrArray<GValueArray<gchar*,gchar*,gchar*,gchar*>> *snake_descriptions, ...) or EggDBus' void tp_badger_call_add_mushrooms (TpBadger *proxy, const gchar *species, guint n_mushrooms, EggDBusPtrArray<EggDBusStructure<ssss>> *snake_descriptions, ...) The fact that C doesn't really have templates means that we haven't lost any significant amount of type-safety, since all the <> are invisible to the compiler anyway, and GVariant is just as able to do runtime type checking as EggDBusPtrArray (and considerably better at it than GPtrArray/GValueArray). This applies doubly to GArray, where the compiler can't even know check that the programmer knows how large the elements of the array are.
One argument for a GVariant-like approach is the number of copies. It seems rather unlikely that there are many existing APIs which just so happen to be an exact match for the types that GDBus (or dbus-glib) wants, so for an API with an existing data model, the options are: * take your existing data, marshal it into GPtrArray/GArray/GValueArray (most likely, a copy), pass it to the binding, let the binding marshal it into a message (copy) and send it * take your existing data, marshal it into something whose layout matches a message (copy), pass it to the binding and let it marshal it into a message (*) and send it and if you have the freedom to change the format of the existing data, you can either: * store your data in GPtrArray/GArray/GValueArray like the binding wants, pass it to the binding, let the binding marshal it into a message (copy) and send it * store your data in something whose layout matches a message, pass it to the binding and let it marshal it into a message (*) and send it In the current libdbus, (*) would have to be another copy, because DBusMessage always owns its own buffer, but in the approach taken by ndesk-dbus and (as far as I can tell) desrt's GVariant-based GBus, it can become a series of writes straight into the socket, which Linux can make zero-copy under the right circumstances. I assume that if we continue to use libdbus, it would be reasonable for it to have API for sending the payload of a message, with which we could do zero-copy message sending; if we move to something like GBus, then happily, it's already been implemented. In Telepathy, because D-Bus is such a core part of what we do, in practice we end up with a lot of our data structures being exactly the ones dbus-glib wants, with various unnecessary deep-copies via the GObject property mechanism. I don't think using GVariant would be any harder for us than EggDBusVariant. libdbus and GVariant also avoid copying arrays of fixed-length data, but a GObject mapping that used gint to represent int32 would always have to copy int32 arrays on ILP64 architectures (do we have any of those yet? I suspect it's only a matter of time), and a GObject mapping using GArray would have to copy all received fixed-length arrays at least once anyway in order to get them in a GArray.
(In reply to comment #68) > Red Hat used to get tons of bugs about stuff persisting after logout (gconfd > and such), drives sysadmins insane. I'm still not convinced that "ensure all my processes commit suicide when they should" is in-scope for an inter-process communication system. YMMV.
> I'm still not convinced that "ensure all my processes commit suicide when they > should" is in-scope for an inter-process communication system. YMMV. dbus is not an IPC system generically, it's a specific sort of IPC system. Lifecycle management has been explicitly in the short description of "the point" of dbus from day 1. http://dbus.freedesktop.org/doc/dbus-faq.html#other-ipc http://dbus.freedesktop.org/doc/dbus-specification.html#introduction http://dbus.freedesktop.org/doc/dbus-tutorial.html#uses From tutorial: "D-Bus is designed for two specific cases: * Communication between desktop applications in the same desktop session; to allow integration of the desktop session as a whole, and address issues of process lifecycle (when do desktop components start and stop running). * Communication between the desktop session and the operating system, where the operating system would typically include the kernel and any system daemons or processes." Note, "address issues of process lifecycle (when do desktop components start and stop running)" From spec: "D-Bus is designed for two specific use cases ... [session and system bus] ... D-Bus is not intended to be a generic IPC system for any possible application, and intentionally omits many features found in other IPC systems for this reason. ... D-Bus may turn out to be useful in unanticipated applications, but future versions of this spec and the reference implementation probably will not incorporate features that interfere with the core use cases." Note, "will not incorporate features that interfere with core use cases" You get the idea - this has always been an explicit dbus goal. It was one of the main reasons for switching from corba in fact.
(In reply to comment #65) > First of all, const in C is kind of a mess (it's slightly better in C++). I'm aware of that - "nobody understands const in C, part 17,326" is a frequent utterance on #telepathy :-) - but this shouldn't stop us from letting the compiler help us not to make mistakes. Having const annotations at least in the cases where they happen to be OK is a useful piece of documentation, if nothing else, and GArray/GPtrArray happen to be things that work OK when const. > Anyway, actually the intention here is that the user is free to change the > value. One example is actually the ChatRoom example we talked about on > dbus-list some time ago. I'll recap the general idea here. I'm amused that your example is basically a mini-Telepathy :-) > The idea here is that you have a remote object that implements the org.ChatRoom > interface. Now, the participants in the chat room are available as a property, > :Participants, say, with signature a{ss}. > > For performance reasons you don't really want to emit PropertiesChanged() every > time this happens because users part and join every few seconds and the > participant list can get huge so it's a lot of data to transfer all the time. > So you have other signals ParticipantJoined(ss) and ParticipantParted(ss). Right, we have exactly this pattern for Group members in Telepathy (there are several disjoint sets of members in different states, plus a MembersChanged signal). > The idea here is that you can subclass the generated proxy class for > org.ChatRoom like [1] to synthesize signals for properties changed. ... > static void > on_dbus_signal (GDBusProxy *proxy, > const gchar *signal_name, > const gchar *signature, > GValueArray *args) > { > GDBusVariant *value; > GPtrArray *p; > > value = g_dbus_proxy_get_cached_property (proxy, > "Participants", > NULL); > g_assert (g_dbus_variant_is_ptr_array (value)); > p = g_dbus_variant_get_ptr_array (value); > // modify p here using @args > g_value_unref (value); > g_signal_emit_by_name (proxy, "g-dbus-proxy-properties-changed"); > } Library-user-mutable data owned by the generated proxy worries me, to be honest - it's a big piece of implementation-detail being pushed into the API. I also don't think it'll always, or even often, be the case that the ideal format for the cached version of this sort of data is exactly how GDBus (or dbus-glib, or for that matter QtDBus) decides it wants to store it. In the Telepathy version of this API, as mapped in telepathy-glib and telepathy-qt4, the property and the signal are both in terms of arrays of handles (roughly, ref-counted GQuarks) with some metadata attached, but what we really want to store is a set (unordered collection) of TpContact objects or Tp::Contact objects (respectively). If we were being more OO about our API for contacts (contacts in Telepathy use the IDs-for-objects anti-pattern, for performance reasons), we'd be using be an array of object paths, and we'd still want to translate that into an array of actual objects. Once we have the array of actual objects, there's no point caching the array of object paths any more. We definitely do want to use D-Bus properties, because the inherent namespacing gives us extensibility, and GetAll gives us round-trip reduction. I think our ideas about what a D-Bus <property> means might differ from yours, but as implementors of one of the larger D-Bus APIs, I hope our opinions about <property> carry some weight :-) I need to escape from the office now, but I'll post some thoughts on client-side proxies tomorrow. TpProxy in telepathy-glib was originally designed as a workaround for dbus-glib deficiencies, but has turned into a sufficiently useful abstraction that telepathy-qt4 essentially just translates it into C++.
Regarding GVariant and type mapping: My view is that we're basically talking about what C types to use for complex types. This is going to be a long reply, but just for spoilers, I think we can both have what we want. So anyway, the code in the GDBus branch basically map (complex) D-Bus types to GArray, GPtrArray and GHashTable because, hey, that's what people are used to in GLib and these data types are efficient and work reasonably well. For example, for a D-Bus method interface Bar { Frobnicate (IN as strv, IN a{s(ii)} hash_of_structs, IN ay array_of_ints, IN a(ii) array_of_structs, OUT v some_variant) }; will map to (with FooBar being the generated GInterface - also, I'm using _sync() methods to keep things simple, exactly same thing applies to async methods) gboolean foo_bar_invoke_frobnicate_sync (FooBar *bar, gchar **strv, GHashTable *hash_of_structs, GArray *array_of_ints, GPtrArray *array_of_structs, GDBusVariant **out_some_variant, GCancellable *cancellable, GError **error); with suitable API docs explaining that @hash_of_structs needs to be a hash from strings into GDBusStructure. Let us consider what the GVariant thing would look like (I'm assuming level-0 stuff will be split into arguments otherwise it'd be just DBusMessage all over again): gboolean foo_bar_invoke_frobnicate_sync (FooBar *bar, GVariant **strv, GVariant *hash_of_structs, GVariant *array_of_ints, GVariant *array_of_structs, GVariant **out_some_variant, GCancellable *cancellable, GError **error); So the idea basically is that GVariant is the new universal container type, isn't it? For a multitude of reasons I don't think this API is desirable; you are throwing away type-safety in favor of performance. While I can sympathize with this, I'm just not sure it's a good *default* and it's certainly not API I'd want to use myself. I'd just use DBusMessage instead ;-) (handwaving here, no-one prevents us from enhancing DBusMessage to be like GVariant) Anyway, as I said initially I don't think our design should prevent this kind of API should you really want it for whatever reason. (And I've never actually said GVariant was unwanted, I only said it's not exactly the API I'd want to use myself or push on others by default (GVariant kind of requires you to get things right; there's no compiler helping you if you get the arg order for foo_bar_invoke_frobnicate_sync() wrong. Btw, I also dislike the name GVariant because it leads people to believe it is a variant datatype instead of a serialization format. And certainly, having both GDBusVariant (which is a true variant) and GVariant is for me a non-starter. Suggest to call it GBinaryBlog, GMessage or something else. But that's up to the GLib maintainers I suppose) So I think the key observation here is that the implementation of foo_bar_invoke_frobnicate_sync(), e.g. the generated code is going to be super simple. Indulge me gboolean foo_bar_invoke_frobnicate_sync (FooBar *bar, gchar **strv, GHashTable *hash_of_structs, GArray *array_of_ints, GPtrArray *array_of_structs, GDBusVariant **out_some_variant, GCancellable *cancellable, GError **error) { return g_dbus_proxy_invoke_method_sync ( G_DBUS_PROXY (bar), "Frobnicate", "asa{s(ii)}aya(ii)", "v", -1, cancellable, error, G_TYPE_STRV, strv, G_TYPE_HASH_TABLE, hash_of_structs, G_TYPE_ARRAY, array_of_ints, G_TYPE_PTR_ARRAY, array_of_structs, G_TYPE_INVALID, G_TYPE_DBUS_VARIANT, out_some_variant, G_TYPE_INVALID); } and for the GVariant case it could be ... error, G_TYPE_VARIANT, strv, G_TYPE_VARIANT, hash_of_structs, G_TYPE_VARIANT, array_of_ints, G_TYPE_VARIANT, array_of_structs, G_TYPE_INVALID, G_TYPE_VARIANT, out_some_variant, G_TYPE_INVALID); Now, *nothing* prevents us from teaching the D-Bus <-> GType type mapper about G_TYPE_VARIANT and just doing the right thing. You'd just annotate your IDL (or introspection XML) with the desired GType you want. You could even mix and match or specify defaults to the code generator. If we were we to do this, we'd use GVariant to build the message blob to send to libdbus-1 to get this onto the wire. We'd also need to change GDBusVariant to take a ref to the message blob instead of storing a GValue (to avoid the performance overhead). Sure, this is a lot of handwaving, someone (you or Ryan perhaps, I'm not exactly signing up because I don't think it's interesting in the first place but that's just me ;-) would actually need to tweak the type mapper and do the requisite libdbus-1 changes. Anyway, another observation is that some tweakability in the type mapper is needed. For example, it would be nice to add annotations etc. to IDL to specify whether you want a GStrv or a GPtrArray of strings. And Vala might use libgee containers or something else.
(In reply to comment #79) > One argument for a GVariant-like approach is the number of copies. Your analysis is somewhat flawed, not sure how I see using GVariant as a container type instead of GHashTable, GPtrArray and GArray *really* changes anything. I mean, GVariant will need a place to store pointers too. Also, FYI (see bug 580450), GHashTable, GArray and GPtrArray are refcounted, not deep-copied on _ref() and no-one forces you to inserted copied data into these any more than with GVariant. The only real difference, I think, is that - GArrays will incur a single memcpy() into the DBusMessage instance - GStrv is deep-copied, we might want to change to using GPtrArray just for this reason - or at least make it possible to do (cf. last paragraph of comment 83) where performance is a concern (don't try to prematurely optimize things). The point about not having to copy data into a DBusMessage makes sense but as pointed out in comment 83, I don't think trading off optimization for type-safety and familiar types is worth *by default*. I'd also like to see real profiles from real programs in the field indicating this is a bottleneck...
a couple of people have been asking if i could chime in here. i only have a couple of comments. (In reply to comment #83) > gchar **strv, type safety. it's nice. i like this. > GHashTable *hash_of_structs, where's the type safety? what comes out of the hash? > GArray *array_of_ints, > GPtrArray *array_of_structs, ditto.... this is actually much worse than GVariant for type safety because now when you pull the wrong pointer type out of the GHashTable, you get no errors at all (not even runtime assertions or g_critical). you can try and add some hacks to deal with this (like annotate a GHashTable with GTypes of its key/value) but when you're dealing with complex dbus types for these things, that will only get you so far. > Let us consider what the GVariant thing would look like (I'm assuming level-0 > stuff will be split into arguments otherwise it'd be just DBusMessage all over > again): > > gboolean foo_bar_invoke_frobnicate_sync (FooBar *bar, > GVariant **strv, > GVariant *hash_of_structs, > GVariant *array_of_ints, > GVariant *array_of_structs, > GVariant **out_some_variant, > GCancellable *cancellable, > GError **error); this is a very strong misrepresentation. If you look at GBus it's setup more like: foo_bar_invoke (GBusMessage *message) { const gchar *string, const int *int_array; GVariant *some_variant; g_bus_message_get (message, "s&aiv", &string, &int_array, &some_variant) ... do stuff here ... } magic format strings at the start vs. interleaving of G_TYPE_CONSTANTS is a point of personal preference. i like the printf style, myself. > throwing away type-safety in favor of performance. this is a red herring. dealing with awkwardly-constructed arrays of structure types or hash tables of pointers to gvalues (or gdbusvariants) (etc, etc) is a mess. gvariant might be slightly more typing and "less familiar" than using those types, but you absolutely cannot argue that it is not a better fit. i dont' really understand the idea of wanting to hide the fact that you're doing RPC using dbus (and the dbus type system) under "familiar types". > (And I've never actually said GVariant was unwanted, I only said it's not > exactly the API I'd want to use myself or push on others by default (GVariant > kind of requires you to get things right; there's no compiler helping you if > you get the arg order for foo_bar_invoke_frobnicate_sync() wrong. that's more or less true, but on the other hand, it doesn't require you to generate code. g_variant_new/get (and the gbus calls that are based on them) work rather similarly to how g_object_set/get work. we have no type safety there and people cope. varargs are a little evil, but often they are just the easiest way to do things in C. > Btw, I also dislike the name GVariant because it leads people to believe it is > a variant datatype instead of a serialization format. And certainly, having > both GDBusVariant (which is a true variant) and GVariant is for me a > non-starter. Suggest to call it GBinaryBlog, GMessage or something else. But > that's up to the GLib maintainers I suppose) can you explain why you believe that these two things are different in terms of the API that they present to the user? the fact that gvariant has an implementation that supports efficient (de)serialisation under its API doesn't mean that that API is not that of a variant datatype... > and for the GVariant case it could be > > ... > error, > G_TYPE_VARIANT, strv, > G_TYPE_VARIANT, hash_of_structs, > G_TYPE_VARIANT, array_of_ints, > G_TYPE_VARIANT, array_of_structs, > G_TYPE_INVALID, > G_TYPE_VARIANT, out_some_variant, > G_TYPE_INVALID); assuming you use exactly your API, maybe. but why not this way?: G_TYPE_VARIANT, "as", strv, G_TYPE_VARIANT, "a{s(...)}", hash_of_structs, G_TYPE_VARIANT, "ai", array_of_ints, ... or, even do it the way that gbus actually does it: g_bus_call_async (..., ..., "asaiv", array_of_strings, array_of_ints, variant); gvariant might be nice for performance reasons, but that's not the point. a gvariant is a value that goes over dbus. we're talking about doing IPC over dbus. why are we trying to hide that behind friendly types that aren't a great fit?
(I've finally got round to writing a blog post about telepathy-glib's approach to a D-Bus object model, which we've also been reimplementing in telepathy-qt4: <http://smcv.pseudorandom.co.uk/2009/05/tp-proxy/>. You might find it interesting; I'll post edited highlights here later when discussing object mappings, but for now I think this GType stuff is more fundamental.) > Also, FYI > (see bug 580450), GHashTable, GArray and GPtrArray are refcounted, not > deep-copied on _ref() and no-one forces you to inserted copied data into these > any more than with GVariant. While we're on the subject of implementation details, I don't see how GPtrArray and GHashTable can possibly be used safely as a collection of pointers into a DBusMessage, given that they are now refcounted rather than deep-copied, but there seems to be no way for them to reference the DBusMessage and release it when freed (a pointer into a DBusMessage's internal buffer is not "simply freeable", to use Rob McQueen's terminology from dbus-glib - even if the GPtrArray owns one ref to the DBusMessage per element of the array, a destructor that invoked dbus_message_unref would need an extra argument that is not the pointer). The concrete difference in my mind between GVariant and any other GType mapping, though, is that GVariant's data model *is* D-Bus's data model, rather than some representation or mapping of it. This makes it uniquely useful to represent an array of D-Bus data losslessly, in its original form, which can be transformed into whatever you like if you so wish. The type safety argument is one that I have a lot of sympathy for, but I think there's a hierarchy for what should happen when you get your types wrong: compiler error > runtime error > segfault > silent corruption Mixing up uses of GVariant-as-array and GVariant-as-struct can't give you a compiler error, that's true, but it can at least give you a legible runtime error, which is better than the segfault you'll generally get from misinterpreting the value in a GHashTable (is it a GPtrArray? is it a GValueArray? is it a GValue?), and far better than the silent corruption that often results from incorrect varargs. Mixing up two different purposes of a GHashTable (e.g. one is string => string and the other is string => variant) won't be caught by the compiler and is harder to diagnose than an assertion failure in GVariant. Familiar types are a bit of a red herring, in my opinion; one C sequence/map API is much like any other, and pulling data out of a GVariant doesn't seem to be any harder than a GArray (apart from the map lookup API I mentioned above, which could be added). I have a more fundamental philosophical objection to GType mapping, though, which is that however you want to choose to represent the D-Bus data model, its data model is not GType, it's something else. GVariant's data model, by contrast, is identical to D-Bus' (actually it's a superset, which does worry me a bit, but that just means we need to remove or #ifdef its extensions unless/until we have a decent plan for getting it supported throughout D-Bus). I feel as though I've been here once before, when I took over dbus-python from J5, and I've seen various of the failure modes in OLPC/Sugar code. The problems with mapping from "native" types to D-Bus types are that each native type has to marshal to no more than one D-Bus type, and you have to cope with the inevitable attempts to marshal native types that cannot be represented on D-Bus (None is the usual example in Python, but more on this below). I've been here in Python, and it's really not pretty. dbus-python can be told explicitly what D-Bus types to use, but in the absence of that information, it tries to guess what D-Bus types the user wanted, using a combination of Introspect() data (which makes dbus-python clients remotely crashable by a malicious or simply buggy service!) and blatant guesswork. In practice, this tends to fail in highly subtle ways. The way to force the issue and make the code actually reliable is to pass in D-Bus types explicitly when sending anything. This means that if the design goal of dbus-python was to hide D-Bus from the API user, then it has failed :-) In the GLib case, if you just have G_TYPE_HASH_TABLE to disambiguate how to encode a{a{ss}}, then you've determined the C type for the outer dict. How is the inner dict encoded? The GType can't tell you, and you're back with using EggDBus' default mapping. If that happens to match what the library user wanted, great; if not, they'll have to adapt to it anyway, so why was it an advantage for them to specify their own GType? Similarly, for variants we don't even know what the other process will be sending us, so we have to have EggDBus' default mapping and hope it's good enough. If you're using GTypes for disambiguation, I think the only possible way to get everything you want is to synthesize one GType for each D-Bus type + concrete representation pair, which is dbus-glib's approach; this turns out not to be a great idea either (I'm sure Rob and I can elaborate on this if you want). Conversely, the problem with mapping D-Bus types into "native" types, apart from some copying, is that each D-Bus type has to unmarshal to exactly one native type. The first issue with this is that some types may be unrepresentable - I'm pleased to see that you plan to fix this by introducing new GTypes for int16, uint16, D-Bus object path, and D-Bus signature (in my view that's the only sensible solution). The next problem is what happens when there's more than one possible "native" representation for something. Do you want your string array to be a GStrv or a GPtrArray of gchar* ? Or, in fact, is what you really want a GHashTable<gchar*,dummy> because your D-Bus array of strings really represents a set of strings? (Telepathy has a lot of arrays of uint32 which should really be sets, and uses QSet<uint> for them in telepathy-qt4.) I'm sure EggDBus allows for writing an alternative unmarshaller for a different GType, but again, that doesn't work for variants, the GType isn't expressive enough for containers unless you go the dbus-glib route and synthesize a pile of extra GTypes, and there's quite a heavy documentation cost to "more than one way to do it". I've been here in Python - you can choose via keyword arguments to unmarshal byte arrays as either an Array of Byte (subclasses of: list of int), or as a ByteArray (string subclass). This seemed like a great idea when I first wrote it, but I've since realised that it's needlessly confusing. Similarly, dbus-glib lets you write alternative marshallers and unmarshallers (in theory), but I don't know anyone who has ever used this feature, and you still have to deal with its default choice of representation as soon as you have a variant in your API (like the ubiquitous a{sv}). More fundamental than all this, I don't think *hiding* D-Bus should be a goal of *any* of its object mappings - I'm all for making it easier, but I don't think making it transparent is either achievable or desirable. I realise this may be controversial to some, but I think it's the only way. The API of any D-Bus service (or client) is written in terms of the D-Bus wire protocol, which means the D-Bus data model. It simply isn't possible to interoperate with something written in a different toolkit, language or even binding if you can't agree on whether what is sent over the wire is an a{sv} or an a{s(uuai)}, and it makes me sad every time I see a Python or GObject programmer describing their D-Bus API as "(string, dict) -> int" or "(string, GHashTable<guint,GStrv>) -> guint". I realise most D-Bus APIs are currently written for private communication between the foo daemon and the foo client (which happen to share a lot of libraries and a development team, and probably have one bus name and one object with one interface), but I think that's a bug (or at best, an incomplete implementation) more than it's a feature. Sooner or later, someone else will want to interoperate with foo (isn't that the idea of freedesktop technologies?) and will discover that the foo developers don't actually know what their own public API is. Given that for interoperability, you have to have some idea what your D-Bus API is, it's really not an unreasonable burden to mention it in your source code (and indeed davidz's API proposals all seem to do so). I personally find 'as' considerably *easier* to read than either DBUS_TYPE_ARRAY_AS_STRING DBUS_TYPE_STRING_AS_STRING or (G_TYPE_ARRAY, G_TYPE_STRING), and I think using mnemonic characters for the D-Bus types was a brilliant piece of design. Similar micro-languages made up of mnemonic characters can be found in standard C (printf and friends), the Java standard library, the C# standard library, the Python standard library (the struct module), the CPython runtime (PyArg_ParseTuple and Py_BuildValue), Perl, PHP (the date function), and even Telepathy's XMPP implementation (we have a varargs-based micro-language to compose XML data structures, with '(' and ')' to open and close elements, '@' to introduce attributes, '*' to store a pointer to the current node and so on). As a practical benefit of making it completely clear that the D-Bus type system is what's being used, it's hopefully quite obvious what you can and can't put in your variant when populating an a{sv} - in particular, no more developers becoming surprised when they can't send their Python datetime object or their tree of GNodes over D-Bus without first thinking about the correct representation.
Hey Ryan! (In reply to comment #85) > a couple of people have been asking if i could chime in here. i only have a > couple of comments. > > (In reply to comment #83) > > gchar **strv, > type safety. it's nice. i like this. > > > GHashTable *hash_of_structs, > where's the type safety? what comes out of the hash? > > > GArray *array_of_ints, > > GPtrArray *array_of_structs, > ditto.... > > this is actually much worse than GVariant for type safety because now when you > pull the wrong pointer type out of the GHashTable, you get no errors at all > (not even runtime assertions or g_critical). Uh, this is no different from other GLib programs and we've been able to cope with this without problems (it's not like people have had a need to peek at the GType of hash table kes or values). Typically naming the variable in a sensible way goes a long way. Also, the type is part of the programming contract (e.g. D-Bus interface) and the generated docs for this will tell the user how to use it, e.g. it is entirely possible to autogenerate docs that says @array_of_structs: An array of #GDBusStructure holding for the signature (ii). Free with g_ptr_array_unref(). or even better @array_of_structs: An array of #FooPair structures. Free with g_ptr_array_unref(). with FooPair being a typedef for (or subclass of) GDBusStructure with foo_pair_new(), foo_pair_get_first(), foo_pair_get_second() being the type-safe convenience API. I don't think saying "much worse than GVariant" is a fair comment either - you already know the D-Bus signature of the type per the programming contract. > > throwing away type-safety in favor of performance. > > this is a red herring. dealing with awkwardly-constructed arrays of structure > types or hash tables of pointers to gvalues (or gdbusvariants) (etc, etc) is a > mess. gvariant might be slightly more typing and "less familiar" than using > those types, but you absolutely cannot argue that it is not a better fit. Hey, I don't think saying that one is better than the other can be an absolute truth or something like that. It's mostly a question of taste what kind of APIs you prefer (type-safe vs. varargs vs. serialization). It also depends on how careful you are (for example you want safe APIs to give to your rank and file programmers while rockstars get it right the first time with varargs based APIS) and, of course, how complex the service is. For example, for something simple like hooking up to Rhythmbox, I'd think that generating code for a few method calls and properties would be overkill. So I'd use GDBusProxy directly. However dealing with APIs like http://hal.freedesktop.org/docs/DeviceKit-disks/Device.html or, for that matter, the Telepathy or NetworkManager stacks, I really don't want that. And I want my D-Bus clients to fail at the compilation level when I'm changing stuff, otherwise things will keep breaking. > that's more or less true, but on the other hand, it doesn't require you to > generate code. g_variant_new/get (and the gbus calls that are based on them) > work rather similarly to how g_object_set/get work. we have no type safety > there and people cope. varargs are a little evil, but often they are just the > easiest way to do things in C. All classes normally provide these socalled "C bindings" for properties that will give you type-safety so that doesn't really apply. Sure, if I'm in a hurry, I use g_object_get|set() myself but it doesn't really scale I think. It's not like these "C bindings" are there just because someone felt like adding it, it's there because g_object_set|get isn't really a good cut if you write a lot of production quality code. > but why not this way?: > > G_TYPE_VARIANT, "as", strv, > G_TYPE_VARIANT, "a{s(...)}", hash_of_structs, > G_TYPE_VARIANT, "ai", array_of_ints, Can do, I'm not opposed to changing it > gvariant might be nice for performance reasons, but that's not the point. a > gvariant is a value that goes over dbus. we're talking about doing IPC over > dbus. why are we trying to hide that behind friendly types that aren't a great > fit? Turns out that if you do non-trivial D-Bus services, then using a varags based API isn't really a winner for the reasons stated above. I also want the values from the remote service to be available in a data type that I'm already familiar with so I can use operations, routines and idioms that are well-established for said data types (g_ptr_array_sort(), g_hash_table_foreach(), exo-ish property bindings, notify::foo and so on). I'm not really sure we have a lot of things to disagree on here. If people want to use GVariant as the new super-container to rule them all the way to the white house (prez punt!), more power to them and they can either use GDBusProxy directly or annotate the XML/IDL to specify that type as I suggested in comment 83. But please realize that not everyone wants to trade in GPtrArray, GArray, GHashTable just because you think that a varargs API is the only way forward. FWIW, am not really interested in discussing varargs vs. generated code if it's not clear already - I mean, it's emacs vs vi all over again ;-) - I think, instead, it would be a lot more useful to figure out how to make something like GVariant fit into this work as a first class citizen so the hand-waving in comment 83 can become something more concrete and people can start using it if they so desire.
Re: GVariant and how it might fit in here, I just posed on gtk-devel-list http://mail.gnome.org/archives/gtk-devel-list/2009-May/msg00008.html since I think it's a topic that requires broader discussion than this bug report. Once we have some input from there, we should resume that part of the D-Bus<->GType discussion. (This bug report is starting to get unwieldy so that's also one reason for deferring the GVariant discussion to the mailing list.)
This bug is FIXED, isn't it? :)