diff --git a/doc/xml/release.xml b/doc/xml/release.xml index 4a3bec3e7..acfcefc07 100644 --- a/doc/xml/release.xml +++ b/doc/xml/release.xml @@ -97,6 +97,10 @@ + + + +

Config parsing implemented in C and passed to Perl as JSON.

 diff --git a/src/common/log.c b/src/common/log.c index 57aca4928..ee5152b17 100644 --- a/src/common/log.c +++ b/src/common/log.c @@ -15,8 +15,8 @@ Log Handler Module variables ***********************************************************************************************************************************/ // Log levels -LogLevel logLevelStdOut = logLevelOff; -LogLevel logLevelStdErr = logLevelOff; +LogLevel logLevelStdOut = logLevelError; +LogLevel logLevelStdErr = logLevelError; // Log file handles int logHandleStdOut = STDOUT_FILENO; diff --git a/src/config/parse.c b/src/config/parse.c index d2cc600dd..97cc0fd77 100644 --- a/src/config/parse.c +++ b/src/config/parse.c @@ -65,6 +65,9 @@ optionFind(const String *option) /*********************************************************************************************************************************** Parse the command-line arguments and config file to produce final config data + +??? Add validation of section names and check all sections for invalid options in the check command. It's too expensive to add the +logic to this critical path code. ***********************************************************************************************************************************/ void configParse(int argListSize, const char *argList[]) @@ -213,9 +216,9 @@ configParse(int argListSize, const char *argList[]) if (commandParamList != NULL) cfgCommandParamSet(commandParamList); - // Enable logging except for local and remote commands + // Enable logging (except for local and remote commands) so config file warnings will be output if (cfgCommand() != cfgCmdLocal && cfgCommand() != cfgCmdRemote) - logInit(logLevelOff, logLevelWarn, false); + logInit(logLevelWarn, logLevelWarn, false); // Phase 2: parse config file unless --no-config passed // --------------------------------------------------------------------------------------------------------------------- @@ -269,6 +272,7 @@ configParse(int argListSize, const char *argList[]) { String *section = strLstGet(sectionList, sectionIdx); StringList *keyList = iniSectionKeyList(config, section); + KeyValue *optionFound = kvNew(); // Loop through keys to search for options for (unsigned int keyIdx = 0; keyIdx < strLstSize(keyList); keyIdx++) @@ -301,9 +305,18 @@ configParse(int argListSize, const char *argList[]) continue; } - // Continue if this option has already been found - if (parseOptionList[optionId].found) - continue; + // Make sure this option does not appear in the same section with an alternate name + Variant *optionFoundKey = varNewInt(optionId); + const Variant *optionFoundName = kvGet(optionFound, optionFoundKey); + + if (optionFoundName != NULL) + { + THROW( + OptionInvalidError, "'%s' contains duplicate options ('%s', '%s') in section '[%s]'", + strPtr(configFile), strPtr(key), strPtr(varStr(optionFoundName)), strPtr(section)); + } + else + kvPut(optionFound, optionFoundKey, varNewStr(key)); // Continue if the option is not valid for this command if (!cfgDefOptionValid(commandDefId, optionDefId)) @@ -320,36 +333,47 @@ configParse(int argListSize, const char *argList[]) continue; } - // Only get the option if it is valid for this section - if ((stanza != NULL && sectionIdx < 2) || cfgDefOptionSection(optionDefId) == cfgDefSectionGlobal) + // Continue if stanza option is in a global section + if (cfgDefOptionSection(optionDefId) == cfgDefSectionStanza && + strBeginsWithZ(section, CFGDEF_SECTION_GLOBAL)) { - const Variant *value = iniGetDefault(config, section, key, NULL); - - if (varType(value) == varTypeString && strSize(varStr(value)) == 0) - THROW(OptionInvalidValueError, "section '%s', key '%s' must have a value", strPtr(section), - strPtr(key)); - - parseOptionList[optionId].found = true; - parseOptionList[optionId].source = cfgSourceConfig; - - // Convert boolean to string - if (cfgDefOptionType(optionDefId) == cfgDefOptTypeBoolean) - { - if (strcasecmp(strPtr(varStr(value)), "n") == 0) - parseOptionList[optionId].negate = true; - else if (strcasecmp(strPtr(varStr(value)), "y") != 0) - THROW(OptionInvalidError, "boolean option '%s' must be 'y' or 'n'", strPtr(key)); - } - // Else add the string value - else if (varType(value) == varTypeString) - { - parseOptionList[optionId].valueList = strLstNew(); - strLstAdd(parseOptionList[optionId].valueList, varStr(value)); - } - // Else add the string list - else - parseOptionList[optionId].valueList = strLstNewVarLst(varVarLst(value)); + LOG_WARN( + "'%s' contains stanza-only option '%s' in global section '%s'", strPtr(configFile), strPtr(key), + strPtr(section)); + continue; } + + // Continue if this option has already been found in another section + if (parseOptionList[optionId].found) + continue; + + // Get the option value + const Variant *value = iniGetDefault(config, section, key, NULL); + + if (varType(value) == varTypeString && strSize(varStr(value)) == 0) + THROW(OptionInvalidValueError, "section '%s', key '%s' must have a value", strPtr(section), + strPtr(key)); + + parseOptionList[optionId].found = true; + parseOptionList[optionId].source = cfgSourceConfig; + + // Convert boolean to string + if (cfgDefOptionType(optionDefId) == cfgDefOptTypeBoolean) + { + if (strcasecmp(strPtr(varStr(value)), "n") == 0) + parseOptionList[optionId].negate = true; + else if (strcasecmp(strPtr(varStr(value)), "y") != 0) + THROW(OptionInvalidError, "boolean option '%s' must be 'y' or 'n'", strPtr(key)); + } + // Else add the string value + else if (varType(value) == varTypeString) + { + parseOptionList[optionId].valueList = strLstNew(); + strLstAdd(parseOptionList[optionId].valueList, varStr(value)); + } + // Else add the string list + else + parseOptionList[optionId].valueList = strLstNewVarLst(varVarLst(value)); } } } diff --git a/test/src/module/common/logTest.c b/test/src/module/common/logTest.c index 1a24dc098..95c34791f 100644 --- a/test/src/module/common/logTest.c +++ b/test/src/module/common/logTest.c @@ -30,11 +30,11 @@ testRun() // ***************************************************************************************************************************** if (testBegin("logInit()")) { - TEST_RESULT_INT(logLevelStdOut, logLevelOff, "console logging is off"); - TEST_RESULT_INT(logLevelStdErr, logLevelOff, "stderr logging is off"); - TEST_RESULT_VOID(logInit(logLevelInfo, logLevelError, true), "init logging"); - TEST_RESULT_INT(logLevelStdOut, logLevelInfo, "console logging is info"); + TEST_RESULT_INT(logLevelStdOut, logLevelError, "console logging is error"); TEST_RESULT_INT(logLevelStdErr, logLevelError, "stderr logging is error"); + TEST_RESULT_VOID(logInit(logLevelInfo, logLevelWarn, true), "init logging"); + TEST_RESULT_INT(logLevelStdOut, logLevelInfo, "console logging is info"); + TEST_RESULT_INT(logLevelStdErr, logLevelWarn, "stderr logging is warn"); } // ***************************************************************************************************************************** diff --git a/test/src/module/config/loadTest.c b/test/src/module/config/loadTest.c index 71406716d..1359e63a7 100644 --- a/test/src/module/config/loadTest.c +++ b/test/src/module/config/loadTest.c @@ -20,9 +20,10 @@ testRun() cfgLoad(strLstSize(argList), strLstPtr(argList)), OptionRequiredError, "archive-get command requires option: stanza"); + TEST_RESULT_INT(logLevelStdOut, logLevelWarn, "console logging is error"); + TEST_RESULT_INT(logLevelStdErr, logLevelWarn, "stderr logging is error"); + // ------------------------------------------------------------------------------------------------------------------------- - TEST_RESULT_INT(logLevelStdOut, logLevelOff, "console logging is off"); - TEST_RESULT_INT(logLevelStdErr, logLevelWarn, "stderr logging is warn"); argList = strLstNew(); strLstAdd(argList, strNew("pgbackrest")); @@ -36,22 +37,13 @@ testRun() TEST_RESULT_VOID(cfgLoad(strLstSize(argList), strLstPtr(argList)), "load local config"); TEST_RESULT_STR(strPtr(cfgExe()), "pgbackrest", "check exe"); - TEST_RESULT_INT(logLevelStdOut, logLevelOff, "console logging is off"); + TEST_RESULT_INT(logLevelStdOut, logLevelWarn, "console logging is warn"); TEST_RESULT_INT(logLevelStdErr, logLevelWarn, "stderr logging is warn"); // ------------------------------------------------------------------------------------------------------------------------- - argList = strLstNew(); - strLstAdd(argList, strNew("pgbackrest")); - strLstAdd(argList, strNew("--stanza=db")); - strLstAdd(argList, strNew("--pg1-path=/path/to/db")); - strLstAdd(argList, strNew("--repo1-path=/path/to/repo")); - strLstAdd(argList, strNew("stanza-create")); - TEST_RESULT_VOID(cfgLoadParam(strLstSize(argList), strLstPtr(argList), strNew("pgbackrest2")), "load local config"); TEST_RESULT_STR(strPtr(cfgExe()), "pgbackrest2", "check exe"); - TEST_RESULT_INT(logLevelStdOut, logLevelWarn, "console logging is warn"); - TEST_RESULT_INT(logLevelStdErr, logLevelWarn, "stderr logging is warn"); // ------------------------------------------------------------------------------------------------------------------------- argList = strLstNew(); diff --git a/test/src/module/config/parseTest.c b/test/src/module/config/parseTest.c index bd0d88994..befac0c3f 100644 --- a/test/src/module/config/parseTest.c +++ b/test/src/module/config/parseTest.c @@ -254,6 +254,23 @@ testRun() strLstSize(argList), strLstPtr(argList)), OptionInvalidValueError, "section 'global', key 'compress' must have a value"); + // ------------------------------------------------------------------------------------------------------------------------- + argList = strLstNew(); + strLstAdd(argList, strNew(TEST_BACKREST_EXE)); + strLstAdd(argList, strNew("--stanza=db")); + strLstAdd(argList, strNewFmt("--config=%s", strPtr(configFile))); + strLstAdd(argList, strNew(TEST_COMMAND_BACKUP)); + + storagePut(storageLocal(), configFile, bufNewStr(strNew( + "[db]\n" + "pg1-path=/path/to/db\n" + "db-path=/also/path/to/db\n" + ))); + + TEST_ERROR(configParse( + strLstSize(argList), strLstPtr(argList)), OptionInvalidError, + strPtr(strNewFmt("'%s' contains duplicate options ('db-path', 'pg1-path') in section '[db]'", strPtr(configFile)))); + // ------------------------------------------------------------------------------------------------------------------------- argList = strLstNew(); strLstAdd(argList, strNew(TEST_BACKREST_EXE)); @@ -335,6 +352,7 @@ testRun() "no-compress=y\n" "archive-copy=y\n" "online=y\n" + "pg1-path=/not/path/to/db\n" "\n" "[db:backup]\n" "compress=n\n" @@ -354,8 +372,9 @@ testRun() "WARN: '%s' contains option 'recovery-option' invalid for section 'db:backup'\n" "WARN: '%s' contains invalid option 'bogus'\n" "WARN: '%s' contains negate option 'no-compress'\n" - "WARN: '%s' contains command-line only option 'online'", - strPtr(configFile), strPtr(configFile), strPtr(configFile), strPtr(configFile)))); + "WARN: '%s' contains command-line only option 'online'\n" + "WARN: '%s' contains stanza-only option 'pg1-path' in global section 'global:backup'", + strPtr(configFile), strPtr(configFile), strPtr(configFile), strPtr(configFile), strPtr(configFile)))); TEST_RESULT_STR(strPtr(cfgOptionStr(cfgOptPgPath)), "/path/to/db", " pg1-path is set"); TEST_RESULT_INT(cfgOptionSource(cfgOptPgPath), cfgSourceConfig, " pg1-path is source config");