Bug 34527 - document UnknownObject, UnknownInterface errors
Summary: document UnknownObject, UnknownInterface errors
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: Other All
: medium normal
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
URL:
Whiteboard: review+ from smcv, 1.5
Keywords: patch
Depends on:
Blocks:
 
Reported: 2011-02-21 04:16 UTC by Simon McVittie
Modified: 2011-03-11 14:17 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH] connection: hook UnknownObject and UnknownInterface up where appropriate (119.03 KB, patch)
2011-03-09 19:17 UTC, Lennart Poettering
Details | Splinter Review
[PATCH] connection: hook UnknownObject and UnknownInterface up where appropriate (119.03 KB, patch)
2011-03-09 19:19 UTC, Lennart Poettering
Details | Splinter Review
[PATCH] protocol: introduce four new errors (2.08 KB, patch)
2011-03-09 19:19 UTC, Lennart Poettering
Details | Splinter Review
new hookup patch (5.21 KB, patch)
2011-03-10 12:40 UTC, Lennart Poettering
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2011-02-21 04:16:40 UTC
Lennart proposed a patch to fix the fact that missing objects, missing interfaces and missing methods all result in us raising UnknownMethod:

http://lists.freedesktop.org/archives/dbus/2011-February/014107.html

libdbus can only distinguish between UnknownObject and the others, so with Lennart's implementation it will never actually raise UnknownInterface itself, but the well-known string is made available for bindings and reimplementations.

For dbus 1.4 I'd be happy to add the well-known strings to dbus-protocol.h if Lennart wants that, but I'd rather not take the rest of the patch.

For dbus 1.5 I think the whole change looks good.
Comment 1 Lennart Poettering 2011-03-09 19:17:44 UTC
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(-)
Comment 2 Lennart Poettering 2011-03-09 19:19:13 UTC
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(-)
Comment 3 Lennart Poettering 2011-03-09 19:19:48 UTC
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(-)
Comment 4 Lennart Poettering 2011-03-09 19:21:19 UTC
The "four new errors" patch is suppsoed to be merged into dbus-1.4, the other into dbus-1.6.
Comment 5 David Zeuthen (not reading bugmail) 2011-03-10 06:32:57 UTC
(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>
Comment 6 Simon McVittie 2011-03-10 06:50:06 UTC
Review of attachment 44297 [details] [review]:

Looks good for 1.4.

Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Comment 7 Simon McVittie 2011-03-10 07:00:16 UTC
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);

++
Comment 8 Lennart Poettering 2011-03-10 12:12:53 UTC
The protocol.h part is now commited.
Comment 9 Lennart Poettering 2011-03-10 12:40:27 UTC
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.
Comment 10 Simon McVittie 2011-03-11 03:12:55 UTC
Review of attachment 44332 [details] [review]:

Looks good to me, 1.5 only
Comment 11 Lennart Poettering 2011-03-11 14:17:31 UTC
Committed. Closing.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.