Bug 24307 - [PATCH] GetAllMatchRules for application debugging
Summary: [PATCH] GetAllMatchRules for application debugging
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: medium enhancement
Assignee: Simon McVittie
QA Contact: John (J5) Palmieri
URL:
Whiteboard: review?
Keywords: patch
Depends on: 34040
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-04 10:34 UTC by Alban Crequy
Modified: 2014-09-25 13:13 UTC (History)
9 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Implement GetConnectionMatchRules (7.57 KB, patch)
2009-10-04 10:34 UTC, Alban Crequy
Details | Splinter Review
matchrulelist.py (935 bytes, application/octet-stream)
2009-10-04 10:39 UTC, Alban Crequy
Details
Implement message type in match_rule_to_string (1.84 KB, patch)
2010-10-07 05:25 UTC, Christian Dywan
Details | Splinter Review
Add GetConnectionMatchRules in the bus driver (5.99 KB, patch)
2010-10-07 05:29 UTC, Christian Dywan
Details | Splinter Review
[PATCH] Implement GetConnectionMatchRules on the Stats interface (10.20 KB, patch)
2014-06-30 14:33 UTC, Alban Crequy
Details | Splinter Review
Script using GetConnectionMatchRules (3.48 KB, text/plain)
2014-07-15 14:22 UTC, Alban Crequy
Details
[PATCH 1/5] Implement GetAllMatchRules on the Stats interface (9.87 KB, patch)
2014-09-24 15:44 UTC, Alban Crequy
Details | Splinter Review
[PATCH 2/5] Stats: GetAllMatchRules: add tests (8.72 KB, patch)
2014-09-24 15:44 UTC, Alban Crequy
Details | Splinter Review
[PATCH 3/5] match_rule_to_string: returns NULL if no memory instead of looping (2.65 KB, patch)
2014-09-24 15:45 UTC, Alban Crequy
Details | Splinter Review
[PATCH 4/5] match_rule_to_string: fix escaping (5.08 KB, patch)
2014-09-24 15:46 UTC, Alban Crequy
Details | Splinter Review
[PATCH 5/5] match_rule_to_string: add test (2.11 KB, patch)
2014-09-24 15:46 UTC, Alban Crequy
Details | Splinter Review
Describe quoting for match rules (2.43 KB, patch)
2014-09-24 16:58 UTC, Simon McVittie
Details | Splinter Review
[PATCH 1/5] Implement GetAllMatchRules on the Stats interface (9.73 KB, patch)
2014-09-24 17:00 UTC, Alban Crequy
Details | Splinter Review
Use ISO C strchr() instead of BSD index() (890 bytes, patch)
2014-09-25 10:49 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-04 10:34:42 UTC
Created attachment 30040 [details] [review]
Implement GetConnectionMatchRules

Applications subscribing to D-Bus messages with a too broad match rule are a common cause of useless wake-up. In order to find those guilty applications, it is useful to get the list of match rules registered to dbus-daemon.

The attached patch is a first implementation of a D-Bus method GetConnectionMatchRules() on the bus driver /org/freedesktop/DBus.

GetConnectionMatchRules(s: name) -> s: rules

The patch applies on top of Will's optimize-matching branch (see bug #23117). I plan to replace the return type "s" by an array of match rules "as". But it is already useful to find problems in applications connecting to dbus-daemon.
Comment 1 Alban Crequy 2009-10-04 10:39:29 UTC
Created attachment 30041 [details]
matchrulelist.py

Python script to retrieve all D-Bus match rules on the session or system bus.

Useful to find match rules without a message type, or without an interface: those match rules defeat the optimizations in #23117.
Comment 2 Colin Walters 2009-10-16 10:11:26 UTC
Haven't looked at the implementation yet, but I don't really like non-standard debugging interfaces as part of org.freedesktop.DBus interface. 

My intuition here is that what would be nicer is something like a "DumpState" call which would dump all sorts of things into say one big XML blob, like:

* match rules
* number of pending/processed messages
* all connections
  - process id
  - binary

And maybe create a new interface for this?  org.freedesktop.DBus.Debugging?  Not sure.



Comment 3 Alban Crequy 2009-10-16 10:46:44 UTC
I agree that we should avoid confusion with standard interfaces. I am a bit afraid of the security implication, on the system bus for example.

org.freedesktop.DBus.Debugging would be ok for me. But why DumpState with an XML blob? I would prefer to have a D-Bus-like interface and avoid XML. It would be easier to implement, both dbus-daemon side and debugging tool side.

I also like the idea to have process id and binary command line. If there could be a way to integrate this nicely with Bustle, it would be great :-)
Comment 4 Colin Walters 2009-10-16 10:55:33 UTC
(In reply to comment #3)
> I agree that we should avoid confusion with standard interfaces. I am a bit
> afraid of the security implication, on the system bus for example.

Yes; we might key this off the <allow eavesdrop/> permission?  But that doesn't really work that well.

> org.freedesktop.DBus.Debugging would be ok for me. But why DumpState with an
> XML blob? I would prefer to have a D-Bus-like interface and avoid XML. It would
> be easier to implement, both dbus-daemon side and debugging tool side.

It could be an a{sv} too I guess, I don't really have a strong opinion.
Comment 5 Christian Dywan 2010-10-07 05:25:10 UTC
Created attachment 39253 [details] [review]
Implement message type in match_rule_to_string

This is only the part that implements the missing bits in match_rule_to_string, I reckon this is not problematic on its own right.
Comment 6 Christian Dywan 2010-10-07 05:29:27 UTC
Created attachment 39254 [details] [review]
Add GetConnectionMatchRules in the bus driver

This is the GetConnectionMatchRules on top of that, with leaks fixed (even if there's no agreement I figure it's better to have a proper patch here).
Comment 7 Simon McVittie 2011-01-05 04:38:30 UTC
Review of attachment 39253 [details] [review]:

I think this is good to have, so I'm inclined to apply it despite the points below, and create a follow-up patch that fixes them.

::: bus/signals.c
@@ +138,3 @@
+      if (rule->message_type == DBUS_MESSAGE_TYPE_INVALID)
+        {
+          if (!_dbus_string_append_printf (&str, "type='INVALID'"))

This doesn't need to be a printf: we're appending a literal.

@@ +141,3 @@
+            goto nomem;
+        }
+      else if (rule->message_type == DBUS_MESSAGE_TYPE_METHOD_CALL)

I'd prefer to have a switch (rule->message_type) with a default case.
Comment 8 Colin Walters 2011-01-05 08:54:48 UTC
(In reply to comment #7)
> Review of attachment 39253 [details] [review]:
> 
> I think this is good to have, so I'm inclined to apply it despite the points
> below, and create a follow-up patch that fixes them.

Well, my most preferred option, as I mentioned in the mailing list a while back, would be an entirely separate /var/run/dbus/system_bus_management_socket
that the system bus listens to, and could be used for all sorts of stuff that really doesn't belong as public API on org.freedesktop.DBus, such as the existing UpdateActivationEnvironment.

I don't think that would be an unreasonable amount of work, and would allow us to fix other issues (for example, I pretty strongly think eavesdropping should work this way.  The bus shouldn't block normal message delivery on eavesdroppers, and it would also allow us to always enable eavesdropping and secure it with a separate policy.)

Besides all that, this patch seems like a big one-off really.  I mean, I expect how it would be used is that people do an e.g. Fedora 14 install, patch dbus to dump the match rules, find application offenders, and fix them.  After that the patch isn't needed again for a while.

If we just stick it on org.freedesktop.DBus like we did with UpdateActivationEnvironment, it's forever, and even worse it would normally never be used.
Comment 9 Simon McVittie 2011-01-31 06:25:17 UTC
(In reply to comment #8)
> Besides all that, this patch seems like a big one-off really.  I mean, I expect
> how it would be used is that people do an e.g. Fedora 14 install, patch dbus to
> dump the match rules, find application offenders, and fix them.  After that the
> patch isn't needed again for a while.

In Maemo (which uses D-Bus pretty heavily, on an embedded device) this has generally been done iteratively, so perhaps we wouldn't have this enabled for a final release, but it'd be desirable for most of the development cycle.

Also, this and similar patches seem less likely to bit-rot if this functionality is merged, disabled in ./configure for distros/manual installation, and enabled for developers. Perhaps the default could be tied to maintainer mode, like _dbus_verbose is?

My medium-term plan is to make the bus daemon able to implement multiple interfaces (Bug #33757), add --enable-debugging or something, and have an o.fd.DBus.Debugging interface which is conditionally-compiled, and contains:

* this
* various numeric stats in pairs of current value and peak usage:
  - number of match rules
  - number of connections
  - in general, anything there's a limit for in session.conf

On the same or a different interface and ./configure option, we could have similar stats per-connection.

For the system bus, we could have these only available via the hypothetical management socket, but the session bus still needs the same functionality (detecting misuse of D-Bus on the session bus is just as useful).
Comment 10 Colin Walters 2011-01-31 11:00:05 UTC
(In reply to comment #9)
>
> My medium-term plan is to make the bus daemon able to implement multiple
> interfaces (Bug #33757), add --enable-debugging or something, and have an
> o.fd.DBus.Debugging interface which is conditionally-compiled, and contains:

That sounds good to me.
 
> For the system bus, we could have these only available via the hypothetical
> management socket, but the session bus still needs the same functionality
> (detecting misuse of D-Bus on the session bus is just as useful).

Well, there's no reason the session bus couldn't also allocate a different management socket too; the cost is pretty minimal.  Not blocking the session on an eavesdropper has as much value as it does for the system.

But I agree it's effectively orthonogal, and the first step is defining and implementing the new debugging/profiling interfaces.
Comment 11 Simon McVittie 2011-10-21 02:38:47 UTC
I've been reminded that this bug is still here and bit-rotting. I'm working on GDBus thread-safety at the moment, but this is next on my list (unless you want to take it, Cosimo?)

(In reply to comment #9)
> My medium-term plan is to make the bus daemon able to implement multiple
> interfaces (Bug #33757), add --enable-debugging or something, and have an
> o.fd.DBus.Debugging interface which is conditionally-compiled

This is now o.fd.DBus.Stats, which is #ifdef'd optional functionality. I plan to rebase this branch onto Stats (so GetConnectionMatchRules is a method on Stats - it's not really a statistic, but the general idea is the same).

GetConnectionMatchRules is information disclosure, so we should restrict it to root-only in the system bus' default policy. I'm inclined to make the entire Stats interface root-only, just to be sure (I don't think anything in it is sensitive, but still).
Comment 12 Cosimo Alfarano 2011-10-24 02:03:43 UTC
(In reply to comment #11)
> I've been reminded that this bug is still here and bit-rotting. I'm working on
> GDBus thread-safety at the moment, but this is next on my list (unless you want
> to take it, Cosimo?)

sure, although I'll be able to take it in two days, feel free to work on it if you need/want to earlier.
Comment 13 Cosimo Alfarano 2011-11-09 05:25:56 UTC
(In reply to comment #11)
> This is now o.fd.DBus.Stats, which is #ifdef'd optional functionality. I plan

It's o.fd.DBus.Debug.Stats which probably opens some room for a simple
o.fd.DBus.Debug with subsantially the same access & compile-time policies.

I'm working on it under Debug.Stats though, it's really easy to add the other Interface.

Leaving it outside Stats seems cleaner and more intuitive for who might look for it, I don't know if it's worth to add an interface just for it, from a comment it seems that there is a significant(is it?) lookup cost which increases over the number of interfaces.
It's probably enough to put Debug as last, before Debug.Stats, though.
Comment 14 Simon McVittie 2012-06-05 03:42:32 UTC
triaging: low/enhancement
Comment 15 Alban Crequy 2014-06-30 10:42:51 UTC
Comment on attachment 39253 [details] [review]
Implement message type in match_rule_to_string

Mark this patch as obsolete: See Bug #34040 Comment #1, and commit 85e9a (Jan 2011). match_rule_to_string() now uses dbus_message_type_to_string().
Comment 16 Alban Crequy 2014-06-30 14:33:31 UTC
Created attachment 102017 [details] [review]
[PATCH] Implement GetConnectionMatchRules on the Stats interface

Updated the GetConnectionMatchRules with the following changes:
- implemented on the Stats interface. Needs dbus compiled with --enable-stats
- instead of a big string, new signature:
   - input: ""
   - output: "a{sas}"

Example how it could look like:

$ dbus-send --print-reply --dest=org.freedesktop.DBus /org/freedesktop/DBus \
      org.freedesktop.DBus.Debug.Stats.GetConnectionMatchRules
method return sender=org.freedesktop.DBus -> dest=:1.13 reply_serial=2
   array [
      dict entry(
         string ":1.4"
         array [
         ]
      )
      dict entry(
         string ":1.9"
         array [
            string "type='signal',interface='org.freedesktop.DBus',member='NameOwnerChanged'"
         ]
      )
      dict entry(
         string ":1.11"
         array [
            string "eavesdrop='true'"
         ]
      )
   ]
Comment 17 Alban Crequy 2014-07-01 16:10:00 UTC
(In reply to comment #16)
> Created attachment 102017 [details] [review] [review]
> [PATCH] Implement GetConnectionMatchRules on the Stats interface

In order to enable / disable the Stats interface without recompiling dbus, I filed Bug #80759.
Comment 18 Alban Crequy 2014-07-15 14:22:30 UTC
Created attachment 102858 [details]
Script using GetConnectionMatchRules

I used this script to find match rules with the patterns I was interested in, such as member='NameOwnerChanged' without arg0 criteria or match rules without 'sender' criteria.
Comment 19 Alban Crequy 2014-08-21 14:34:30 UTC
I would like the match rules and other stats to be easily available through d-feet:
https://bugzilla.gnome.org/show_bug.cgi?id=735167
Comment 20 Simon McVittie 2014-09-10 14:30:02 UTC
Comment on attachment 102017 [details] [review]
[PATCH] Implement GetConnectionMatchRules on the Stats interface

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

::: bus/driver.c
@@ +1789,4 @@
>  static const MessageHandler stats_message_handlers[] = {
>    { "GetStats", "", "a{sv}", bus_stats_handle_get_stats },
>    { "GetConnectionStats", "s", "a{sv}", bus_stats_handle_get_connection_stats },
> +  { "GetConnectionMatchRules", "", "a{sas}", bus_stats_handle_get_connection_match_rules },

GetConnectionFoo usually means "take a bus name as an argument, and return foo for that bus name", whereas I think this is returning info about all connections simultaneously. So I'd prefer GetMatchRules or GetAllMatchRules or something.

::: bus/signals.c
@@ +125,5 @@
> +#ifdef DBUS_ENABLE_STATS
> +#define MATCH_RULE_TO_STRING
> +#endif
> +
> +#ifdef MATCH_RULE_TO_STRING

I'd prefer #if defined(DBUS_ENABLE_VERBOSE_MODE) || defined(DBUS_ENABLE_STATS), tbh

@@ +130,2 @@
>  /* Note this function does not do escaping, so it's only
>   * good for debug spew at the moment

Not ideal that it doesn't escape things correctly; consumers of this API will have to take that into account. Unless the comment is a lie, or you fix it.

@@ +1175,5 @@
> +              BusMatchRule *rule = link->data;
> +
> +              if (rule->matches_go_to == conn_filter)
> +                {
> +                  char *s = match_rule_to_string (rule);

if (s == NULL)
  return FALSE;

@@ +1196,5 @@
> +          BusMatchRule *rule = link->data;
> +
> +          if (rule->matches_go_to == conn_filter)
> +            {
> +              char *s = match_rule_to_string (rule);

if (s == NULL)
  return FALSE;

::: bus/stats.c
@@ +41,4 @@
>                              DBusMessage    *message,
>                              DBusError      *error)
>  {
> +  BusContext *context;

This patch-band has already been applied, Bug #81043

@@ +50,5 @@
>  
>    _DBUS_ASSERT_ERROR_IS_CLEAR (error);
>  
> +  context = bus_transaction_get_context (transaction);
> +  connections = bus_context_get_connections (context);

This patch-band has already been applied, Bug #81043

@@ +300,5 @@
> +          goto oom;
> +        }
> +
> +      if (!dbus_message_iter_close_container (&entry_iter, &arr_iter))
> +        goto oom;

Still need to abandon the arr_iter and the hash_iter here

@@ +302,5 @@
> +
> +      if (!dbus_message_iter_close_container (&entry_iter, &arr_iter))
> +        goto oom;
> +      if (!dbus_message_iter_close_container (&hash_iter, &entry_iter))
> +        goto oom;

Still need to abandon the hash_iter here
Comment 21 Alban Crequy 2014-09-24 15:44:12 UTC
Created attachment 106796 [details] [review]
[PATCH 1/5] Implement GetAllMatchRules on the Stats interface

v2:
 - fixes from smcv's review
 - fix memory leaks
Comment 22 Alban Crequy 2014-09-24 15:44:45 UTC
Created attachment 106797 [details] [review]
[PATCH 2/5] Stats: GetAllMatchRules: add tests
Comment 23 Alban Crequy 2014-09-24 15:45:20 UTC
Created attachment 106798 [details] [review]
[PATCH 3/5] match_rule_to_string: returns NULL if no memory instead of looping
Comment 24 Alban Crequy 2014-09-24 15:46:07 UTC
Created attachment 106799 [details] [review]
[PATCH 4/5] match_rule_to_string: fix escaping
Comment 25 Alban Crequy 2014-09-24 15:46:34 UTC
Created attachment 106800 [details] [review]
[PATCH 5/5] match_rule_to_string: add test
Comment 26 Simon McVittie 2014-09-24 16:04:14 UTC
Comment on attachment 106796 [details] [review]
[PATCH 1/5] Implement GetAllMatchRules on the Stats interface

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

::: bus/signals.c
@@ +1157,5 @@
> +
> +      _dbus_hash_iter_init (matchmaker->rules_by_type[i].rules_by_iface, &iter);
> +      while (_dbus_hash_iter_next (&iter))
> +        {
> +          list =  *((DBusList **)_dbus_hash_iter_get_value (&iter));

The data types for "list" and "list link" are conventionally DBusList ** and DBusList * respectively, so I'd prefer:

DBusList **list;

list = _dbus_hash_iter_get_value (&iter));
do_things_with (list);

rather than using &list.

(Using &list like that would be functionally wrong if you altered the list, but as it happens, you don't.)

@@ +1159,5 @@
> +      while (_dbus_hash_iter_next (&iter))
> +        {
> +          list =  *((DBusList **)_dbus_hash_iter_get_value (&iter));
> +          link = _dbus_list_get_first_link (&list);
> +          while (link != NULL)

I'd personally use

for (link = _dbus_list_get_first_link (list);
     link != NULL;
     link = _dbus_list_get_next_link (list))

and eliminate the "next" variable, but that's your call.

(applies twice in this function)

@@ +1182,5 @@
> +
> +              link = next;
> +            }
> +        }
> +      list = matchmaker->rules_by_type[i].rules_without_iface;

With list being a DBusList **, you could use &matchmaker->rules_by_type[i].rules_without_iface here.
Comment 27 Simon McVittie 2014-09-24 16:06:50 UTC
Comment on attachment 106797 [details] [review]
[PATCH 2/5] Stats: GetAllMatchRules: add tests

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

OK

::: bus/dispatch.c
@@ +1609,5 @@
> +    {
> +      if (dbus_message_is_error (message,
> +                                 DBUS_ERROR_NO_MEMORY))
> +        {
> +          ; /* good, this is a valid response */

No need for the semicolon here, and it introduces an empty statement which some compilers warn about. I would prefer without it.

@@ +1622,5 @@
> +  else
> +    {
> +      if (dbus_message_get_type (message) == DBUS_MESSAGE_TYPE_METHOD_RETURN)
> +        {
> +          ; /* good, expected */

ditto
Comment 28 Simon McVittie 2014-09-24 16:08:04 UTC
Comment on attachment 106798 [details] [review]
[PATCH 3/5] match_rule_to_string: returns NULL if no memory instead of looping

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

OK
Comment 29 Alban Crequy 2014-09-24 16:21:22 UTC
Comment on attachment 106797 [details] [review]
[PATCH 2/5] Stats: GetAllMatchRules: add tests

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

::: bus/dispatch.c
@@ +1609,5 @@
> +    {
> +      if (dbus_message_is_error (message,
> +                                 DBUS_ERROR_NO_MEMORY))
> +        {
> +          ; /* good, this is a valid response */

This pattern was copied from others functions: see check_hello_message() and a dozen of other similar functions.
Comment 30 Simon McVittie 2014-09-24 16:34:20 UTC
Comment on attachment 106799 [details] [review]
[PATCH 4/5] match_rule_to_string: fix escaping

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

::: bus/signals.c
@@ +137,5 @@
> +          if (!_dbus_string_append_printf (str, "%.*s", (int) (next - p), p))
> +            return FALSE;
> +          /* Horrible escape sequence: single quote cannot be escaped inside
> +           * a single quoted string. So we close the single quote, escape the
> +           * single quote, and reopen a single quote.

This is absolutely horrible, and the fact that you're writing C makes it look even worse... but it appears to be correct. I looked up the quoting rules in the D-Bus Specification, with the predictable result that I found they are undocumented. I'm preparing a patch.

@@ +176,1 @@
>              dbus_message_type_to_string (rule->message_type)))

This change is unnecessary, because dbus_message_type_to_string() cannot output a string containing a comma, apostrophe or backslash... but your version is more obviously correct, which is the best sort of correct. So I like it better anyway.
Comment 31 Simon McVittie 2014-09-24 16:35:25 UTC
Comment on attachment 106800 [details] [review]
[PATCH 5/5] match_rule_to_string: add test

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

OK
Comment 32 Simon McVittie 2014-09-24 16:41:53 UTC
(In reply to comment #29)
> This pattern was copied from others functions: see check_hello_message() and
> a dozen of other similar functions.

OK, either change it or don't, up to you. I would also be happy to review a patch that replaced all our empty statements with empty {...} blocks (where not already present) containing an appropriate comment such as /* do nothing */ (where not already present).
Comment 33 Simon McVittie 2014-09-24 16:58:33 UTC
Created attachment 106801 [details] [review]
Describe quoting for match rules

I wish I could say "I can't believe this was never documented", but
it wouldn't be true.
Comment 34 Alban Crequy 2014-09-24 17:00:45 UTC
Created attachment 106802 [details] [review]
[PATCH 1/5] Implement GetAllMatchRules on the Stats interface

v3: fixes suggested by smcv in Comment #26
- Use "DBusList **" types
- Use "for" loops
Comment 35 Simon McVittie 2014-09-24 17:19:59 UTC
Comment on attachment 106802 [details] [review]
[PATCH 1/5] Implement GetAllMatchRules on the Stats interface

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

Looks good. I'll apply these.
Comment 36 Alban Crequy 2014-09-24 17:22:44 UTC
Comment on attachment 106801 [details] [review]
Describe quoting for match rules

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

The patch looks good.

To be honest, I wonder whether it would be better to change the implementation (and spec) to be able to escape a quote inside single quotes. It would *probably* not break existing applications.

I'm relieved that D-Bus well-known names, interface names, path names and member names are not allowed to contain a quote. Otherwise, it would break g_dbus_connection_signal_subscribe() because it does not escape quotes when building the match rule string. NameOwnerChanged rules with arg0 are fine too because arg0 is a well-known/unique name without a quote. But it worth checking if g_dbus_connection_signal_subscribe() is used with a non-NameOwnerChanged rule with arg0 by some projects, especially if arg0 comes from user input.

http://codesearch.debian.net/search?q=g_dbus_connection_signal_subscribe
Comment 37 Simon McVittie 2014-09-25 10:32:22 UTC
(In reply to comment #36)
> To be honest, I wonder whether it would be better to change the
> implementation (and spec) to be able to escape a quote inside single quotes.
> It would *probably* not break existing applications.

I would be inclined to just fix any existing implementors that are wrong (e.g. GDBus). Breaking API seems like a riskier fix, and documenting existing behaviour means there is a reference that people can use.
Comment 38 Simon McVittie 2014-09-25 10:49:35 UTC
Created attachment 106846 [details] [review]
Use ISO C strchr() instead of BSD index()

---

Sorry, I didn't spot this while reviewing Attachment #106799 [details]. I'm not 100% sure how portable index() or <strings.h> are, and there's a straightforward replacement with a better name in ISO C, so we should use that. OK to apply?
Comment 39 Alban Crequy 2014-09-25 11:15:12 UTC
Comment on attachment 106846 [details] [review]
Use ISO C strchr() instead of BSD index()

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

It looks good to me.
Comment 40 Simon McVittie 2014-09-25 13:13:19 UTC
Fixed in git for 1.9.0, thanks


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.