Bug 24317 - arg0namespace matching in dbus-daemon
Summary: arg0namespace matching in dbus-daemon
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.3.x (devel)
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: John (J5) Palmieri
URL:
Whiteboard: r?
Keywords: patch
Depends on: 31818
Blocks: 30210 36388
  Show dependency treegraph
 
Reported: 2009-10-05 04:59 UTC by Alban Crequy
Modified: 2011-04-19 04:40 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
signals.h: rename argument in declaration to match implementation (1.02 KB, patch)
2011-04-07 07:28 UTC, Simon McVittie
Details | Splinter Review
re-word description of arg0namespace (2.16 KB, patch)
2011-04-07 07:28 UTC, Simon McVittie
Details | Splinter Review
Document when arg0namespace was added, for completeness (1.04 KB, patch)
2011-04-07 07:31 UTC, Simon McVittie
Details | Splinter Review
Remove support for trailing "." on arg0namespace (6.52 KB, patch)
2011-04-07 08:32 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alban Crequy 2009-10-05 04:59:34 UTC
Prefix matching in dbus-daemon would be useful for Telepathy Mission Control. Will started a branch 'argx-prefix-matching' for that:
http://git.collabora.co.uk/?p=user/wjt/dbus.git;a=shortlog;h=refs/heads/argx-prefix-matching
Comment 1 Will Thompson 2009-10-05 05:33:50 UTC
To clarify why this is useful for people not familiar with implementation details of Telepathy:

The Telepathy framework has a number of "connection managers", which are daemons implementing various IM protocols. They take bus names of the form "org.freedesktop.Telepathy.ConnectionManager.<name>". Similarly, Telepathy UIs take names of the form "org.freedesktop.Telepathy.Client.<name>". Mission Control is a daemon which acts somewhat like a switchboard, handing out objects on the CMs to clients, and thus needs to know which clients and CMs are running.

Currently, it has to watch all name owner changes, but it'd be better if it could watch names matching the relevant patterns.
Comment 2 Will Thompson 2010-11-21 09:11:51 UTC
Here's a cleaned-up implementation of this, a mere 16 months after I started it.

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.)
Comment 3 Simon McVittie 2010-11-22 04:46:16 UTC
> +  dbus_bool_t namespace = FALSE;

I think is_namespace would probably be (a) clearer, and (b) safer when considering hybrid C/C++ compilers.

> Match messages whose first argument is a bus name or interface name within
> the specified namespace.

I'd prefer "whose first argument has the STRING type, and is a bus name or ...", to be completely unambiguous.
Comment 4 Will Thompson 2010-11-23 02:59:36 UTC
(In reply to comment #3)
> > +  dbus_bool_t namespace = FALSE;
> 
> I think is_namespace would probably be (a) clearer, and (b) safer when
> considering hybrid C/C++ compilers.
> 
> > Match messages whose first argument is a bus name or interface name within
> > the specified namespace.
> 
> I'd prefer "whose first argument has the STRING type, and is a bus name or
> ...", to be completely unambiguous.

Squashed these changes into the relevant patches.
Comment 5 Simon McVittie 2010-12-07 09:38:53 UTC
In commit 2f7b11158b497e759:
+                                             dbus_bool_t       is_path,
+                                             dbus_bool_t       prefix);

In the implementation the latter is called is_namespace. With that change, this looks fine to me.
Comment 6 Will Thompson 2011-01-16 02:16:01 UTC
While sketching out client-side code using this feature, I wonder whether the option of including the namespace itself in the watch is useful.

In the current implementation, "foo.bar." matches anything below foo.bar, and "foo.bar" matches anything below it or "foo.bar" itself. I can't really think why you'd ever want the latter.

If we declared that you don't (and if you do, you can add a second rule for arg0='') then the description in the spec and the implementation might be clearer.

(I'm okay with leaving it how it is; I'm just wondering aloud.)
Comment 7 Simon McVittie 2011-02-18 06:20:13 UTC
(In reply to comment #6)
> If we declared that you don't (and if you do, you can add a second rule for
> arg0='') then the description in the spec and the implementation might be
> clearer.

We agreed that this simplification would be useful when discussing this in person.
Comment 8 Simon McVittie 2011-04-07 05:05:16 UTC
(In reply to comment #6)
> In the current implementation, "foo.bar." matches anything below foo.bar, and
> "foo.bar" matches anything below it or "foo.bar" itself. I can't really think
> why you'd ever want the latter.

Equally, I don't think there's any particularly compelling reason why you *wouldn't* want it to match? The worst case is you get too many signals.

The reason I'm interested in the possibility of making it match is that Bug #34870 adds path_prefix, which I think might be relying on path_prefix="/badger" matching signals from /badger as well as from /badger/mushroom, and it'd be unnecessarily confusing if arg0namespace and path_prefix didn't behave the same at boundaries.

(davidz: are you relying on that or not?)
Comment 9 Simon McVittie 2011-04-07 07:28:10 UTC
Created attachment 45374 [details] [review]
signals.h: rename argument in declaration to match implementation

(In reply to comment #5)
> In commit 2f7b11158b497e759:
> +                                             dbus_bool_t       is_path,
> +                                             dbus_bool_t       prefix);
> 
> In the implementation the latter is called is_namespace. With that change, this
> looks fine to me.

Here, have a patch.
Comment 10 Simon McVittie 2011-04-07 07:28:43 UTC
Created attachment 45375 [details] [review]
re-word description of arg0namespace

It's unclear at first reading whether "may contain only one element"
means "elements >= 1, as an exception to the usual rule that
elements >= 2" (which is what was intended), or "elements == 1".
 
"Like a bus name or interface name" is a little ambiguous because they
have different syntactic restrictions: specifically allow any valid bus
name, which also allows all interface names.
Comment 11 Simon McVittie 2011-04-07 07:31:10 UTC
Created attachment 45379 [details] [review]
Document when arg0namespace was added, for completeness
Comment 12 Simon McVittie 2011-04-07 08:32:57 UTC
Created attachment 45383 [details] [review]
Remove support for trailing "." on arg0namespace
Comment 13 David Zeuthen (not reading bugmail) 2011-04-07 08:35:30 UTC
(In reply to comment #8)
> (In reply to comment #6)
> > In the current implementation, "foo.bar." matches anything below foo.bar, and
> > "foo.bar" matches anything below it or "foo.bar" itself. I can't really think
> > why you'd ever want the latter.
> 
> Equally, I don't think there's any particularly compelling reason why you
> *wouldn't* want it to match? The worst case is you get too many signals.
> 
> The reason I'm interested in the possibility of making it match is that Bug
> #34870 adds path_prefix, which I think might be relying on
> path_prefix="/badger" matching signals from /badger as well as from
> /badger/mushroom, and it'd be unnecessarily confusing if arg0namespace and
> path_prefix didn't behave the same at boundaries.
> 
> (davidz: are you relying on that or not?)

Yeah, for the org.fd.DBus.ObjectManager, we want to match messages with the object paths

 /net/Corp/App/MushroomManager
 /net/Corp/App/MushroomManager/*

ideally with a single match rule.
Comment 14 Simon McVittie 2011-04-07 09:29:15 UTC
Fixed in git for 1.5.0


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.