notification: dbus transport plugins #1825
Conversation
src/libs/io/io.c
Outdated
| * @param name Function name | ||
| * @return Pointer to function | ||
| */ | ||
| static size_t getPluginFunction (Plugin * plugin, const char * name) |
There was a problem hiding this comment.
@markus2330: Is there a way to share functions like these internally (e.g. with libraries and plugins) and not export them? I feel like I'm missing some obvious choice here :)
There was a problem hiding this comment.
To share a library function (in any way outside of the source file/library itself) requires the function to be exported (including the c file is a hack only suitable for tests).
To share a function in a plugin you can use the contract (the exports). Of course you could realize something similar for your library but that most likely is overkill.
Btw. do you know elektraInvokeGetFunction from the elektra-invoke library implemented in src/libs/invoke/invoke.c?
It looks very similar to your code.
There was a problem hiding this comment.
Ok, thank you very much!
Yes I have seen it, but it uses ElektraInvokeHandle which wraps Plugin. I'm using getPluginFunction at various points where I directly have a Plugin handle available (like when iterating global plugins or passing the I/O binding to list's subplugins and in the notification library to access register*-functions).
How about adding this particular function to libplugin as elektraPluginGetFunction?
There was a problem hiding this comment.
Yes, this is a very good idea!
I am even thinking about making elektra-invoke dependent on elektra-plugin to use your function. Both getPluginFunction and elektraInvokeGetFunction are lacking the optimization to directly use plugin->kdb* when requesting one of the 5 standard functions (open, close, get, set, error).
If we add the dep to elektra-plugin from elektra-invoke we could get rid of any access to Plugin's struct within elektra-invoke, making elektra-invoke independent of internals of the Plugin's struct. elektra-plugin obviously needs by design access to the struct.
There was a problem hiding this comment.
Ok, great! I will add it to libplugin.
I can make the changes to elektra-invoke later on within this PR.
src/libs/io/io.c
Outdated
| return func; | ||
| } | ||
|
|
||
| static void debugKeySet (char * name, KeySet * ks) |
There was a problem hiding this comment.
This function yields a warning because static functions need to be used:
https://build.libelektra.org/jenkins/view/compiler%20warnings/job/elektra-gcc-configure-debian-log/1309/warningsResult/source.-2911739015347291038/#57
There was a problem hiding this comment.
Thanks! I better enable -Werror to get these messages locally too.
src/libs/notification/notification.c
Outdated
|
|
||
| void elektraNotificationDoUpdate (Key * key, void * data) | ||
| { | ||
| KDB * kdb = (KDB *)data; |
There was a problem hiding this comment.
@markus2330 plugins get this callback passed on "openNotification" (exported function by plugin). the additional callback data (of type void *) is a KDB handle. Since I need to call kdbGet() the handle is required.
I'm not sure if this can be bypassed. Maybe passing a struct that wraps the handle to mask it, but that's security by obscurity :)
There was a problem hiding this comment.
Why is void* used instead of KDB*?
Yes, it can make sense to have an extra struct, then you can pass more than a KDB* handle to the callback.
In any case: This is a question of API design. Why should it be related to security?
There was a problem hiding this comment.
I haven't decided about the struct but didn't want to use KDB so I used void for now.
I thought in our discussion on why KBD was not in Plugin security was the problem.
If having KDB passed in a struct to plugins is not an issue, I'll use a struct to keep the API easier to maintain.
|
I would like to add automated tests for the dbus plugins. This requires either mocking of the dbus library functions or having a dbus daemon running on the build server. |
At least on macOS (Travis) you should be able to start dbus with sudo dbus-daemon --systemfails with the message
, at least on my machine. Maybe the same command works on the Linux Travis build jobs. Anyway, I would just recommend you try it. If you think the build times are too long, I would recommend you just remove most of the code in |
|
Installing dbus on the Jenkins agents is no problem, I suppose most agents already have dbus. In any case, it would be good if you check within tests if dbus is available and stop the test if it won't work. |
|
Ok great! Thanks for looking into it and for the tip, @sanssecours! Build times should not be affected (unless I need dbus needs to be compiled of course), but my own Travis account is probably useful until I figure out the build setup. @markus2330 Sure, I will make the tests fail with a warning if dbus is not available. |
Please succeed with warning. If test cases do not run through, people won't run them. |
waht
force-pushed
the
notification_dbus
branch
2 times, most recently
from
83c3d5c to
e1a0efe
Compare
|
The dbus plugin tests now only fail when no D-Bus connection (neither system nor session bus) was possible. On MacOs the tests use the session bus, on Linux the system bus. You were talking about having a warning (i.e. a printf) and skipping the further tests, right? My concern is that the warning would not be visible when running the tests with @markus2330 please review my pull request - I'll go hunting for the memory leak from #1824 :) |
cmake/Modules/LibAddPlugin.cmake
Outdated
| @@ -45,7 +45,7 @@ function (add_plugintest testname) | |||
| restore_variable (${PLUGIN_NAME} ARG_COMPILE_DEFINITIONS) | |||
| restore_variable (${PLUGIN_NAME} ARG_INCLUDE_DIRECTORIES) | |||
| restore_variable (${PLUGIN_NAME} ARG_INCLUDE_SYSTEM_DIRECTORIES) | |||
| restore_variable (${PLUGIN_NAME} ARG_LINK_ELEKTRA) | |||
| restore_variable (${PLUGIN_NAME} ARG_LINK_ELEKTRA ALLOW_MATCH) | |||
There was a problem hiding this comment.
@markus2330 the dbusrecv plugin required io_uv for testing: While the dbusrecv plugin relies on the ElektraIoInterface, to test the code a I/O binding was required.
I relaxed the consistency checks so that add_plugintest allows additional libraries. The plugin libraries still need to be included.
There was a problem hiding this comment.
Would be better if we can avoid circular deps.
If we really would need such a hack, we should introduce LINK_LIBRARIES_CIRCULAR or similar which adds the ALLOW_MATCH (the name ALLOW_MATCH is not really good, too?)
There was a problem hiding this comment.
Why would the deps be circluar?
It allows to add additional libraries for plugin tests.
If we really would need such a hack, we should introduce LINK_LIBRARIES_CIRCULAR or similar which adds the ALLOW_MATCH (the name ALLOW_MATCH is not really good, too?)
Okay. How about LINK_LIBRARIES_ADDITIONAL and "ALLOW_ADDITIONAL"?
There was a problem hiding this comment.
Ahh, I misunderstood what you are trying to achieve.
Yes, fresh names are a much better solution. What about UNITTEST_LINK_LIBRARIES? (ADDITIONAL is too generic, then it is unclear -- like it was for me -- that you only need it for the tests.)
|
jenkins build all please |
|
jenkins build gcc-configure-debian-musl please |
|
jenkins build multiconfig-gcc47-cmake-options please |
|
jenkins build gcc-configure-debian-musl please |
|
jenkins build all please |
|
Ok I realized that we have a similar issue (and curiously the same missing symbol In order to resolve my issue preloading the UbSan c++ library is required. The ASan library (as in #1783) does not work for me (the output says I changed my PR so that it is possible to preload multiple libraries. |
|
jenkins build haskell please |
1 similar comment
|
jenkins build haskell please |
|
jenkins build docker please |
|
Okay. I've improved the decision and moved the new functions to elektra-invoke. |
|
jenkins build all please |
|
Obviously the dbus tests fail without the daemon. Shall we wait until the build agents are updated or simply make the tests print a warning and exit normally? |
Yes, that is okay. I even wrote that 22 days ago, see above ("Please succeed with warning. If test cases do not run through, people won't run them."). |
|
Ah I see :) I will change the tests accordingly. |
|
jenkins build all please |
|
jenkins build all please |
|
I'll check the PRs asap. Is there any order I need to merge them? |
|
|
||
| Encapsulating plugins export a function called `deferredCall` with the | ||
| declaration | ||
| `void ElektraDeferredCall (Plugin * plugin, char * name, KeySet * parameters)`. |
| The called function receive their parameters via a KeySet. | ||
|
|
||
| While called functions could return data using the `parameters` KeySet (or a | ||
| seperate KeySet) there is no defined moment when the data can be collected. |
|
|
||
| Plugins that support deferred calls shall have the following declaration for | ||
| their functions | ||
| `void ElektraDeferredCallable (Plugin * plugin, KeySet * parameters)`. |
There was a problem hiding this comment.
Doesn't the absence of deferredCall already tell? What are the parameters for?
|
|
||
| ## Related decisions | ||
|
|
||
| - Elektra's invoke functionality will be extended to also allow use of deferred |
| It is recommended to use callbacks passed as `parameters`. | ||
| The callback function declaration is not affected by this decision. | ||
|
|
||
| ## Related decisions |
There was a problem hiding this comment.
Is this also already implemented? Why related decision?
| { | ||
| DBusConnection * connection = priv->connection; | ||
|
|
||
| // TODO currently not possible because dbus uses multiple watches for the same fd |
| @@ -19,6 +21,9 @@ int elektraListSet (Plugin * handle, KeySet * ks, Key * parentKey); | |||
| int elektraListError (Plugin * handle, KeySet * ks, Key * parentKey); | |||
| int elektraListAddPlugin (Plugin * handle, KeySet * pluginConfig); | |||
| int elektraListEditPlugin (Plugin * handle, KeySet * pluginConfig); | |||
| void elektraListSetIoBinding (Plugin * handle, ElektraIoInterface * binding); | |||
There was a problem hiding this comment.
I could not find the comment below anymore. Can you describe the deps somewhere explicitly?
|
Thank you, great work! I added some small comments to be fixed in a later PR. |
Purpose
This PR contains dbus transport plugins for the notification feature.
Checklist