db/pgbackrest
1
0
Fork 0
mirror of https://github.com/pgbackrest/pgbackrest.git synced 2025-09-16 09:06:18 +02:00
Code Issues Releases Activity

Fix some bugs in C config code:

Browse Source 
1) Error when the same option is defined multiple times in a section using alternate names.
2) Fix logging of invalid command error.
3) Warn when a stanza-only option is in a global section.

Also, make a note to add validation of section names to the check command.

Per review by Cynthia Shang.
This commit is contained in:
David Steele
2018-02-05 15:48:26 -05:00
parent d35757c05f
commit 808b6000eb
6 changed files with 92 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
doc/xml/release.xml
View File

@@ -97,6 +97,10 @@
</release-item>
<release-item>
<release-item-contributor-list>
<release-item-reviewer id="shang.cynthia"/>
</release-item-contributor-list>
<p>Config parsing implemented in C and passed to Perl as JSON.</p>
</release-item>

4
src/common/log.c
View File

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

40
src/config/parse.c
View File

@@ -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,9 +333,21 @@ 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))
{
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)
@@ -353,7 +378,6 @@ configParse(int argListSize, const char *argList[])
}
}
}
}
// Phase 3: validate option definitions and load into configuration
// ---------------------------------------------------------------------------------------------------------------------

8
test/src/module/common/logTest.c
View File

@@ -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");
}
// *****************************************************************************************************************************

16
test/src/module/config/loadTest.c
View File

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

23
test/src/module/config/parseTest.c
View File

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