Merge remote-tracking branch 'origin/yaml-config-fixes' into integration_2025_w24 (!3466)

YAML related updates

- Add common function config_common_getdefault
- Handle exceptions from the yaml-cpp library, transform them into
  understandable errors written to stderr
- Add some unit tests.

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 */
+}

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
 }

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);
 

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 <cstring>
 #include <string>
@@ -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);
@@ -207,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, &params[i]);
-    } else {
-      SetDefault(cfg, &params[i]);
-    }
+  if (node && node[std::string(param->optname)]) {
+    SetNonDefault(cfg, node, param);
+  } else {
+    config_common_getdefault(cfg, param, nullptr);
   }
 }
 
@@ -229,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;
 }
 
@@ -249,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;
@@ -272,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++) {
@@ -282,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;
 }
 
@@ -323,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;
+        }
+      }
     }
   }
 

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)

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

common/config/yaml/tests/test_read_str_as_int.yaml

@@ -0,0 +1,2 @@
+test:
+  var: "test_var"

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<configmodule_interface_t*>(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<configmodule_interface_t*>(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<configmodule_interface_t*>(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();