From 987df62ec014c9b3f21b1ff9cd6db9a33f837a84 Mon Sep 17 00:00:00 2001 From: David Steele Date: Sun, 26 Apr 2020 17:22:37 -0400 Subject: [PATCH] Centralize enforcement of option constraints. Each option type enforced its own constraints but there was a lot of duplication. Centralize the enforcement to remove the duplication. Also convert the option type assert to a production error. This is unlikely to happen in production but the test is quite cheap so it can't hurt. Finally, add a NULL check. Most option types can never be NULL. --- src/config/config.c | 86 +++++++++++++++-------------- test/src/module/config/configTest.c | 30 +++------- 2 files changed, 51 insertions(+), 65 deletions(-) diff --git a/src/config/config.c b/src/config/config.c index 2d73e824f..ad9f82706 100644 --- a/src/config/config.c +++ b/src/config/config.c @@ -802,6 +802,37 @@ cfgOptionResetSet(ConfigOption optionId, bool reset) } /**********************************************************************************************************************************/ +// Helper to enforce contraints when getting options +static Variant * +cfgOptionInternal(ConfigOption optionId, VariantType typeRequested, bool nullAllowed) +{ + FUNCTION_TEST_BEGIN(); + FUNCTION_TEST_PARAM(ENUM, optionId); + FUNCTION_TEST_PARAM(ENUM, typeRequested); + FUNCTION_TEST_PARAM(BOOL, nullAllowed); + FUNCTION_TEST_END(); + + ASSERT(optionId < CFG_OPTION_TOTAL); + + // If the option is not NULL then check it is the requested type + Variant *result = configStatic.option[optionId].value; + + if (result != NULL) + { + if (varType(result) != typeRequested) + { + THROW_FMT( + AssertError, "option '%s' is type %u but %u was requested", cfgOptionName(optionId), varType(result), + typeRequested); + } + } + // Else check the option is allowed to be NULL + else + CHECK(nullAllowed); + + FUNCTION_TEST_RETURN(result); +} + const Variant * cfgOption(ConfigOption optionId) { @@ -821,10 +852,7 @@ cfgOptionBool(ConfigOption optionId) FUNCTION_LOG_PARAM(ENUM, optionId); FUNCTION_LOG_END(); - ASSERT(optionId < CFG_OPTION_TOTAL); - ASSERT(varType(configStatic.option[optionId].value) == varTypeBool); - - FUNCTION_LOG_RETURN(BOOL, varBool(configStatic.option[optionId].value)); + FUNCTION_LOG_RETURN(BOOL, varBool(cfgOptionInternal(optionId, varTypeBool, false))); } double @@ -834,10 +862,7 @@ cfgOptionDbl(ConfigOption optionId) FUNCTION_LOG_PARAM(ENUM, optionId); FUNCTION_LOG_END(); - ASSERT(optionId < CFG_OPTION_TOTAL); - ASSERT(varType(configStatic.option[optionId].value) == varTypeDouble); - - FUNCTION_LOG_RETURN(DOUBLE, varDbl(configStatic.option[optionId].value)); + FUNCTION_LOG_RETURN(DOUBLE, varDbl(cfgOptionInternal(optionId, varTypeDouble, false))); } int @@ -847,10 +872,7 @@ cfgOptionInt(ConfigOption optionId) FUNCTION_LOG_PARAM(ENUM, optionId); FUNCTION_LOG_END(); - ASSERT(optionId < CFG_OPTION_TOTAL); - ASSERT(varType(configStatic.option[optionId].value) == varTypeInt64); - - FUNCTION_LOG_RETURN(INT, varIntForce(configStatic.option[optionId].value)); + FUNCTION_LOG_RETURN(INT, varIntForce(cfgOptionInternal(optionId, varTypeInt64, false))); } int64_t @@ -860,10 +882,7 @@ cfgOptionInt64(ConfigOption optionId) FUNCTION_LOG_PARAM(ENUM, optionId); FUNCTION_LOG_END(); - ASSERT(optionId < CFG_OPTION_TOTAL); - ASSERT(varType(configStatic.option[optionId].value) == varTypeInt64); - - FUNCTION_LOG_RETURN(INT64, varInt64(configStatic.option[optionId].value)); + FUNCTION_LOG_RETURN(INT64, varInt64(cfgOptionInternal(optionId, varTypeInt64, false))); } unsigned int @@ -873,10 +892,7 @@ cfgOptionUInt(ConfigOption optionId) FUNCTION_LOG_PARAM(ENUM, optionId); FUNCTION_LOG_END(); - ASSERT(optionId < CFG_OPTION_TOTAL); - ASSERT(varType(configStatic.option[optionId].value) == varTypeInt64); - - FUNCTION_LOG_RETURN(UINT, varUIntForce(configStatic.option[optionId].value)); + FUNCTION_LOG_RETURN(UINT, varUIntForce(cfgOptionInternal(optionId, varTypeInt64, false))); } uint64_t @@ -886,10 +902,7 @@ cfgOptionUInt64(ConfigOption optionId) FUNCTION_LOG_PARAM(ENUM, optionId); FUNCTION_LOG_END(); - ASSERT(optionId < CFG_OPTION_TOTAL); - ASSERT(varType(configStatic.option[optionId].value) == varTypeInt64); - - FUNCTION_LOG_RETURN(UINT64, varUInt64Force(configStatic.option[optionId].value)); + FUNCTION_LOG_RETURN(UINT64, varUInt64Force(cfgOptionInternal(optionId, varTypeInt64, false))); } const KeyValue * @@ -899,10 +912,7 @@ cfgOptionKv(ConfigOption optionId) FUNCTION_LOG_PARAM(ENUM, optionId); FUNCTION_LOG_END(); - ASSERT(optionId < CFG_OPTION_TOTAL); - ASSERT(varType(configStatic.option[optionId].value) == varTypeKeyValue); - - FUNCTION_LOG_RETURN(KEY_VALUE, varKv(configStatic.option[optionId].value)); + FUNCTION_LOG_RETURN(KEY_VALUE, varKv(cfgOptionInternal(optionId, varTypeKeyValue, false))); } const VariantList * @@ -912,19 +922,19 @@ cfgOptionLst(ConfigOption optionId) FUNCTION_LOG_PARAM(ENUM, optionId); FUNCTION_LOG_END(); - ASSERT(optionId < CFG_OPTION_TOTAL); - ASSERT(configStatic.option[optionId].value == NULL || varType(configStatic.option[optionId].value) == varTypeVariantList); + Variant *optionValue = cfgOptionInternal(optionId, varTypeVariantList, true); - if (configStatic.option[optionId].value == NULL) + if (optionValue == NULL) { MEM_CONTEXT_BEGIN(configStatic.memContext) { - configStatic.option[optionId].value = varNewVarLst(varLstNew()); + optionValue = varNewVarLst(varLstNew()); + configStatic.option[optionId].value = optionValue; } MEM_CONTEXT_END(); } - FUNCTION_LOG_RETURN(VARIANT_LIST, varVarLst(configStatic.option[optionId].value)); + FUNCTION_LOG_RETURN(VARIANT_LIST, varVarLst(optionValue)); } const String * @@ -934,15 +944,7 @@ cfgOptionStr(ConfigOption optionId) FUNCTION_LOG_PARAM(ENUM, optionId); FUNCTION_LOG_END(); - ASSERT(optionId < CFG_OPTION_TOTAL); - ASSERT(configStatic.option[optionId].value == NULL || varType(configStatic.option[optionId].value) == varTypeString); - - const String *result = NULL; - - if (configStatic.option[optionId].value != NULL) - result = varStr(configStatic.option[optionId].value); - - FUNCTION_LOG_RETURN_CONST(STRING, result); + FUNCTION_LOG_RETURN_CONST(STRING, varStr(cfgOptionInternal(optionId, varTypeString, true))); } void diff --git a/test/src/module/config/configTest.c b/test/src/module/config/configTest.c index a0bf8cd48..5f43d11c2 100644 --- a/test/src/module/config/configTest.c +++ b/test/src/module/config/configTest.c @@ -151,21 +151,15 @@ testRun(void) TEST_RESULT_VOID(cfgOptionSet(cfgOptOnline, cfgSourceParam, varNewStrZ("1")), "set online"); TEST_RESULT_BOOL(cfgOptionBool(cfgOptOnline), true, "online is set"); TEST_RESULT_INT(cfgOptionSource(cfgOptOnline), cfgSourceParam, "online source is set"); - TEST_ERROR( - cfgOptionDbl(cfgOptOnline), AssertError, - "assertion 'varType(configStatic.option[optionId].value) == varTypeDouble' failed"); - TEST_ERROR( - cfgOptionInt64(cfgOptOnline), AssertError, - "assertion 'varType(configStatic.option[optionId].value) == varTypeInt64' failed"); + TEST_ERROR(cfgOptionDbl(cfgOptOnline), AssertError, "option 'online' is type 0 but 1 was requested"); + TEST_ERROR(cfgOptionInt64(cfgOptOnline), AssertError, "option 'online' is type 0 but 3 was requested"); TEST_RESULT_VOID(cfgOptionSet(cfgOptCompressLevel, cfgSourceParam, varNewInt64(1)), "set compress-level"); TEST_RESULT_INT(cfgOptionInt(cfgOptCompressLevel), 1, "compress-level is set"); TEST_RESULT_VOID(cfgOptionSet(cfgOptCompressLevel, cfgSourceDefault, varNewStrZ("3")), "set compress-level"); TEST_RESULT_INT(cfgOptionUInt(cfgOptCompressLevel), 3, "compress-level is set"); TEST_RESULT_INT(cfgOptionSource(cfgOptCompressLevel), cfgSourceDefault, "compress source is set"); - TEST_ERROR( - cfgOptionBool(cfgOptCompressLevel), AssertError, - "assertion 'varType(configStatic.option[optionId].value) == varTypeBool' failed"); + TEST_ERROR(cfgOptionBool(cfgOptCompressLevel), AssertError, "option 'compress-level' is type 3 but 0 was requested"); TEST_RESULT_VOID( cfgOptionSet(cfgOptArchivePushQueueMax, cfgSourceParam, varNewInt64(999999999999)), "set archive-push-queue-max"); @@ -177,9 +171,7 @@ testRun(void) TEST_RESULT_VOID(cfgOptionSet(cfgOptProtocolTimeout, cfgSourceConfig, varNewStrZ("3.3")), "set protocol-timeout"); TEST_RESULT_DOUBLE(cfgOptionDbl(cfgOptProtocolTimeout), 3.3, "protocol-timeout is set"); TEST_RESULT_INT(cfgOptionSource(cfgOptProtocolTimeout), cfgSourceConfig, "protocol-timeout source is set"); - TEST_ERROR( - cfgOptionKv(cfgOptProtocolTimeout), AssertError, - "assertion 'varType(configStatic.option[optionId].value) == varTypeKeyValue' failed"); + TEST_ERROR(cfgOptionKv(cfgOptProtocolTimeout), AssertError, "option 'protocol-timeout' is type 1 but 4 was requested"); TEST_RESULT_VOID(cfgOptionSet(cfgOptProtocolTimeout, cfgSourceConfig, NULL), "set protocol-timeout to NULL"); TEST_RESULT_PTR(cfgOption(cfgOptProtocolTimeout), NULL, "protocol-timeout is not set"); @@ -189,10 +181,7 @@ testRun(void) "option 'recovery-option' must be set with KeyValue variant"); TEST_RESULT_VOID(cfgOptionSet(cfgOptRecoveryOption, cfgSourceConfig, varNewKv(kvNew())), "set recovery-option"); TEST_RESULT_INT(varLstSize(kvKeyList(cfgOptionKv(cfgOptRecoveryOption))), 0, "recovery-option is set"); - TEST_ERROR( - cfgOptionLst(cfgOptRecoveryOption), AssertError, - "assertion 'configStatic.option[optionId].value == NULL" - " || varType(configStatic.option[optionId].value) == varTypeVariantList' failed"); + TEST_ERROR(cfgOptionLst(cfgOptRecoveryOption), AssertError, "option 'recovery-option' is type 4 but 8 was requested"); TEST_RESULT_INT(varLstSize(cfgOptionLst(cfgOptDbInclude)), 0, "db-include defaults to empty"); TEST_ERROR( @@ -200,10 +189,7 @@ testRun(void) "option 'db-include' must be set with VariantList variant"); TEST_RESULT_VOID(cfgOptionSet(cfgOptDbInclude, cfgSourceConfig, varNewVarLst(varLstNew())), "set db-include"); TEST_RESULT_INT(varLstSize(cfgOptionLst(cfgOptDbInclude)), 0, "db-include is set"); - TEST_ERROR( - cfgOptionStr(cfgOptDbInclude), AssertError, - "assertion 'configStatic.option[optionId].value == NULL" - " || varType(configStatic.option[optionId].value) == varTypeString' failed"); + TEST_ERROR(cfgOptionStr(cfgOptDbInclude), AssertError, "option 'db-include' is type 8 but 5 was requested"); TEST_RESULT_PTR(cfgOptionStr(cfgOptStanza), NULL, "stanza defaults to null"); TEST_ERROR( @@ -211,9 +197,7 @@ testRun(void) "option 'stanza' must be set with String variant"); TEST_RESULT_VOID(cfgOptionSet(cfgOptStanza, cfgSourceConfig, varNewStrZ("db")), "set stanza"); TEST_RESULT_STR_Z(cfgOptionStr(cfgOptStanza), "db", "stanza is set"); - TEST_ERROR( - cfgOptionInt(cfgOptStanza), AssertError, - "assertion 'varType(configStatic.option[optionId].value) == varTypeInt64' failed"); + TEST_ERROR(cfgOptionInt(cfgOptStanza), AssertError, "option 'stanza' is type 5 but 3 was requested"); // ------------------------------------------------------------------------------------------------------------------------- TEST_RESULT_VOID(cfgInit(), "config init resets value");