Checkconf (2nd attempt) #733
Conversation
see ElektraInitiative#559 for details
|
Strange, that is an error I had fixed before. Did you rebase to the latest version? Or did the build server merge incorrectly? |
|
On my Fedora CMake fails with similar error messages. |
|
I rebased to the latest version. |
|
Something is still wrong: |
|
Ohh, sorry, I see. I fixed full and broke shared. (Above is not the build error I referred to when I said rebase might be wrong. It was about the xmltool symbol duplication.) |
|
I think it is fixed now. |
|
Yeah Fedora looks good too (compiling and unit tests passing). |
|
SUCCESS 🎉 |
|
Great! We will tomorrow have a look at it together! |
|
@Namoshek review please |
| catch (...) | ||
| { | ||
| throw; | ||
| } |
There was a problem hiding this comment.
Is it really necessary to catch and re-throw the same error? If you are not going to clean up something else in the catch-clause, it seems not necessary.
There was a problem hiding this comment.
@markus2330 I just copied from ModulesPluginDatabase::lookupInfo. Should I change it there too?
There was a problem hiding this comment.
If you copied it from somewhere, there might be an use case I didn't think of. I mean, it is quite obvious that you see this way a lot easier that there is an exception thrown by the invoked method and that it is escalated, but I guess the compiler should warn you about the exception anyway when using the getSymbol function without a try-catch clause.
Btw. it is quite hard to find anything useful regarding this construct, so I can't really tell if it is useful/good practice.
There was a problem hiding this comment.
@Namoshek I agree with you - it looks odd. The code was introduced in 10e15c4 by @markus2330 .
There was a problem hiding this comment.
Yes, I tried to fix #479 which is a real strange error: some exceptions thrown from libtool to kdb are not catched in kdb even though kdb contains a catch-all block. As this try/catch did not help to fix the problem you can safely remove it.
|
Thank you for the feedback, @Namoshek ! |
|
|
||
| - 0: config was not changed (was ok) | ||
| - 1: config is changed (now ok) | ||
| - -1: config not ok, could not fix it |
There was a problem hiding this comment.
The text was not supposed to be used 1:1, it is not even always a sentence ;)
It also does not establish that iff the function returns -1 an error has to be set.
Also please give an example.
There was a problem hiding this comment.
Also please give an example.
By "example" do you mean like a reference implementation of the function?
There was a problem hiding this comment.
Only something simple that would be useful in context of this tutorial. E.g. validating if the comment-character is only one character long or something like this..
@iandonnelly What do you think fits best?
| - 1: The configuration has been changed and now it is OK | ||
| - -1: The configuration was not OK and could not be fixed. An error has to be set to errorKey. | ||
|
|
||
| The following example demonstrates how to limit the length of the values within the plugin configuration to 3 charaters. |
There was a problem hiding this comment.
@markus2330 @Namoshek @iandonnelly is this a viable example?
the test case at testtool_backendbuilder.cpp:571 checkconfOkChanged is not OK. I suspect something is wrong in BackendBuilder::addPlugin
| checkConfPtr checkConfFunction = nullptr; | ||
| try | ||
| { | ||
| checkConfFunction = reinterpret_cast<checkConfPtr> (pluginDatabase->getSymbol (newPlugin, "checkconf")); |
There was a problem hiding this comment.
maybe we can return 0 here instead of exception?
| static int checkconfLookup_a_abc (ckdb::Key * errorKey, ckdb::KeySet * config) | ||
| { | ||
| ckdb::Key * k = ckdb::ksLookupByName (config, "/a", 0); | ||
| if (k && strcmp (ckdb::keyString (k), "abc") == 0) |
There was a problem hiding this comment.
Why having such comparision? The testcase could be even easier.
There was a problem hiding this comment.
We could use std::string for the comparison instead of strcmp().
There was a problem hiding this comment.
I mean: the mocks could simply append something and return 1, or return 0/-1; without any if.
Test removing Keys from the plugin configuration
|
My clang-format behaviour differs from the build server. I think you are using clang-format 3.8 (which is not available in the default Fedora repos) |
|
Yes, clang-format 3.8 is needed. You can download packages for many distributions at http://llvm.org/releases/download.html |
| ckdb::Key * k = ckdb::ksLookupByName (config, "user/b", 0); | ||
| if (!k) | ||
| { | ||
| ckdb::ksAppendKey (config, ckdb::keyNew ("user/b", KEY_VALUE, "test", KEY_END)); |
There was a problem hiding this comment.
AppendKey will overwrite the key if it exists. The name of the function suggests that it will always append.
|
Are both configurations supposed to be checked by checkconf? Do we pass these additional configurations as new paramter to checkconf? |
|
No, The backendConf should be in After calling |
|
(Could you please update this information as comment to |
|
Btw. I updated the docu how to pop keys from a keyset, see f41e01b Its sometimes quite confusing how to pop all or specific keys from a keyset. |
Done!
Like this?
Like iterating over the KeySet and checking with |
|
@markus2330 Did you forget to reformat |
|
Ohh, sorry. Is fixed in a614938 Please add test cases for changes to plugin+backend config. Separation works with ksCut("system"/"user"). |
|
Last question: does it make sense that |
|
Yes, it makes sense to have checkconf calls in BackendBuilder::addPlugin to support checkconf for the global plugins and export/import backendBuilder (yet to come), e.g. because crypto might be used in both of these scenarios. So maybe the backendConf should be moved up to BackendBuilder, a Btw. there is also SpecReader (kdb mount-spec), but it sits on top of MountBackendBuilder, so its irrelevant for this discussion. |
|
@Namoshek @markus2330 final review please! |
|
Finally its merged, great job! |
TODO
See #559 and #725 .