Created attachment 241921 [details] [review]
GDBusConnection: remove an unused g_variant_get()

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.

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.

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.

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.

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.

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

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.

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

s/but use SetFoo() properties/but use SetFoo() methods instead/ ... obviously. Hope this clarifies.

Incidentally, accountsservice has no 'allow interaction' parameter on its Set() methods... it always assumes true.

(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

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?

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

No, please don't commit these.

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

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

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

Patches pushed, plus the suggested change.