Bug 66107

Created attachment 81301 [details] [review]
[PATCH] dbus-monitor: --monitor can not be used with --profile

Created attachment 81438 [details] [review]
[PATCH 1/3] dbus-monitor: --monitor can not be used with --profile

"--monitor" and "--profile" are conflict with each other in fact, so this patch does that.

Created attachment 81439 [details] [review]
[PATCH 2/3] dbus-monitor: remove redundant match rules

There are only four types of message, since dbus-monitor will monitor all of these four types, so just remove "type=" from match rules is fine.

Created attachment 81440 [details] [review]
[PATCH 3/3] dbus-monitor: keep backwards compatibility

keep backwards compatibility with old dbus which have no eavesdropping as a key of match rule, see more at #bug 66114.

Comment on attachment 81438 [details] [review]
[PATCH 1/3] dbus-monitor: --monitor can not be used with --profile

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

I'm not sure I like this. Having later contradictory options override earlier ones is pretty common: for instance, "gcc -Wno-error -Werror" is equivalent to "gcc -Werror".

On the other hand, GNU coreutils grep does produce an error if you try to use both -F and -E - it's a matter of taste really.

When presented with this sort of "it's a matter of opinion" decision, I tend to go for "keep things as they are" if there's no compelling reason either way: it's less work, and can't break existing working scripts. So I'm inclined to say no to this (but if another maintainer feels strongly about it, they're welcome to overrule me).

Comment on attachment 81439 [details] [review]
[PATCH 2/3] dbus-monitor: remove redundant match rules

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

Fine. Applied for 1.7.6

Comment on attachment 81440 [details] [review]
[PATCH 3/3] dbus-monitor: keep backwards compatibility

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

If someone had written this patch before 1.6 was the stable release, I'd be more enthusiastic about it. As it is, though, the stable release of dbus has had the "new" behaviour for 16 months - is it really worth giving debug tools backwards-compatibility with a 16-month-old dbus-daemon? What's the use-case? Presumably anyone whose distribution has dbus-daemon 1.4 or older (e.g. Debian 6) will be using dbus-monitor 1.4, too...

::: tools/dbus-monitor.c
@@ +377,4 @@
>  
>    if (numFilters)
>      {
> +      int offset = 0;

Conceptually, shouldn't this be a size_t?

@@ +377,5 @@
>  
>    if (numFilters)
>      {
> +      int offset = 0;
> +fallback:

I don't like backwards goto: it obscures the control flow and is usually an indication that you should have used a proper control structure, like a loop.

(I do like to use goto to emulate "try/catch" and "try/finally" in C - I think that's one of its few legitimate uses.)

@@ +390,5 @@
> +	      if (i == 0 && offset == 0)
> +		{
> +		  offset = strlen (EAVESDROPPING_RULE) + 1;
> +		  dbus_error_free (&error);
> +		  goto fallback;

How about this (pseudocode in approximately diff syntax) which would avoid needing the goto?

+  size_t offset = 0;
+
   for (i = 0; i < j; i++)
     {
-      dbus_bus_add_match (...);
+      dbus_bus_add_match (... + offset ...);

+      if (i == 0 && offset == 0 && dbus_error_is_set (&error))
+        {
+          /* We might be talking to a pre-1.5.6 dbus-daemon
+           * which wouldn't understand eavesdrop=true.
+           * If this works, carry on with offset > 0
+           * on the remaining iterations. */
+          offset = strlen (EAVESDROPPING_RULE) + 1;
+          dbus_error_free (&error);
+          dbus_bus_add_match (...);
+        }
+
       if (dbus_error_is_set (&error))
         ... original code continues ...

(In reply to comment #5)
> Comment on attachment 81438 [details] [review] [review]
> [PATCH 1/3] dbus-monitor: --monitor can not be used with --profile
> 
> Review of attachment 81438 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure I like this. Having later contradictory options override
> earlier ones is pretty common: for instance, "gcc -Wno-error -Werror" is
> equivalent to "gcc -Werror".
> 
> On the other hand, GNU coreutils grep does produce an error if you try to
> use both -F and -E - it's a matter of taste really.
> 
> When presented with this sort of "it's a matter of opinion" decision, I tend
> to go for "keep things as they are" if there's no compelling reason either
> way: it's less work, and can't break existing working scripts. So I'm
> inclined to say no to this (but if another maintainer feels strongly about
> it, they're welcome to overrule me).

OK, fear enough.

(In reply to comment #7)
> Comment on attachment 81440 [details] [review] [review]
> [PATCH 3/3] dbus-monitor: keep backwards compatibility
> 
> Review of attachment 81440 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> If someone had written this patch before 1.6 was the stable release, I'd be
> more enthusiastic about it. As it is, though, the stable release of dbus has
> had the "new" behaviour for 16 months - is it really worth giving debug
> tools backwards-compatibility with a 16-month-old dbus-daemon? What's the
> use-case? Presumably anyone whose distribution has dbus-daemon 1.4 or older
> (e.g. Debian 6) will be using dbus-monitor 1.4, too...

Yep, Debian is a major distro which may not release a new major release in 16 months. :-).

> 
> ::: tools/dbus-monitor.c
> @@ +377,4 @@
> >  
> >    if (numFilters)
> >      {
> > +      int offset = 0;
> 
> Conceptually, shouldn't this be a size_t?
> 
> @@ +377,5 @@
> >  
> >    if (numFilters)
> >      {
> > +      int offset = 0;
> > +fallback:
> 
> I don't like backwards goto: it obscures the control flow and is usually an
> indication that you should have used a proper control structure, like a loop.
> 
> (I do like to use goto to emulate "try/catch" and "try/finally" in C - I
> think that's one of its few legitimate uses.)
> 
> @@ +390,5 @@
> > +	      if (i == 0 && offset == 0)
> > +		{
> > +		  offset = strlen (EAVESDROPPING_RULE) + 1;
> > +		  dbus_error_free (&error);
> > +		  goto fallback;
> 
> How about this (pseudocode in approximately diff syntax) which would avoid
> needing the goto?
> 
> +  size_t offset = 0;
> +
>    for (i = 0; i < j; i++)
>      {
> -      dbus_bus_add_match (...);
> +      dbus_bus_add_match (... + offset ...);
> 
> +      if (i == 0 && offset == 0 && dbus_error_is_set (&error))
> +        {
> +          /* We might be talking to a pre-1.5.6 dbus-daemon
> +           * which wouldn't understand eavesdrop=true.
> +           * If this works, carry on with offset > 0
> +           * on the remaining iterations. */
> +          offset = strlen (EAVESDROPPING_RULE) + 1;
> +          dbus_error_free (&error);
> +          dbus_bus_add_match (...);
> +        }
> +
>        if (dbus_error_is_set (&error))
>          ... original code continues ...

Created attachment 87315 [details] [review]
[PATCH v2] dbus-monitor: keep backwards compatibility

(In reply to comment #9)
> > What's the
> > use-case? Presumably anyone whose distribution has dbus-daemon 1.4 or older
> > (e.g. Debian 6) will be using dbus-monitor 1.4, too...
> 
> Yep, Debian is a major distro which may not release a new major release in
> 16 months. :-)

Debian 6 is the "oldstable" (previous stable) release. Debian 7 is the current stable, and has D-Bus 1.6.

In any case, I would expect Debian stable users to use the dbus-monitor from their dbus package, not some random updated version: the whole point of having a stable distribution is that things don't change unnecessarily.

Your patch looks fine though, so I applied it. Preparing 1.7.6 now.