Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only a single listener allowed per name using dbus.connect_signal #1071

Closed
hoelzro opened this issue Sep 7, 2016 · 4 comments
Closed

Only a single listener allowed per name using dbus.connect_signal #1071

hoelzro opened this issue Sep 7, 2016 · 4 comments

Comments

@hoelzro
Copy link
Contributor

hoelzro commented Sep 7, 2016

Output of awesome --version:

awesome v3.5.9 (Mighty Ravendark)
 • Build: Sep  7 2016 08:27:17 for x86_64 by gcc version 6.1.1 (rob@eridanus)
 • Compiled against Lua 5.1.5 (running with Lua 5.1)
 • D-Bus support: ✔

How to reproduce the issue:

Call this twice: dbus.connect_signal('org.freedesktop.DBus.Properties', function() end)

Actual result:

dbus.connect_signal returns nil both times, a warning is emitted to Awesome's output, and the second callback function is not invoked for org.freedesktop.DBus.Properties DBus events.

Expected result:

Ideally, I would like for multiple functions to be able to listen for the same DBus event.

Barring that, it would be nice if dbus.connect_signal returned true upon success and nil, string upon failure, like many other Lua functions, so that scripters could know from their script whether or not they were able to register the callback. In addition, it would be nice if there were a way to get the callback currently associated with the name so that the user could manually chain callbacks, like so:

local interface_name = 'org.freedesktop.DBus.Properties'
local function callback(...)
end
if not dbus.connect_signal(interface_name, callback) then
  local current_callback = dbus.get_connection(interface_name)
  dbus.disconnect_signal(interface_name, current_callback)
  dbus.connect_signal(interface_name, function(...)
    pcall(current_callback, ...)
    pcall(callback, ...)
  end)
end
@psychon
Copy link
Member

psychon commented Sep 8, 2016

Awesome's dbus bindings are quite... basic. Also, writing awesome-specific dbus Lua bindings limits their use to awesome. The long-term goal is that someone comes up with generic dbus Lua bindings (based on lgi) that also work in awesome. When this happens, all of the dbus-based code in awesome('s C code) would be dropped.

Thus, I'm not really enthusiastic for improving the existing bindings.

Anyway:

  • Multiple functions connected to the same signal does not work. Even though these are called "signal", this is not a signal in the dbus sense. This is also used for method calls and here the return value of the function is the return value for the dbus call (see e.g. lib/awful/remote.lua). For this reason, we cannot just allow multiple functions to be connected.
  • Chaining callbacks: I don't really see the point...? What would you do with the return value of the existing callback? Just ignore it? That's not good.
  • Returning a boolean from connect_signal telling you if it was successful: Hm, fine with me. Does anyone want to convert the following into a pull request?
diff --git a/dbus.c b/dbus.c
index dc5bcaf..5eb30a8 100644
--- a/dbus.c
+++ b/dbus.c
@@ -761,6 +761,8 @@ luaA_dbus_remove_match(lua_State *L)
  *
  * @param interface A string with the interface name.
  * @param func The function to call.
+ * @treturn boolean true on success, false if the signal could not be connected
+ * because another function is already connected.
  * @function connect_signal
  */
 static int
@@ -771,10 +773,14 @@ luaA_dbus_connect_signal(lua_State *L)
     signal_t *sig = signal_array_getbyid(&dbus_signals,
                                          a_strhash((const unsigned char *) name));
     if(sig)
+    {
         luaA_warn(L, "cannot add signal %s on D-Bus, already existing", name);
-    else
+        lua_pushboolean(L, false);
+    } else {
         signal_connect(&dbus_signals, name, luaA_object_ref(L, 2));
-    return 0;
+        lua_pushboolean(L, true);
+    }
+    return 1;
 }

 /** Remove a signal receiver on the D-Bus.

Unrelated(?) issue:
Awesome currently just prints a warning to stderr "cannot add signal[...]" on the second connect call. However, disconnect does not clean up enough so that the warning still appears.
What I mean is that the following example does not work:

local function f() end
dbus.connect_signal("foo", f)
dbus.disconnect_signal("foo", f)
dbus.connect_signal("foo", function() end) -- This does not work, but prints a warning

Untested patch for this would be (if anyone wants, this can also be turned into a PR (after having been tested; perhaps even adding a new test to tests/ for this? Dunno if that would be too much)):

diff --git a/common/signal.h b/common/signal.h
index d9d5e45..46bad94 100644
--- a/common/signal.h
+++ b/common/signal.h
@@ -92,6 +92,8 @@ signal_disconnect(signal_array_t *arr, const char *name, const void *ref)
             if(ref == *func)
             {
                 cptr_array_remove(&sigfound->sigfuncs, func);
+                if(sigfound->sigfuncs.len == 0)
+                    signal_array_remove(arr, sigfound);
                 return true;
             }
     }

Edit: The above patch also fixes a crash. If a dbus function call fo "foo" comes in after the above connect-disconnect-dance, we crash.

@hoelzro
Copy link
Contributor Author

hoelzro commented Sep 9, 2016

@psychon Thanks for the quick and thorough feedback; it seems that my problems stem from my false assumption that dbus.connect_signal is used for listening to DBus signals. If I understand you correctly, it sounds like connect_signal is used to implement methods on DBus interfaces exposed by the window manager, in which case my request doesn't really make sense!

As all I want is for two decoupled parts of my configuration to listen for the same DBus signal, what would be the recommended approach? AFAICT, awesome could add a separate function in the dbus module for just listening for signal events, but you stated you'd rather avoid improving the internal dbus module, which makes sense to me.

Alternatively, I could use a library like ldbus to connect to dbus for this purpose, but then I would have to poll at an interval to receive signals, as awesome doesn't expose its event loop to Lua, right?

As far as that change above goes, I just submitted at PR for it: #1078

@psychon
Copy link
Member

psychon commented Sep 9, 2016

Alternatively, I could use a library like ldbus to connect to dbus for this purpose, but then I would have to poll at an interval to receive signals, as awesome doesn't expose its event loop to Lua, right?

I'm not sure. Via https://github.com/daurnimator/ldbus I got to the docs for dbus_bus_get() https://dbus.freedesktop.org/doc/api/html/group__DBusBus.html#ga77ba5250adb84620f16007e1b023cf26 which states that

If a connection to the bus already exists, then that connection is returned.

Since awesome's C code sets up the session bus (DBUS_BUS_SESSION) and the system bus (DBUS_BUS_SYSTEM) correctly (at least it should do so), it might be that you can just use ldbus from within awesome without doing anything more about it.

Scratch that. After looking at our code, awesome will actively(?) break things. Its dbus-integration "steals" all dbus messages that come in.

as awesome doesn't expose its event loop to Lua, right?

Awesome's event loop as a glib GMainLoop and that one can be accessed through lgi. Put differently: Anything based on glib should just automatically work.
So it should be possible to integrate ldbus with awesome/lgi somehow, but I don't know the details.

Edit:

As all I want is for two decoupled parts of my configuration to listen for the same DBus signal, what would be the recommended approach?

I didn't even know that we handle dbus signals correctly and hand them to Lua. Hm... this is all quite a horrible API (both dbus signals and dbus function calls look the same to Lua, I think; a lot of information seems to be missing).

In that case, perhaps we should just drop the restriction on only one connection to a signal? Only the first one will be called for function calls, but dbus signals should be handled correctly. I don't know...

@hoelzro
Copy link
Contributor Author

hoelzro commented Sep 9, 2016

@psychon Ok, thanks for the info! I think since my primary concern has been addressed, I'm going to close this issue.

@hoelzro hoelzro closed this as completed Sep 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants