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

compiler warning in `dbus2#make-vtable' #5

Closed
retroj opened this issue Nov 21, 2016 · 6 comments
Closed

compiler warning in `dbus2#make-vtable' #5

retroj opened this issue Nov 21, 2016 · 6 comments
Labels

Comments

@retroj
Copy link
Collaborator

retroj commented Nov 21, 2016

Warning: in toplevel procedure `dbus2#make-vtable':
  (dbus2.scm:842) in procedure call to `dbus2#vtable-message_function-set!', expected argument #2 of type `(or boolean pointer locative)' but was given an argument of type `(procedure fn (* * *) fixnum)'
@retroj retroj added the bug label Nov 21, 2016
@retroj
Copy link
Collaborator Author

retroj commented Nov 21, 2016

The problem is mentioned here: https://bugs.call-cc.org/ticket/850

The warning is due to the fact that the vtable-message_function is declared as type c-pointer, but a plain Scheme closure object is set to that pointer. This is not correct, and should usually cause a segfault since Scheme objects are not generally usable as pointers. In this case it happens to extract a pointer to the procedure correctly because closures are represented similarly to pointer objects, but Scheme closures expect an argument count and a continuation as the first two parameters.

If you want to pass a C function pointer correctly, I think you'll need to declare a function in C (or use define-external or something similar) and use foreign-value with type c-pointer to reify it as a pointer.

@ryuslash
Copy link

Ok, after some research here is what I have found. Please correct me if I'm wrong.

dbus_connection_register_object_path is a function to register a handler to a certain object path. So if you register the path /net/retroj/mowedline and a message is sent to the net.retroj.mowedline.update method under the /net/retroj/mowedline path, the registered handler would get called. That handler then seems to have to figure out (by inspecting the message) what to do next (update a widget in this case). This same handler would be called if net.retroj.mowedline.quit is called under the /net/retroj/mowedline path. If net.retroj.mowedline.update is called under another path, this handler wouldn't fire, that would have to be another handler registered with dbus_connection_register_object_path.

The vtable in the argument to dbus_connection_register_object_path gets added to dbus' internal object tree, so that when a message is sent to a certain path, dbus knows the handler to call for that message. It also contains a function that is called when the handler is unregistered, but I don't know at this time why you might want to do that. It's the handler function that causes the warning at this moment. I have very little knowledge of chicken's foreign-function interface, but from what I've read it's kind of a pain. A normal scheme closure can't be passed to a C function like this, and define-external doesn't capture the lexical environment, which the current function passed along relies on.

So much for what dbus_connection_register_object_path (and register-path) does. Now for why we would or wouldn't want to use it.

It seems like libdbus has 3 levels of difficulty/complexity/flexibility for getting messages from the message queue.

  • The most complex one involves using dbus_connection_dispatch along with DBusWatch and DBusTimeout and is asynchronous.

  • A simpler alternative is using dbus_connection_read_write_dispatch, which is synchronous and comparatively easy.

  • The simplest one is the one that the dbus egg is using right now, dbus_connection_pop_message. This is said to only be useful for very simple applications.

When using either dbus_connection_*_dispatch function, handlers are called as registered with, for example, dbus_connection_register_object_path and dbus_connection_add_filter. dbus_connection_pop_message ignores these handlers.

Right now the dbus egg uses dbus_connection_pop_message and handles dispatching to methods and signal handlers itself. Since handlers registered with the libdbus functions are ignored, using dbus_connection_register_object_path won't do anything.

We could move to one of the more complex methods of handling messages, but for the moment the dbus egg seems to work fine as far as I can tell, and I don't see a downside to the dispatching being handled completely in scheme. I'm only aware of mowedline using it however, so it is possible of course that I just don't know about it.

Also, if we do move to one of the more complex methods, I don't think that register-path would be a method that should be part of the api. libdbus is very low-level and isn't normally recommended for regular use (on their website), and I think a scheme library would want to provide a more high-level api than this anyway (as it now does, registering methods and signal handlers).

@retroj
Copy link
Collaborator Author

retroj commented Nov 26, 2016

On the somewhat broader issue of how should dbus-egg's main loop work, an issue that I ran into with mowedline is detailed here: retroj/mowedline#22 flood of dbus messages bogs down mowedline

The gist is that if we use dbus_connection_pop_message as now, applications are forced to use something like (thread-sleep! 0.01) in their dbus thread. This lets them not consume 100% cpu, but also makes them susceptible to getting bogged down when a flood of dbus messages arrives. The solution I proposed in that ticket was a method that would allow us to use thread-wait-for-i/o! instead, which is equivalent to select(). I found this in the dbus documentation though:

DO NOT read or write to the file descriptor, or try to select() on it; use DBusWatch for main loop integration. reference

@retroj
Copy link
Collaborator Author

retroj commented Nov 26, 2016

Without rewriting the entire egg to use DBusWatch (complicated and uncertain), we may be able to get the equivalent of select() with dbus_connection_get_dispatch_status(). If dbus-egg exposes this api method, programs can use it to handle all waiting dbus messages in between explicit calls to thread-sleep!.

https://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga893d18d8b36ffb371f16d13645071289

@retroj
Copy link
Collaborator Author

retroj commented Nov 26, 2016

Opened issue #6 to discuss the question of handling all waiting messages. In this ticket we can just decide what to do about the original vtable issue. I think that we should remove the vtable and associated code for now, until such time as we decide to rewrite the egg to use DBusWatch instead of dbus_connection_pop_message.

retroj pushed a commit that referenced this issue Nov 29, 2016
`dbus_connection_register_object_path` is part of a different api for
interacting with DBus than the one this egg is using.  VTables are not
needed for the way this egg gets messages from DBus, namely
`dbus_connection_pop_message`.  The bindings here are also incorrect and
produced a compiler warning:

    Warning: in toplevel procedure `dbus2#make-vtable': (dbus2.scm:842)
      in procedure call to `dbus2#vtable-message_function-set!',
      expected argument #2 of type `(or boolean pointer locative)' but
      was given an argument of type `(procedure fn (* * *) fixnum)'

The simplest thing is to remove this code until such time as we decide
to rewrite the egg to use another method (like DBusWatch) to handle
messages.

This problem is also described here: https://bugs.call-cc.org/ticket/850

Resolves issue #5.
@retroj
Copy link
Collaborator Author

retroj commented Nov 29, 2016

Done

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

No branches or pull requests

2 participants