Bug 34870

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.

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

Created attachment 44235 [details] [review]
Proposed patch

Here's a patch that does this.

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

Patch looks fine to me.

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.

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.

Created attachment 45377 [details] [review]
specification: fix versioning

We've added things since 0.15, so this isn't still 0.15.

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!

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.

Created attachment 45384 [details] [review]
Check parsing (or otherwise) of path_namespace in match rules

Created attachment 45385 [details] [review]
Mention dbus-specification.xml's separate versioning in HACKING

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

(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

Fixed in git for 1.5.0

(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