* bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
@ 2011-09-22 23:01 Julien Danjou
2011-09-23 15:37 ` Michael Albinus
0 siblings, 1 reply; 12+ messages in thread
From: Julien Danjou @ 2011-09-22 23:01 UTC (permalink / raw)
To: 9581
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
2011-09-22 23:01 bug#9581: 24.0.50; dbus-unregister-object fails if service is nil Julien Danjou
@ 2011-09-23 15:37 ` Michael Albinus
2011-09-23 16:12 ` Julien Danjou
0 siblings, 1 reply; 12+ messages in thread
From: Michael Albinus @ 2011-09-23 15:37 UTC (permalink / raw)
To: Julien Danjou; +Cc: 9581
Julien Danjou <julien@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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
2011-09-23 15:37 ` Michael Albinus
@ 2011-09-23 16:12 ` Julien Danjou
2011-09-24 11:50 ` Michael Albinus
0 siblings, 1 reply; 12+ messages in thread
From: Julien Danjou @ 2011-09-23 16:12 UTC (permalink / raw)
To: Michael Albinus; +Cc: 9581
[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]
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
[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
2011-09-23 16:12 ` Julien Danjou
@ 2011-09-24 11:50 ` Michael Albinus
2011-09-24 14:19 ` Julien Danjou
0 siblings, 1 reply; 12+ messages in thread
From: Michael Albinus @ 2011-09-24 11:50 UTC (permalink / raw)
To: Julien Danjou; +Cc: 9581
Julien Danjou <julien@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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
2011-09-24 11:50 ` Michael Albinus
@ 2011-09-24 14:19 ` Julien Danjou
2011-09-24 14:37 ` Michael Albinus
0 siblings, 1 reply; 12+ messages in thread
From: Julien Danjou @ 2011-09-24 14:19 UTC (permalink / raw)
To: Michael Albinus; +Cc: 9581
[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]
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
[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
2011-09-24 14:19 ` Julien Danjou
@ 2011-09-24 14:37 ` Michael Albinus
2011-09-25 11:38 ` Julien Danjou
0 siblings, 1 reply; 12+ messages in thread
From: Michael Albinus @ 2011-09-24 14:37 UTC (permalink / raw)
To: Julien Danjou; +Cc: 9581
Julien Danjou <julien@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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
2011-09-24 14:37 ` Michael Albinus
@ 2011-09-25 11:38 ` Julien Danjou
2011-09-25 11:59 ` Michael Albinus
0 siblings, 1 reply; 12+ messages in thread
From: Julien Danjou @ 2011-09-25 11:38 UTC (permalink / raw)
To: Michael Albinus; +Cc: 9581
[-- Attachment #1: Type: text/plain, Size: 2755 bytes --]
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
"‚\x02\x01")))
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\x02\x01")
dbus-call-method(:session "org.freedesktop.DBus" "/org/freedesktop/DBus" "org.freedesktop.DBus" "RemoveMatch" "\202\x02\x01")
Where the last string is obviously the same as I talked about above. :)
--
Julien Danjou
[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
2011-09-25 11:38 ` Julien Danjou
@ 2011-09-25 11:59 ` Michael Albinus
2011-09-25 12:19 ` Julien Danjou
0 siblings, 1 reply; 12+ messages in thread
From: Michael Albinus @ 2011-09-25 11:59 UTC (permalink / raw)
To: Julien Danjou; +Cc: 9581
Julien Danjou <julien@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
> "‚\x01")))
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\x01")
> dbus-call-method(:session "org.freedesktop.DBus" "/org/freedesktop/DBus" "org.freedesktop.DBus" "RemoveMatch" "\202\x01")
>
> 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
2011-09-25 11:59 ` Michael Albinus
@ 2011-09-25 12:19 ` Julien Danjou
2011-09-25 16:03 ` Michael Albinus
0 siblings, 1 reply; 12+ messages in thread
From: Julien Danjou @ 2011-09-25 12:19 UTC (permalink / raw)
To: Michael Albinus; +Cc: 9581
[-- Attachment #1: Type: text/plain, Size: 1931 bytes --]
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
[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
2011-09-25 12:19 ` Julien Danjou
@ 2011-09-25 16:03 ` Michael Albinus
2011-09-26 12:16 ` Julien Danjou
0 siblings, 1 reply; 12+ messages in thread
From: Michael Albinus @ 2011-09-25 16:03 UTC (permalink / raw)
To: Julien Danjou; +Cc: 9581
Julien Danjou <julien@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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
2011-09-25 16:03 ` Michael Albinus
@ 2011-09-26 12:16 ` Julien Danjou
2011-09-26 13:57 ` Michael Albinus
0 siblings, 1 reply; 12+ messages in thread
From: Julien Danjou @ 2011-09-26 12:16 UTC (permalink / raw)
To: Michael Albinus; +Cc: 9581
[-- Attachment #1: Type: text/plain, Size: 955 bytes --]
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
[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#9581: 24.0.50; dbus-unregister-object fails if service is nil
2011-09-26 12:16 ` Julien Danjou
@ 2011-09-26 13:57 ` Michael Albinus
0 siblings, 0 replies; 12+ messages in thread
From: Michael Albinus @ 2011-09-26 13:57 UTC (permalink / raw)
To: Julien Danjou; +Cc: 9581-done
Julien Danjou <julien@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.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-09-26 13:57 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-22 23:01 bug#9581: 24.0.50; dbus-unregister-object fails if service is nil Julien Danjou
2011-09-23 15:37 ` Michael Albinus
2011-09-23 16:12 ` Julien Danjou
2011-09-24 11:50 ` Michael Albinus
2011-09-24 14:19 ` Julien Danjou
2011-09-24 14:37 ` Michael Albinus
2011-09-25 11:38 ` Julien Danjou
2011-09-25 11:59 ` Michael Albinus
2011-09-25 12:19 ` Julien Danjou
2011-09-25 16:03 ` Michael Albinus
2011-09-26 12:16 ` Julien Danjou
2011-09-26 13:57 ` Michael Albinus
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).