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

FOGL-7959 Improve error handling for plugin load failure #1142

Merged
merged 22 commits into from
Aug 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
3cc0062
Error reporting improvements
MarkRiddoch Jul 18, 2023
aaeb2f6
Improve JSON parse error reporting
MarkRiddoch Jul 27, 2023
39ebe8c
Merge branch 'develop' into FOGL-7959
MarkRiddoch Aug 10, 2023
3e21c8a
FOGL-7959 Capture some of the common reasons plugins fail to load
MarkRiddoch Aug 11, 2023
8c235ea
Merge branch 'develop' into FOGL-7959
MarkRiddoch Aug 11, 2023
39f8415
Fix typos
MarkRiddoch Aug 17, 2023
c27286c
Merge branch 'FOGL-7959' of https://github.com/fledge-iot/fledge into…
MarkRiddoch Aug 17, 2023
b6a7fba
Merge branch 'develop' into FOGL-7959
MarkRiddoch Aug 17, 2023
c68610d
Removing using namespace from string_utils header file
MarkRiddoch Aug 17, 2023
6a7cd41
Removing using namespace from string_utils header file
MarkRiddoch Aug 17, 2023
5d2ba61
Update test
MarkRiddoch Aug 17, 2023
c4dc37d
Update tests
MarkRiddoch Aug 18, 2023
c05ebf7
Fix unit tests
MarkRiddoch Aug 18, 2023
b1893d3
Merge branch 'develop' into FOGL-7959
MarkRiddoch Aug 18, 2023
cc0e32b
Test checkin to add loggign to PR unit tester
MarkRiddoch Aug 18, 2023
090f5a2
Merge branch 'FOGL-7959' of https://github.com/fledge-iot/fledge into…
MarkRiddoch Aug 18, 2023
8dc37aa
Using -j1 in RunAllTests
MarkRiddoch Aug 18, 2023
d8a30b0
Using -j1 in RunAllTests
MarkRiddoch Aug 18, 2023
a90160c
Include strign_utils in postgres test
MarkRiddoch Aug 18, 2023
ed9fa59
Use entire common library for Postgres test
MarkRiddoch Aug 18, 2023
6e8754d
Add library location
MarkRiddoch Aug 18, 2023
93cd157
Add extra library requirements
MarkRiddoch Aug 18, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions C/common/config_category.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <stdlib.h>
#include <logger.h>
#include <stdexcept>
#include <string_utils.h>


using namespace std;
Expand All @@ -44,8 +45,8 @@ ConfigCategories::ConfigCategories(const std::string& json)
doc.Parse(json.c_str());
if (doc.HasParseError())
{
Logger::getLogger()->error("Configuration parse error in %s: %s at %d", json.c_str(),
GetParseError_En(doc.GetParseError()), (unsigned)doc.GetErrorOffset());
Logger::getLogger()->error("Configuration parse error in %s: %s at %d, '%s'", json.c_str(),
GetParseError_En(doc.GetParseError()), (unsigned)doc.GetErrorOffset(), StringAround(json, (unsigned)doc.GetErrorOffset()).c_str());
throw new ConfigMalformed();
}
if (doc.HasMember("categories"))
Expand Down Expand Up @@ -140,9 +141,10 @@ ConfigCategory::ConfigCategory(const string& name, const string& json) : m_name(
doc.Parse(json.c_str());
if (doc.HasParseError())
{
Logger::getLogger()->error("Configuration parse error in category '%s', %s: %s at %d",
Logger::getLogger()->error("Configuration parse error in category '%s', %s: %s at %d, '%s'",
name.c_str(), json.c_str(),
GetParseError_En(doc.GetParseError()), (unsigned)doc.GetErrorOffset());
GetParseError_En(doc.GetParseError()), (unsigned)doc.GetErrorOffset(),
StringAround(json, (unsigned)doc.GetErrorOffset()));
throw new ConfigMalformed();
}

Expand Down Expand Up @@ -1117,10 +1119,14 @@ ConfigCategory::CategoryItem::CategoryItem(const string& name,
check.Parse(m_value.c_str());
if (check.HasParseError())
{
Logger::getLogger()->error("The JSON configuration item %s has a parse error: %s",
m_name.c_str(), GetParseError_En(check.GetParseError()));
throw new runtime_error(GetParseError_En(check.GetParseError()));
}
if (!check.IsObject())
{
Logger::getLogger()->error("The JSON configuration item %s is not a valid JSON objects",
m_name.c_str());
throw new runtime_error("'value' JSON property is not an object");
}
}
Expand Down Expand Up @@ -1221,10 +1227,14 @@ ConfigCategory::CategoryItem::CategoryItem(const string& name,
check.Parse(m_default.c_str());
if (check.HasParseError())
{
Logger::getLogger()->error("The JSON configuration item %s has a parse error in the default value: %s",
m_name.c_str(), GetParseError_En(check.GetParseError()));
throw new runtime_error(GetParseError_En(check.GetParseError()));
}
if (!check.IsObject())
{
Logger::getLogger()->error("The JSON configuration item %s default is not a valid JSON object",
m_name.c_str());
throw new runtime_error("'default' JSON property is not an object");
}
}
Expand Down
18 changes: 10 additions & 8 deletions C/common/include/string_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include <sstream>
#include <iomanip>

using namespace std;

void StringReplace(std::string& StringToManage,
const std::string& StringToSearch,
Expand All @@ -24,25 +23,28 @@ void StringReplaceAll(std::string& StringToManage,
const std::string& StringToSearch,
const std::string& StringReplacement);

string StringSlashFix(const string& stringToFix);
std::string StringSlashFix(const std::string& stringToFix);
std::string evaluateParentPath(const std::string& path, char separator);
std::string extractLastLevel(const std::string& path, char separator);

void StringStripCRLF(std::string& StringToManage);
string StringStripWhiteSpacesAll(const std::string& original);
string StringStripWhiteSpacesExtra(const std::string& original);
std::string StringStripWhiteSpacesAll(const std::string& original);
std::string StringStripWhiteSpacesExtra(const std::string& original);
void StringStripQuotes(std::string& StringToManage);

string urlEncode(const string& s);
string urlDecode(const string& s);
void StringEscapeQuotes(string& s);
std::string urlEncode(const std::string& s);
std::string urlDecode(const std::string& s);
void StringEscapeQuotes(std::string& s);

char *trim(char *str);
std::string StringLTrim(const std::string& str);
std::string StringRTrim(const std::string& str);
std::string StringTrim(const std::string& str);

bool IsRegex(const string &str);
bool IsRegex(const std::string &str);

std::string StringAround(const std::string& str, unsigned int pos,
unsigned int after = 30, unsigned int before = 10);


#endif
17 changes: 17 additions & 0 deletions C/common/string_utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -445,3 +445,20 @@ bool IsRegex(const string &str) {

return (nChar != 0);
}

/**
* Return a new string that extracts from the passed in string either side
* of a position within the string.
*
* @param str The string to return a portion of
* @param pos The position around which to extract a portion
* @param after The number of characters after the position to return, defaults to 30 if omitted
* @param before The number of characters before the position to return, defaults to 10
*/
std::string StringAround(const std::string& str, unsigned int pos,
unsigned int after, unsigned int before)
{
size_t start = pos > before ? (pos - before) : 0;
size_t len = before + after;
return str.substr(start, len);
}
11 changes: 6 additions & 5 deletions C/plugins/utils/get_plugin_info.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,22 +41,23 @@ int main(int argc, char *argv[])
exit(1);
}

openlog("Fledge PluginInfo", LOG_PID|LOG_CONS, LOG_USER);
ashish-jabble marked this conversation as resolved.
Show resolved Hide resolved
setlogmask(LOG_UPTO(LOG_WARNING));

if (access(argv[1], F_OK|R_OK) != 0)
{
fprintf(stderr, "Unable to access library file '%s', exiting...\n", argv[1]);
syslog(LOG_ERR, "Unable to access library file '%s', exiting...\n", argv[1]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be retain fprintf to stderr also (in addition to syslog) so that errors can be seen on console also.

exit(2);
}

openlog(argv[0], LOG_PID|LOG_CONS, LOG_USER);
setlogmask(LOG_UPTO(LOG_WARNING));

if ((hndl = dlopen(argv[1], RTLD_GLOBAL|RTLD_LAZY)) != NULL)
{
func_t infoEntry = (func_t)dlsym(hndl, routine);
if (infoEntry == NULL)
{
// Unable to find plugin_info entry point
fprintf(stderr, "Plugin library %s does not support %s function : %s\n", argv[1], routine, dlerror());
syslog(LOG_ERR, "Plugin library %s does not support %s function : %s\n", argv[1], routine, dlerror());
dlclose(hndl);
closelog();
exit(3);
Expand All @@ -66,7 +67,7 @@ int main(int argc, char *argv[])
}
else
{
fprintf(stderr, "dlopen failed: %s\n", dlerror());
syslog(LOG_ERR, "dlopen failed: %s\n", dlerror());
}
closelog();

Expand Down
12 changes: 12 additions & 0 deletions C/services/common/include/binary_plugin_handle.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,26 @@ class BinaryPluginHandle : public PluginHandle
public:
// for the Storage plugin
BinaryPluginHandle(const char *name, const char *path, tPluginType type) {
dlerror(); // Clear the existing error
handle = dlopen(path, RTLD_LAZY);
if (!handle)
{
Logger::getLogger()->error("Unable to load storage plugin %s, %s",
AmandeepArora marked this conversation as resolved.
Show resolved Hide resolved
name, dlerror());
}

Logger::getLogger()->debug("%s - storage plugin / RTLD_LAZY - name :%s: path :%s:", __FUNCTION__, name, path);
}

// for all the others plugins
BinaryPluginHandle(const char *name, const char *path) {
dlerror(); // Clear the existing error
handle = dlopen(path, RTLD_LAZY|RTLD_GLOBAL);
if (!handle)
{
Logger::getLogger()->error("Unable to load plugin %s, %s",
name, dlerror());
}

Logger::getLogger()->debug("%s - other plugin / RTLD_LAZY|RTLD_GLOBAL - name :%s: path :%s:", __FUNCTION__, name, path);
}
Expand Down
3 changes: 2 additions & 1 deletion C/services/common/include/plugin_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class PluginManager {

private:
PluginManager();
std::string findPlugin(std::string name, std::string _type, std::string _plugin_path, PLUGIN_TYPE type);

private:
std::list<PLUGIN_HANDLE> plugins;
Expand All @@ -69,7 +70,7 @@ class PluginManager {
std::map<PLUGIN_HANDLE, PluginHandle*>
pluginHandleMap;
Logger* logger;
tPluginType m_pluginType;
tPluginType m_pluginType;
};

#endif
15 changes: 11 additions & 4 deletions C/services/common/plugin_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "rapidjson/error/en.h"
#include <algorithm>
#include <config_category.h>
#include <string_utils.h>

using namespace std;
using namespace rapidjson;
Expand Down Expand Up @@ -77,15 +78,19 @@ void updateJsonPluginConfig(PLUGIN_INFORMATION *info, string json_plugin_name, s
doc.Parse(json_plugin_defaults.c_str());
if (doc.HasParseError())
{
logger->error("doc JSON parsing failed");
logger->error("Parse error in plugin '%s' defaults: %s at %d '%s'", json_plugin_name.c_str(),
GetParseError_En(doc.GetParseError()), (unsigned)doc.GetErrorOffset(),
StringAround(json_plugin_defaults, (unsigned)doc.GetErrorOffset()));
return;
}

Document docBase;
docBase.Parse(info->config);
if (docBase.HasParseError())
{
logger->error("docBase JSON parsing failed");
logger->error("Parse error in plugin '%s' information defaults: %s at %d '%s'", json_plugin_name.c_str(),
GetParseError_En(doc.GetParseError()), (unsigned)doc.GetErrorOffset(),
StringAround(info->config, (unsigned)doc.GetErrorOffset()));
return;
}

Expand Down Expand Up @@ -176,7 +181,9 @@ void updateJsonPluginConfig(PLUGIN_INFORMATION *info, string json_plugin_name, s
doc2.Parse(info->config);
if (doc2.HasParseError())
{
logger->error("doc2 JSON parsing failed");
logger->error("Parse error in information returned from plugin: %s at %d '%s'",
GetParseError_En(doc2.GetParseError()), (unsigned)doc2.GetErrorOffset(),
StringAround(info->config, (unsigned)doc2.GetErrorOffset()));
}
if (doc2.HasMember("plugin"))
{
Expand Down Expand Up @@ -208,7 +215,7 @@ void updateJsonPluginConfig(PLUGIN_INFORMATION *info, string json_plugin_name, s
* @param type The plugin type
* @return string The absolute path of plugin
*/
string findPlugin(string name, string _type, string _plugin_path, PLUGIN_TYPE type)
string PluginManager::findPlugin(string name, string _type, string _plugin_path, PLUGIN_TYPE type)
{
if (type != BINARY_PLUGIN && type != PYTHON_PLUGIN && type != JSON_PLUGIN)
{
Expand Down
3 changes: 3 additions & 0 deletions python/fledge/services/core/api/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ def get_plugin_info(name, dir):
out, err = p.communicate()
res = out.decode("utf-8")
jdoc = json.loads(res)
except json.decoder.JSONDecodeError as err:
_logger.error("Failed to parse JSON data returned from the plugin information of {}, {} line {} column {}".format(name, err.msg, err.lineno, err.colno))
Copy link
Member

@ashish-jabble ashish-jabble Aug 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No harm to keep specific exception type catch block here. But as we already have multi line stack trace support available at python side; So these type of errors will catch in general exception block and definitely it points to exact problem cause error.

Use multi line stack trace logger style.

_logger.error(err, "Failed to parse JSON data returned from the plugin information of {}.".format(name))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The concept here is to give the user a very clear error message they can pick up on rather than expect them to look at something more general and work out what went wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Previously, if we have some invalid JSON for a plugin it raises below
ERROR: logger: fledge.services.core.api.utils: json.decoder.JSONDecodeError: Expecting ',' delimiter: line 1 column 2870 (char 2869)

And with the given change
ERROR: logger: fledge.services.core.api.utils: Failed to parse JSON data returned from the plugin information of PLUGIN_NAME, Expecting ',' delimiter line 1 column 2870

So the trailing message still seems to be more developer related logger.

return {}
except (OSError, ValueError) as err:
_logger.error(err, "{} C plugin get info failed.".format(name))
return {}
Expand Down
22 changes: 22 additions & 0 deletions tests/unit/C/common/test_config_category.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,23 @@ const char *optionals =

const char *json_quotedSpecial = R"QS({ "key" : "test \"a\"", "description" : "Test \"description\"", "value" : {"description" : { "description" : "The description of this \"Fledge\" service", "type" : "string", "value" : "The \"Fledge\" admini\\strative API", "default" : "The \"Fledge\" administra\tive API" }, "name" : { "description" : "The name of this \"Fledge\" service", "type" : "string", "value" : "\"Fledge\"", "default" : "\"Fledge\"" }, "complex" : { "description" : "A JSON configuration parameter", "type" : "json", "value" : {"first":"Fledge","second":"json"}, "default" : {"first":"Fledge","second":"json"} }} })QS";

const char *json_parse_error = "{\"description\": {"
"\"value\": \"The Fledge administrative API\","
"\"type\": \"string\","
"\"default\": \"The Fledge administrative API\","
"\"description\": \"The description of this Fledge service\"},"
"\"name\": {"
"\"value\": \"Fledge\","
"\"type\": \"string\","
"\"default\": \"Fledge\","
"\"description\": \"The name of this Fledge service\"},"
"error : here,"
"\"complex\": {" \
"\"value\": { \"first\" : \"Fledge\", \"second\" : \"json\" },"
"\"type\": \"json\","
"\"default\": {\"first\" : \"Fledge\", \"second\" : \"json\" },"
"\"description\": \"A JSON configuration parameter\"}}";

TEST(CategoriesTest, Count)
{
ConfigCategories confCategories(categories);
Expand Down Expand Up @@ -654,3 +671,8 @@ TEST(CategoryTestQuoted, toJSONQuotedSpecial)
confCategory.setDescription("Test \"description\"");
ASSERT_EQ(0, confCategory.toJSON().compare(json_quotedSpecial));
}

TEST(Categorytest, parseError)
{
EXPECT_THROW(ConfigCategory("parseTest", json_parse_error), ConfigMalformed*);
}
13 changes: 12 additions & 1 deletion tests/unit/C/common/test_string_utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -215,4 +215,15 @@ TEST(TestIsRegex, AllCases)




TEST(TestAround, Extract)
{
string longString("not shownpreamble123This part is after the location");
string s = StringAround(longString, 19);
EXPECT_STREQ(s.c_str(), "preamble123This part is after the locati");
s = StringAround(longString, 19, 10);
EXPECT_STREQ(s.c_str(), "preamble123This part");
s = StringAround(longString, 19, 10, 5);
EXPECT_STREQ(s.c_str(), "ble123This part");
s = StringAround(longString, 5);
EXPECT_STREQ(s.c_str(), "not shownpreamble123This part is after t");
}
4 changes: 2 additions & 2 deletions tests/unit/C/scripts/RunAllTests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ if [ ! -d results ] ; then
fi

if [ -f "./CMakeLists.txt" ] ; then
echo -n "Compiling libraries..."
(rm -rf build && mkdir -p build && cd build && cmake -DCMAKE_BUILD_TYPE=Debug .. && make ${jobs} && cd ..) > /dev/null
echo "Compiling libraries..."
(rm -rf build && mkdir -p build && cd build && cmake -DCMAKE_BUILD_TYPE=Debug .. && make ${jobs} && cd ..)
echo "done"
fi

Expand Down
Loading