-
Notifications
You must be signed in to change notification settings - Fork 0
issue: 4207451 Move from env params to config file #2
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
base: vNext
Are you sure you want to change the base?
Conversation
src/core/config/config_bundle.h
Outdated
| #include "descriptors/config_descriptor.h" | ||
|
|
||
| struct config_bundle { | ||
| std::map<std::string, std::experimental::any> m_config_data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The use of std::experimental::any may not be the best choice for storing configuration data, as it can lead to runtime errors if the wrong type is accessed.
Suggested Change: Consider using a more type-safe approach, such as a std::map<std::string, std::variant<...>> where the variant contains the possible types of configuration data.
Severity (1 - 4): 3 - MAJOR
Line: 6
| std::map<std::string, std::experimental::any> m_config_data; | |
| std::map<std::string, std::variant<int, std::string, bool>> m_config_data; |
| if (it == parameter_map.end()) { | ||
| throw_xlio_exception("Parameter descriptor for '" + key + "' not found"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The current implementation throws an exception when a parameter descriptor is not found. This might not be the desired behavior in all cases, as it can lead to program termination. A more flexible approach would be to return an optional value or a status code.
Suggested Change: Consider returning an optional value or a status code instead of throwing an exception.
Severity (1 - 4): 3 - MAJOR
Lines: 7-9
| if (it == parameter_map.end()) { | |
| throw_xlio_exception("Parameter descriptor for '" + key + "' not found"); | |
| } | |
| auto it = parameter_map.find(key); | |
| if (it == parameter_map.end()) { | |
| return std::nullopt; // or a status code | |
| } | |
| return it->second; |
| config_descriptor load_descriptors() override; | ||
|
|
||
| private: | ||
| const char *m_json_string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The m_json_string pointer is not being initialized or checked for null before use, which can lead to undefined behavior.
Suggested Change: Initialize m_json_string to nullptr in the constructors and check for null before using it.
Severity (1 - 4): 3 - MAJOR
Line: 16
| const char *m_json_string; | |
| const char *m_json_string = nullptr; |
| bool parameter_descriptor::validate(const std::experimental::any &val) const | ||
| { | ||
| // 1) Check type | ||
| if (*m_expected_type != std::type_index(val.type())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The comparison between *m_expected_type and std::type_index(val.type()) may not work as expected if the std::type_index objects are not properly initialized.
Suggested Change: Use the == operator to compare the std::type_index objects directly, without dereferencing the pointer.
Severity (1 - 4): 3 - MAJOR
Line: 39
| if (*m_expected_type != std::type_index(val.type())) { | |
| if (m_expected_type != std::type_index(val.type())) { |
src/core/config/config.h
Outdated
|
|
||
| // Attempt to cast to the requested type T. | ||
| try { | ||
| return std::experimental::any_cast<T>(raw_value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The std::experimental::any_cast function is used, which is experimental and may not be supported in all compilers or future versions of the standard.
Suggested Change: Use std::any_cast instead, which is a standard function since C++17.
Severity (1 - 4): 3 - MAJOR
Line: 16
| return std::experimental::any_cast<T>(raw_value); | |
| return std::any_cast<T>(raw_value); |
| void parse_constraints(void *constraints_obj, parameter_descriptor &pd, | ||
| const std::type_index &ti); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The function parse_constraints takes a void* as an argument, which can lead to type safety issues.
Suggested Change: Consider using a more specific type or a smart pointer instead of void*.
Severity (1 - 4): 3 - MAJOR
Lines: 25-26
| void parse_constraints(void *constraints_obj, parameter_descriptor &pd, | |
| const std::type_index &ti); | |
| No specific fix can be provided without more context, but the use of `void*` should be reviewed. |
tests/unit_tests/config/utils.h
Outdated
| setenv("XLIO_INLINE_CONFIG", | ||
| inline_config, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The setenv function is used with a hardcoded variable name and value, which may not be suitable for all environments or use cases.
Suggested Change: Consider making the variable name and value configurable or using a more flexible approach to set environment variables.
Severity (1 - 4): 3 - MAJOR
Lines: 24-25
| setenv("XLIO_INLINE_CONFIG", | |
| inline_config, 1); | |
| // Consider using a more flexible approach, such as using a std::string for the variable name and value | |
| setenv(varName.c_str(), varValue.c_str(), 1); |
| env_setter setter(""); | ||
| inline_loader loader("XLIO_INLINE_CONFIG"); | ||
|
|
||
| ASSERT_THROW(loader.load_all(), xlio_exception); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The test case TEST(config, inline_loader_empty) is expecting an exception of type xlio_exception to be thrown when loader.load_all() is called with an empty environment variable. However, it does not verify that the exception is thrown with the correct error message or code.
Suggested Change: It would be beneficial to verify that the exception is thrown with the expected error message or code to ensure that the correct error handling is in place.
Severity (1 - 4): 3 - MAJOR
Line: 35
| ASSERT_THROW(loader.load_all(), xlio_exception); | |
| ASSERT_THROW(loader.load_all(), xlio_exception); | |
| // Additional verification for the exception's error message or code | |
| try { | |
| loader.load_all(); | |
| FAIL() << "Expected xlio_exception to be thrown"; | |
| } catch (const xlio_exception& e) { | |
| EXPECT_STREQ("Expected error message", e.what()); | |
| } |
|
|
||
| TEST(config, inline_loader_double_commas_works) | ||
| { | ||
| env_setter setter("core.log.level=10,,core.log.file_path=/var/log/xlio.log,core.exit_report=true"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The test case TEST(config, inline_loader_double_commas_works) is using an environment variable with double commas to test the inline loader. However, it does not test the scenario where there are multiple consecutive commas.
Suggested Change: It would be beneficial to add additional test cases to cover this scenario and ensure that the inline loader handles it correctly.
Severity (1 - 4): 3 - MAJOR
Line: 40
| env_setter setter("core.log.level=10,,core.log.file_path=/var/log/xlio.log,core.exit_report=true"); | |
| TEST(config, inline_loader_multiple_consecutive_commas) { | |
| env_setter setter("core.log.level=10,,,core.log.file_path=/var/log/xlio.log,core.exit_report=true"); | |
| inline_loader loader("XLIO_INLINE_CONFIG"); | |
| loader.load_all(); | |
| ASSERT_EQ("10", loader.get("core.log.level")); | |
| ASSERT_EQ("/var/log/xlio.log", loader.get("core.log.file_path")); | |
| ASSERT_EQ("true", loader.get("core.exit_report")); | |
| } |
tests/unit_tests/config/utils.h
Outdated
| inline_config, 1); | ||
| } | ||
|
|
||
| ~env_setter() { unsetenv("XLIO_INLINE_CONFIG"); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The unsetenv function is used with a hardcoded variable name, which may not match the actual variable name used in the env_setter constructor.
Suggested Change: Use the same variable name as in the constructor, or consider making the variable name configurable.
Severity (1 - 4): 3 - MAJOR
Line: 28
| ~env_setter() { unsetenv("XLIO_INLINE_CONFIG"); } | |
| ~env_setter() { unsetenv(varName.c_str()); } | |
| // Add a member variable to store the variable name in the constructor: | |
| // std::string varName = "XLIO_INLINE_CONFIG"; |
| class parameter_descriptor { | ||
| public: | ||
| explicit parameter_descriptor() = default; | ||
| explicit parameter_descriptor(const std::experimental::any &def, std::type_index type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The constructor parameter_descriptor(const std::experimental::any &def, std::type_index type) should validate if the provided def matches the type to ensure consistency.
Suggested Change: Add a check to ensure the type of def matches type to prevent potential type mismatches.
Severity (1 - 4): 3 - MAJOR
Line: 15
| explicit parameter_descriptor(const std::experimental::any &def, std::type_index type); | |
| explicit parameter_descriptor(const std::experimental::any &def, std::type_index type) : m_default_value(def), m_expected_type(std::make_unique<std::type_index>(type)) { | |
| if (def.type() != type) { | |
| throw std::invalid_argument("Default value type does not match the expected type."); | |
| } | |
| } |
| if (!current_obj) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The code checks if the current object is null and continues to the next iteration if it is. However, it does not log or handle this situation in any way.
Suggested Change: Consider logging a warning or throwing an exception when encountering a null object, as it may indicate an issue with the JSON data.
Severity (1 - 4): 3 - MAJOR
Lines: 21-23
| if (!current_obj) { | |
| continue; | |
| } | |
| if (!current_obj) { | |
| // Log a warning or throw an exception | |
| // For example: | |
| // throw_xlio_exception("Null object encountered at " + current_prefix); | |
| continue; | |
| } |
src/core/config/utils/json_parsing.h
Outdated
| void flatten_json(json_object *root, | ||
| std::function<bool(json_object *, const std::string &)> object_visitor, | ||
| std::function<void(json_object *, const std::string &)> primitive_object_visitor); No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The function flatten_json takes a raw pointer json_object *root which may lead to memory leaks or dangling pointers if not handled properly.
Suggested Change: Consider using a smart pointer (e.g. std::unique_ptr or std::shared_ptr) to manage the memory of the json_object instance.
Severity (1 - 4): 3 - MAJOR
Lines: 7-9
| void flatten_json(json_object *root, | |
| std::function<bool(json_object *, const std::string &)> object_visitor, | |
| std::function<void(json_object *, const std::string &)> primitive_object_visitor); | |
| void flatten_json(std::unique_ptr<json_object> root, | |
| std::function<bool(json_object *, const std::string &)> object_visitor, | |
| std::function<void(json_object *, const std::string &)> primitive_object_visitor); | |
| Note: The above code fix assumes that the `json_object` instance is dynamically allocated and should be managed by a smart pointer. If the instance is not dynamically allocated, a different approach should be taken to ensure proper memory management. |
| public: | ||
| explicit parameter_descriptor() = default; | ||
| explicit parameter_descriptor(const std::experimental::any &def, std::type_index type); | ||
| parameter_descriptor(parameter_descriptor &&pd) noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The move constructor parameter_descriptor(parameter_descriptor &&pd) noexcept should properly transfer ownership of m_expected_type to prevent memory leaks.
Suggested Change: Use std::move to transfer ownership of m_expected_type.
Severity (1 - 4): 3 - MAJOR
Line: 16
| parameter_descriptor(parameter_descriptor &&pd) noexcept; | |
| parameter_descriptor(parameter_descriptor &&pd) noexcept : m_default_value(std::move(pd.m_default_value)), m_expected_type(std::move(pd.m_expected_type)), m_constraints(std::move(pd.m_constraints)) {} |
| // missing '}' in the end of the json string | ||
| TEST(xlio_config, json_loader_invalid_json) | ||
| { | ||
| conf_file_writer json_config = conf_file_writer(R"({ "core": { "log": { "level": 2 } } )"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The JSON string is missing a closing bracket, which will cause a parsing error.
Suggested Change: Add a closing bracket to the end of the JSON string to make it valid.
Severity (1 - 4): 3 - MAJOR
Line: 42
| conf_file_writer json_config = conf_file_writer(R"({ "core": { "log": { "level": 2 } } )"); | |
| conf_file_writer json_config = conf_file_writer(R"({ "core": { "log": { "level": 2 } } })"); |
| snprintf(formatted_message, sizeof(formatted_message), | ||
| "xlio_error <%s> (errno=%d %s) in %s:%d\n", message, errnum, strerror(errnum), | ||
| filename, lineno); | ||
| formatted_message[sizeof(formatted_message) - 1] = '\0'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: The snprintf function does not guarantee null-termination if the formatted string is longer than the buffer size.
Suggested Change: Use snprintf with caution and ensure null-termination.
Severity (1 - 4): 3 - MAJOR
Lines: 40-43
| snprintf(formatted_message, sizeof(formatted_message), | |
| "xlio_error <%s> (errno=%d %s) in %s:%d\n", message, errnum, strerror(errnum), | |
| filename, lineno); | |
| formatted_message[sizeof(formatted_message) - 1] = '\0'; | |
| snprintf(formatted_message, sizeof(formatted_message), "xlio_error <%s> (errno=%d %s) in %s:%d", | |
| message, errnum, strerror(errnum), filename, lineno); | |
| formatted_message[sizeof(formatted_message) - 1] = '\0'; |
e59a4a0 to
1beb24a
Compare
33973d5 to
213fbc3
Compare
68c449e to
6d2a418
Compare
MVP: Move from env params to config file. This change introduces a new configuration file that will be used to configure the system. The configuration file is a JSON file that contains all the configuration parameters that were previously passed as environment variables. In the case of a failure to load the configuration file, the system will fall back to the old environment variables method. See HLD and LLD for more information. This patch also adds unit-tests for the new configuration system. Signed-off-by: Tomer Cabouly <[email protected]>
Complete transformation of the XLIO configuration descriptor to a standard JSON Schema (draft-07) format, preserving the full hierarchical structure, data types, constraints, and default values from the original configuration. Key improvements: - Implemented proper oneOf schema handling for properties - Added complete descriptions for all properties and objects This schema enables formal validation of configuration files and supports tooling for IDE integration, documentation generation, and configuration validation. Signed-off-by: Tomer Cabouly <[email protected]>
Change the transport_control property from a string to an array of objects, where each object contains: - id: String identifier for the transport control - name: String name for the application - actions: Array of string actions To support this change, add full array handling to the config system: - Update JSON descriptor - define array types and nested properties - Implement array type recognition and default value handling - Modify JSON loader to process arrays instead of throwing an exception - Add comprehensive unit tests for arrays This enables more structured configuration for transport control rules and provides general array support throughout the configuration system. Signed-off-by: Tomer Cabouly <[email protected]>
Reorganize the configuration subsystem to improve maintainability by: - Rename core classes for clarity: config → config_provider and config_manager → config_registry - Create new config_strings namespace with centralized string constants - Remove config_bundle abstraction and integrate directly in registry - Improve error handling with better type validation and constraints - Split configuration processing into smaller, targeted methods - Use string constants throughout codebase for consistent error msgs - Follow consistent code style and formatting - Deploy xlio_config_schema.json as part of libxlio package Signed-off-by: Tomer Cabouly <[email protected]>
Replace all instances of registry.get_value<int64_t> with registry.get_value<decltype(var)> throughout the codebase. This change: - Improves type safety - ensure config values match target variable type - Eliminates manual range checking and explicit casting code - Reduces risk of data corruption from improper type conversion - Makes the code more concise and easier to maintain This change simplifies many lines of code while improving safety, as any type mismatch will now be caught at compile time rather than requiring manual runtime validation. As part of a representation limitation of config provider, I've added a new unit test that will fail if a size_t value is requested as it's not supported, as integers are represented as int64_t in the config. Signed-off-by: Tomer Cabouly <[email protected]>
Integrate libjson-c as a statically linked library. Instead of relying on the shared object at runtime. This change compiles libjson-c directly within our project, eliminating external package dependencies and potential runtime compatibility issues. Signed-off-by: Tomer Cabouly <[email protected]>
Enhance configuration system robustness and maintainability: - Replace static err strs with ERROR_STRING macro for function context - json_object_handle class implementing RAII pattern for JSON object - Improve memory management and fix potential leaks in JSON processing - JSON schema with detailed titles and descriptions for all properties - Extract large functions into smaller, focused implementations - Improve type safety between int64_t and size_t - Rename error_format namespace to type_format for clarity - Refactor descriptor provider with cleaner error handling - Extract configuration profile settings into separate methods - Add README.md for new configuration system Signed-off-by: Tomer Cabouly <[email protected]>
1. Added documentation for XLIO_CUSTOM_CONFIG_FILE environment variable to specify custom JSON configuration file location 2. Set default JSON configuration file path to /etc/libxlio_config.json 3. Updated internal #define statements to use dot notation configuration paths instead of legacy environment variables 4. Added Python scripts for automatic generation of documentation. 5. Completely rewrote the README configuration documentation to use the new hierarchical structure with sections for each configuration area 6. Changed environment variable behavior from opt-out to opt-in with XLIO_USE_NEW_CONFIG=1 7. Added Doxygen documentation to classes in the configuration subsystem This improves maintainability and usability of the configuration system while maintaining backward compatibility with legacy env variables. Signed-off-by: Tomer Cabouly <[email protected]>
b1c3fdb to
8155734
Compare
Suppressed various false positive warnings detected by coverity. These warnings included: - Memory leak warnings using temporary std::string objects and c_str() - Uncaught exception warnings in destructor methods - Other similar static analysis false alarms The suppressed warnings refer to code patterns where memory is properly managed through C++ RAII mechanisms or exceptions are properly handled within the context. Signed-off-by: Tomer Cabouly <[email protected]>
This patch restructures the XLIO configuration system for organization: 1. Rename 'observability' namespace to 'monitor' 2. Reorganize parameter paths: - Move WRE counts from buffers to rings section - Rename 'select' polling to 'iomux' to better reflect its purpose - Group related parameters more logically 3. Flatten completion queue interrupt moderation params 4. Improve parameter naming for clarity: - sendfile_limit → sendfile_cache_limit - dup2_support → dup2_close_fd - rx_duration_usec → blocking_rx_poll_usec - ts_conversion → hw_ts_conversion 5. Move profiles.spec to top-level namespace All related code, mappings, and documentation are updated. Signed-off-by: Tomer Cabouly <[email protected]>
MVP: Move from env params to config file.
This change introduces a new configuration file that will be used to configure the system.
The configuration file is a JSON file that
contains all the configuration parameters that were previously passed as environment variables.
In the case of a failure to load the configuration file, the system will fall back to the old environment variables method.
See HLD and LLD for more information.
This patch also adds unit-tests for the new configuration system.
Description
Please provide a summary of the change.
What
Subject: what this PR is doing in one line.
Why ?
Justification for the PR. If there is existing issue/bug please reference.
How ?
It is optional but for complex PRs please provide information about the design,
architecture, approach, etc.
Change type
What kind of change does this PR introduce?
Check list