After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 698375 - [gdbus] better support for async interface implementation
[gdbus] better support for async interface implementation
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gdbus
unspecified
Other All
: Normal normal
: ---
Assigned To: David Zeuthen (not reading bugmail)
gtkdev
Depends on:
Blocks:
 
 
Reported: 2013-04-19 15:44 UTC by Allison Karlitskaya (desrt)
Modified: 2013-06-22 17:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GDBusConnection: remove an unused g_variant_get() (1018 bytes, patch)
2013-04-19 15:45 UTC, Allison Karlitskaya (desrt)
none Details | Review
GDBusConnection: move 'Set' typecheck to worker (4.18 KB, patch)
2013-04-19 15:45 UTC, Allison Karlitskaya (desrt)
none Details | Review
GDBusConnection: some straight-up refactoring (6.82 KB, patch)
2013-04-19 15:45 UTC, Allison Karlitskaya (desrt)
none Details | Review
GDBusMethodInvocation: add 'property_info' (8.77 KB, patch)
2013-04-19 15:45 UTC, Allison Karlitskaya (desrt)
none Details | Review
GDBusConnection: allow async property handling (16.00 KB, patch)
2013-04-19 15:45 UTC, Allison Karlitskaya (desrt)
none Details | Review
GDBusMethodInvocation: add property return checks (3.19 KB, patch)
2013-04-19 15:46 UTC, Allison Karlitskaya (desrt)
none Details | Review

Description Allison Karlitskaya (desrt) 2013-04-19 15:44:51 UTC
The current state of the art is pretty bad.  See patches.
Comment 1 Allison Karlitskaya (desrt) 2013-04-19 15:45:25 UTC
Created attachment 241921 [details] [review]
GDBusConnection: remove an unused g_variant_get()
Comment 2 Allison Karlitskaya (desrt) 2013-04-19 15:45:35 UTC
Created attachment 241922 [details] [review]
GDBusConnection: move 'Set' typecheck to worker

We presently do a lot of checks on property sets (signature check,
correct interface, property exists, etc.) from the worker thread before
dispatching the call to the user's thread.  The typecheck, however, is
saved until just before calling the user's vfunc, in their thread.

My best guess is that this was done to save having to unpack the value
from the tuple twice (since we don't unpack it until we're just about
the call the user).

This patch moves the check to the same place as all of the other checks.

The purpose of this change is to allow for sharing this check with the
(soon-to-be-introduced) case of handing property sets from
method_call().

This change has a minor side effect: error messages generated by sending
invalid values to property sets are no longer guaranteed to be correctly
ordered with respect to the void returns from successful property sets.
They will instead be correctly ordered with respect to the other error
messages.
Comment 3 Allison Karlitskaya (desrt) 2013-04-19 15:45:42 UTC
Created attachment 241923 [details] [review]
GDBusConnection: some straight-up refactoring

Separate the code for validating a method call from the code for
actually scheduling it for dispatch.

This will allow property Get/Set/GetAll calls to be dispatched to the
method_call handler without duplicating a lot of code.
Comment 4 Allison Karlitskaya (desrt) 2013-04-19 15:45:47 UTC
Created attachment 241924 [details] [review]
GDBusMethodInvocation: add 'property_info'

Add a field on GDBusMethodInvocation for GDBusPropertyInfo.

For now, it is always %NULL.  It will be set in future patches.
Comment 5 Allison Karlitskaya (desrt) 2013-04-19 15:45:53 UTC
Created attachment 241925 [details] [review]
GDBusConnection: allow async property handling

The existing advice in the documentation to "simply" register the
"org.freedesktop.DBus.Properties" interface if you want to handle
properties asynchronously is pretty unreasonable.  If you want to handle
this interface you have to deal with all properties for all interfaces
on the path, and you have to do all of the checking for yourself.  You
also have to provide your own introspection data.

Introduce a new convention for dealing with properties asynchronously.

If the user provides NULL for their get_property() or set_property()
functions in the vtable and has properties registered then the
properties are sent to the method_call() handler.  We get lucky here
that this function takes an "interface_name" parameter that we can set
to "org.freedesktop.DBus.Properties".

We also do the user the favour of setting the GDBusPropertyInfo on the
GDBusMethodInvocation for their convenience (for much the same reasons
as they might want the already-available GDBusMethodInfo).

Add a testcase as well as a bunch of documentation about this new
feature.
Comment 6 Allison Karlitskaya (desrt) 2013-04-19 15:46:04 UTC
Created attachment 241926 [details] [review]
GDBusMethodInvocation: add property return checks

Add some type checking for the values returned from async property
handling calls, similar in spirit to the type checking we do for normal
method calls.
Comment 7 David Zeuthen (not reading bugmail) 2013-04-19 17:57:57 UTC
(In reply to comment #0)
> The current state of the art is pretty bad.  See patches.

Just FWIW, it's done intentionally this way; I firmly believe that encouraging people to have async property getters and setters is a Bad Idea(tm). That's why it's so hard.

FWIW, the Right Way(tm) to think about D-Bus properties, is that they're cheap to get (and you get them in batches), very cacheable (so they need to be small) and you get notified when they change. Anything that doesn't fit this bill, use a D-Bus method e.g. add a Foo.GetBigBlob() method (and possibly a Foo::BigBlobChanged() signal) instead of Foo:BigBlob property.

Of course, D-Bus is a cross-desktop thing (and even used beyond the desktop), so you'll get as many opinions about what a D-Bus property is and isn't from as many people as there are... well, you know what I mean... But, anyway, that's my 50,000 feet point of view :-).

That said, I'm not opposed  to your patch of punting to the method_call() handler if get_property and set_property are both NULL.

(Btw, I wish we could all just use things like gdbus-codegen(1) instead of using low-level interfaces. Oh well.)
Comment 8 Allison Karlitskaya (desrt) 2013-04-19 18:35:57 UTC
The main reason that we're doing this is for accountsservice.  We want to do a polkit check for setting properties and return failure if that fails.  This should be done async, of course...

See https://bugs.freedesktop.org/show_bug.cgi?id=63733 about that.
Comment 9 David Zeuthen (not reading bugmail) 2013-04-19 18:55:53 UTC
(In reply to comment #8)
> The main reason that we're doing this is for accountsservice.  We want to do a
> polkit check for setting properties and return failure if that fails.  This
> should be done async, of course...

This is wrong. The recommendation for system services has always been to _never ever_ use writable properties but use SetFoo() properties. In this case, you _really_ want that anyway because you want to be able to specify that e.g. no user interaction should happen when checking for authorization. This is for example how udisks work, see

 http://udisks.freedesktop.org/docs/latest/gdbus-org.freedesktop.UDisks2.Drive.html#gdbus-method-org-freedesktop-UDisks2-Drive.SetConfiguration

So, yeah, what you want in GLib is for enabling bad behavior in accountservice... I don't think that's a good idea. Better to just do it right in accountservice, please.
Comment 10 David Zeuthen (not reading bugmail) 2013-04-19 18:57:18 UTC
s/but use SetFoo() properties/but use SetFoo() methods instead/ ... obviously. Hope this clarifies.
Comment 11 Allison Karlitskaya (desrt) 2013-04-19 19:04:55 UTC
Incidentally, accountsservice has no 'allow interaction' parameter on its Set() methods... it always assumes true.
Comment 12 David Zeuthen (not reading bugmail) 2013-04-19 19:10:12 UTC
(In reply to comment #11)
> Incidentally, accountsservice has no 'allow interaction' parameter on its Set()
> methods... it always assumes true.

Probably because the only user right now is the settings dialog in GNOME and D-Bus methods are only called in response to user actions... but you could imagine a desktop shell poking accountservice via D-Bus on login or when it, say, notices a new user photo in the cloud and wanting to update the OS-local copy. Stuff like that.

Generally, if API stability is a concern (like for e.g. udisks and I guess accountsservice too), I always add a "a{sv} options" parameter to the tail end, like this:

 http://udisks.freedesktop.org/docs/latest/udisks-std-options.html
Comment 13 Lars Karlitski 2013-06-21 14:51:02 UTC
Review of attachment 241926 [details] [review]:

::: gio/gdbusmethodinvocation.c
@@ +444,3 @@
+              g_warning ("Value returned from property 'Get' call for '%s' should be '%s' but is '%s'",
+                         invocation->property_info->name, invocation->property_info->signature,
+                         g_variant_get_type_string (nested));

Shouldn't there be a `goto out` here as well?
Comment 14 Lars Karlitski 2013-06-21 18:12:45 UTC
Otherwise these patches look good to me and work as advertised. (I'll refrain from spamming everyone with empty accepted-commit-now messages for each of those patches.)
Comment 15 David Zeuthen (not reading bugmail) 2013-06-21 18:46:11 UTC
No, please don't commit these.
Comment 16 Allison Karlitskaya (desrt) 2013-06-21 19:37:48 UTC
(In reply to comment #7)
> That said, I'm not opposed  to your patch of punting to the method_call()
> handler if get_property and set_property are both NULL.

(In reply to comment #15)
> No, please don't commit these.

Did you change your mind?
Comment 17 David Zeuthen (not reading bugmail) 2013-06-21 21:24:26 UTC
(In reply to comment #16)
> (In reply to comment #7)
> > That said, I'm not opposed  to your patch of punting to the method_call()
> > handler if get_property and set_property are both NULL.
> 
> (In reply to comment #15)
> > No, please don't commit these.
> 
> Did you change your mind?

The part about punting to method_call() is fine. I erroneously assumed you were adding to the GDBusInterfaceVTable in a later patch but on second look, seems like you're not. Don't know what I was smoking, sorry about that - I'll be more careful in the future...
Comment 18 David Zeuthen (not reading bugmail) 2013-06-21 21:27:36 UTC
(Btw, I still stand by the advice in comment 7 and comment 9. We really should add a "Best Practices" section to the "Migrating to GDBus" chapter. Heck, it should probably be part of the D-Bus docs even - it's really not D-Bus specific.)
Comment 19 Allison Karlitskaya (desrt) 2013-06-22 17:43:01 UTC
Patches pushed, plus the suggested change.