From d1206d9878136592a0afbf02cfe0d75b9c1bce1c Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Fri, 17 Nov 2023 20:33:05 +0100 Subject: [PATCH] Refactor celix_properties fill/create entry Also add/improve some additional error handling in properties. --- .../celix_version/CMakeLists.txt | 1 + .../celix_version/include/celix_version_ei.h | 2 + .../celix_version/src/celix_version_ei.cc | 8 + libs/utils/gtest/CMakeLists.txt | 1 + .../src/PropertiesErrorInjectionTestSuite.cc | 17 +++ libs/utils/gtest/src/PropertiesTestSuite.cc | 17 ++- libs/utils/src/properties.c | 142 ++++++++++-------- 7 files changed, 126 insertions(+), 62 deletions(-) diff --git a/libs/utils/error_injector/celix_version/CMakeLists.txt b/libs/utils/error_injector/celix_version/CMakeLists.txt index a89f7d9d5..bbdc8aced 100644 --- a/libs/utils/error_injector/celix_version/CMakeLists.txt +++ b/libs/utils/error_injector/celix_version/CMakeLists.txt @@ -22,5 +22,6 @@ target_link_libraries(version_ei PUBLIC Celix::error_injector Celix::utils) # It plays nicely with address sanitizer this way. target_link_options(version_ei INTERFACE LINKER:--wrap,celix_version_createVersionFromString + LINKER:--wrap,celix_version_copy ) add_library(Celix::version_ei ALIAS version_ei) diff --git a/libs/utils/error_injector/celix_version/include/celix_version_ei.h b/libs/utils/error_injector/celix_version/include/celix_version_ei.h index bccaff508..348612ad0 100644 --- a/libs/utils/error_injector/celix_version/include/celix_version_ei.h +++ b/libs/utils/error_injector/celix_version/include/celix_version_ei.h @@ -27,6 +27,8 @@ extern "C" { CELIX_EI_DECLARE(celix_version_createVersionFromString, celix_version_t*); +CELIX_EI_DECLARE(celix_version_copy, celix_version_t*); + #ifdef __cplusplus } #endif diff --git a/libs/utils/error_injector/celix_version/src/celix_version_ei.cc b/libs/utils/error_injector/celix_version/src/celix_version_ei.cc index 7a9b2f252..03ade7328 100644 --- a/libs/utils/error_injector/celix_version/src/celix_version_ei.cc +++ b/libs/utils/error_injector/celix_version/src/celix_version_ei.cc @@ -19,6 +19,7 @@ #include "celix_version_ei.h" extern "C" { + celix_version_t *__real_celix_version_createVersionFromString(const char *versionStr); CELIX_EI_DEFINE(celix_version_createVersionFromString, celix_version_t*) celix_version_t *__wrap_celix_version_createVersionFromString(const char *versionStr) { @@ -26,4 +27,11 @@ celix_version_t *__wrap_celix_version_createVersionFromString(const char *versio return __real_celix_version_createVersionFromString(versionStr); } +celix_version_t* __real_celix_version_copy(const celix_version_t* version); +CELIX_EI_DEFINE(celix_version_copy, celix_version_t*) +celix_version_t* __wrap_celix_version_copy(const celix_version_t* version) { + CELIX_EI_IMPL(celix_version_copy); + return __real_celix_version_copy(version); +} + } \ No newline at end of file diff --git a/libs/utils/gtest/CMakeLists.txt b/libs/utils/gtest/CMakeLists.txt index 784e8742e..bfb9a5531 100644 --- a/libs/utils/gtest/CMakeLists.txt +++ b/libs/utils/gtest/CMakeLists.txt @@ -113,6 +113,7 @@ if (EI_TESTS) Celix::asprintf_ei Celix::string_hash_map_ei Celix::long_hash_map_ei + Celix::version_ei GTest::gtest GTest::gtest_main ) target_include_directories(test_utils_with_ei PRIVATE ../src) #for version_private (needs refactoring of test) diff --git a/libs/utils/gtest/src/PropertiesErrorInjectionTestSuite.cc b/libs/utils/gtest/src/PropertiesErrorInjectionTestSuite.cc index ea507e2dd..468b45f32 100644 --- a/libs/utils/gtest/src/PropertiesErrorInjectionTestSuite.cc +++ b/libs/utils/gtest/src/PropertiesErrorInjectionTestSuite.cc @@ -26,12 +26,14 @@ #include "celix_properties.h" #include "celix_properties_private.h" #include "celix_utils_private_constants.h" +#include "celix_version.h" #include "celix_string_hash_map_ei.h" #include "celix_utils_ei.h" #include "malloc_ei.h" #include "stdio_ei.h" #include "celix_utils_ei.h" +#include "celix_version_ei.h" class PropertiesErrorInjectionTestSuite : public ::testing::Test { public: @@ -283,3 +285,18 @@ TEST_F(PropertiesErrorInjectionTestSuite, LoadFromStringFailureTest) { ASSERT_EQ(1, celix_err_getErrorCount()); celix_err_resetErrors(); } + +TEST_F(PropertiesErrorInjectionTestSuite, LoadSetVersionFailureTest) { + // Given a celix properties object + celix_autoptr(celix_properties_t) props = celix_properties_create(); + // And a version object + celix_autoptr(celix_version_t) version = celix_version_create(1, 2, 3, "qualifier"); + // When a celix_version_copy error injection is set for celix_properties_setVersion + celix_ei_expect_celix_version_copy((void*)celix_properties_setVersion, 0, nullptr); + // Then the celix_properties_setVersion call fails + auto status = celix_properties_setVersion(props, "key", version); + ASSERT_EQ(status, CELIX_ENOMEM); + //And a celix err msg is pushed + ASSERT_EQ(1, celix_err_getErrorCount()); + celix_err_resetErrors(); +} diff --git a/libs/utils/gtest/src/PropertiesTestSuite.cc b/libs/utils/gtest/src/PropertiesTestSuite.cc index 7dd9cf582..f77abf82a 100644 --- a/libs/utils/gtest/src/PropertiesTestSuite.cc +++ b/libs/utils/gtest/src/PropertiesTestSuite.cc @@ -19,11 +19,13 @@ #include +#include + +#include "celix_err.h" #include "celix_properties.h" #include "celix_properties_internal.h" #include "celix_utils.h" #include "properties.h" -#include "celix_err.h" using ::testing::MatchesRegex; @@ -709,3 +711,16 @@ TEST_F(PropertiesTestSuite, GetStatsTest) { EXPECT_EQ(stats.mapStatistics.nrOfEntries, 200); printStats(&stats); } + +TEST_F(PropertiesTestSuite, SetLongMaxMinTest) { + celix_autoptr(celix_properties_t) props = celix_properties_create(); + ASSERT_EQ(CELIX_SUCCESS, celix_properties_setLong(props, "max", LONG_MAX)); + ASSERT_EQ(CELIX_SUCCESS, celix_properties_setLong(props, "min", LONG_MIN)); + EXPECT_EQ(LONG_MAX, celix_properties_getAsLong(props, "max", 0)); + EXPECT_EQ(LONG_MIN, celix_properties_getAsLong(props, "min", 0)); +} + +TEST_F(PropertiesTestSuite, SetDoubleWithLargeStringRepresentationTest) { + celix_autoptr(celix_properties_t) props = celix_properties_create(); + ASSERT_EQ(CELIX_SUCCESS, celix_properties_setDouble(props, "large_str_value", 12345678901234567890.1234567890)); +} diff --git a/libs/utils/src/properties.c b/libs/utils/src/properties.c index af0320594..f511c4171 100644 --- a/libs/utils/src/properties.c +++ b/libs/utils/src/properties.c @@ -172,53 +172,50 @@ static void celix_properties_freeString(celix_properties_t* properties, char* st */ static celix_status_t celix_properties_fillEntry(celix_properties_t* properties, celix_properties_entry_t* entry, - const char** strValue, - const long* longValue, - const double* doubleValue, - const bool* boolValue, - celix_version_t* versionValue) { - char convertedValueBuffer[32]; - if (strValue != NULL) { - entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_STRING; - entry->value = celix_properties_createString(properties, *strValue); - entry->typed.strValue = entry->value; - } else if (longValue != NULL) { + const celix_properties_entry_t* prototype) { + char convertedValueBuffer[20]; + if (prototype->valueType == CELIX_PROPERTIES_VALUE_TYPE_VERSION) { + entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_VERSION; + entry->typed.versionValue = prototype->typed.versionValue; + bool written = celix_version_fillString(prototype->typed.versionValue, convertedValueBuffer, sizeof(convertedValueBuffer)); + if (written) { + entry->value = celix_properties_createString(properties, convertedValueBuffer); + } else { + entry->value = celix_version_toString(prototype->typed.versionValue); + } + } else if (prototype->valueType == CELIX_PROPERTIES_VALUE_TYPE_LONG) { entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_LONG; - entry->typed.longValue = *longValue; + entry->typed.longValue = prototype->typed.longValue; int written = snprintf(convertedValueBuffer, sizeof(convertedValueBuffer), "%li", entry->typed.longValue); - if (written < 0 || written >= sizeof(convertedValueBuffer)) { + if (written < 0 || written <= sizeof(convertedValueBuffer)) { entry->value = celix_properties_createString(properties, convertedValueBuffer); } else { - char* val = NULL; - asprintf(&val, "%li", entry->typed.longValue); - entry->value = val; + //LCOV_EXCL_START + assert(false); // should not happen, LONG_MAX str is 19 chars, LONG_MIN str is 20 chars + //LCOV_EXCL_STOP } - } else if (doubleValue != NULL) { + } else if (prototype->valueType == CELIX_PROPERTIES_VALUE_TYPE_DOUBLE) { entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_DOUBLE; - entry->typed.doubleValue = *doubleValue; + entry->typed.doubleValue = prototype->typed.doubleValue; int written = snprintf(convertedValueBuffer, sizeof(convertedValueBuffer), "%f", entry->typed.doubleValue); - if (written < 0 || written >= sizeof(convertedValueBuffer)) { + if (written < 0 || written <= sizeof(convertedValueBuffer)) { entry->value = celix_properties_createString(properties, convertedValueBuffer); } else { char* val = NULL; asprintf(&val, "%f", entry->typed.doubleValue); entry->value = val; } - } else if (boolValue != NULL) { + } else if (prototype->valueType == CELIX_PROPERTIES_VALUE_TYPE_BOOL) { entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_BOOL; - entry->typed.boolValue = *boolValue; + entry->typed.boolValue = prototype->typed.boolValue; entry->value = entry->typed.boolValue ? CELIX_PROPERTIES_BOOL_TRUE_STRVAL : CELIX_PROPERTIES_BOOL_FALSE_STRVAL; - } else /*versionValue*/ { - assert(versionValue != NULL); - entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_VERSION; - entry->typed.versionValue = versionValue; - bool written = celix_version_fillString(versionValue, convertedValueBuffer, sizeof(convertedValueBuffer)); - if (written) { - entry->value = celix_properties_createString(properties, convertedValueBuffer); - } else { - entry->value = celix_version_toString(versionValue); - } + } else /*string value*/ { + assert(prototype->valueType == CELIX_PROPERTIES_VALUE_TYPE_STRING); + entry->valueType = CELIX_PROPERTIES_VALUE_TYPE_STRING; + entry->value = celix_properties_createString(properties, prototype->typed.strValue); + entry->typed.strValue = entry->value; } + if (entry->value == NULL) { return CELIX_ENOMEM; } @@ -259,26 +256,26 @@ static celix_properties_entry_t* celix_properties_createEntryWithNoCopy(celix_pr * Only 1 of the types values (strValue, LongValue, etc) should be provided. */ static celix_properties_entry_t* celix_properties_createEntry(celix_properties_t* properties, - const char** strValue, - const long* longValue, - const double* doubleValue, - const bool* boolValue, - celix_version_t* versionValue) { + const celix_properties_entry_t* prototype) { celix_properties_entry_t* entry = celix_properties_allocEntry(properties); if (entry == NULL) { + if (prototype->valueType == CELIX_PROPERTIES_VALUE_TYPE_VERSION) { + celix_version_destroy((celix_version_t*)prototype->typed.versionValue); + } celix_err_pushf("Cannot allocate property entry"); - celix_version_destroy(versionValue); return NULL; } - celix_status_t status = - celix_properties_fillEntry(properties, entry, strValue, longValue, doubleValue, boolValue, versionValue); + celix_status_t status = celix_properties_fillEntry(properties, entry, prototype); if (status != CELIX_SUCCESS) { celix_err_pushf("Cannot fill property entry"); - celix_version_destroy(versionValue); + if (prototype->valueType == CELIX_PROPERTIES_VALUE_TYPE_VERSION) { + celix_version_destroy((celix_version_t*)prototype->typed.versionValue); + } if (entry >= properties->entriesBuffer && entry <= (properties->entriesBuffer + CELIX_PROPERTIES_OPTIMIZATION_ENTRIES_BUFFER_SIZE)) { - // entry is part of the properties entries buffer -> nop. + // entry is part of the properties entries buffer -> decrease the currentEntriesBufferIndex + properties->currentEntriesBufferIndex -= 1; } else { free(entry); } @@ -303,28 +300,27 @@ static void celix_properties_destroyEntry(celix_properties_t* properties, celix_ /** * Create and add entry and optionally use the short properties optimization buffers. - * Only 1 of the types values (strValue, LongValue, etc) should be provided. + * The prototype is used to determine the type of the value. */ static celix_status_t celix_properties_createAndSetEntry(celix_properties_t* properties, const char* key, - const char** strValue, - const long* longValue, - const double* doubleValue, - const bool* boolValue, - celix_version_t* versionValue) { + const celix_properties_entry_t* prototype) { if (!properties) { - celix_version_destroy(versionValue); + if (prototype->valueType == CELIX_PROPERTIES_VALUE_TYPE_VERSION) { + celix_version_destroy((celix_version_t*)prototype->typed.versionValue); + } return CELIX_SUCCESS; // silently ignore } if (!key) { - celix_version_destroy(versionValue); + if (prototype->valueType == CELIX_PROPERTIES_VALUE_TYPE_VERSION) { + celix_version_destroy((celix_version_t*)prototype->typed.versionValue); + } celix_err_pushf("Cannot set property with NULL key"); return CELIX_ILLEGAL_ARGUMENT; } - celix_properties_entry_t* entry = - celix_properties_createEntry(properties, strValue, longValue, doubleValue, boolValue, versionValue); + celix_properties_entry_t* entry = celix_properties_createEntry(properties, prototype); if (!entry) { return CELIX_ENOMEM; } @@ -587,10 +583,10 @@ celix_status_t celix_properties_store(celix_properties_t* properties, const char celix_err_push("Header cannot contain newlines. Ignoring header."); } else if (header) { rc = fputc('#', file); - if (rc != 0) { + if (rc != EOF) { rc = fputs(header, file); } - if (rc != 0) { + if (rc != EOF) { rc = fputc('\n', file); } } @@ -679,7 +675,10 @@ celix_properties_entry_t* celix_properties_getEntry(const celix_properties_t* pr } celix_status_t celix_properties_set(celix_properties_t* properties, const char* key, const char* value) { - return celix_properties_createAndSetEntry(properties, key, &value, NULL, NULL, NULL, NULL); + celix_properties_entry_t prototype; + prototype.valueType = CELIX_PROPERTIES_VALUE_TYPE_STRING; + prototype.typed.strValue = value; + return celix_properties_createAndSetEntry(properties, key, &prototype); } celix_status_t celix_properties_setWithoutCopy(celix_properties_t* properties, char* key, char* value) { @@ -770,7 +769,10 @@ long celix_properties_getAsLong(const celix_properties_t* props, const char* key } celix_status_t celix_properties_setLong(celix_properties_t* props, const char* key, long value) { - return celix_properties_createAndSetEntry(props, key, NULL, &value, NULL, NULL, NULL); + celix_properties_entry_t prototype; + prototype.valueType = CELIX_PROPERTIES_VALUE_TYPE_LONG; + prototype.typed.longValue = value; + return celix_properties_createAndSetEntry(props, key, &prototype); } double celix_properties_getAsDouble(const celix_properties_t* props, const char* key, double defaultValue) { @@ -784,7 +786,10 @@ double celix_properties_getAsDouble(const celix_properties_t* props, const char* } celix_status_t celix_properties_setDouble(celix_properties_t* props, const char* key, double val) { - return celix_properties_createAndSetEntry(props, key, NULL, NULL, &val, NULL, NULL); + celix_properties_entry_t prototype; + prototype.valueType = CELIX_PROPERTIES_VALUE_TYPE_DOUBLE; + prototype.typed.doubleValue = val; + return celix_properties_createAndSetEntry(props, key, &prototype); } bool celix_properties_getAsBool(const celix_properties_t* props, const char* key, bool defaultValue) { @@ -798,7 +803,10 @@ bool celix_properties_getAsBool(const celix_properties_t* props, const char* key } celix_status_t celix_properties_setBool(celix_properties_t* props, const char* key, bool val) { - return celix_properties_createAndSetEntry(props, key, NULL, NULL, NULL, &val, NULL); + celix_properties_entry_t prototype; + prototype.valueType = CELIX_PROPERTIES_VALUE_TYPE_BOOL; + prototype.typed.boolValue = val; + return celix_properties_createAndSetEntry(props, key, &prototype); } const celix_version_t* celix_properties_getVersion(const celix_properties_t* properties, @@ -828,12 +836,24 @@ celix_version_t* celix_properties_getAsVersion(const celix_properties_t* propert } celix_status_t -celix_properties_setVersion(celix_properties_t* properties, const char* key, const celix_version_t* version) { - return celix_properties_createAndSetEntry(properties, key, NULL, NULL, NULL, NULL, celix_version_copy(version)); +celix_properties_setVersion(celix_properties_t* props, const char* key, const celix_version_t* version) { + celix_version_t* copy = celix_version_copy(version); + if (copy == NULL) { + celix_err_push("Failed to copy version"); + return CELIX_ENOMEM; + } + + celix_properties_entry_t prototype; + prototype.valueType = CELIX_PROPERTIES_VALUE_TYPE_VERSION; + prototype.typed.versionValue = copy; + return celix_properties_createAndSetEntry(props, key, &prototype); } -celix_status_t celix_properties_setVersionWithoutCopy(celix_properties_t* properties, const char* key, celix_version_t* version) { - return celix_properties_createAndSetEntry(properties, key, NULL, NULL, NULL, NULL, version); +celix_status_t celix_properties_setVersionWithoutCopy(celix_properties_t* props, const char* key, celix_version_t* version) { + celix_properties_entry_t prototype; + prototype.valueType = CELIX_PROPERTIES_VALUE_TYPE_VERSION; + prototype.typed.versionValue = version; + return celix_properties_createAndSetEntry(props, key, &prototype); } int celix_properties_size(const celix_properties_t* properties) {