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

Opts plugin #2471

Merged
merged 41 commits into from Apr 2, 2019

Conversation

Projects
None yet
4 participants
@kodebach
Copy link
Contributor

commented Mar 9, 2019

Closes #2427

Basics

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins)

Review

@markus2330 please review my pull request

{
int argc;

LPWSTR * args = CommandLineToArgvW (GetCommandLineW (), &argc);

This comment has been minimized.

Copy link
@kodebach

kodebach Mar 9, 2019

Author Contributor

This implementation is a bit weird and I'm not entirely sure it will work if any non ASCII characters are passed as command-line arguments, but it is the best I could do.

The only alternative is using GetCommandLine (will use ASCII mode when UNICODE is not defined), but because there is no non-Unicode CommandLineToArgv, we would have to parse the string ourselves. Because ASCII mode in Windows isn't necessarily ASCII, but rather any 8-bit character encoding, that could be even more difficult than it already is (quotes, escaping, etc.)

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 9, 2019

Contributor

Should be okay for now.

@markus2330
Copy link
Contributor

left a comment

Looks nice!

@@ -200,8 +200,25 @@
#cmakedefine HAVE_HSEARCHR
#endif

/* define if the gopts plugin shall use the procfs implementation */

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 9, 2019

Contributor

Better define this only locally for your plugin.

*
*/

#define TESTAPP_PATH "@TESTAPP_PATH@"

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 9, 2019

Contributor

(here)

#elif defined(ELEKTRA_GOPTS_SYSCTL)
#include "gopts_sysctl.h"
#else
// define anyway to get rid of IDE warnings

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 9, 2019

Contributor

define it before (as prototype)?

{
int argc;

LPWSTR * args = CommandLineToArgvW (GetCommandLineW (), &argc);

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 9, 2019

Contributor

Should be okay for now.

@kodebach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

The more I work on this, the more I realize, this plugin won't work the way we intended it to. At least not without some major modifications to the core of Elektra.

  1. The plugin really only makes sense, if it is executed between getstorage and postgetstorage. Otherwise we bypass some or all of the validation that is done in postgetstorage.
  2. The plugin doesn't actually make sense as a global plugin. It has no need to see the whole KDB. In fact, in an ideal world, it would only see the specification of the application currently calling kdbGet. This isn't possible (maybe it is? see idea below) so the next best thing is, mounting the plugin automatically for every mountpoint. But because the number of plugins foreach mountpoint is very limited right now, this isn't a good solution either.
  3. We already knew that some applications (like kdb) will need to disable the plugin, because they access parts of the KDB that might contain other specifications. This again requires modifications of the core of Elektra.

All of these modifications to the core might be possible, but considering that there are some major changes to the plugin framework planned and most of the changes required for this plugin might interfere with these, I don't think now is the right time to implement this plugin.


What might actually solve all these problems, is the specload plugin. Because it is mounted as getstorage all the validation will be done as expected. The specload plugin knows which application the specification belongs to (given by the app config key). It could compare the current executables path (can be obtained by methods similar to the ones gopts uses) and if they match it invokes gopts.

The only problem is, I don't know how the plugin framework handles, if a plugin returns Keys outside the given parentKey.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

The plugin really only makes sense, if it is executed between getstorage and postgetstorage. Otherwise we bypass some or all of the validation that is done in postgetstorage.

We can add further places. Or we could use infos/ordering to make the validation earlier.

It has no need to see the whole KDB. In fact, in an ideal world, it would only see the specification of the application currently calling kdbGet

Actually, you can mount plugins to spec (only). Spec is excluded from cascading mountpoints on purpose.

We already knew that some applications (like kdb) will need to disable the plugin, because they access parts of the KDB that might contain other specifications. This again requires modifications of the core of Elektra.

I think such applications can also simply throw the content from proc/.

The only problem is, I don't know how the plugin framework handles, if a plugin returns Keys outside the given parentKey.

I think the framework currently throws the keys away. But I also think that it somehow makes sense to have proc/ as exception as it is no ordinary namespaces (no plugin is responsible for it).

@kodebach kodebach force-pushed the kodebach:opts_plugin branch from 3007097 to 49567b0 Mar 21, 2019

@kodebach kodebach force-pushed the kodebach:opts_plugin branch from 0f6eaf3 to dfb41d6 Mar 22, 2019

@kodebach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

@markus2330 The plugin is now added as a global plugin by default (like spec). However, I disabled it by default for now. I added a very crude function to enable it for testing.

My next step will be adding kdbEnsure. However, to do that I need to know, what functionality it should support and how generic the contract should be? Should it only support mounting/unmountig global plugins or should it also replace the notification library? (Which AFAIK is only there to setup the internalnotification plugin)

If kdbEnsure should be able to do more than mounting/unmounting, I think we need some interface to the plugins, so the plugin specific code stays isolated...

@kodebach kodebach referenced this pull request Mar 28, 2019

Open

proposal: kdbEnsure #868

@kodebach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

jenkins build all please

@kodebach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

I think this is done now...

@markus2330 please review

@markus2330 markus2330 requested review from sanssecours and mpranj Mar 29, 2019

@mpranj

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

Why is the jenkins build green/OK when there is one terminated/failed build?

@kodebach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

Why is the jenkins build green/OK when there is one terminated/failed build?

I guess only the Jenkins devs could answer that one ....

@kodebach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

jenkins build libelektra please

@mpranj

mpranj approved these changes Mar 30, 2019

Copy link
Member

left a comment

Thank you for this PR! Overall some very nice improvements and cleanups to kdb, list, and internalnotification.
The code is nice and clean. I only reviewed the changes but won't be able to test it on my machine until late this evening.

contract. In principle this a very powerful tool that may be used for a lot of things. For now it only supports a few conditions concerning
plugins:

- You can specify that a plugin should be mounted globally. This is can for example be used to enable the new [gopts](#gopts) plugin.

This comment has been minimized.

Copy link
@mpranj

mpranj Mar 30, 2019

Member

"This is can for example" -> "This can for example"

@@ -171,7 +198,7 @@ compiled against an older 0.8 version of Elektra will continue to work

### Core

- <<TODO>>
- `kdbGet` now calls global postgetstorage plugins with the parent key passed to `kdbGet`, instead of a random mountpoint. _(Klemens Böswirth)_

This comment has been minimized.

Copy link
@mpranj

This comment has been minimized.

Copy link
@kodebach

kodebach Mar 30, 2019

Author Contributor

One thing I noticed (but didn't look into): At least some errors returned from global plugins are suppressed by kdbGet. You may want to look into that while working on #2457.

This comment has been minimized.

Copy link
@mpranj

mpranj Mar 30, 2019

Member

Yep, we didn't even use return values until #2307. The global plugin implementation needs to be re-worked and tested properly, so that it matches the proposal.

@markus2330
Copy link
Contributor

left a comment

Great work. A few change requests.

### <<HIGHLIGHT2>>
### kdbEnsure

`kdbEnsure` is a new function in `elektra-kdb`. It can be used to ensure that a KDB instance meets certain conditions specified in a

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 30, 2019

Contributor

conditions -> clauses (2x)

- Conversely you can also define that a plugin should not be mounted globally, e.g. to disable the `spec` plugin, which is enabled by default.
- Additionally you may want to enforce that a global plugin uses a certain configuration. For this case you can specify that the plugin
should be remounted, i.e. unmounted and immediately mounted again.
- In future non-global plugins will support the same features. But because of the different architecture involved, for now only unmounting

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 30, 2019

Contributor

No need to mention it. We might or might not add it.

@@ -60,10 +60,30 @@ class KDBException : public Exception
return m_str.c_str ();
}

private:
protected:
Key m_key;
mutable std::string m_str;

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 30, 2019

Contributor

Could be private?

ksDel (pluginConfig);
return ensurePluginUnmounted (handle, mountpoint, pluginName, errorKey);
case PLUGIN_STATE_MOUNTED:
return 0; // TODO: ensurePluginMounted (handle, mountpoint, pluginName, pluginConfig, errorKey);

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 30, 2019

Contributor

Also assert here that this is not called? (2x)

* If `<mountpoint>` is NOT `global`, currently only `unmounted` is supported (not `mounted` and `remounted`).
*
* NOTE: This function only works properly, if the list plugin is mounted in all global positions.
* If this is not the case, 1 will be returned, because this is seen as an implicit condition in the contract.

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 30, 2019

Contributor

Better to fail if you cannot do it. The application has no control over if the list plugin is there or not. And it expects that kdbEnsure actually ensured its contract if it returned successfully.

This comment has been minimized.

Copy link
@kodebach

kodebach Mar 31, 2019

Author Contributor

The return value 1 indicates a failure. Only 0 means success.

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 1, 2019

Contributor

This is non-standard compared to other kdb calls. Can you change that to be -1?

This comment has been minimized.

Copy link
@kodebach

kodebach Apr 1, 2019

Author Contributor

I could change it to -2, if you want all negative return values + 0 for success...

1 is returned, if the contract could not ensured, but the kdbEnsure call was entirely valid. e.g. if a plugin could not be mounted

-1 meanwhile is used to indicate that either the contract is malformed, or something else about the kdbEnsure call is wrong.

If 1 is returned, the application should show a meaningful message to the user, because they might need to change their KDB configuration. If -1 is returned, the application probably has a bug, and without changing the code there is little to no chance of fixing the problem...

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 2, 2019

Contributor

I could change it to -2, if you want all negative return values + 0 for success...

I do not think we need this distinction in the return value as you can simply set different errors in the errorKey.

Furthermore, I think it might be useful to indicate if something was done at all.
So I would propose following return codes:

  • 1: could not run kdbEnsure successfully (error is in the errorKey)
    0: kdbEnsure successful but nothing was to do
    1: mounted or unmounted something, so the application now has different mountpoints than the rest of the system

This would be basically identical to return codes of the other kdb* calls (and also the resolver).

Iirc there are now also macros defined for these values.

* This function can be used the given KDB @p handle meets certain conditions,
* specified in @p contract. Currently the following conditions are supported:
*
* - `system/plugins/<mountpoint>/<pluginname>` defines the state of the plugin

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 30, 2019

Contributor

What about the name system/elektra/ensure/plugins/...? system/plugins is not reserved and it will create problems once these keys should be read from KDB.

This comment has been minimized.

Copy link
@kodebach

kodebach Mar 31, 2019

Author Contributor

I can change it, but I don't see a reason, why these keys should be stored in the KDB. They are not configuration, they are static specification. IMO the use case for kdbEnsure is that an application wants to make sure the KDB is in a state that is usable to the application. Changing this contract should only be necessary, if the code changes, i.e. recompiling is necessary.

That is also the reason, why I decided it is okay to consume the contract. If the keys came from the KDB this would be rather inconvenient.

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 1, 2019

Contributor

I can change it, but I don't see a reason, why these keys should be stored in the KDB.

It is not about storing them there but it might be useful to be able to query the contracts of applications (with a functionality similar to the specload).

*
* @param handle contains internal information of @link kdbOpen() opened @endlink key database
* @param contract KeySet containing the contract described above.
* Because of the implementation, this will always be `ksDel()`ed. <b>Even in error cases.</b>

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 30, 2019

Contributor

" Because of the implementation" is redundant information. Maybe you wanted to say: "To allow kdbEnsure(.., ksNew(...)..."

This comment has been minimized.

Copy link
@kodebach

kodebach Mar 31, 2019

Author Contributor

Maybe you wanted to say: "To allow kdbEnsure(.., ksNew(...)..."

That is a nice side effect, but in most cases won't make for good, readable code. The actual reason is that I use ksCut and wanted to avoid having to ksDup first, because the contract shouldn't be needed after calling kdbEnsure anyway.

@@ -20,7 +20,7 @@ if (BUILD_SHARED)
add_executable (kdb $<TARGET_OBJECTS:kdb-objects>)
add_dependencies (kdb kdberrors_generated elektra_error_codes_generated)

target_link_libraries (kdb elektra-core elektra-kdb elektratools)
target_link_libraries (kdb elektra-core elektra-kdb elektratools elektra-proposal)

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 30, 2019

Contributor

This is ugly and should be avoided. Which methods from proposal do you need?

This comment has been minimized.

Copy link
@kodebach

kodebach Mar 31, 2019

Author Contributor

I think, this is actually just an artifact from some earlier tests, because I didn't actually change the kdb tool. I just used it for testing.

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 2, 2019

Contributor

Ok, perfect, then you can simply change it back.


## Usage

The preferred way of using this plugin is via `kdbEnsure`:

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 30, 2019

Contributor

Isn't global mounting possible, too?

This comment has been minimized.

Copy link
@kodebach

kodebach Mar 31, 2019

Author Contributor

Technically yes, but the kdb tool doesn't call kdbEnsure yet (to make sure gopts is unmounted, when accessing third-party configuration), so kdb get etc may break in certain cases.

gopts is unmounted by default for now. My plan was to make it mounted by default in future and just unmount it in kdb. That is however quite tedious work, because each command uses its own KDB instance, not created through some shared means.

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 1, 2019

Contributor

What about subclassing KDB (where gopts is ensured to be not there) and replace all KDB with the subclass? (Should be a simply search&replace.)

This comment has been minimized.

Copy link
@kodebach

kodebach Apr 1, 2019

Author Contributor

I think it would be better to create KDBCommand, an abstract subclass of Command that provides a KDB instance to its subclasses. I think this is a nicer solution, because later on KDB should use gopts for its own config and only the commands accessing the rest of the KDB should use the version without gopts.

I will modify kdb in a follow-up PR and then we might mount gopts by default.

The preferred way of using this plugin is via `kdbEnsure`:

```c
KDB * kdb = kdbOpen (parentKey);

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 30, 2019

Contributor

It would be nice if this code is also as stand-alone minimal-working example available.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

Btw. did you also try if kdbEnsure is available in the non-C++ bindings? (E.g. Python) It would be great if cmd-line parsing would also be available from within Python.

@sanssecours
Copy link
Member

left a comment

The PR looks good as far as I can tell. I am not familiar with the updated parts of Elektra at all though, so I am not really the right person to review this pull request. I also only skimmed over the code, since a proper review would probably require multiple hours of work.

int KDB::ensure (const KeySet & contract, Key & parentKey)
{
// have to ksDup because contract is consumed and ksDel()ed by kdbEnsure
int ret = ckdb::kdbEnsure (handle, ckdb::ksDup (contract.getKeySet ()), parentKey.getKey ());

This comment has been minimized.

Copy link
@sanssecours

sanssecours Mar 31, 2019

Member

I think you can replace

ckdb::ksDup (contract.getKeySet ())

with the shorter

contract.dup ()

here.

This comment has been minimized.

Copy link
@kodebach

kodebach Mar 31, 2019

Author Contributor

No that won't work, because that will cause a double free, because the C++ class KeySet will call ksDel() in the destructor.

*
* @param handle contains internal information of @link kdbOpen() opened @endlink key database
* @param contract KeySet containing the contract described above.
* Because of the implementation, this will always be `ksDel()`ed. <b>Even in error cases.</b>

This comment has been minimized.

Copy link
@sanssecours

sanssecours Mar 31, 2019

Member

Since Doxygen also supports Markdown syntax, I think it might make sense to use

**Even in error cases.**

instead of

<b>Even in error cases.</b>

.

Plugin * elektraListFindPlugin (Plugin * handle, const char * pluginName)
```
`elektraListMountPlugin` can be used to add a new plugin to the config. The placement will be query from the plugin it self (from its

This comment has been minimized.

Copy link
@sanssecours

sanssecours Mar 31, 2019

Member

Did you maybe mean “queried” instead of “query”?

This comment has been minimized.

Copy link
@sanssecours

sanssecours Mar 31, 2019

Member

As far as I know, “it self” should be written as one word in this context.

{
if (strcmp (name, TEST_EMPTY) == 0)
{
*parentKey = keyNew ("spec/tests/gopts", KEY_END);

This comment has been minimized.

Copy link
@sanssecours

sanssecours Mar 31, 2019

Member

Maybe it would make sense to create the parent key only once. As far as I can tell, each of the if-statements creates exactly the same key.

@@ -48,6 +48,23 @@ A list of set-placements for the plugin. Same for "get" and "error"

Plugin specific config.

## Exported functions

This comment has been minimized.

Copy link
@sanssecours

sanssecours Mar 31, 2019

Member

Please use title case for all headings.

This comment has been minimized.

Copy link
@markus2330

markus2330 Mar 31, 2019

Contributor

I think this needs to be added to doc/CODING.md?

@kodebach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

Btw. did you also try if kdbEnsure is available in the non-C++ bindings?

It is available in Python (tested) and should be available in any binding that is auto-generated.

It would be great if cmd-line parsing would also be available from within Python.

It is technically possible right now, but because argv[0] is the python executable the remaining args will also contain the python script file, and the help message will contain the wrong program name. I have a solution for that, but I will put it into a follow-up PR. Together with a full Python example.

@markus2330
Copy link
Contributor

left a comment

Looks very good now! Two small comments.

KS_END);
}

static int setupSpec (void)

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 1, 2019

Contributor

Please mention here that this is only like this to be self-contained, not that an application should do this.

@@ -102,7 +102,7 @@ kdb ls user/tests/multifile
kdb rm -r user/tests/multifile/test.ini
stat ~/.config/multifile/test.ini
stat $(dirname $(kdb file user))/multifile/test.ini

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 1, 2019

Contributor

We also should add build jobs which use non-standard config paths (ideally also with some umlauts, white spaces, ...)

This comment has been minimized.

Copy link
@kodebach

kodebach Apr 1, 2019

Author Contributor

AFAIK most if not all build jobs (certainly the Jenkins ones) use non-standard config paths inside the workspace directory. I also run all local tests with a similar setup.

About umlauts/white-space I am not sure, we might want to create an issue

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 1, 2019

Contributor

So the multifile test cases are not running on our build server?

This comment has been minimized.

Copy link
@kodebach

kodebach Apr 1, 2019

Author Contributor

I just checked and at least the debian-stable-full build runs this test successfully... Maybe some of the changes to kdbGet I made, brought up a hidden bug which was suppressed before

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 1, 2019

Contributor

Thank you for looking into it!

kodebach added some commits Apr 1, 2019

@kodebach

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@markus2330 I think we can merge this, if not, please tell me what I missed

@markus2330 markus2330 merged commit 529a9df into ElektraInitiative:master Apr 2, 2019

5 checks passed

LGTM analysis: C/C++ No new or fixed alerts
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@markus2330

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

Thank you so much for this very nice PR! It is great that not only LCDproc will from now have command-line arguments.

@ulschaef hopefully will also soon have finished the command-line argument completion, which would make our solution complete 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.