Bug 34527

Created attachment 44295 [details] [review]
[PATCH] connection: hook UnknownObject and UnknownInterface up where appropriate


This makes use of UnknownInterface and UnknownObject where appropriate
in the D-Bus core.
---
 bus/driver.c            |    5 +-
 dbus/dbus-connection.c  |  824 ++++++++++++++++++++++++-----------------------
 dbus/dbus-object-tree.c |  148 +++++----
 dbus/dbus-object-tree.h |    7 +-
 4 files changed, 497 insertions(+), 487 deletions(-)

Created attachment 44296 [details] [review]
[PATCH] connection: hook UnknownObject and UnknownInterface up where appropriate


This makes use of UnknownInterface and UnknownObject where appropriate
in the D-Bus core.
---
 bus/driver.c            |    5 +-
 dbus/dbus-connection.c  |  824 ++++++++++++++++++++++++-----------------------
 dbus/dbus-object-tree.c |  148 +++++----
 dbus/dbus-object-tree.h |    7 +-
 4 files changed, 497 insertions(+), 487 deletions(-)

Created attachment 44297 [details] [review]
[PATCH] protocol: introduce four new errors


UnknownInterface, UnknownObject, UnknownProperty and PropertyReadOnly,
as discussed on the ML.

The first two are already used by various bindings, such as the Qt and
Java binding, but have never been made official.
---
 dbus/dbus-protocol.h |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

The "four new errors" patch is suppsoed to be merged into dbus-1.4, the other into dbus-1.6.

(In reply to comment #3)
> Created an attachment (id=44297) [details]
> [PATCH] protocol: introduce four new errors
> 
> 
> UnknownInterface, UnknownObject, UnknownProperty and PropertyReadOnly,
> as discussed on the ML.
> 
> The first two are already used by various bindings, such as the Qt and
> Java binding, but have never been made official.
> ---
>  dbus/dbus-protocol.h |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)

It would be helpful if there was a documented mentioning when to use each error but as discussed on the mailing list in another thread the D-Bus spec is probably not the place for that (a "best practices" addendum to the spec could be). So the patch looks good to me.

(Btw, I've filed https://bugzilla.gnome.org/show_bug.cgi?id=644397 for GDBus to use these new errors.)

Reviewed-by: David Zeuthen <davidz@redhat.com>

Review of attachment 44297 [details] [review]:

Looks good for 1.4.

Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

Review of attachment 44295 [details] [review]:

Please don't change unrelated whitespace in a commit that makes functional changes. This is likely to make it harder to merge 1.4 into master. The rule of thumb I tend to use is that it's good to fix whitespace in lines you're changing anyway, or adjacent to those.

I wouldn't object to separate whitespace-only patches (by which I mean: git diff --ignore-space-change outputs nothing) that could go into both 1.4 and master.

I've marked the actual functional changes I found, which all look good.

::: dbus/dbus-connection.c
@@ +4523,3 @@
   dbus_int32_t reply_serial;
   DBusDispatchStatus status;
+  dbus_bool_t found_object;

++

@@ +4690,3 @@
+                                                  message,
+                                                  &found_object);
+

++

@@ +4729,2 @@
       reply = dbus_message_new_error (message,
+                                      found_object ? DBUS_ERROR_UNKNOWN_METHOD : DBUS_ERROR_UNKNOWN_OBJECT,

++

::: dbus/dbus-object-tree.c
@@ +747,3 @@
 _dbus_object_tree_dispatch_and_unlock (DBusObjectTree          *tree,
+                                       DBusMessage             *message,
+                                       dbus_bool_t             *found_object)

++

@@ +795,3 @@
+  if (found_object)
+    *found_object = !!subtree;
+

++

@@ +1387,3 @@
     }
 
+  result = _dbus_object_tree_dispatch_and_unlock (tree, message, NULL);

++

::: dbus/dbus-object-tree.h
@@ +44,3 @@
 DBusHandlerResult _dbus_object_tree_dispatch_and_unlock    (DBusObjectTree              *tree,
+                                                            DBusMessage                 *message,
+                                                            dbus_bool_t                 *found_object);

++

The protocol.h part is now commited.

Created attachment 44332 [details] [review]
new hookup patch

Here's another try of the hookup patch. This time with --ignore-space-change and updated a little.

Review of attachment 44332 [details] [review]:

Looks good to me, 1.5 only

Committed. Closing.