GNU bug report logs - #9581
24.0.50; dbus-unregister-object fails if service is nil

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 9581 in the body.
You can then email your comments to 9581 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Julien Danjou <julien <at> danjou.info>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.0.50; dbus-unregister-object fails if service is nil
Date: Fri, 23 Sep 2011 01:01:17 +0200

If you use `dbus-register-signal' with a nil SERVICE value, like:

      (dbus-register-signal :session nil
                            "/org/gtk/Private/RemoteVolumeMonitor"
                            "org.gtk.Private.RemoteVolumeMonitor" "VolumeAdded"
                            'identity)

This is valid and works fine. However, on unregister it fails:
Debugger entered--Lisp error: (dbus-error "Call to ReleaseName has wrong args (b, expected s)")
  dbus-call-method(:session "org.freedesktop.DBus" "/org/freedesktop/DBus" "org.freedesktop.DBus" "ReleaseName" nil)
  dbus-unregister-object(((:session "org.gtk.Private.RemoteVolumeMonitor" "VolumeAdded") (nil "/org/gtk/Private/RemoteVolumeMonitor" identity)))

Why does it fail? Because of the following code in
`dbus-unregister-object':

#+begin_src emacs-lisp
	(unless found
	  (dbus-call-method
	   bus dbus-service-dbus dbus-path-dbus dbus-interface-dbus
	   "ReleaseName" service))))
+end_src

And here service is… nil. Which is translated to a boolean (b) but
should be a string (s).

But honestly, I'm not sure what the good fix is. To me, this code is
totally wrong in such a case.

When using `dbus-register-signal', this happens:
1. the dbus_bus_add_match() function is called to add a match on the bus
2. the (match callback) pair is recorded into
   `dbus-registered-objets-table' 

This makes things work. When a signal happens, something is looking into
`dbus-registered-objets-table' and call the callback function.

But to stop listening for a signal, the function to use is
`dbus-unregister-object', and it is doing this:
1. remove the (match callback) pair from `dbus-registered-objets-table'
2. call ReleaseName on the service we were listening

While I agree on point 1., the point 2. is totally irrelevant in such a
case. There's no need to do such a thing, the name has never been
requested with RequestName before.

I think that:
- step 2 should be removed or another function should be created which
  does not send a ReleaseName
- dbus_bus_remove_match() should be used to remove the watch from the
  bus, which would be a lot cleaner.

-- 
Julien Danjou

Message #8 received at 9581 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Julien Danjou <julien <at> danjou.info>
Cc: 9581 <at> debbugs.gnu.org
Subject: Re: bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
Date: Fri, 23 Sep 2011 17:37:31 +0200

Julien Danjou <julien <at> danjou.info> writes:

> When using `dbus-register-signal', this happens:
> 1. the dbus_bus_add_match() function is called to add a match on the bus
> 2. the (match callback) pair is recorded into
>    `dbus-registered-objets-table' 
>
> This makes things work. When a signal happens, something is looking into
> `dbus-registered-objets-table' and call the callback function.
>
> But to stop listening for a signal, the function to use is
> `dbus-unregister-object', and it is doing this:
> 1. remove the (match callback) pair from `dbus-registered-objets-table'
> 2. call ReleaseName on the service we were listening
>
> While I agree on point 1., the point 2. is totally irrelevant in such a
> case. There's no need to do such a thing, the name has never been
> requested with RequestName before.

It's simply an error. We are speaking abut the generalized
`dbus-unregister-object', which is used for both signals and
methods. ReleaseName shall be called only in case a *method* has been
registered; I'll fix this.

> I think that:
> - step 2 should be removed or another function should be created which
>   does not send a ReleaseName

Nope. See above.

> - dbus_bus_remove_match() should be used to remove the watch from the
>   bus, which would be a lot cleaner.

Good point. Registering a signal shall also keep the match string in
dbus-registered-objects-table (it doesn't yet). Then we could use this
string to send RemoveMatch.

I'll prepare a patch for this (hopefully in time before starting the
pretest).

Best regards, Michael.

Message #11 received at 9581 <at> debbugs.gnu.org (full text, mbox):

From: Julien Danjou <julien <at> danjou.info>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 9581 <at> debbugs.gnu.org
Subject: Re: bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
Date: Fri, 23 Sep 2011 18:12:04 +0200

[Message part 1 (text/plain, inline)]

On Fri, Sep 23 2011, Michael Albinus wrote:

>> While I agree on point 1., the point 2. is totally irrelevant in such a
>> case. There's no need to do such a thing, the name has never been
>> requested with RequestName before.
>
> It's simply an error. We are speaking abut the generalized
> `dbus-unregister-object', which is used for both signals and
> methods. ReleaseName shall be called only in case a *method* has been
> registered; I'll fix this.
>
>> I think that:
>> - step 2 should be removed or another function should be created which
>>   does not send a ReleaseName
>
> Nope. See above.

Ok, if you do this in one function, I agree with you then.

>> - dbus_bus_remove_match() should be used to remove the watch from the
>>   bus, which would be a lot cleaner.
>
> Good point. Registering a signal shall also keep the match string in
> dbus-registered-objects-table (it doesn't yet). Then we could use this
> string to send RemoveMatch.
>
> I'll prepare a patch for this (hopefully in time before starting the
> pretest).

It sounds like a solution. What I don't like is that it's not really
opaque, so somebody could mess with this, but well… I might be paranoid. :)

-- 
Julien Danjou

[Message part 2 (application/pgp-signature, inline)]

Message #14 received at 9581 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Julien Danjou <julien <at> danjou.info>
Cc: 9581 <at> debbugs.gnu.org
Subject: Re: bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
Date: Sat, 24 Sep 2011 13:50:29 +0200

Julien Danjou <julien <at> danjou.info> writes:

I've committed a fix for both problems, could you, please, check?

> It sounds like a solution. What I don't like is that it's not really
> opaque, so somebody could mess with this, but well… I might be paranoid. :)

Reading the code, `dbus-registered-objects-table' has become an
unreadable format. Maybe we shall redesign the format, and move most of
the functionality from dbusbind.c to dbus.el. But that's something for
after-the-release.

Best regards, Michael.

Message #17 received at 9581 <at> debbugs.gnu.org (full text, mbox):

From: Julien Danjou <julien <at> danjou.info>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 9581 <at> debbugs.gnu.org
Subject: Re: bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
Date: Sat, 24 Sep 2011 16:19:41 +0200

[Message part 1 (text/plain, inline)]

On Sat, Sep 24 2011, Michael Albinus wrote:

> I've committed a fix for both problems, could you, please, check?

The patch is not enough. It fixes the precise case I reported, but this
fails now:

(setq db
      (dbus-register-signal :session "some.service"
                            "/org/gtk/Private/RemoteVolumeMonitor"
                            "org.gtk.Private.RemoteVolumeMonitor" "VolumeAdded"
                            'identity))
(dbus-unregister-object db)

Debugger entered--Lisp error: (dbus-error "Match rule has a key with no subsequent '=' character")
  dbus-call-method(:session "org.freedesktop.DBus" "/org/freedesktop/DBus" "org.freedesktop.DBus" "RemoveMatch" "Z")
  dbus-unregister-object(((:session "org.gtk.Private.RemoteVolumeMonitor" "VolumeAdded") ("some.service" "/org/gtk/Private/RemoteVolumeMonitor" identity)))
  eval((dbus-unregister-object db) nil)

And, I've not tested, but AFAICS you added a check "(when service …"
before running ReleaseName. Just not that you must not do a ReleaseName
if you are unregistering a match, and I've the feeling that this code
will do.

> Reading the code, `dbus-registered-objects-table' has become an
> unreadable format. Maybe we shall redesign the format, and move most of
> the functionality from dbusbind.c to dbus.el. But that's something for
> after-the-release.

I totally agree with that. It needs to be reworked. :)

-- 
Julien Danjou

[Message part 2 (application/pgp-signature, inline)]

Message #20 received at 9581 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Julien Danjou <julien <at> danjou.info>
Cc: 9581 <at> debbugs.gnu.org
Subject: Re: bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
Date: Sat, 24 Sep 2011 16:37:29 +0200

Julien Danjou <julien <at> danjou.info> writes:

> The patch is not enough. It fixes the precise case I reported, but this
> fails now:
>
> (setq db
>       (dbus-register-signal :session "some.service"
>                             "/org/gtk/Private/RemoteVolumeMonitor"
>                             "org.gtk.Private.RemoteVolumeMonitor" "VolumeAdded"
>                             'identity))
> (dbus-unregister-object db)

I've played exactly this example (replacing "some.service" by
"org.gtk.Private.GduVolumeMonitor" in order to have an existing
service). No problem.

> Debugger entered--Lisp error: (dbus-error "Match rule has a key with no subsequent '=' character")
>   dbus-call-method(:session "org.freedesktop.DBus" "/org/freedesktop/DBus" "org.freedesktop.DBus" "RemoveMatch" "Z")

Where does the "Z" comes from? There will never be such a rule, added by
AddMatch.

Did you compile also dbusbind.c before testing?

Could you apply (dbus-list-hash-table) before calling
`dbus-unregister-object', and show the result?

>> Reading the code, `dbus-registered-objects-table' has become an
>> unreadable format. Maybe we shall redesign the format, and move most of
>> the functionality from dbusbind.c to dbus.el. But that's something for
>> after-the-release.
>
> I totally agree with that. It needs to be reworked. :)

I'll prepare a patch. Locally, there are already some of them waiting
for after-the-release.

Best regards, Michael.

Message #23 received at 9581 <at> debbugs.gnu.org (full text, mbox):

From: Julien Danjou <julien <at> danjou.info>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 9581 <at> debbugs.gnu.org
Subject: Re: bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
Date: Sun, 25 Sep 2011 13:38:21 +0200

[Message part 1 (text/plain, inline)]

On Sat, Sep 24 2011, Michael Albinus wrote:

>> The patch is not enough. It fixes the precise case I reported, but this
>> fails now:
>>
>> (setq db
>>       (dbus-register-signal :session "some.service"
>>                             "/org/gtk/Private/RemoteVolumeMonitor"
>>                             "org.gtk.Private.RemoteVolumeMonitor" "VolumeAdded"
>>                             'identity))
>> (dbus-unregister-object db)
>
> I've played exactly this example (replacing "some.service" by
> "org.gtk.Private.GduVolumeMonitor" in order to have an existing
> service). No problem.

Indeed. It works fine with org.gtk.Private.GduVolumeMonitor as service,
but with "some.service" it fails. Why?

>> Debugger entered--Lisp error: (dbus-error "Match rule has a key with no subsequent '=' character")
>>   dbus-call-method(:session "org.freedesktop.DBus" "/org/freedesktop/DBus" "org.freedesktop.DBus" "RemoveMatch" "Z")
>
> Where does the "Z" comes from? There will never be such a rule, added by
> AddMatch.

It's not a Z. One of our MUA altered this.
On the first try it's " ^H\330"
On the second try it's "Z^B"
On the third try it's "\300#\264"
…
(I register then unregister to make a try)

> Did you compile also dbusbind.c before testing?

Oh yes I'm sure of that. :)

> Could you apply (dbus-list-hash-table) before calling
> `dbus-unregister-object', and show the result?

Yeah. I start emacs-snapshot, then register then call
`dbus-list-hash-table', it messages:

(((:session "org.freedesktop.Notifications" "ActionInvoked")
  (":1.129"
   "org.freedesktop.Notifications" "/org/freedesktop/Notifications"
   notifications-on-action-signal
   "type='signal',interface='org.freedesktop.Notifications',member='ActionInvoked',sender=':1.129',path='/org/freedesktop/Notifications'"))
 ((:session "org.freedesktop.Notifications" "NotificationClosed")
  (":1.129" "org.freedesktop.Notifications"
   "/org/freedesktop/Notifications" notifications-on-closed-signal
   "type='signal',interface='org.freedesktop.Notifications',member='NotificationClosed',sender=':1.129',path='/org/freedesktop/Notifications'"))
 ((:session "org.gtk.Private.RemoteVolumeMonitor" "VolumeAdded")
  (""
   "some.service" "/org/gtk/Private/RemoteVolumeMonitor" identity
   "‚
")))

In case one of our MUA will change the last string on the last line it
shows:
"^A\202^F^B^D^A"

Then I call unregister it yells:
Debugger entered--Lisp error: (dbus-error "Unable to append argument" "\202
")
  dbus-call-method(:session "org.freedesktop.DBus" "/org/freedesktop/DBus" "org.freedesktop.DBus" "RemoveMatch" "\202
")

Where the last string is obviously the same as I talked about above. :)

-- 
Julien Danjou

[Message part 2 (application/pgp-signature, inline)]

Message #26 received at 9581 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Julien Danjou <julien <at> danjou.info>
Cc: 9581 <at> debbugs.gnu.org
Subject: Re: bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
Date: Sun, 25 Sep 2011 13:59:45 +0200

Julien Danjou <julien <at> danjou.info> writes:

>> I've played exactly this example (replacing "some.service" by
>> "org.gtk.Private.GduVolumeMonitor" in order to have an existing
>> service). No problem.
>
> Indeed. It works fine with org.gtk.Private.GduVolumeMonitor as service,
> but with "some.service" it fails. Why?

`dbus-register-signal' checks for the a valid service name, if it isn't
nil. Usually, "some.service" is not known; in my test
`dbus-register-signal' raises an error then.

How did you manage to register your signal with that service?

>> Could you apply (dbus-list-hash-table) before calling
>> `dbus-unregister-object', and show the result?
>
> Yeah. I start emacs-snapshot, then register then call
> `dbus-list-hash-table', it messages:
>
> (((:session "org.freedesktop.Notifications" "ActionInvoked")
>   (":1.129"
>    "org.freedesktop.Notifications" "/org/freedesktop/Notifications"
>    notifications-on-action-signal
>    "type='signal',interface='org.freedesktop.Notifications',member='ActionInvoked',sender=':1.129',path='/org/freedesktop/Notifications'"))
>  ((:session "org.freedesktop.Notifications" "NotificationClosed")
>   (":1.129" "org.freedesktop.Notifications"
>    "/org/freedesktop/Notifications" notifications-on-closed-signal
>    "type='signal',interface='org.freedesktop.Notifications',member='NotificationClosed',sender=':1.129',path='/org/freedesktop/Notifications'"))

These two entries show the correct match rule (the respective last element).

>  ((:session "org.gtk.Private.RemoteVolumeMonitor" "VolumeAdded")
>   (""
>    "some.service" "/org/gtk/Private/RemoteVolumeMonitor" identity
>    "‚
")))

This entry has a corrupted match rule. Again, which trick brings
`dbus-register-signal' to accept it? I must implement a counter-check
for this!

> Then I call unregister it yells:
> Debugger entered--Lisp error: (dbus-error "Unable to append argument" "\202
")
>   dbus-call-method(:session "org.freedesktop.DBus" "/org/freedesktop/DBus" "org.freedesktop.DBus" "RemoveMatch" "\202
")
>
> Where the last string is obviously the same as I talked about above. :)

Which is the correct answer, because there isn't a valid match rule. I
could add a check for a valid match rule before sending the
"RemoveMatch" message, but I believe this is superfluous, because there
is exactly one place that match rule is appended. At this place, we must
prevent wrong values.

Best regards, Michael.

Message #29 received at 9581 <at> debbugs.gnu.org (full text, mbox):

From: Julien Danjou <julien <at> danjou.info>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 9581 <at> debbugs.gnu.org
Subject: Re: bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
Date: Sun, 25 Sep 2011 14:19:07 +0200

[Message part 1 (text/plain, inline)]

On Sun, Sep 25 2011, Michael Albinus wrote:

> `dbus-register-signal' checks for the a valid service name, if it isn't
> nil. Usually, "some.service" is not known; in my test
> `dbus-register-signal' raises an error then.
>
> How did you manage to register your signal with that service?

No idea. But the check seems to be not functionnal here obviously.

>>  ((:session "org.gtk.Private.RemoteVolumeMonitor" "VolumeAdded")
>>   (""
>>    "some.service" "/org/gtk/Private/RemoteVolumeMonitor" identity
>>    "‚")))
>
> This entry has a corrupted match rule. Again, which trick brings
> `dbus-register-signal' to accept it? I must implement a counter-check
> for this!

Yes. If you want me to test a patch before committing it, or to run a
debug patch with some printf or whatever, do not hesitate.

>> Then I call unregister it yells:
>> Debugger entered--Lisp error: (dbus-error "Unable to append argument" "\202")
>>   dbus-call-method(:session "org.freedesktop.DBus" "/org/freedesktop/DBus" "org.freedesktop.DBus" "RemoveMatch" "\202")
>>
>> Where the last string is obviously the same as I talked about above. :)
>
> Which is the correct answer, because there isn't a valid match rule. I
> could add a check for a valid match rule before sending the
> "RemoveMatch" message, but I believe this is superfluous, because there
> is exactly one place that match rule is appended. At this place, we must
> prevent wrong values.

Again, be careful on one last thing. I did a couple of tests in an Emacs
session, and sometimes I saw:

method call sender=:1.254 -> dest=org.freedesktop.DBus serial=27 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=ReleaseName
   string "some.service"

And I was *only* testing dbus-register-signal, so there seems to be
still some case or the "(when service …" stuff is doing ReleaseName even
on a signal match.

-- 
Julien Danjou

[Message part 2 (application/pgp-signature, inline)]

Message #32 received at 9581 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Julien Danjou <julien <at> danjou.info>
Cc: 9581 <at> debbugs.gnu.org
Subject: Re: bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
Date: Sun, 25 Sep 2011 18:03:50 +0200

Julien Danjou <julien <at> danjou.info> writes:

>> `dbus-register-signal' checks for the a valid service name, if it isn't
>> nil. Usually, "some.service" is not known; in my test
>> `dbus-register-signal' raises an error then.
>>
>> How did you manage to register your signal with that service?
>
> No idea. But the check seems to be not functionnal here obviously.

The point was that I have `dbus-debug' set to t "since ever". In this
case, `dbus-get-name-owner' raises an error, which I've seen ...

>>>  ((:session "org.gtk.Private.RemoteVolumeMonitor" "VolumeAdded")
>>>   (""
>>>    "some.service" "/org/gtk/Private/RemoteVolumeMonitor" identity
>>>    "‚")))
>>
>> This entry has a corrupted match rule. Again, which trick brings
>> `dbus-register-signal' to accept it? I must implement a counter-check
>> for this!
>
> Yes. If you want me to test a patch before committing it, or to run a
> debug patch with some printf or whatever, do not hesitate.

Should be fixed now.

> Again, be careful on one last thing. I did a couple of tests in an Emacs
> session, and sometimes I saw:
>
> method call sender=:1.254 -> dest=org.freedesktop.DBus serial=27
> path=/org/freedesktop/DBus; interface=org.freedesktop.DBus;
> member=ReleaseName
>    string "some.service"
>
> And I was *only* testing dbus-register-signal, so there seems to be
> still some case or the "(when service …" stuff is doing ReleaseName even
> on a signal match.

Also fixed.

Thanks for testing, and best regards, Michael.

Message #35 received at 9581 <at> debbugs.gnu.org (full text, mbox):

From: Julien Danjou <julien <at> danjou.info>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 9581 <at> debbugs.gnu.org
Subject: Re: bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
Date: Mon, 26 Sep 2011 14:16:09 +0200

[Message part 1 (text/plain, inline)]

On Sun, Sep 25 2011, Michael Albinus wrote:

> The point was that I have `dbus-debug' set to t "since ever". In this
> case, `dbus-get-name-owner' raises an error, which I've seen ...

Ok, that explains everything! :)

>>> This entry has a corrupted match rule. Again, which trick brings
>>> `dbus-register-signal' to accept it? I must implement a counter-check
>>> for this!
>>
>> Yes. If you want me to test a patch before committing it, or to run a
>> debug patch with some printf or whatever, do not hesitate.
>
> Should be fixed now.

It is fixed now.

>> And I was *only* testing dbus-register-signal, so there seems to be
>> still some case or the "(when service …" stuff is doing ReleaseName even
>> on a signal match.
>
> Also fixed.

This if fixed now.

I think you can close this bug, and maybe reopen one if you want to
track the other changes you want to do after the pretest. You call. :)

-- 
Julien Danjou

[Message part 2 (application/pgp-signature, inline)]

Message #40 received at 9581-done <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Julien Danjou <julien <at> danjou.info>
Cc: 9581-done <at> debbugs.gnu.org
Subject: Re: bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
Date: Mon, 26 Sep 2011 15:57:57 +0200

Julien Danjou <julien <at> danjou.info> writes:

> I think you can close this bug, and maybe reopen one if you want to
> track the other changes you want to do after the pretest. You call. :)

OK, closed.

I don't believe we need another bug report just to remind myself ... I
tend to forget everything (ask my wife!), but this patch is already
prepared in my local repo. I should find it after the release.

Thanks for your patient testing, and best regards, Michael.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.