(In reply to comment #0)
> or P. In that case we should complain on stderr about P nor drop signal
                             ^^^
                             NOT
> emissions for S. We should have a test-case to check that we don't.

I missed a 'not' above, sorry about that. Anyway, just to be crystal clear: the intent is that we can gracefully handle properties and signals being added to a D-Bus interface - in particular g_dbus_proxy_get_cached_property() should return property P and ::g-signal should be fired when receiving signal S even if the GDBusInterfaceInfo set with g_dbus_proxy_set_interface_info() does not know about P or S.

Since gdbus-codegen(1) uses g_dbus_proxy_set_interface_info() this is a big problem... so the fix will need to go into the glib-2-30 branch as well.

Created attachment 198227 [details] [review]
Patch (against glib-2-30)

Here's a patch that does this. Bastien: please check if it works for you. Thanks!

 <hadess> davidz, patch workie, thanks

I've (davidz) also tested this with a couple of other gdbus-codegen(1)-using services (udisks2, goa) and it works fine.

Review of attachment 198227 [details] [review]:

::: gio/gdbusproxy.c
@@ +731,3 @@
     {
+      const gchar *type_string = g_variant_get_type_string (value);
+      if (g_strcmp0 (type_string, info->signature) != 0)

Hm, can either of these actually be null?

@@ +737,3 @@
+                     property_name,
+                     type_string,
+                     info->signature);

Becuase while I don't know offhand whether on e.g. Solaris we use the system printf or not (if we do, then this would segfault), regardless I think it's sort of lame to print (null) in general.

So maybe just g_assert (info->signature != NULL) above?

@@ -2501,3 @@
-      g_warning ("Trying to invoke method %s which isn't in expected interface %s",
-                 method_name, proxy->priv->expected_interface->name);
-    }

Hmm...do you really want to delete this?  I thought this patch was about the service adding new signals and properties - why are we deleting the warning for unknown methods?

If the service adds a method we won't trigger this warning, right?

(In reply to comment #4)
> Review of attachment 198227 [details] [review]:
> 
> ::: gio/gdbusproxy.c
> @@ +731,3 @@
>      {
> +      const gchar *type_string = g_variant_get_type_string (value);
> +      if (g_strcmp0 (type_string, info->signature) != 0)
> 
> Hm, can either of these actually be null?

Nope. I could have used strcmp(3) but that would mean including <stdlib.h>.

> @@ +737,3 @@
> +                     property_name,
> +                     type_string,
> +                     info->signature);
> 
> Becuase while I don't know offhand whether on e.g. Solaris we use the system
> printf or not (if we do, then this would segfault), regardless I think it's
> sort of lame to print (null) in general.
> 
> So maybe just g_assert (info->signature != NULL) above?

I think that's a bit superfluous as info->signature is never NULL. I can add it if you want but it just seems wrong. Thoughts?

> @@ -2501,3 @@
> -      g_warning ("Trying to invoke method %s which isn't in expected interface
> %s",
> -                 method_name, proxy->priv->expected_interface->name);
> -    }
> 
> Hmm...do you really want to delete this?  I thought this patch was about the
> service adding new signals and properties - why are we deleting the warning for
> unknown methods?
> 
> If the service adds a method we won't trigger this warning, right?

It's added for the same reasons as for properties and signals: the program implementing the service-side might have now implement the given method but the GDBusInterfaceInfo object we're using on the client-side does not yet have the method... because... say.. the generated code setting expected_interface is still using an old copy of the XML for the interface.

Sure, you can now say "but how is the program supposed to call the D-Bus method if there's no generated code for it?" and the answer to this is two-fold:

 a) the program could be using g_dbus_proxy_call() manually; and

 b) the program could be written in a high-level language such as
    JS or Python that uses g_dbux_proxy_call()

(In reply to comment #5)
>
> Sure, you can now say "but how is the program supposed to call the D-Bus method
> if there's no generated code for it?" and the answer to this is two-fold:
> 
>  a) the program could be using g_dbus_proxy_call() manually; and

Can't we say that before you use _call, if you've installed introspection information, it has to include the method?

>  b) the program could be written in a high-level language such as
>     JS or Python that uses g_dbux_proxy_call()

I guess those would follow the same rules - install the right info or pass NULL?

(In reply to comment #6)
> (In reply to comment #5)
> >
> > Sure, you can now say "but how is the program supposed to call the D-Bus method
> > if there's no generated code for it?" and the answer to this is two-fold:
> > 
> >  a) the program could be using g_dbus_proxy_call() manually; and
> 
> Can't we say that before you use _call, if you've installed introspection
> information, it has to include the method?
> 
> >  b) the program could be written in a high-level language such as
> >     JS or Python that uses g_dbux_proxy_call()
> 
> I guess those would follow the same rules - install the right info or pass
> NULL?

That's easier said than done.... for example you could be using the GDBusProxy instance through, say, the libgoa-1.0 library or, more realistically, you are using a hand-edited (or even 100% hand-made) XML file describing the interface. (I concede that both of these cases are somewhat artificial.)

Either way, I don't see what you gain by this. Why is a g_warning() at run-time any better having to handle the org.freedesktop.DBus.Error.UnknownMethod ?

I guess my main objection to this whole thing is that we're artificially constraining the user - the original reason (bug 642724 but also before that) for adding this in the first place was that we wanted to protect the user against

 a) badly written services (returning non-conforming properties and
    emitting broken/variable signals); and

 b) himself (using the wrong type when updating cached properties, calling
    the method with wrong types)

First of all, The code generator largely makes this obsolete (since you use the generated code instead of string-based APIs) so it's really only interesting in the cases where you don't generate code. Which includes use of GDBus from high-level languages such as JS or Python.

Second, b) is not really useful - a g_warning() isn't any more helpful than the service returning org.freedesktop.DBus.Error.UnknownMethod - in fact, in many ways it's LESS helpful because the error does not show up in D-Bus traces (dbus-monitor, G_DBUS_DEBUG=all, bustle, etc.). Also, wouldn't you rather want an language-specific exception (the G_DBUS_ERROR_UNKNOWN_METHOD GError turned into an exception) rather than the process scribbling on stderr (that the developer might not even see) or even terminating the VM (depending on flags)?

(Ugh, not sure how all these bits got flipped around - but I blame myself trying to use keynav :-/)

(In reply to comment #7)

> I guess my main objection to this whole thing is that we're artificially
> constraining the user - the original reason (bug 642724 but also before that)
> for adding this in the first place was that we wanted to protect the user
> against
> 
>  a) badly written services (returning non-conforming properties and
>     emitting broken/variable signals); and
> 
>  b) himself (using the wrong type when updating cached properties, calling
>     the method with wrong types)

Ok, I was worried about protecting the user against a method *returning* the wrong types.  This was so you didn't have to sprinkle checks after every method call to see that the returned GVariant has the right type.

I guess the issue here is that we will silently not check the return value if the method is missing from introspection, whereas before we used g_warning().  Right?

This might just be a documentation thing - we say that we only check method returns that are in the introspection data.

Created attachment 198268 [details] [review]
Updated patch

(In reply to comment #9)
> (In reply to comment #7)
> 
> > I guess my main objection to this whole thing is that we're artificially
> > constraining the user - the original reason (bug 642724 but also before that)
> > for adding this in the first place was that we wanted to protect the user
> > against
> > 
> >  a) badly written services (returning non-conforming properties and
> >     emitting broken/variable signals); and
> > 
> >  b) himself (using the wrong type when updating cached properties, calling
> >     the method with wrong types)
> 
> Ok, I was worried about protecting the user against a method *returning* the
> wrong types.  This was so you didn't have to sprinkle checks after every method
> call to see that the returned GVariant has the right type.
> 
> I guess the issue here is that we will silently not check the return value if
> the method is missing from introspection, whereas before we used g_warning(). 
> Right?

That's correct - before the patch there's a

 g_warning("Trying to invoke method %s which isn't in expected interface %s", ..)

but we were still doing the call. The difference here is that we're just doing the call.

> This might just be a documentation thing - we say that we only check method
> returns that are in the introspection data.

Fair enough. See the updated patch for much better docs - in particular the docs for the :g-interface-info property now says

 Ensure that interactions with this proxy conform to the given interface.
 This is mainly to ensure that malformed data received from the other
 peer is ignored. The given GDBusInterfaceInfo is said to be the
 expected interface.

 The checks performed are:

 * When completing a method call, if the type signature of the
   reply message isn't what's expected, the reply is discarded
   and the GError is set to G_IO_ERROR_INVALID_ARGUMENT.

 * Received signals that have a type signature mismatch are dropped
   and a warning is logged via g_warning().

 * Properties received via the initial GetAll() call or via
   the ::PropertiesChanged signal (on the org.freedesktop.DBus.
   Properties interface) or set using g_dbus_proxy_set_cached_property()
   with a type signature mismatch are ignored and a warning is
   logged via g_warning().

 Note that these checks are never done on methods, signals and
 properties that are not referenced in the given GDBusInterfaceInfo,
 since extending a D-Bus interface on the service-side is not considered
 an ABI break.

Also note that the previous patch silently dropped unknown properties (both in the GetAll() and ::PropertiesChanged path). The new patch doesn't - there's also an added test for that.

Created attachment 198343 [details] [review]
Updated patch v2

Yet another update

 - Don't print confusing warnings on stderr
 - Also test that we complain on stderr if a received PropertiesChanged
   signal has a property with a value that does not correspond to the
   expected interface

I'm pretty happy with the test cases now (we check everything that is promised) so am going to commit this on glib-2-30 and master. Thanks for the review.

Comment on attachment 198343 [details] [review]
Updated patch v2

glib-2-30: http://git.gnome.org/browse/glib/commit/?h=glib-2-30&id=519c148848251ecde5477b9362df3b3054ba807b
master: http://git.gnome.org/browse/glib/commit/?id=2b963266b68a3b14afcaa237ed41319c99949e43