From cf3b3b1ca12f85752252ae7b5f316ff60a03ed08 Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Mon, 16 May 2016 22:26:03 +0200 Subject: [PATCH 01/23] add checkconf interface for plugins see #559 for details --- src/libs/tools/include/plugindatabase.hpp | 15 ++++++++++++ src/libs/tools/include/toolexcept.hpp | 29 +++++++++++++++++++++++ src/libs/tools/src/backendbuilder.cpp | 29 +++++++++++++++++++++++ src/libs/tools/src/plugindatabase.cpp | 22 +++++++++++++++++ 4 files changed, 95 insertions(+) diff --git a/src/libs/tools/include/plugindatabase.hpp b/src/libs/tools/include/plugindatabase.hpp index f80657a10a6..b8c19028222 100644 --- a/src/libs/tools/include/plugindatabase.hpp +++ b/src/libs/tools/include/plugindatabase.hpp @@ -30,6 +30,9 @@ namespace tools */ class PluginDatabase { +protected: + typedef void (*func_t) (); + public: /** * @brief list all plugins @@ -68,6 +71,16 @@ class PluginDatabase */ virtual std::string lookupInfo (PluginSpec const & whichplugin, std::string const & which) const = 0; + /** + * @brief get exported plugin symbol + * + * @param whichplugin from which plugin? + * @param which which symbol would you like to look up? + * + * @return the function pointer to the exported symbol + */ + virtual func_t getSymbol (PluginSpec const & whichplugin, std::string const & which) const = 0; + /** * @brief lookup which plugin handles meta data * @@ -118,6 +131,7 @@ class ModulesPluginDatabase : public PluginDatabase std::vector listAllPlugins () const; PluginDatabase::Status status (PluginSpec const & whichplugin) const; std::string lookupInfo (PluginSpec const & spec, std::string const & which) const; + func_t getSymbol (PluginSpec const & whichplugin, std::string const & which) const; PluginSpec lookupMetadata (std::string const & which) const; PluginSpec lookupProvides (std::string const & provides) const; }; @@ -135,6 +149,7 @@ class MockPluginDatabase : public ModulesPluginDatabase std::vector listAllPlugins () const; PluginDatabase::Status status (PluginSpec const & whichplugin) const; std::string lookupInfo (PluginSpec const & spec, std::string const & which) const; + func_t getSymbol (PluginSpec const & whichplugin, std::string const & which) const; }; } } diff --git a/src/libs/tools/include/toolexcept.hpp b/src/libs/tools/include/toolexcept.hpp index 8c167883eed..b69bf6f6ae0 100644 --- a/src/libs/tools/include/toolexcept.hpp +++ b/src/libs/tools/include/toolexcept.hpp @@ -157,6 +157,35 @@ struct PluginAlreadyInserted : public PluginCheckException std::string m_str; }; +struct PluginConfigInvalid : public PluginCheckException +{ + explicit PluginConfigInvalid (Key key) : m_key (key), m_str () + { + } + + explicit PluginConfigInvalid (std::string const & message) : m_str (message) + { + } + + virtual const char * what () const throw () override + { + if (m_str.empty ()) + { + std::stringstream ss; + ss << "The provided plugin configuration is not valid!\n"; + ss << "Errors/Warnings during the check were:\n"; + printError (ss, m_key); + printWarnings (ss, m_key); + m_str = ss.str (); + } + return m_str.c_str (); + } + +private: + Key m_key; + mutable std::string m_str; +}; + struct BadPluginName : public PluginCheckException { explicit BadPluginName (std::string name) diff --git a/src/libs/tools/src/backendbuilder.cpp b/src/libs/tools/src/backendbuilder.cpp index 2837f762830..1ea6d7009d9 100644 --- a/src/libs/tools/src/backendbuilder.cpp +++ b/src/libs/tools/src/backendbuilder.cpp @@ -384,12 +384,18 @@ void BackendBuilder::recommendPlugin (std::string name) * @pre Needs to be a unique new name (use refname if you want to add the same module multiple times) * * Will automatically resolve virtual plugins to actual plugins. + * + * Also calls the checkconf function if provided by the plugin. The checkconf function has the + * following signature: int checkconf (Key * errorKey, KeySet * config) and allows a plugin to + * verify its configuration at mount time. * * @see resolveNeeds() * @param plugin */ void BackendBuilder::addPlugin (PluginSpec const & plugin) { + typedef int (*checkConfPtr) (ckdb::Key *, ckdb::KeySet *); + for (auto & p : toAdd) { if (p.getFullName () == plugin.getFullName ()) @@ -409,6 +415,29 @@ void BackendBuilder::addPlugin (PluginSpec const & plugin) newPlugin.appendConfig (provides.getConfig ()); } + // call plugin's checkconf function (if provided) + // this enables a plugin to verify its configuration at mount time + checkConfPtr checkConfFunction = nullptr; + try + { + checkConfFunction = reinterpret_cast (pluginDatabase->getSymbol (newPlugin, "checkconf")); + } + catch (...) + { + // the checkconf operation is optional, so no worries if it was not found + } + + if (checkConfFunction != nullptr) + { + ckdb::Key * errorKey = ckdb::keyNew (0); + int checkResult = checkConfFunction (errorKey, newPlugin.getConfig ().getKeySet ()); + if (checkResult != 0) + { + throw PluginConfigInvalid (errorKey); + } + ckdb::keyDel (errorKey); + } + toAdd.push_back (newPlugin); sort (); } diff --git a/src/libs/tools/src/plugindatabase.cpp b/src/libs/tools/src/plugindatabase.cpp index f17b5fb1d2f..daa3f8a4871 100644 --- a/src/libs/tools/src/plugindatabase.cpp +++ b/src/libs/tools/src/plugindatabase.cpp @@ -223,6 +223,21 @@ std::string ModulesPluginDatabase::lookupInfo (PluginSpec const & spec, std::str return plugin->lookupInfo (which); } +PluginDatabase::func_t ModulesPluginDatabase::getSymbol (PluginSpec const & spec, std::string const & which) const +{ + PluginPtr plugin; + try + { + plugin = impl->modules.load (spec.getName (), spec.getConfig ()); + } + catch (...) + { + throw; + } + + return plugin->getSymbol (which); +} + PluginSpec ModulesPluginDatabase::lookupMetadata (std::string const & which) const { std::vector allPlugins = listAllPlugins (); @@ -367,5 +382,12 @@ std::string MockPluginDatabase::lookupInfo (PluginSpec const & spec, std::string return ""; } + +PluginDatabase::func_t MockPluginDatabase::getSymbol (PluginSpec const & spec ELEKTRA_UNUSED, + std::string const & which ELEKTRA_UNUSED) const +{ + // TODO implement mockup + return NULL; +} } } From 5858ed65df506459f53793ac80b594950678452a Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Tue, 17 May 2016 14:04:44 +0200 Subject: [PATCH 02/23] add checkconf example to template plugin --- src/libs/tools/src/backendbuilder.cpp | 2 +- src/plugins/template/template.c | 10 ++++++++++ src/plugins/template/template.h | 1 + 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/libs/tools/src/backendbuilder.cpp b/src/libs/tools/src/backendbuilder.cpp index 1ea6d7009d9..4dbdd478f41 100644 --- a/src/libs/tools/src/backendbuilder.cpp +++ b/src/libs/tools/src/backendbuilder.cpp @@ -431,7 +431,7 @@ void BackendBuilder::addPlugin (PluginSpec const & plugin) { ckdb::Key * errorKey = ckdb::keyNew (0); int checkResult = checkConfFunction (errorKey, newPlugin.getConfig ().getKeySet ()); - if (checkResult != 0) + if (checkResult != 1) { throw PluginConfigInvalid (errorKey); } diff --git a/src/plugins/template/template.c b/src/plugins/template/template.c index 0d1758673ed..9b732cedf93 100644 --- a/src/plugins/template/template.c +++ b/src/plugins/template/template.c @@ -38,6 +38,7 @@ int elektraTemplateGet (Plugin * handle ELEKTRA_UNUSED, KeySet * returned ELEKTR keyNew ("system/elektra/modules/template/exports/get", KEY_FUNC, elektraTemplateGet, KEY_END), keyNew ("system/elektra/modules/template/exports/set", KEY_FUNC, elektraTemplateSet, KEY_END), keyNew ("system/elektra/modules/template/exports/error", KEY_FUNC, elektraTemplateError, KEY_END), + keyNew ("system/elektra/modules/template/exports/checkconf", KEY_FUNC, elektraTemplateCheckConfig, KEY_END), #include ELEKTRA_README (template) keyNew ("system/elektra/modules/template/infos/version", KEY_VALUE, PLUGINVERSION, KEY_END), KS_END); ksAppend (returned, contract); @@ -64,6 +65,15 @@ int elektraTemplateError (Plugin * handle ELEKTRA_UNUSED, KeySet * returned ELEK return 1; // success } +int elektraTemplateCheckConfig (Key * errorKey, KeySet * conf) +{ + // validate plugin configuration + // this function is optional + // if the export symbol "exports/checkconf" is provided, the function will be called during the mount phase + + return 1; // success +} + Plugin * ELEKTRA_PLUGIN_EXPORT (template) { // clang-format off diff --git a/src/plugins/template/template.h b/src/plugins/template/template.h index e3c16d5f3ac..82d83fdb8e4 100644 --- a/src/plugins/template/template.h +++ b/src/plugins/template/template.h @@ -18,6 +18,7 @@ int elektraTemplateClose (Plugin * handle, Key * errorKey); int elektraTemplateGet (Plugin * handle, KeySet * ks, Key * parentKey); int elektraTemplateSet (Plugin * handle, KeySet * ks, Key * parentKey); int elektraTemplateError (Plugin * handle, KeySet * ks, Key * parentKey); +int elektraTemplateCheckConfig (Key * errorKey, KeySet * conf); Plugin * ELEKTRA_PLUGIN_EXPORT (template); From 7fd641684404d57a41992cf108331f68e74608fd Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Tue, 17 May 2016 14:27:22 +0200 Subject: [PATCH 03/23] mark optional functions in template plugin --- src/plugins/template/template.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/plugins/template/template.c b/src/plugins/template/template.c index 9b732cedf93..f1984b5f4f8 100644 --- a/src/plugins/template/template.c +++ b/src/plugins/template/template.c @@ -15,6 +15,7 @@ int elektraTemplateOpen (Plugin * handle ELEKTRA_UNUSED, Key * errorKey ELEKTRA_UNUSED) { // plugin initialization logic + // this function is optional return 1; // success } @@ -22,6 +23,7 @@ int elektraTemplateOpen (Plugin * handle ELEKTRA_UNUSED, Key * errorKey ELEKTRA_ int elektraTemplateClose (Plugin * handle ELEKTRA_UNUSED, Key * errorKey ELEKTRA_UNUSED) { // free all plugin resources and shut it down + // this function is optional return 1; // success } @@ -54,6 +56,7 @@ int elektraTemplateGet (Plugin * handle ELEKTRA_UNUSED, KeySet * returned ELEKTR int elektraTemplateSet (Plugin * handle ELEKTRA_UNUSED, KeySet * returned ELEKTRA_UNUSED, Key * parentKey ELEKTRA_UNUSED) { // get all keys + // this function is optional return 1; // success } @@ -61,6 +64,7 @@ int elektraTemplateSet (Plugin * handle ELEKTRA_UNUSED, KeySet * returned ELEKTR int elektraTemplateError (Plugin * handle ELEKTRA_UNUSED, KeySet * returned ELEKTRA_UNUSED, Key * parentKey ELEKTRA_UNUSED) { // set all keys + // this function is optional return 1; // success } @@ -69,7 +73,6 @@ int elektraTemplateCheckConfig (Key * errorKey, KeySet * conf) { // validate plugin configuration // this function is optional - // if the export symbol "exports/checkconf" is provided, the function will be called during the mount phase return 1; // success } From 93e607c49be49cffdfc70eea04e87d276883f172 Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Tue, 17 May 2016 14:39:33 +0200 Subject: [PATCH 04/23] documentation of checkconf --- doc/tutorials/plugins.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/doc/tutorials/plugins.md b/doc/tutorials/plugins.md index 64567c02ef5..eb4634252f5 100644 --- a/doc/tutorials/plugins.md +++ b/doc/tutorials/plugins.md @@ -23,6 +23,7 @@ Additionally, there is one more function called [ELEKTRA_PLUGIN_EXPORT](http://doc.libelektra.org/api/current/html/group__plugin.html#gabe78724d2d477eef39997fd9b85bff16), where once again `Plugin` should be replaced with the name of the plug-in, this time in lower-case. So for my line plugin this function would be `ELEKTRA_PLUGIN_EXPORT(line)`. +The developer may define elektraPluginCheckConf() if configuration validation at mount time is desired. The KDB relies on the first five functions for interacting with configuration files stored in the key database. Calls for kdbGet() and kdbClose() will call the functions elektraPluginGet() and elektraPluginClose() respectively for the @@ -322,6 +323,14 @@ The `elektraPluginOpen` and `elektraPluginClose` functions are not commonly used reviewing. `elektraPluginOpen` function runs before `elektraPluginGet` and is useful to do initialization if necessary for the plug-in. On the other hand `elektraPluginClose` is run after other functions of the plug-in and can be useful for freeing up resources. +### elektraPluginCheckConf ### + +The `elektraPluginCheckConf` function may be used for validation of the plugin configuration during mount time. The signature of the function is: + + int elektraLineCheckConfig (Key * errorKey, KeySet * conf) + +The configuration of the plugin is provided as `conf`. The function may report errors using the `errorKey` and the return value. + ### ELEKTRA_PLUGIN_EXPORT ### The last function, one that is always needed in a plug-in, is `ELEKTRA_PLUGIN_EXPORT`. This functions is responsible for letting Elektra know that From 034d202759a2fa78bb5747c67717fa30d0a4be64 Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Tue, 17 May 2016 14:39:47 +0200 Subject: [PATCH 05/23] fix compile warning --- src/plugins/template/template.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/template/template.c b/src/plugins/template/template.c index f1984b5f4f8..4b5cfb0c295 100644 --- a/src/plugins/template/template.c +++ b/src/plugins/template/template.c @@ -69,7 +69,7 @@ int elektraTemplateError (Plugin * handle ELEKTRA_UNUSED, KeySet * returned ELEK return 1; // success } -int elektraTemplateCheckConfig (Key * errorKey, KeySet * conf) +int elektraTemplateCheckConfig (Key * errorKey ELEKTRA_UNUSED, KeySet * conf ELEKTRA_UNUSED) { // validate plugin configuration // this function is optional From c91f567d099b2ea8da606908100385e96c731af6 Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Wed, 18 May 2016 08:38:04 +0200 Subject: [PATCH 06/23] establish return value convention for checkconf --- doc/tutorials/plugins.md | 6 ++++++ src/libs/tools/src/backendbuilder.cpp | 2 +- src/plugins/template/template.c | 6 +++++- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/doc/tutorials/plugins.md b/doc/tutorials/plugins.md index eb4634252f5..4b9e9f5db9c 100644 --- a/doc/tutorials/plugins.md +++ b/doc/tutorials/plugins.md @@ -331,6 +331,12 @@ The `elektraPluginCheckConf` function may be used for validation of the plugin c The configuration of the plugin is provided as `conf`. The function may report errors using the `errorKey` and the return value. +The following convention was established for the return value of `elektraPluginCheckConf`: + +- 0: config was not changed (was ok) +- 1: config is changed (now ok) +- -1: config not ok, could not fix it + ### ELEKTRA_PLUGIN_EXPORT ### The last function, one that is always needed in a plug-in, is `ELEKTRA_PLUGIN_EXPORT`. This functions is responsible for letting Elektra know that diff --git a/src/libs/tools/src/backendbuilder.cpp b/src/libs/tools/src/backendbuilder.cpp index 4dbdd478f41..cb61f95108f 100644 --- a/src/libs/tools/src/backendbuilder.cpp +++ b/src/libs/tools/src/backendbuilder.cpp @@ -431,7 +431,7 @@ void BackendBuilder::addPlugin (PluginSpec const & plugin) { ckdb::Key * errorKey = ckdb::keyNew (0); int checkResult = checkConfFunction (errorKey, newPlugin.getConfig ().getKeySet ()); - if (checkResult != 1) + if (checkResult == -1) { throw PluginConfigInvalid (errorKey); } diff --git a/src/plugins/template/template.c b/src/plugins/template/template.c index 4b5cfb0c295..231dd22ae7c 100644 --- a/src/plugins/template/template.c +++ b/src/plugins/template/template.c @@ -74,7 +74,11 @@ int elektraTemplateCheckConfig (Key * errorKey ELEKTRA_UNUSED, KeySet * conf ELE // validate plugin configuration // this function is optional - return 1; // success + // the return codes have the following meaning: + // 0: config was not changed (was ok) + // 1: config is changed (now ok) + // -1: config not ok, could not fix + return 0; } Plugin * ELEKTRA_PLUGIN_EXPORT (template) From c18cb8e7a0f7be9ce455ad118a1004b666b88c1d Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Wed, 18 May 2016 09:05:54 +0200 Subject: [PATCH 07/23] documentation of checkconf --- src/plugins/doc/doc.c | 14 ++++++++++++++ src/plugins/doc/doc.h | 22 ++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/plugins/doc/doc.c b/src/plugins/doc/doc.c index 7402086f585..9f5112934c9 100644 --- a/src/plugins/doc/doc.c +++ b/src/plugins/doc/doc.c @@ -107,6 +107,7 @@ int elektraDocGet (Plugin * plugin ELEKTRA_UNUSED, KeySet * returned, Key * pare keyNew ("system/elektra/modules/doc/exports/get", KEY_FUNC, elektraDocGet, KEY_END), keyNew ("system/elektra/modules/doc/exports/set", KEY_FUNC, elektraDocSet, KEY_END), keyNew ("system/elektra/modules/doc/exports/error", KEY_FUNC, elektraDocError, KEY_END), + keyNew ("system/elektra/modules/doc/exports/checkconf", KEY_FUNC, elektraDocCheckConf, KEY_END), #include ELEKTRA_README (doc) keyNew ("system/elektra/modules/doc/infos/version", KEY_VALUE, PLUGINVERSION, KEY_END), KS_END); ksAppend (returned, contract); @@ -183,6 +184,19 @@ static void saveToDisc (Key * k ELEKTRA_UNUSED) { } +//![validate configuration] +int elektraDocCheckConf (Key * errorKey ELEKTRA_UNUSED, KeySet * conf ELEKTRA_UNUSED) +{ + /* validate plugin configuration */ + + // the return codes have the following meaning: + // 0: config was not changed (was ok) + // 1: config is changed (now ok) + // -1: config not ok, could not fix + return 0; +} +//![validate configuration] + //![set full] static void usercode (KDB * handle, KeySet * keyset, Key * key) { diff --git a/src/plugins/doc/doc.h b/src/plugins/doc/doc.h index 34911c99d5d..2eb72606ec6 100644 --- a/src/plugins/doc/doc.h +++ b/src/plugins/doc/doc.h @@ -494,6 +494,28 @@ int elektraDocSet (Plugin * handle, KeySet * returned, Key * parentKey); */ int elektraDocError (Plugin * handle, KeySet * returned, Key * parentKey); +/** + * @brief Validate plugin configuration at mount time. + * + * During the mount phase the BackendBuilder calls this method, + * if it is provided by the plugin. + * + * In this method the plugin configuration can be checked for validity or + * integrity. Missing items can be added to complete the configuration. + * + * @param errorKey is used to propagate error messages to the caller + * @param conf contains the plugin configuration to be validated + * + * @retval 1 on success: config is changed (now ok) + * @retval 0 on success: config was not changed (was ok) + * @retval -1 on failure: config not ok, could not be fixed + * Set an error using #ELEKTRA_SET_ERROR to inform the user what went wrong. + * Additionally you can add any number of warnings with + * #ELEKTRA_ADD_WARNING. + * + * @ingroup plugin + */ +int elektraDocCheckConf (Key * errorKey, KeySet * conf); /** * @} From 78cdd34a8945d9d26b6179bd2400e782c78ded50 Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Fri, 20 May 2016 07:45:10 +0200 Subject: [PATCH 08/23] remove odd exception handling in plugindatabase.cpp --- src/libs/tools/src/plugindatabase.cpp | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/src/libs/tools/src/plugindatabase.cpp b/src/libs/tools/src/plugindatabase.cpp index daa3f8a4871..51306137b79 100644 --- a/src/libs/tools/src/plugindatabase.cpp +++ b/src/libs/tools/src/plugindatabase.cpp @@ -211,30 +211,14 @@ PluginDatabase::Status ModulesPluginDatabase::status (PluginSpec const & spec) c std::string ModulesPluginDatabase::lookupInfo (PluginSpec const & spec, std::string const & which) const { PluginPtr plugin; - try - { - plugin = impl->modules.load (spec.getName (), spec.getConfig ()); - } - catch (...) - { - throw; - } - + plugin = impl->modules.load (spec.getName (), spec.getConfig ()); return plugin->lookupInfo (which); } PluginDatabase::func_t ModulesPluginDatabase::getSymbol (PluginSpec const & spec, std::string const & which) const { PluginPtr plugin; - try - { - plugin = impl->modules.load (spec.getName (), spec.getConfig ()); - } - catch (...) - { - throw; - } - + plugin = impl->modules.load (spec.getName (), spec.getConfig ()); return plugin->getSymbol (which); } From dff7a6e3e4976b2ffb0f06dfe0a8ad31cef87c58 Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Fri, 20 May 2016 08:01:08 +0200 Subject: [PATCH 09/23] prettify code in plugindatabase.cpp --- src/libs/tools/src/plugindatabase.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libs/tools/src/plugindatabase.cpp b/src/libs/tools/src/plugindatabase.cpp index 51306137b79..531d1186876 100644 --- a/src/libs/tools/src/plugindatabase.cpp +++ b/src/libs/tools/src/plugindatabase.cpp @@ -210,15 +210,13 @@ PluginDatabase::Status ModulesPluginDatabase::status (PluginSpec const & spec) c std::string ModulesPluginDatabase::lookupInfo (PluginSpec const & spec, std::string const & which) const { - PluginPtr plugin; - plugin = impl->modules.load (spec.getName (), spec.getConfig ()); + PluginPtr plugin = impl->modules.load (spec.getName (), spec.getConfig ()); return plugin->lookupInfo (which); } PluginDatabase::func_t ModulesPluginDatabase::getSymbol (PluginSpec const & spec, std::string const & which) const { - PluginPtr plugin; - plugin = impl->modules.load (spec.getName (), spec.getConfig ()); + PluginPtr plugin = impl->modules.load (spec.getName (), spec.getConfig ()); return plugin->getSymbol (which); } From 5e146af241fb9d3ce1e54bc537facd7fc0abb681 Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Fri, 20 May 2016 08:29:18 +0200 Subject: [PATCH 10/23] improve checkconf documentation --- doc/tutorials/plugins.md | 8 ++++---- src/plugins/doc/doc.c | 6 +++--- src/plugins/doc/doc.h | 6 +++--- src/plugins/template/template.c | 6 +++--- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/doc/tutorials/plugins.md b/doc/tutorials/plugins.md index 4b9e9f5db9c..2a9f2ae404f 100644 --- a/doc/tutorials/plugins.md +++ b/doc/tutorials/plugins.md @@ -329,13 +329,13 @@ The `elektraPluginCheckConf` function may be used for validation of the plugin c int elektraLineCheckConfig (Key * errorKey, KeySet * conf) -The configuration of the plugin is provided as `conf`. The function may report errors using the `errorKey` and the return value. +The configuration of the plugin is provided as `conf`. The function may report an error or warnings using the `errorKey` and the return value. The following convention was established for the return value of `elektraPluginCheckConf`: -- 0: config was not changed (was ok) -- 1: config is changed (now ok) -- -1: config not ok, could not fix it +- 0: The configuration was OK and has not been changed +- 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. ### ELEKTRA_PLUGIN_EXPORT ### diff --git a/src/plugins/doc/doc.c b/src/plugins/doc/doc.c index 9f5112934c9..48a3e954753 100644 --- a/src/plugins/doc/doc.c +++ b/src/plugins/doc/doc.c @@ -190,9 +190,9 @@ int elektraDocCheckConf (Key * errorKey ELEKTRA_UNUSED, KeySet * conf ELEKTRA_UN /* validate plugin configuration */ // the return codes have the following meaning: - // 0: config was not changed (was ok) - // 1: config is changed (now ok) - // -1: config not ok, could not fix + // 0: The configuration was OK and has not been changed + // 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. return 0; } //![validate configuration] diff --git a/src/plugins/doc/doc.h b/src/plugins/doc/doc.h index 2eb72606ec6..b2f3441104b 100644 --- a/src/plugins/doc/doc.h +++ b/src/plugins/doc/doc.h @@ -506,9 +506,9 @@ int elektraDocError (Plugin * handle, KeySet * returned, Key * parentKey); * @param errorKey is used to propagate error messages to the caller * @param conf contains the plugin configuration to be validated * - * @retval 1 on success: config is changed (now ok) - * @retval 0 on success: config was not changed (was ok) - * @retval -1 on failure: config not ok, could not be fixed + * @retval 1 on success: the configuration was OK and has not been changed. + * @retval 0 on success: the configuration has been changed and now it is OK. + * @retval -1 on failure: the configuration was not OK and could not be fixed. * Set an error using #ELEKTRA_SET_ERROR to inform the user what went wrong. * Additionally you can add any number of warnings with * #ELEKTRA_ADD_WARNING. diff --git a/src/plugins/template/template.c b/src/plugins/template/template.c index 231dd22ae7c..ae7825ec817 100644 --- a/src/plugins/template/template.c +++ b/src/plugins/template/template.c @@ -75,9 +75,9 @@ int elektraTemplateCheckConfig (Key * errorKey ELEKTRA_UNUSED, KeySet * conf ELE // this function is optional // the return codes have the following meaning: - // 0: config was not changed (was ok) - // 1: config is changed (now ok) - // -1: config not ok, could not fix + // 0: The configuration was OK and has not been changed + // 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. return 0; } From 0caed1eb6be502a6df602c227bc047c0dc31fb74 Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Fri, 20 May 2016 10:43:38 +0200 Subject: [PATCH 11/23] add checkconf example to plugins.md tutorial --- doc/tutorials/plugins.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/doc/tutorials/plugins.md b/doc/tutorials/plugins.md index 2a9f2ae404f..45ae6d7a3ee 100644 --- a/doc/tutorials/plugins.md +++ b/doc/tutorials/plugins.md @@ -337,6 +337,24 @@ The following convention was established for the return value of `elektraPluginC - 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. + + int elektraLineCheckConfig (Key * errorKey, KeySet * conf) + { + Key * cur; + ksRewind (conf); + while ((cur = ksNext (conf)) != 0) + { + const char * value = keyString (cur); + if (strlen (value) > 3) + { + ELEKTRA_SET_ERRORF (ELEKTRA_ERROR_VALUE_LENGTH, errorKey, "value %s is more than 3 characters long", value); + return -1; // The configuration was not OK and could not be fixed + } + } + return 0; // The configuration was OK and has not been changed + } + ### ELEKTRA_PLUGIN_EXPORT ### The last function, one that is always needed in a plug-in, is `ELEKTRA_PLUGIN_EXPORT`. This functions is responsible for letting Elektra know that From 0639d040e4589d0691d98902d417b4ec29bdff31 Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Fri, 20 May 2016 18:55:02 +0200 Subject: [PATCH 12/23] add unit tests for checkconf (WARNING: contains bug) the test case at testtool_backendbuilder.cpp:571 checkconfOkChanged is not OK. I suspect something is wrong in BackendBuilder::addPlugin --- src/libs/tools/src/backendbuilder.cpp | 13 +++- src/libs/tools/src/plugindatabase.cpp | 33 ++++++++- .../tools/tests/testtool_backendbuilder.cpp | 70 +++++++++++++++++++ 3 files changed, 112 insertions(+), 4 deletions(-) diff --git a/src/libs/tools/src/backendbuilder.cpp b/src/libs/tools/src/backendbuilder.cpp index cb61f95108f..76a2dca3a96 100644 --- a/src/libs/tools/src/backendbuilder.cpp +++ b/src/libs/tools/src/backendbuilder.cpp @@ -430,11 +430,22 @@ void BackendBuilder::addPlugin (PluginSpec const & plugin) if (checkConfFunction != nullptr) { ckdb::Key * errorKey = ckdb::keyNew (0); - int checkResult = checkConfFunction (errorKey, newPlugin.getConfig ().getKeySet ()); + ckdb::KeySet * pluginConfig = newPlugin.getConfig ().dup (); + int checkResult = checkConfFunction (errorKey, pluginConfig); if (checkResult == -1) { + ckdb::ksDel (pluginConfig); throw PluginConfigInvalid (errorKey); } + else if (checkResult == 1) + { + KeySet modifiedPluginConfig = KeySet (pluginConfig); + newPlugin.appendConfig (modifiedPluginConfig); + } + else + { + ckdb::ksDel (pluginConfig); + } ckdb::keyDel (errorKey); } diff --git a/src/libs/tools/src/plugindatabase.cpp b/src/libs/tools/src/plugindatabase.cpp index 531d1186876..f4cbe9ba106 100644 --- a/src/libs/tools/src/plugindatabase.cpp +++ b/src/libs/tools/src/plugindatabase.cpp @@ -365,10 +365,37 @@ std::string MockPluginDatabase::lookupInfo (PluginSpec const & spec, std::string return ""; } -PluginDatabase::func_t MockPluginDatabase::getSymbol (PluginSpec const & spec ELEKTRA_UNUSED, - std::string const & which ELEKTRA_UNUSED) const +/** +* @brief reference checkconf implementation for the unit tests. +* +* Checks for the key "a" to be set to "abc" and fails if it can not find it. +* Checks for the key "b" and appends it to the configuration if it can not find it. +*/ +static int checkconfMock (ckdb::Key * errorKey, ckdb::KeySet * conf) +{ + ckdb::Key * k; + + k = ckdb::ksLookupByName (conf, "user/a", 0); + if (!k || strncmp (ckdb::keyString (k), "abc", 3)) + { + return -1; // invalid config, could not fix + } + + k = ckdb::ksLookupByName (conf, "user/b", 0); + if (!k) + { + ckdb::ksAppendKey (conf, ckdb::keyNew ("user/b", 0)); + return 1; // changed config, OK + } + return 0; // did not change config, OK +} + +PluginDatabase::func_t MockPluginDatabase::getSymbol (PluginSpec const & spec, std::string const & which) const { - // TODO implement mockup + if (which.find ("checkconf") != std::string::npos && spec.getFullName ().find ("checkconf") != std::string::npos) + { + return reinterpret_cast (checkconfMock); + } return NULL; } } diff --git a/src/libs/tools/tests/testtool_backendbuilder.cpp b/src/libs/tools/tests/testtool_backendbuilder.cpp index 6f49195ecc9..7c9a6356647 100644 --- a/src/libs/tools/tests/testtool_backendbuilder.cpp +++ b/src/libs/tools/tests/testtool_backendbuilder.cpp @@ -546,3 +546,73 @@ TEST (BackendBuilder, resolveDoubleRecommends) EXPECT_EQ (bb.cbegin ()[1], PluginSpec ("a")); EXPECT_EQ (bb.cbegin ()[2], PluginSpec ("c")); } + +TEST (BackendBuilder, checkconfOkNoChange) +{ + using namespace kdb; + using namespace kdb::tools; + std::shared_ptr mpd = std::make_shared (); + mpd->data[PluginSpec ("checkconf1")]["provides"] = "test123"; + BackendBuilderInit bbi (mpd); + BackendBuilder bb (bbi); + PluginSpec spec ("checkconf1"); + KeySet pluginConfig; + Key a; + a.setName ("user/a"); + a.setString ("abc"); + Key b; + b.setName ("user/b"); + pluginConfig.append (a); + pluginConfig.append (b); + spec.appendConfig (pluginConfig); + bb.addPlugin (spec); +} + +TEST (BackendBuilder, checkconfOkChanged) +{ + using namespace kdb; + using namespace kdb::tools; + std::shared_ptr mpd = std::make_shared (); + mpd->data[PluginSpec ("checkconf1")]["provides"] = "test123"; + BackendBuilderInit bbi (mpd); + BackendBuilder bb (bbi); + PluginSpec spec ("checkconf1"); + KeySet pluginConfig; + Key a; + a.setName ("user/a"); + a.setString ("abc"); + pluginConfig.append (a); + spec.appendConfig (pluginConfig); + bb.addPlugin (spec); + // we expect b to be added now + spec.getConfig ().get ("user/b"); +} + +TEST (BackendBuilder, checkconfNotOKwrongValue) +{ + using namespace kdb; + using namespace kdb::tools; + std::shared_ptr mpd = std::make_shared (); + mpd->data[PluginSpec ("checkconf1")]["provides"] = "test123"; + BackendBuilderInit bbi (mpd); + BackendBuilder bb (bbi); + PluginSpec spec ("checkconf1"); + KeySet pluginConfig; + Key a; + a.setName ("user/a"); + a.setString ("wrong value"); + pluginConfig.append (a); + spec.appendConfig (pluginConfig); + EXPECT_THROW (bb.addPlugin (spec), PluginConfigInvalid); +} + +TEST (BackendBuilder, checkconfNotOKmissing) +{ + using namespace kdb; + using namespace kdb::tools; + std::shared_ptr mpd = std::make_shared (); + mpd->data[PluginSpec ("checkconf3")]["c"] = "something"; + BackendBuilderInit bbi (mpd); + BackendBuilder bb (bbi); + EXPECT_THROW (bb.addPlugin (PluginSpec ("checkconf3")), PluginConfigInvalid); +} \ No newline at end of file From 6de728090604a4dedc46dccfc0a0a63999f4ffce Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Fri, 20 May 2016 20:23:17 +0200 Subject: [PATCH 13/23] replace exception with 0-return in ModulesPluginDatabase::getSymbol --- src/libs/tools/include/plugindatabase.hpp | 2 +- src/libs/tools/src/backendbuilder.cpp | 13 ++----------- src/libs/tools/src/plugindatabase.cpp | 11 +++++++++-- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/libs/tools/include/plugindatabase.hpp b/src/libs/tools/include/plugindatabase.hpp index b8c19028222..e675bb6d5ba 100644 --- a/src/libs/tools/include/plugindatabase.hpp +++ b/src/libs/tools/include/plugindatabase.hpp @@ -77,7 +77,7 @@ class PluginDatabase * @param whichplugin from which plugin? * @param which which symbol would you like to look up? * - * @return the function pointer to the exported symbol + * @return the function pointer to the exported symbol or NULL if the symbol was not found */ virtual func_t getSymbol (PluginSpec const & whichplugin, std::string const & which) const = 0; diff --git a/src/libs/tools/src/backendbuilder.cpp b/src/libs/tools/src/backendbuilder.cpp index 76a2dca3a96..139a93dc9b4 100644 --- a/src/libs/tools/src/backendbuilder.cpp +++ b/src/libs/tools/src/backendbuilder.cpp @@ -417,17 +417,8 @@ void BackendBuilder::addPlugin (PluginSpec const & plugin) // call plugin's checkconf function (if provided) // this enables a plugin to verify its configuration at mount time - checkConfPtr checkConfFunction = nullptr; - try - { - checkConfFunction = reinterpret_cast (pluginDatabase->getSymbol (newPlugin, "checkconf")); - } - catch (...) - { - // the checkconf operation is optional, so no worries if it was not found - } - - if (checkConfFunction != nullptr) + checkConfPtr checkConfFunction = reinterpret_cast (pluginDatabase->getSymbol (newPlugin, "checkconf")); + if (checkConfFunction) { ckdb::Key * errorKey = ckdb::keyNew (0); ckdb::KeySet * pluginConfig = newPlugin.getConfig ().dup (); diff --git a/src/libs/tools/src/plugindatabase.cpp b/src/libs/tools/src/plugindatabase.cpp index f4cbe9ba106..9004393c825 100644 --- a/src/libs/tools/src/plugindatabase.cpp +++ b/src/libs/tools/src/plugindatabase.cpp @@ -216,8 +216,15 @@ std::string ModulesPluginDatabase::lookupInfo (PluginSpec const & spec, std::str PluginDatabase::func_t ModulesPluginDatabase::getSymbol (PluginSpec const & spec, std::string const & which) const { - PluginPtr plugin = impl->modules.load (spec.getName (), spec.getConfig ()); - return plugin->getSymbol (which); + try + { + PluginPtr plugin = impl->modules.load (spec.getName (), spec.getConfig ()); + return plugin->getSymbol (which); + } + catch (...) + { + return NULL; + } } PluginSpec ModulesPluginDatabase::lookupMetadata (std::string const & which) const From 7bb49be9fd88259147c85ff8098ddb5ebad1054c Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Fri, 20 May 2016 21:11:09 +0200 Subject: [PATCH 14/23] split mockup functions in MockPluginDatabase --- src/libs/tools/include/plugindatabase.hpp | 6 ++ src/libs/tools/src/plugindatabase.cpp | 32 ++------- .../tools/tests/testtool_backendbuilder.cpp | 65 ++++++++++++------- 3 files changed, 55 insertions(+), 48 deletions(-) diff --git a/src/libs/tools/include/plugindatabase.hpp b/src/libs/tools/include/plugindatabase.hpp index e675bb6d5ba..b6bc9a57122 100644 --- a/src/libs/tools/include/plugindatabase.hpp +++ b/src/libs/tools/include/plugindatabase.hpp @@ -142,6 +142,8 @@ class ModulesPluginDatabase : public PluginDatabase class MockPluginDatabase : public ModulesPluginDatabase { public: + typedef int (*checkConfPtr) (ckdb::Key *, ckdb::KeySet *); + /// only data from here will be returned /// @note that it is ordered by name, i.e., different ref-names cannot be distinguished mutable std::unordered_map, PluginSpecHash, PluginSpecName> data; @@ -150,6 +152,10 @@ class MockPluginDatabase : public ModulesPluginDatabase PluginDatabase::Status status (PluginSpec const & whichplugin) const; std::string lookupInfo (PluginSpec const & spec, std::string const & which) const; func_t getSymbol (PluginSpec const & whichplugin, std::string const & which) const; + void setCheckconfFunction (checkConfPtr const newCheckconf); + +private: + checkConfPtr checkconf = NULL; }; } } diff --git a/src/libs/tools/src/plugindatabase.cpp b/src/libs/tools/src/plugindatabase.cpp index 9004393c825..1d205dac048 100644 --- a/src/libs/tools/src/plugindatabase.cpp +++ b/src/libs/tools/src/plugindatabase.cpp @@ -372,38 +372,18 @@ std::string MockPluginDatabase::lookupInfo (PluginSpec const & spec, std::string return ""; } -/** -* @brief reference checkconf implementation for the unit tests. -* -* Checks for the key "a" to be set to "abc" and fails if it can not find it. -* Checks for the key "b" and appends it to the configuration if it can not find it. -*/ -static int checkconfMock (ckdb::Key * errorKey, ckdb::KeySet * conf) +PluginDatabase::func_t MockPluginDatabase::getSymbol (PluginSpec const & spec ELEKTRA_UNUSED, std::string const & which) const { - ckdb::Key * k; - - k = ckdb::ksLookupByName (conf, "user/a", 0); - if (!k || strncmp (ckdb::keyString (k), "abc", 3)) - { - return -1; // invalid config, could not fix - } - - k = ckdb::ksLookupByName (conf, "user/b", 0); - if (!k) + if (which == "checkconf") { - ckdb::ksAppendKey (conf, ckdb::keyNew ("user/b", 0)); - return 1; // changed config, OK + return reinterpret_cast (checkconf); } - return 0; // did not change config, OK + return NULL; } -PluginDatabase::func_t MockPluginDatabase::getSymbol (PluginSpec const & spec, std::string const & which) const +void MockPluginDatabase::setCheckconfFunction (const MockPluginDatabase::checkConfPtr newCheckconf) { - if (which.find ("checkconf") != std::string::npos && spec.getFullName ().find ("checkconf") != std::string::npos) - { - return reinterpret_cast (checkconfMock); - } - return NULL; + checkconf = newCheckconf; } } } diff --git a/src/libs/tools/tests/testtool_backendbuilder.cpp b/src/libs/tools/tests/testtool_backendbuilder.cpp index 7c9a6356647..212bc594656 100644 --- a/src/libs/tools/tests/testtool_backendbuilder.cpp +++ b/src/libs/tools/tests/testtool_backendbuilder.cpp @@ -20,6 +20,8 @@ #include #include +#include + #include #include @@ -547,33 +549,23 @@ TEST (BackendBuilder, resolveDoubleRecommends) EXPECT_EQ (bb.cbegin ()[2], PluginSpec ("c")); } -TEST (BackendBuilder, checkconfOkNoChange) +static int checkconfLookup_a_abc (ckdb::Key * errorKey, ckdb::KeySet * config) { - using namespace kdb; - using namespace kdb::tools; - std::shared_ptr mpd = std::make_shared (); - mpd->data[PluginSpec ("checkconf1")]["provides"] = "test123"; - BackendBuilderInit bbi (mpd); - BackendBuilder bb (bbi); - PluginSpec spec ("checkconf1"); - KeySet pluginConfig; - Key a; - a.setName ("user/a"); - a.setString ("abc"); - Key b; - b.setName ("user/b"); - pluginConfig.append (a); - pluginConfig.append (b); - spec.appendConfig (pluginConfig); - bb.addPlugin (spec); + ckdb::Key * k = ckdb::ksLookupByName (config, "/a", 0); + if (k && strcmp (ckdb::keyString (k), "abc") == 0) + { + return 0; + } + return -1; } -TEST (BackendBuilder, checkconfOkChanged) +TEST (BackendBuilder, checkconfOkNoChange) { using namespace kdb; using namespace kdb::tools; std::shared_ptr mpd = std::make_shared (); mpd->data[PluginSpec ("checkconf1")]["provides"] = "test123"; + mpd->setCheckconfFunction (checkconfLookup_a_abc); BackendBuilderInit bbi (mpd); BackendBuilder bb (bbi); PluginSpec spec ("checkconf1"); @@ -584,8 +576,6 @@ TEST (BackendBuilder, checkconfOkChanged) pluginConfig.append (a); spec.appendConfig (pluginConfig); bb.addPlugin (spec); - // we expect b to be added now - spec.getConfig ().get ("user/b"); } TEST (BackendBuilder, checkconfNotOKwrongValue) @@ -594,6 +584,7 @@ TEST (BackendBuilder, checkconfNotOKwrongValue) using namespace kdb::tools; std::shared_ptr mpd = std::make_shared (); mpd->data[PluginSpec ("checkconf1")]["provides"] = "test123"; + mpd->setCheckconfFunction (checkconfLookup_a_abc); BackendBuilderInit bbi (mpd); BackendBuilder bb (bbi); PluginSpec spec ("checkconf1"); @@ -612,7 +603,37 @@ TEST (BackendBuilder, checkconfNotOKmissing) using namespace kdb::tools; std::shared_ptr mpd = std::make_shared (); mpd->data[PluginSpec ("checkconf3")]["c"] = "something"; + mpd->setCheckconfFunction (checkconfLookup_a_abc); BackendBuilderInit bbi (mpd); BackendBuilder bb (bbi); EXPECT_THROW (bb.addPlugin (PluginSpec ("checkconf3")), PluginConfigInvalid); -} \ No newline at end of file +} + +static int checkconfAppend_b_bcd (ckdb::Key * errorKey, ckdb::KeySet * config) +{ + ckdb::Key * k = ckdb::ksLookupByName (config, "user/b", 0); + if (!k) + { + ckdb::ksAppendKey (config, ckdb::keyNew ("user/b", KEY_VALUE, "test", KEY_END)); + return 1; + } + return 0; +} + +TEST (BackendBuilder, checkconfOkChanged) +{ + using namespace kdb; + using namespace kdb::tools; + std::shared_ptr mpd = std::make_shared (); + mpd->data[PluginSpec ("checkconf1")]["provides"] = "test123"; + mpd->setCheckconfFunction (checkconfAppend_b_bcd); + BackendBuilderInit bbi (mpd); + BackendBuilder bb (bbi); + PluginSpec spec ("checkconf1"); + KeySet pluginConfig; + spec.appendConfig (pluginConfig); + bb.addPlugin (spec); + // we expect b to be added now + spec = *bb.begin (); + EXPECT_EQ (spec.getConfig ().get ("user/b"), "test"); +} From 274a395b647bb53bc593d2b1871bbd95f74179f9 Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Sat, 21 May 2016 09:25:31 +0200 Subject: [PATCH 15/23] simplify checkconf test cases --- .../tools/tests/testtool_backendbuilder.cpp | 33 ++++--------------- 1 file changed, 6 insertions(+), 27 deletions(-) diff --git a/src/libs/tools/tests/testtool_backendbuilder.cpp b/src/libs/tools/tests/testtool_backendbuilder.cpp index 212bc594656..e5c3212eab8 100644 --- a/src/libs/tools/tests/testtool_backendbuilder.cpp +++ b/src/libs/tools/tests/testtool_backendbuilder.cpp @@ -20,8 +20,6 @@ #include #include -#include - #include #include @@ -549,10 +547,10 @@ TEST (BackendBuilder, resolveDoubleRecommends) EXPECT_EQ (bb.cbegin ()[2], PluginSpec ("c")); } -static int checkconfLookup_a_abc (ckdb::Key * errorKey, ckdb::KeySet * config) +static int checkconfLookup (ckdb::Key * errorKey, ckdb::KeySet * config) { ckdb::Key * k = ckdb::ksLookupByName (config, "/a", 0); - if (k && strcmp (ckdb::keyString (k), "abc") == 0) + if (k) { return 0; } @@ -565,7 +563,7 @@ TEST (BackendBuilder, checkconfOkNoChange) using namespace kdb::tools; std::shared_ptr mpd = std::make_shared (); mpd->data[PluginSpec ("checkconf1")]["provides"] = "test123"; - mpd->setCheckconfFunction (checkconfLookup_a_abc); + mpd->setCheckconfFunction (checkconfLookup); BackendBuilderInit bbi (mpd); BackendBuilder bb (bbi); PluginSpec spec ("checkconf1"); @@ -578,38 +576,19 @@ TEST (BackendBuilder, checkconfOkNoChange) bb.addPlugin (spec); } -TEST (BackendBuilder, checkconfNotOKwrongValue) -{ - using namespace kdb; - using namespace kdb::tools; - std::shared_ptr mpd = std::make_shared (); - mpd->data[PluginSpec ("checkconf1")]["provides"] = "test123"; - mpd->setCheckconfFunction (checkconfLookup_a_abc); - BackendBuilderInit bbi (mpd); - BackendBuilder bb (bbi); - PluginSpec spec ("checkconf1"); - KeySet pluginConfig; - Key a; - a.setName ("user/a"); - a.setString ("wrong value"); - pluginConfig.append (a); - spec.appendConfig (pluginConfig); - EXPECT_THROW (bb.addPlugin (spec), PluginConfigInvalid); -} - TEST (BackendBuilder, checkconfNotOKmissing) { using namespace kdb; using namespace kdb::tools; std::shared_ptr mpd = std::make_shared (); mpd->data[PluginSpec ("checkconf3")]["c"] = "something"; - mpd->setCheckconfFunction (checkconfLookup_a_abc); + mpd->setCheckconfFunction (checkconfLookup); BackendBuilderInit bbi (mpd); BackendBuilder bb (bbi); EXPECT_THROW (bb.addPlugin (PluginSpec ("checkconf3")), PluginConfigInvalid); } -static int checkconfAppend_b_bcd (ckdb::Key * errorKey, ckdb::KeySet * config) +static int checkconfAppend (ckdb::Key * errorKey, ckdb::KeySet * config) { ckdb::Key * k = ckdb::ksLookupByName (config, "user/b", 0); if (!k) @@ -626,7 +605,7 @@ TEST (BackendBuilder, checkconfOkChanged) using namespace kdb::tools; std::shared_ptr mpd = std::make_shared (); mpd->data[PluginSpec ("checkconf1")]["provides"] = "test123"; - mpd->setCheckconfFunction (checkconfAppend_b_bcd); + mpd->setCheckconfFunction (checkconfAppend); BackendBuilderInit bbi (mpd); BackendBuilder bb (bbi); PluginSpec spec ("checkconf1"); From d15a8d910fa64b524950af5d167a9cef98bbfecb Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Sat, 21 May 2016 10:43:39 +0200 Subject: [PATCH 16/23] add checkconf test case Test removing Keys from the plugin configuration --- .../tools/tests/testtool_backendbuilder.cpp | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/libs/tools/tests/testtool_backendbuilder.cpp b/src/libs/tools/tests/testtool_backendbuilder.cpp index e5c3212eab8..34c33131fb3 100644 --- a/src/libs/tools/tests/testtool_backendbuilder.cpp +++ b/src/libs/tools/tests/testtool_backendbuilder.cpp @@ -616,3 +616,31 @@ TEST (BackendBuilder, checkconfOkChanged) spec = *bb.begin (); EXPECT_EQ (spec.getConfig ().get ("user/b"), "test"); } + +static int checkconfDelete (ckdb::Key * errorKey, ckdb::KeySet * config) +{ + ckdb::ksCopy (config, NULL); + return 1; +} + +TEST (BackendBuilder, checkconfOkRemoved) +{ + using namespace kdb; + using namespace kdb::tools; + std::shared_ptr mpd = std::make_shared (); + mpd->data[PluginSpec ("checkconf1")]["provides"] = "test123"; + mpd->setCheckconfFunction (checkconfDelete); + BackendBuilderInit bbi (mpd); + BackendBuilder bb (bbi); + PluginSpec spec ("checkconf1"); + KeySet pluginConfig; + Key a; + a.setName ("user/a"); + a.setString ("abc"); + pluginConfig.append (a); + spec.appendConfig (pluginConfig); + bb.addPlugin (spec); + // we expect b to be added now + spec = *bb.begin (); + EXPECT_THROW (spec.getConfig ().get ("user/a"), KeyNotFoundException); +} From 7bf749b64ac81e6889362876ed07f912ab48e0fe Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Sat, 21 May 2016 10:51:02 +0200 Subject: [PATCH 17/23] add PluginSpec::setConfig() --- src/libs/tools/include/pluginspec.hpp | 7 ++++--- src/libs/tools/src/backendbuilder.cpp | 2 +- src/libs/tools/src/pluginspec.cpp | 17 ++++++++++++++--- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/libs/tools/include/pluginspec.hpp b/src/libs/tools/include/pluginspec.hpp index 18c07d5743c..2c32c795faf 100644 --- a/src/libs/tools/include/pluginspec.hpp +++ b/src/libs/tools/include/pluginspec.hpp @@ -51,6 +51,7 @@ class PluginSpec void setName (std::string const & name); void appendConfig (KeySet config); + void setConfig (KeySet config); void validate (std::string const & str) const; @@ -97,13 +98,13 @@ struct PluginSpecHash }; #ifdef ELEKTRA_PLUGINSPEC_WITH_COMPARE -bool operator== (PluginSpec const & self, PluginSpec const & other); -bool operator!= (PluginSpec const & self, PluginSpec const & other); +bool operator==(PluginSpec const & self, PluginSpec const & other); +bool operator!=(PluginSpec const & self, PluginSpec const & other); #endif typedef std::vector PluginSpecVector; -std::ostream & operator<< (std::ostream & os, PluginSpec const & spec); +std::ostream & operator<<(std::ostream & os, PluginSpec const & spec); } } diff --git a/src/libs/tools/src/backendbuilder.cpp b/src/libs/tools/src/backendbuilder.cpp index 139a93dc9b4..a9d121ac1aa 100644 --- a/src/libs/tools/src/backendbuilder.cpp +++ b/src/libs/tools/src/backendbuilder.cpp @@ -431,7 +431,7 @@ void BackendBuilder::addPlugin (PluginSpec const & plugin) else if (checkResult == 1) { KeySet modifiedPluginConfig = KeySet (pluginConfig); - newPlugin.appendConfig (modifiedPluginConfig); + newPlugin.setConfig (modifiedPluginConfig); } else { diff --git a/src/libs/tools/src/pluginspec.cpp b/src/libs/tools/src/pluginspec.cpp index 138f138533f..9dfe0a2ba6f 100644 --- a/src/libs/tools/src/pluginspec.cpp +++ b/src/libs/tools/src/pluginspec.cpp @@ -195,6 +195,17 @@ void PluginSpec::appendConfig (KeySet c) config.append (c); } +/** + * @brief Set plugin config + * + * @param c new config to be used as plugin config + */ +void PluginSpec::setConfig (KeySet c) +{ + config.clear (); + config.append (c); +} + /** * @brief Check if str starts with a-z and then only has chars a-z, 0-9 or underscore (_) * @@ -222,7 +233,7 @@ void PluginSpec::validate (std::string const & n) const * @brief Compare two pluginspec if their value is equal * @note the content of getConfig() will be only compared with keynames, not content! */ -bool operator== (PluginSpec const & self, PluginSpec const & other) +bool operator==(PluginSpec const & self, PluginSpec const & other) { return self.getName () == other.getName () && self.getRefName () == other.getRefName () && self.getConfig () == other.getConfig (); } @@ -231,7 +242,7 @@ bool operator== (PluginSpec const & self, PluginSpec const & other) * @brief Compare two pluginspec if their value is not equal * @note the content of getConfig() will be only compared with keynames, not content! */ -bool operator!= (PluginSpec const & self, PluginSpec const & other) +bool operator!=(PluginSpec const & self, PluginSpec const & other) { return !(self == other); } @@ -239,7 +250,7 @@ bool operator!= (PluginSpec const & self, PluginSpec const & other) /** * @brief Output the name, refname and size of config */ -std::ostream & operator<< (std::ostream & os, PluginSpec const & spec) +std::ostream & operator<<(std::ostream & os, PluginSpec const & spec) { os << "name: " << spec.getName () << " refname: " << spec.getRefName () << " configsize: " << spec.getConfig ().size (); return os; From e9ccac64755e2b36ae8f567be0af9f22f2c4e052 Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Sat, 21 May 2016 11:03:57 +0200 Subject: [PATCH 18/23] reformat source --- src/libs/tools/include/pluginspec.hpp | 6 +++--- src/libs/tools/src/pluginspec.cpp | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libs/tools/include/pluginspec.hpp b/src/libs/tools/include/pluginspec.hpp index 2c32c795faf..23569de28a8 100644 --- a/src/libs/tools/include/pluginspec.hpp +++ b/src/libs/tools/include/pluginspec.hpp @@ -98,13 +98,13 @@ struct PluginSpecHash }; #ifdef ELEKTRA_PLUGINSPEC_WITH_COMPARE -bool operator==(PluginSpec const & self, PluginSpec const & other); -bool operator!=(PluginSpec const & self, PluginSpec const & other); +bool operator== (PluginSpec const & self, PluginSpec const & other); +bool operator!= (PluginSpec const & self, PluginSpec const & other); #endif typedef std::vector PluginSpecVector; -std::ostream & operator<<(std::ostream & os, PluginSpec const & spec); +std::ostream & operator<< (std::ostream & os, PluginSpec const & spec); } } diff --git a/src/libs/tools/src/pluginspec.cpp b/src/libs/tools/src/pluginspec.cpp index 9dfe0a2ba6f..ccb17e169d3 100644 --- a/src/libs/tools/src/pluginspec.cpp +++ b/src/libs/tools/src/pluginspec.cpp @@ -233,7 +233,7 @@ void PluginSpec::validate (std::string const & n) const * @brief Compare two pluginspec if their value is equal * @note the content of getConfig() will be only compared with keynames, not content! */ -bool operator==(PluginSpec const & self, PluginSpec const & other) +bool operator== (PluginSpec const & self, PluginSpec const & other) { return self.getName () == other.getName () && self.getRefName () == other.getRefName () && self.getConfig () == other.getConfig (); } @@ -242,7 +242,7 @@ bool operator==(PluginSpec const & self, PluginSpec const & other) * @brief Compare two pluginspec if their value is not equal * @note the content of getConfig() will be only compared with keynames, not content! */ -bool operator!=(PluginSpec const & self, PluginSpec const & other) +bool operator!= (PluginSpec const & self, PluginSpec const & other) { return !(self == other); } @@ -250,7 +250,7 @@ bool operator!=(PluginSpec const & self, PluginSpec const & other) /** * @brief Output the name, refname and size of config */ -std::ostream & operator<<(std::ostream & os, PluginSpec const & spec) +std::ostream & operator<< (std::ostream & os, PluginSpec const & spec) { os << "name: " << spec.getName () << " refname: " << spec.getRefName () << " configsize: " << spec.getConfig ().size (); return os; From ac8c148031563055c080e2c948455050300c84fd Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Sat, 21 May 2016 11:24:07 +0200 Subject: [PATCH 19/23] simplify checkconf mockup in testtool_backendbuilder.cpp --- src/libs/tools/tests/testtool_backendbuilder.cpp | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/libs/tools/tests/testtool_backendbuilder.cpp b/src/libs/tools/tests/testtool_backendbuilder.cpp index 34c33131fb3..cb8998d37f9 100644 --- a/src/libs/tools/tests/testtool_backendbuilder.cpp +++ b/src/libs/tools/tests/testtool_backendbuilder.cpp @@ -590,13 +590,8 @@ TEST (BackendBuilder, checkconfNotOKmissing) static int checkconfAppend (ckdb::Key * errorKey, ckdb::KeySet * config) { - ckdb::Key * k = ckdb::ksLookupByName (config, "user/b", 0); - if (!k) - { - ckdb::ksAppendKey (config, ckdb::keyNew ("user/b", KEY_VALUE, "test", KEY_END)); - return 1; - } - return 0; + ckdb::ksAppendKey (config, ckdb::keyNew ("user/b", KEY_VALUE, "test", KEY_END)); + return 1; } TEST (BackendBuilder, checkconfOkChanged) From 742cf1a2bf00b1b32ba6617d540856ce22b1023a Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Sat, 21 May 2016 13:15:18 +0200 Subject: [PATCH 20/23] document the meaning of MountBackendBuilder::mountConf --- src/libs/tools/include/backendbuilder.hpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/libs/tools/include/backendbuilder.hpp b/src/libs/tools/include/backendbuilder.hpp index 2d3e6081a12..61bee29f9f2 100644 --- a/src/libs/tools/include/backendbuilder.hpp +++ b/src/libs/tools/include/backendbuilder.hpp @@ -169,7 +169,13 @@ class MountBackendBuilder : public MountBackendInterface, public BackendBuilder { Key mountpoint; KeySet backendConf; + + /** + * Contains the keys of system/elektra/mountpoints. + * It is needed to detect if a mountpoint already exists. + */ KeySet mountConf; + std::string configfile; public: From 5d92d69b85c30589e3a84b11f378a019cbe5ffb8 Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Sun, 22 May 2016 19:40:17 +0200 Subject: [PATCH 21/23] move backendConfig up to BackendBuilder --- src/libs/tools/include/backendbuilder.hpp | 11 ++++++----- src/libs/tools/src/backendbuilder.cpp | 12 +++++++++++- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/libs/tools/include/backendbuilder.hpp b/src/libs/tools/include/backendbuilder.hpp index 61bee29f9f2..6fc3e441223 100644 --- a/src/libs/tools/include/backendbuilder.hpp +++ b/src/libs/tools/include/backendbuilder.hpp @@ -94,6 +94,9 @@ class BackendBuilder : public BackendInterface void removeProvided (std::vector & needs) const; void removeMetadata (std::set & needsMetadata) const; +protected: + KeySet backendConf; + public: explicit BackendBuilder (BackendBuilderInit const & bbi = BackendBuilderInit ()); @@ -147,6 +150,9 @@ class BackendBuilder : public BackendInterface void resolveRecommends (); void fillPlugins (BackendInterface & b) const; + + void setBackendConfig (KeySet const & ks); + KeySet getBackendConfig (); }; /** @@ -168,7 +174,6 @@ class GlobalPluginsBuilder : public BackendBuilder class MountBackendBuilder : public MountBackendInterface, public BackendBuilder { Key mountpoint; - KeySet backendConf; /** * Contains the keys of system/elektra/mountpoints. @@ -190,10 +195,6 @@ class MountBackendBuilder : public MountBackendInterface, public BackendBuilder std::string getMountpoint () const; void setBackendConfig (KeySet const & ks); - KeySet getBackendConfig () - { - return backendConf; - } void useConfigFile (std::string file); std::string getConfigFile () const; diff --git a/src/libs/tools/src/backendbuilder.cpp b/src/libs/tools/src/backendbuilder.cpp index a9d121ac1aa..91520f61bf4 100644 --- a/src/libs/tools/src/backendbuilder.cpp +++ b/src/libs/tools/src/backendbuilder.cpp @@ -459,6 +459,16 @@ void BackendBuilder::fillPlugins (BackendInterface & b) const } } +void BackendBuilder::setBackendConfig (KeySet const & ks) +{ + backendConf = ks; +} + +KeySet BackendBuilder::getBackendConfig () +{ + return backendConf; +} + GlobalPluginsBuilder::GlobalPluginsBuilder (BackendBuilderInit const & bbi) : BackendBuilder (bbi) { } @@ -520,7 +530,7 @@ std::string MountBackendBuilder::getMountpoint () const void MountBackendBuilder::setBackendConfig (KeySet const & ks) { - backendConf = ks; + BackendBuilder::setBackendConfig (ks); } void MountBackendBuilder::useConfigFile (std::string file) From a904008970d3bebdc5b4bea2730c1540ce7bde98 Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Sun, 22 May 2016 20:48:05 +0200 Subject: [PATCH 22/23] pass backend config to checkconf --- src/libs/tools/src/backendbuilder.cpp | 13 +++++ .../tools/tests/testtool_backendbuilder.cpp | 52 ++++++++++++++++++- 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/libs/tools/src/backendbuilder.cpp b/src/libs/tools/src/backendbuilder.cpp index 91520f61bf4..edfd5e7444d 100644 --- a/src/libs/tools/src/backendbuilder.cpp +++ b/src/libs/tools/src/backendbuilder.cpp @@ -421,7 +421,12 @@ void BackendBuilder::addPlugin (PluginSpec const & plugin) if (checkConfFunction) { ckdb::Key * errorKey = ckdb::keyNew (0); + + // merge plugin config and backend config together ckdb::KeySet * pluginConfig = newPlugin.getConfig ().dup (); + ckdb::ksAppend (pluginConfig, backendConf.getKeySet ()); + + // call the plugin's checkconf function int checkResult = checkConfFunction (errorKey, pluginConfig); if (checkResult == -1) { @@ -430,8 +435,16 @@ void BackendBuilder::addPlugin (PluginSpec const & plugin) } else if (checkResult == 1) { + // separate plugin config from the backend config + ckdb::Key * backendParent = ckdb::keyNew ("system/", KEY_END); + ckdb::KeySet * newBackendConfig = ckdb::ksCut (pluginConfig, backendParent); + + // take over the new configuration KeySet modifiedPluginConfig = KeySet (pluginConfig); + KeySet modifiedBackendConfig = KeySet (newBackendConfig); + newPlugin.setConfig (modifiedPluginConfig); + setBackendConfig (modifiedBackendConfig); } else { diff --git a/src/libs/tools/tests/testtool_backendbuilder.cpp b/src/libs/tools/tests/testtool_backendbuilder.cpp index cb8998d37f9..67f76cdaf28 100644 --- a/src/libs/tools/tests/testtool_backendbuilder.cpp +++ b/src/libs/tools/tests/testtool_backendbuilder.cpp @@ -618,7 +618,7 @@ static int checkconfDelete (ckdb::Key * errorKey, ckdb::KeySet * config) return 1; } -TEST (BackendBuilder, checkconfOkRemoved) +TEST (BackendBuilder, checkconfOkRemovedPluginConfig) { using namespace kdb; using namespace kdb::tools; @@ -635,7 +635,55 @@ TEST (BackendBuilder, checkconfOkRemoved) pluginConfig.append (a); spec.appendConfig (pluginConfig); bb.addPlugin (spec); - // we expect b to be added now + // we expect a to be removed now spec = *bb.begin (); EXPECT_THROW (spec.getConfig ().get ("user/a"), KeyNotFoundException); } + +TEST (BackendBuilder, checkconfOkRemovedBackendConfig) +{ + using namespace kdb; + using namespace kdb::tools; + std::shared_ptr mpd = std::make_shared (); + mpd->data[PluginSpec ("checkconf1")]["provides"] = "test123"; + mpd->setCheckconfFunction (checkconfDelete); + BackendBuilderInit bbi (mpd); + BackendBuilder bb (bbi); + PluginSpec spec ("checkconf1"); + KeySet pluginConfig; + spec.appendConfig (pluginConfig); + KeySet backendConfig; + Key b; + b.setName ("system/b"); + b.setString ("xyz"); + backendConfig.append (b); + bb.setBackendConfig (backendConfig); + bb.addPlugin (spec); + // we expect b to be removed now + spec = *bb.begin (); + EXPECT_THROW (bb.getBackendConfig ().get ("system/b"), KeyNotFoundException); +} + +static int checkconfAppendBackendConf (ckdb::Key * errorKey, ckdb::KeySet * config) +{ + ckdb::ksAppendKey (config, ckdb::keyNew ("system/a", KEY_VALUE, "abc", KEY_END)); + return 1; +} + +TEST (BackendBuilder, checkconfOkAppendBackendConfig) +{ + using namespace kdb; + using namespace kdb::tools; + std::shared_ptr mpd = std::make_shared (); + mpd->data[PluginSpec ("checkconf1")]["provides"] = "test123"; + mpd->setCheckconfFunction (checkconfAppendBackendConf); + BackendBuilderInit bbi (mpd); + BackendBuilder bb (bbi); + PluginSpec spec ("checkconf1"); + KeySet pluginConfig; + spec.appendConfig (pluginConfig); + bb.addPlugin (spec); + // we expect b to be added now + spec = *bb.begin (); + EXPECT_EQ (bb.getBackendConfig ().get ("system/a"), "abc"); +} From aed705309287809445e47571b03f0e0fd59179fe Mon Sep 17 00:00:00 2001 From: Peter Nirschl Date: Sun, 22 May 2016 20:50:21 +0200 Subject: [PATCH 23/23] fix memory leak in backendbuilder.cpp --- src/libs/tools/src/backendbuilder.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libs/tools/src/backendbuilder.cpp b/src/libs/tools/src/backendbuilder.cpp index edfd5e7444d..5c974172e4c 100644 --- a/src/libs/tools/src/backendbuilder.cpp +++ b/src/libs/tools/src/backendbuilder.cpp @@ -445,6 +445,8 @@ void BackendBuilder::addPlugin (PluginSpec const & plugin) newPlugin.setConfig (modifiedPluginConfig); setBackendConfig (modifiedBackendConfig); + + ckdb::keyDel (backendParent); } else {