Bug 91755 - [SPEC EXTENSION][PATCH] Add new "arg0has=" string array matches
Summary: [SPEC EXTENSION][PATCH] Add new "arg0has=" string array matches
Status: RESOLVED MOVED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2015-08-25 17:53 UTC by Lennart Poettering
Modified: 2018-10-12 21:23 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
preparation: fix whitespace in the spec XML (65.34 KB, patch)
2015-08-25 17:53 UTC, Lennart Poettering
Details | Splinter Review
the actual patch (1.93 KB, patch)
2015-08-25 17:53 UTC, Lennart Poettering
Details | Splinter Review
signals: factor out match_rule_matches_string_arg() (6.05 KB, patch)
2015-11-06 16:40 UTC, Simon McVittie
Details | Splinter Review
signals: add support for arg0has keyword (12.52 KB, patch)
2015-11-06 16:40 UTC, Simon McVittie
Details | Splinter Review

Description Lennart Poettering 2015-08-25 17:53:22 UTC
Created attachment 117913 [details] [review]
preparation: fix whitespace in the spec XML

This adds support for "arg0has=" style matches that test strings against messages containing string arrays ("as"). It applies if there's at least one string in the array that matches.

This is useful for implementing udev style "tagging" of devices.

This has been discussed on the ML a while back:

http://lists.freedesktop.org/archives/dbus/2015-April/016666.html

There's also a sd-bus PR implementing this:

https://github.com/systemd/systemd/pull/1036

This bug also adds a preparatory patch containing a trailing whitespace cleanup for the spec, to make it easier to edit and patch.
Comment 1 Lennart Poettering 2015-08-25 17:53:51 UTC
Created attachment 117914 [details] [review]
the actual patch
Comment 2 Simon McVittie 2015-08-25 18:04:51 UTC
Comment on attachment 117913 [details] [review]
preparation: fix whitespace in the spec XML

If we had always had a policy of no trailing whitespace (and no tabs), I'd certainly be fine with applying that policy like this.

However, we deliberately avoid fixing whitespace that is not in or adjacent to the lines you're touching for some other reason, so that patches can backport more easily.

I think this particular change is OK because we never backport the spec anyway (what would that even mean?), but please don't do the same to anything else in the tree.
Comment 3 Simon McVittie 2015-08-25 18:06:18 UTC
Comment on attachment 117914 [details] [review]
the actual patch

Review of attachment 117914 [details] [review]:
-----------------------------------------------------------------

This looks good, and I like the new name better, but it shouldn't land until the reference implementation understands arg0has.

I'm about to release 1.10.0; we can perhaps have this in 1.11.0.
Comment 4 Lennart Poettering 2015-08-25 18:13:22 UTC
Is it really necessary to add this to the reference implementation? 

For us (systemd upstream) this would be no priority, since we only need this for the udev usecase, but that's nothing we'll do on dbus1, but only on kdbus. Hence we are fine if this is implemented in sd-bus/kdbus only for now, as long as it is in the spec.
Comment 5 Lennart Poettering 2015-08-25 18:15:41 UTC
Also, I think dbus should enforce a stricter whitespace regime and refuse unclean commits, and clean up the existing codebase once. While this will making bakports harder across the cleanup boundary, later on it will makes things much easier!

But anyway, this is of course a separate issue, and the only reason I added this here as second patch is that it annoyed me so much when I added the spec extension, and I thought it might be time to start the clean-up at least on the more auxiliary parts of the codebase.
Comment 6 Simon McVittie 2015-11-06 16:39:33 UTC
(In reply to Lennart Poettering from comment #4)
> Is it really necessary to add this to the reference implementation? 

Well, yes. Reference implementations should support as much of the protocol/API as is technically possible, otherwise they make a poor reference :-)
Comment 7 Simon McVittie 2015-11-06 16:40:04 UTC
Created attachment 119446 [details] [review]
signals: factor out match_rule_matches_string_arg()
Comment 8 Simon McVittie 2015-11-06 16:40:25 UTC
Created attachment 119447 [details] [review]
signals: add support for arg0has keyword
Comment 9 Simon McVittie 2015-11-06 16:44:37 UTC
(In reply to Lennart Poettering from comment #4)
> For us (systemd upstream) this would be no priority, since we only need this
> for the udev usecase, but that's nothing we'll do on dbus1, but only on
> kdbus.

Is this still the case at this point? Do you have other use-cases for this feature?

Looking for "tags" on messages seems generically useful, and it isn't actually very much code, so I wouldn't necessarily object to merging it for 1.11 even if you aren't going to use it any time soon.
Comment 10 Simon McVittie 2015-11-07 10:31:53 UTC
ssh://people.freedesktop.org/~smcv/dbus arg0has-91755
http://cgit.freedesktop.org/~smcv/dbus/log/?h=arg0has-91755
Comment 11 Simon McVittie 2015-11-07 10:42:02 UTC
Comment on attachment 117913 [details] [review]
preparation: fix whitespace in the spec XML

(applied)
Comment 12 Simon McVittie 2016-06-30 13:02:12 UTC
If this is something that you want, or that anyone wants, I'm still waiting for review on the implementation.
Comment 13 GitLab Migration User 2018-10-12 21:23:15 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/130.


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.