From c1c4c65a936aa5fd09f935b50112ee0c107d77b5 Mon Sep 17 00:00:00 2001 From: Bartosz Podrygajlo Date: Mon, 19 May 2025 22:01:46 +0200 Subject: [PATCH 1/2] Unify setting default parameters in config libraries Add common function config_common_getdefault --- common/config/config_common.c | 60 +++++++++++++++++++ common/config/config_common.h | 1 + common/config/libconfig/config_libconfig.c | 14 ++--- common/config/yaml/config_yaml.cpp | 70 +--------------------- 4 files changed, 70 insertions(+), 75 deletions(-) diff --git a/common/config/config_common.c b/common/config/config_common.c index 2bca71d912..c617c426d7 100644 --- a/common/config/config_common.c +++ b/common/config/config_common.c @@ -308,3 +308,63 @@ void config_assign_int(configmodule_interface_t *cfg, paramdef_t *cfgoptions, ch break; } } + +/** + * @brief Assigns the default value to a configuration parameter if available. + * + * This function checks the type of the parameter and assigns its default value + * if it is defined. It handles various parameter types, including strings, integers, + * arrays, doubles, IPv4 addresses, and lists. If the parameter is marked with + * PARAMFLAG_DONOTREAD, it is ignored. + * + * @param[in] cfg Pointer to the configuration module interface. + * @param[in] cfgoption Pointer to the parameter definition structure. + * @param[in] prefix Optional prefix for the parameter name (can be NULL). + * @return 1 if the default value was set, 0 if ignored, or -1 on error/unsupported type. + */ +int config_common_getdefault(configmodule_interface_t *cfg, paramdef_t *cfgoption, char *prefix) +{ + if( (cfgoption->paramflags & PARAMFLAG_DONOTREAD) != 0) { + return 0; + } + + switch(cfgoption->type) { + case TYPE_STRING: + return config_setdefault_string(cfg, cfgoption, prefix); + + case TYPE_STRINGLIST: + return config_setdefault_stringlist(cfg, cfgoption, prefix); + + case TYPE_UINT8: + case TYPE_INT8: + case TYPE_UINT16: + case TYPE_INT16: + case TYPE_UINT32: + case TYPE_INT32: + case TYPE_MASK: + return config_setdefault_int(cfg, cfgoption, prefix); + + case TYPE_UINT64: + case TYPE_INT64: + return config_setdefault_int64(cfg, cfgoption, prefix); + + case TYPE_UINTARRAY: + case TYPE_INTARRAY: + return config_setdefault_intlist(cfg, cfgoption, prefix); + + case TYPE_DOUBLE: + return config_setdefault_double(cfg, cfgoption, prefix); + + case TYPE_IPV4ADDR: + return config_setdefault_ipv4addr(cfg, cfgoption, prefix); + + case TYPE_LIST: + // No default value for list type + fprintf(stderr,"[CONFIG] %s.%s TYPE_LIST has no default value\n", prefix, cfgoption->optname); + return -1; + + default: + fprintf(stderr,"[CONFIG] %s.%s type %i not supported\n", prefix, cfgoption->optname, cfgoption->type); + return -1; + } /* switch on param type */ +} diff --git a/common/config/config_common.h b/common/config/config_common.h index 8e595e4556..00211c9519 100644 --- a/common/config/config_common.h +++ b/common/config/config_common.h @@ -50,6 +50,7 @@ int config_setdefault_double(configmodule_interface_t *cfg, paramdef_t *cfgoptio int config_setdefault_ipv4addr(configmodule_interface_t *cfg, paramdef_t *cfgoptions, char *prefix); void *config_allocate_new(configmodule_interface_t *cfg, int sz, bool autoFree); void config_assign_int(configmodule_interface_t *cfg, paramdef_t *cfgoptions, char *fullname, int val); +int config_common_getdefault(configmodule_interface_t *cfg, paramdef_t *cfgoption, char *prefix); #ifdef __cplusplus } diff --git a/common/config/libconfig/config_libconfig.c b/common/config/libconfig/config_libconfig.c index 0934024391..0770fb2af8 100644 --- a/common/config/libconfig/config_libconfig.c +++ b/common/config/libconfig/config_libconfig.c @@ -356,7 +356,7 @@ int config_libconfig_get(configmodule_interface_t *cfg, paramdef_t *cfgoptions, snprintf( *cfgoptions[i].strptr , cfgoptions[i].numelt, "%s", str); printf_params(cfg, "[LIBCONFIG] %s: \"%s\"\n", cfgpath, *cfgoptions[i].strptr); } else { - defval = config_setdefault_string(cfg, &cfgoptions[i], prefix); + defval = config_common_getdefault(cfg, &cfgoptions[i], prefix); } break; @@ -367,7 +367,7 @@ int config_libconfig_get(configmodule_interface_t *cfg, paramdef_t *cfgoptions, if ( setting != NULL) { read_strlist(cfg, &cfgoptions[i], setting, cfgpath); } else { - defval = config_setdefault_stringlist(cfg, &(cfgoptions[i]), prefix); + defval = config_common_getdefault(cfg, &(cfgoptions[i]), prefix); } break; @@ -383,7 +383,7 @@ int config_libconfig_get(configmodule_interface_t *cfg, paramdef_t *cfgoptions, config_check_valptr(cfg, &(cfgoptions[i]), sizeof(*cfgoptions[i].iptr), 1); config_assign_int(cfg, &(cfgoptions[i]), cfgpath, u); } else { - defval = config_setdefault_int(cfg, &(cfgoptions[i]), prefix); + defval = config_common_getdefault(cfg, &(cfgoptions[i]), prefix); } break; @@ -401,7 +401,7 @@ int config_libconfig_get(configmodule_interface_t *cfg, paramdef_t *cfgoptions, printf_params(cfg, "[LIBCONFIG] %s: %ld\n", cfgpath, *cfgoptions[i].i64ptr); } } else { - defval = config_setdefault_int64(cfg, &(cfgoptions[i]), prefix); + defval = config_common_getdefault(cfg, &(cfgoptions[i]), prefix); } break; @@ -413,7 +413,7 @@ int config_libconfig_get(configmodule_interface_t *cfg, paramdef_t *cfgoptions, if ( setting != NULL) { read_intarray(cfg, &cfgoptions[i], setting, cfgpath); } else { - defval = config_setdefault_intlist(cfg, &(cfgoptions[i]), prefix); + defval = config_common_getdefault(cfg, &(cfgoptions[i]), prefix); } break; @@ -424,14 +424,14 @@ int config_libconfig_get(configmodule_interface_t *cfg, paramdef_t *cfgoptions, *cfgoptions[i].dblptr = dbl; printf_params(cfg, "[LIBCONFIG] %s: %lf\n", cfgpath, *(cfgoptions[i].dblptr)); } else { - defval = config_setdefault_double(cfg, &(cfgoptions[i]), prefix); + defval = config_common_getdefault(cfg, &(cfgoptions[i]), prefix); } break; case TYPE_IPV4ADDR: if ( !config_lookup_string(&(libconfig_privdata.cfg),cfgpath, (const char **)&str)) { - defval = config_setdefault_ipv4addr(cfg, &(cfgoptions[i]), prefix); + defval = config_common_getdefault(cfg, &(cfgoptions[i]), prefix); } else { int rst = config_assign_ipv4addr(cfg, cfgoptions, str); diff --git a/common/config/yaml/config_yaml.cpp b/common/config/yaml/config_yaml.cpp index 138163ce0e..0da2d4bf56 100644 --- a/common/config/yaml/config_yaml.cpp +++ b/common/config/yaml/config_yaml.cpp @@ -25,8 +25,7 @@ extern "C" { #include "common/config/config_userapi.h" void *config_allocate_new(configmodule_interface_t *cfg, int sz, bool autoFree); void config_check_valptr(configmodule_interface_t *cfg, paramdef_t *cfgoptions, int elt_sz, int nb_elt); -#include "../config_userapi.h" -#include "../config_common.h" +int config_common_getdefault(configmodule_interface_t *cfg, paramdef_t *cfgoption, char *prefix); } #include #include @@ -44,71 +43,6 @@ class YamlConfig { } }; -void SetDefault(configmodule_interface_t *cfg, paramdef_t *param) -{ - switch (param->type) { - case TYPE_INT: - *param->iptr = param->defintval; - break; - case TYPE_UINT: - *param->uptr = param->defuintval; - break; - case TYPE_STRING: - if (param->defstrval != nullptr) { - if (param->numelt == 0) { - config_check_valptr(cfg, param, 1, strlen(param->defstrval) + 1); - } - sprintf(*param->strptr, "%s", param->defstrval); - } - break; - case TYPE_INT8: - *param->i8ptr = param->defintval; - break; - case TYPE_UINT8: - *param->u8ptr = param->defuintval; - break; - case TYPE_INT16: - *param->i16ptr = param->defintval; - break; - case TYPE_UINT16: - *param->u16ptr = param->defuintval; - break; - case TYPE_INT64: - *param->i64ptr = param->defint64val; - break; - case TYPE_UINT64: - *param->u64ptr = param->defint64val; - break; - case TYPE_DOUBLE: - *param->dblptr = param->defdblval; - break; - case TYPE_MASK: - *param->uptr = param->defuintval; - break; - case TYPE_STRINGLIST: - if (param->defstrlistval != nullptr) { - param->strlistptr = param->defstrlistval; - } - break; - case TYPE_INTARRAY: - if (param->defintarrayval) { - param->iptr = param->defintarrayval; - } - break; - case TYPE_UINTARRAY: - if (param->defintarrayval) { - param->uptr = (uint32_t *)param->defintarrayval; - } - break; - case TYPE_IPV4ADDR: - config_setdefault_ipv4addr(cfg, param, param->defstrval); - break; - default: - AssertFatal(false, "Unhandled type %d", param->type); - } - param->paramflags |= PARAMFLAG_PARAMSETDEF; -} - void SetNonDefault(configmodule_interface_t *cfg, const YAML::Node &node, paramdef_t *param) { auto optname = std::string(param->optname); @@ -213,7 +147,7 @@ void GetParams(configmodule_interface_t *cfg, const YAML::Node &node, paramdef_t if (node && node[std::string(params[i].optname)]) { SetNonDefault(cfg, node, ¶ms[i]); } else { - SetDefault(cfg, ¶ms[i]); + config_common_getdefault(cfg, ¶ms[i], nullptr); } } } From 5cb1113674c51128b8bc822ebb2731178b9ed044 Mon Sep 17 00:00:00 2001 From: Bartosz Podrygajlo Date: Tue, 3 Jun 2025 08:54:45 +0200 Subject: [PATCH 2/2] Handle YAML Exceptions --- common/config/yaml/config_yaml.cpp | 54 ++++++++++++++----- common/config/yaml/tests/CMakeLists.txt | 2 + common/config/yaml/tests/malformed.yaml | 13 +++++ .../yaml/tests/test_read_str_as_int.yaml | 2 + common/config/yaml/tests/test_yaml_config.cpp | 42 ++++++++++++++- 5 files changed, 97 insertions(+), 16 deletions(-) create mode 100644 common/config/yaml/tests/malformed.yaml create mode 100644 common/config/yaml/tests/test_read_str_as_int.yaml diff --git a/common/config/yaml/config_yaml.cpp b/common/config/yaml/config_yaml.cpp index 0da2d4bf56..0b9139c407 100644 --- a/common/config/yaml/config_yaml.cpp +++ b/common/config/yaml/config_yaml.cpp @@ -141,14 +141,12 @@ void SetNonDefault(configmodule_interface_t *cfg, const YAML::Node &node, paramd param->paramflags |= PARAMFLAG_PARAMSET; } -void GetParams(configmodule_interface_t *cfg, const YAML::Node &node, paramdef_t *params, int num_params) +void GetParam(configmodule_interface_t *cfg, const YAML::Node &node, paramdef_t *param) { - for (auto i = 0; i < num_params; i++) { - if (node && node[std::string(params[i].optname)]) { - SetNonDefault(cfg, node, ¶ms[i]); - } else { - config_common_getdefault(cfg, ¶ms[i], nullptr); - } + if (node && node[std::string(param->optname)]) { + SetNonDefault(cfg, node, param); + } else { + config_common_getdefault(cfg, param, nullptr); } } @@ -163,7 +161,15 @@ extern "C" int config_yaml_init(configmodule_interface_t *cfg) pthread_mutex_init(&cfg->memBlocks_mutex, NULL); memset(cfg->oneBlock, 0, sizeof(cfg->oneBlock)); - config_yaml::config = new config_yaml::YamlConfig(std::string(cfgP[0])); + try { + config_yaml::config = new config_yaml::YamlConfig(std::string(cfgP[0])); + } catch (const YAML::BadFile &e) { + fprintf(stderr, "[CONFIG] Error loading YAML file '%s': %s\n", cfgP[0], e.what()); + return -1; + } catch (const YAML::Exception &e) { + fprintf(stderr, "[CONFIG] Error parsing YAML file '%s': %s\n", cfgP[0], e.what()); + return -1; + } return 0; } @@ -183,14 +189,14 @@ YAML::Node find_node(const std::string &prefix, YAML::Node node) if (key.at(0) == '[' && key.back() == ']') { // The key is an index to a sequence if (!current.IsSequence()) { - throw std::invalid_argument("Incorrect yaml file structure"); + throw std::invalid_argument("Expected a sequence at " + key); } int index = std::stoi(key.substr(1, key.size() - 2)); current = current[index]; continue; } if (!current.IsMap()) { - throw std::invalid_argument("Incorrect yaml file structure"); + throw std::invalid_argument("Expected a map at " + key); } if (!current[key]) { return config_yaml::invalid_node; @@ -206,8 +212,14 @@ extern "C" int config_yaml_get(configmodule_interface_t *cfg, paramdef_t *cfgopt if (prefix != nullptr) { p = std::string(prefix); } - auto node = find_node(p, YAML::Clone(config_yaml::config->config)); - if (node == config_yaml::invalid_node) { + YAML::Node node; + try { + node = find_node(p, YAML::Clone(config_yaml::config->config)); + if (node == config_yaml::invalid_node) { + return -1; + } + } catch (const std::invalid_argument &e) { + fprintf(stderr, "[CONFIG] Error finding node '%s': %s\n", p.c_str(), e.what()); return -1; } for (auto i = 0; i < numoptions; i++) { @@ -216,7 +228,14 @@ extern "C" int config_yaml_get(configmodule_interface_t *cfg, paramdef_t *cfgopt config_check_valptr(cfg, &cfgoptions[i], sizeof(void *), 1); } } - config_yaml::GetParams(cfg, node, cfgoptions, numoptions); + for (int i = 0; i < numoptions; i++) { + try { + config_yaml::GetParam(cfg, node, &cfgoptions[i]); + } catch (const YAML::Exception &e) { + fprintf(stderr, "[CONFIG] Config error: prefix: %s, setting: %s, error: %s\n", p.c_str(), cfgoptions[i].optname, e.what()); + return -1; + } + } return numoptions; } @@ -257,7 +276,14 @@ extern "C" int config_yaml_getlist(configmodule_interface_t *cfg, } } - config_yaml::GetParams(cfg, node[i], ParamList->paramarray[i], numparams); + for (int j = 0; j < numparams; j++) { + try { + config_yaml::GetParam(cfg, node[i], &ParamList->paramarray[i][j]); + } catch (const YAML::Exception &e) { + fprintf(stderr, "[CONFIG] Config error: prefix: %s, setting: %s, error: %s\n", path, ParamList->paramarray[i][j].optname, e.what()); + return -1; + } + } } } diff --git a/common/config/yaml/tests/CMakeLists.txt b/common/config/yaml/tests/CMakeLists.txt index d33fa57658..e3f1002d79 100644 --- a/common/config/yaml/tests/CMakeLists.txt +++ b/common/config/yaml/tests/CMakeLists.txt @@ -11,3 +11,5 @@ configure_file(test_list_of_mappings.yml test_list_of_mappings.yml COPYONLY) configure_file(test_int_array.yaml test_int_array.yaml COPYONLY) configure_file(test_read_mapping_as_list.yaml test_read_mapping_as_list.yaml COPYONLY) configure_file(test_ipv4.yaml test_ipv4.yaml COPYONLY) +configure_file(test_read_str_as_int.yaml test_read_str_as_int.yaml COPYONLY) +configure_file(malformed.yaml malformed.yaml COPYONLY) diff --git a/common/config/yaml/tests/malformed.yaml b/common/config/yaml/tests/malformed.yaml new file mode 100644 index 0000000000..c7f5fff55b --- /dev/null +++ b/common/config/yaml/tests/malformed.yaml @@ -0,0 +1,13 @@ +test: + - +bad_file: + - "testvalue1" + - "testvalue2" + - "testvalue3" + - "testvalue4" + - "testvalue5" + - "testvalue6" + - "testvalue7" + - "testvalue8" + - "testvalue9" + - "testvalue10" - yaml error diff --git a/common/config/yaml/tests/test_read_str_as_int.yaml b/common/config/yaml/tests/test_read_str_as_int.yaml new file mode 100644 index 0000000000..a055e855d5 --- /dev/null +++ b/common/config/yaml/tests/test_read_str_as_int.yaml @@ -0,0 +1,2 @@ +test: + var: "test_var" diff --git a/common/config/yaml/tests/test_yaml_config.cpp b/common/config/yaml/tests/test_yaml_config.cpp index 384a1f406c..814e3a3635 100644 --- a/common/config/yaml/tests/test_yaml_config.cpp +++ b/common/config/yaml/tests/test_yaml_config.cpp @@ -78,8 +78,9 @@ TEST(yaml_config, yaml_get_existing_values) { uint16_t value; p.type = TYPE_UINT16; p.u16ptr = &value; + p.paramflags = PARAMFLAG_MANDATORY; char prefix[] = "test"; - config_yaml_get(cfg, &p, 1, prefix); + EXPECT_EQ(config_yaml_get(cfg, &p, 1, prefix), 1); EXPECT_EQ(value, i); } @@ -94,7 +95,7 @@ TEST(yaml_config, yaml_get_non_existing_values) { cfg->cfgP[0] = strdup("test1.yaml"); EXPECT_EQ(config_yaml_init(cfg), 0); - // Testing paremters present in the test node + // Testing parameters not present in the test node paramdef_t p = {0}; for (auto i = 4; i <= 5; i++) { sprintf(p.optname, "%s%d", "value", i); @@ -421,6 +422,43 @@ TEST(yaml_config, test_read_ipv4) { free(p.defstrval); } +TEST(yaml_config, yaml_read_str_as_int) { + configmodule_interface_t *cfg = static_cast(calloc(1, sizeof(*cfg))); + cfg->cfgP[0] = strdup("test_read_str_as_int.yaml"); + EXPECT_EQ(config_yaml_init(cfg), 0); + + // Testing paremters present in the test node + paramdef_t p = {0}; + uint16_t value; + p.type = TYPE_UINT16; + p.u16ptr = &value; + p.paramflags = PARAMFLAG_MANDATORY; + char prefix[] = "test"; + sprintf(p.optname, "%s", "var"); + EXPECT_EQ(config_yaml_get(cfg, &p, 1, prefix), -1); + + + config_yaml_end(cfg); + free(cfg->cfgP[0]); + end_configmodule(cfg); +} + +TEST(yaml_config, yaml_open_non_existing_file) { + configmodule_interface_t *cfg = static_cast(calloc(1, sizeof(*cfg))); + cfg->cfgP[0] = strdup("non_existing_file.yaml"); + EXPECT_EQ(config_yaml_init(cfg), -1); + free(cfg->cfgP[0]); + end_configmodule(cfg); +} + +TEST(yaml_config, malformed_file) { + configmodule_interface_t *cfg = static_cast(calloc(1, sizeof(*cfg))); + cfg->cfgP[0] = strdup("malformed.yaml"); + EXPECT_EQ(config_yaml_init(cfg), -1); + free(cfg->cfgP[0]); + end_configmodule(cfg); +} + int main(int argc, char** argv) { logInit();