Bug 34870 - Add path_prefix match rule
Add path_prefix match rule
Status: RESOLVED FIXED
Product: dbus
Classification: Unclassified
Component: core
unspecified
Other All
: medium normal
Assigned To: Simon McVittie
John (J5) Palmieri
: patch
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-01 07:08 UTC by David Zeuthen (not reading bugmail)
Modified: 2011-04-07 11:30 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Proposed patch (9.83 KB, patch)
2011-03-08 07:37 UTC, David Zeuthen (not reading bugmail)
Details | Splinter Review
path_prefix: anchor matches at path-component boundaries, and give examples (3.81 KB, patch)
2011-04-07 07:29 UTC, Simon McVittie
Details | Splinter Review
specification: fix versioning (1.37 KB, patch)
2011-04-07 07:30 UTC, Simon McVittie
Details | Splinter Review
Rename path_prefix to path_namespace and disallow trailing '/' (13.33 KB, patch)
2011-04-07 08:32 UTC, Simon McVittie
Details | Splinter Review
Check parsing (or otherwise) of path_namespace in match rules (1.31 KB, patch)
2011-04-07 08:33 UTC, Simon McVittie
Details | Splinter Review
Mention dbus-specification.xml's separate versioning in HACKING (1.39 KB, patch)
2011-04-07 08:33 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Zeuthen (not reading bugmail) 2011-03-01 07:08:03 UTC
This is needed for an optimized implementation of the client-side of the proposed ObjectManager interface, see bug 34869. See

 http://lists.freedesktop.org/archives/dbus/2011-March/014170.html

for discussion.
Comment 1 David Zeuthen (not reading bugmail) 2011-03-01 07:10:19 UTC
See bug 34870 for the path_prefix bug. FWIW, I'd like to commit a version of the spec patch without path_prefix before resolving bug 34870... should be fine, exiting patch only mentions path_prefix in a non-normative way. Once we resolve bug 34870 we can change that section to use path_prefix again.
Comment 2 David Zeuthen (not reading bugmail) 2011-03-01 07:11:29 UTC
(In reply to comment #1)
> See bug 34870 for the path_prefix bug. FWIW, I'd like to commit a version of
> the spec patch without path_prefix before resolving bug 34870... should be
> fine, exiting patch only mentions path_prefix in a non-normative way. Once we
> resolve bug 34870 we can change that section to use path_prefix again.

(This was supposed to go to on bug 34869, not this bug. Sorry about that.)
Comment 3 David Zeuthen (not reading bugmail) 2011-03-08 07:37:09 UTC
Created attachment 44235 [details] [review]
Proposed patch

Here's a patch that does this.
Comment 4 David Zeuthen (not reading bugmail) 2011-03-08 08:43:17 UTC
For the record, GDBusProxyManager (which will be part of GLib 2.30) now uses this match rule when available

 http://cgit.freedesktop.org/~david/gdbus-binding-tool/commit/?id=7dc0d1001df48dfa458834a80400ed2c38a4f0d8
Comment 5 Lennart Poettering 2011-03-09 19:46:02 UTC
Patch looks fine to me.
Comment 6 Simon McVittie 2011-04-07 03:56:45 UTC
Review of attachment 44235 [details] [review]:

::: doc/dbus-specification.xml
@@ +3724,3 @@
                 <row>
+                  <entry><literal>path_prefix</literal></entry>
+                  <entry>An object path optionally ending in a slash</entry>

It'd be good if the semantics here were the same as the semantics of arg0namespace in Bug #24317, either by changing this or by changing arg0namespace.

Should the pattern /foo/bar match /foo/bar/baz?

Should the pattern /foo/bar match /foo/bar?

Should the pattern /foo/bar match /foo/barrier?

Should the pattern /foo/bar/ match /foo/bar/baz?

Should the pattern /foo/bar/ match /foo/bar?

Should the pattern /foo/bar/ match /foo/barrier?

It appears your answers are: yes, yes, yes, yes, no, no.

The answers analogous to Will's proposed patch for Bug #24317 would be: yes, yes, no, yes, no, no.

However, we propose to simplify arg0namespace to "has the given namespace + "." as a prefix", which would change the answers to: yes, no, no, disallowed, disallowed, disallowed.

It's not clear to me whether your GDBus patch requires that path_prefix="/foo/bar" matches /foo/bar itself. Does it? I wouldn't particularly mind having that as a special case, i.e. "yes, yes, no, disallowed, disallowed, disallowed".

One thing Will has thought about in Bug #24317 is the effect on hypothetical kernel D-Bus:

> I made the semantics specific to bus name/interface namespaces; in effect, the
> rule has to match up to (and possibly including) a period in the message
> argument. I also narrowed the scope to matching the zeroeth argument. This
> makes its purpose and semantics clearer.

> (It also means that in a hypothetical Kernel D-Bus future where the bus daemon
> is replaced by multicast Unix sockets, with clients using socket filters to
> filter messages based on match rules, we only need to add a header containing
> each period-separated prefix of the zeroth argument (if it's a string), rather
> than every prefix of every string argument.)

Requiring implementations of match rules to support arbitrary prefixes, as opposed to prefixes at any "/" boundary, would require more kernel-level headers for this situation, I think?

@@ +3729,3 @@
+                      Matches messages which are sent from or to an
+                      object for which the object path is a prefix of
+                      the given value. Examples of matches are

I think it's worth making this more explicit by saying which of the things above would be matched by /foo/bar (or /com/example/Application/ObjectManager or whatever).

I recommend using example paths in the example.com or example.net namespace, which would have the side-effect of reminding people that the syntax is not "everything starts with org" :-)

@@ +3737,3 @@
+                        This match key was added in version 0.15 of the
+                        D-Bus specification and implemented by the bus
+                        daemon in dbus 1.4.7 and later.

I don't think this should be added to the 1.4.x stable branch; if we're adding things like this, there's no point in having a stable branch at all. Save it for 1.5.
Comment 7 Simon McVittie 2011-04-07 07:29:51 UTC
Created attachment 45376 [details] [review]
path_prefix: anchor matches at path-component boundaries, and give examples

It seems wrong that path_prefix="/foo" matches /foobar, and it isn't
difficult or expensive to check.
Comment 8 Simon McVittie 2011-04-07 07:30:08 UTC
Created attachment 45377 [details] [review]
specification: fix versioning

We've added things since 0.15, so this isn't still 0.15.
Comment 9 David Zeuthen (not reading bugmail) 2011-04-07 07:45:26 UTC
As discussed on IRC, I'm totally OK with this if we rename it to path_namespace. I still think the namespace rules are harder to grasp than simple prefix rules, but that's probably an ok trade-off for consistency... Thanks!
Comment 10 Simon McVittie 2011-04-07 08:32:22 UTC
Created attachment 45382 [details] [review]
Rename path_prefix to path_namespace and disallow trailing '/'

Also disallow having both path and path_namespace in the same match rule
(it wouldn't make sense, path is more specific than path_namespace).

As per IRC discussion with davidz and wjt.
Comment 11 Simon McVittie 2011-04-07 08:33:16 UTC
Created attachment 45384 [details] [review]
Check parsing (or otherwise) of path_namespace in match rules
Comment 12 Simon McVittie 2011-04-07 08:33:33 UTC
Created attachment 45385 [details] [review]
Mention dbus-specification.xml's separate versioning in HACKING
Comment 13 David Zeuthen (not reading bugmail) 2011-04-07 08:37:14 UTC
(In reply to comment #10)
> Created an attachment (id=45382) [details]
> Rename path_prefix to path_namespace and disallow trailing '/'
> 
> Also disallow having both path and path_namespace in the same match rule
> (it wouldn't make sense, path is more specific than path_namespace).
> 
> As per IRC discussion with davidz and wjt.

Looks great to me. Thanks a bunch for doing this.
Comment 14 David Zeuthen (not reading bugmail) 2011-04-07 08:38:02 UTC
(In reply to comment #12)
> Created an attachment (id=45385) [details]
> Mention dbus-specification.xml's separate versioning in HACKING

Excellent idea. Looks good to me. Thanks
Comment 15 Simon McVittie 2011-04-07 09:29:08 UTC
Fixed in git for 1.5.0
Comment 16 David Zeuthen (not reading bugmail) 2011-04-07 11:30:01 UTC
(In reply to comment #15)
> Fixed in git for 1.5.0

Cool, thanks! I can verify that this works in the ObjectManager implementation (and its test suite) destined for GLib 2.30. The change is here

 http://cgit.freedesktop.org/~david/gdbus-binding-tool/commit/?id=6f9c4061ed73b72714854cc27459c855bd47bbfa