db/pgbackrest
1
0
Fork 0
mirror of https://github.com/pgbackrest/pgbackrest.git synced 2025-03-03 14:52:21 +02:00
Code Issues Releases Activity

Fix incorrect error message for duplicate options in configuration files.

Browse Source 
Duplicating a non-multi-value option was not throwing the correct message when the option was a boolean.

The reason was that the option was being validated as a boolean before the multi-value check was being done.  The validation code assumed it was operating on a string but was instead operating on a string list causing an assertion to fail.

Since it's not safe to do the multi-value check so late, move it up to the command-line and configuration file parse phases instead.

Reported by Jesper St John.
This commit is contained in:
David Steele 2018-09-27 17:48:40 +01:00
parent be2271f6d3
commit 5404628148
3 changed files with 38 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
doc/xml/release.xml
View File

@ -23,6 +23,14 @@
<p>Fix missing missing URI encoding in S3 driver.</p>
</release-item>
<release-item>
<release-item-contributor-list>
<release-item-ideator id="jesper.st.john"/>
</release-item-contributor-list>
<p>Fix incorrect error message for duplicate options in configuration files.</p>
</release-item>
<release-item>
<p>Fix incorrectly reported error return in <id>info</id> logging. A return code of 1 from the <cmd>archive-get</cmd> was being logged as an error message at <id>info</id> level but otherwise worked correctly.</p>
</release-item>

30
src/config/parse.c
View File

@ -507,8 +507,12 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
THROW_FMT(OptionInvalidError, "option '%s' cannot be set and reset", cfgOptionName(optionId));
// Add the argument
if (optionList[optionListIdx].has_arg == required_argument)
if (optionList[optionListIdx].has_arg == required_argument &&
cfgDefOptionMulti(cfgOptionDefIdFromId(optionId)))
{
strLstAdd(parseOptionList[optionId].valueList, strNew(optarg));
}
// Error if the option does not accept multiple arguments
else
THROW_FMT(OptionInvalidError, "option '%s' cannot be set multiple times", cfgOptionName(optionId));
}
@ -759,8 +763,16 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
parseOptionList[optionId].found = true;
parseOptionList[optionId].source = cfgSourceConfig;
// Convert boolean to string
if (cfgDefOptionType(optionDefId) == cfgDefOptTypeBoolean)
if (varType(value) == varTypeVariantList)
{
// Error if the option cannot be specified multiple times
if (!cfgDefOptionMulti(optionDefId))
THROW_FMT(OptionInvalidError, "option '%s' cannot be set multiple times", cfgOptionName(optionId));
parseOptionList[optionId].valueList = strLstNewVarLst(varVarLst(value));
}
// Convert boolean
else if (cfgDefOptionType(optionDefId) == cfgDefOptTypeBoolean)
{
if (strcasecmp(strPtr(varStr(value)), "n") == 0)
parseOptionList[optionId].negate = true;
@ -768,14 +780,11 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
THROW_FMT(OptionInvalidValueError, "boolean option '%s' must be 'y' or 'n'", strPtr(key));
}
// Else add the string value
else if (varType(value) == varTypeString)
else
{
parseOptionList[optionId].valueList = strLstNew();
strLstAdd(parseOptionList[optionId].valueList, varStr(value));
}
// Else add the string list
else
parseOptionList[optionId].valueList = strLstNewVarLst(varVarLst(value));
}
}
}
@ -802,13 +811,6 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
cfgCommandName(cfgCommand()));
}
// Error if this option does not allow multiple arguments
if (parseOption->valueList != NULL && strLstSize(parseOption->valueList) > 1 &&
!cfgDefOptionMulti(cfgOptionDefIdFromId(optionId)))
{
THROW_FMT(OptionInvalidError, "option '%s' cannot have multiple arguments", cfgOptionName(optionId));
}
// Is the option valid for this command? If not, there is nothing more to do.
cfgOptionValidSet(optionId, cfgDefOptionValid(commandDefId, optionDefId));

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

@ -646,7 +646,7 @@ testRun(void)
strLstAdd(argList, strNew("--compress-level=3"));
TEST_ERROR(
configParse(strLstSize(argList), strLstPtr(argList), false), OptionInvalidError,
"option 'compress-level' cannot have multiple arguments");
"option 'compress-level' cannot be set multiple times");
// -------------------------------------------------------------------------------------------------------------------------
argList = strLstNew();
@ -966,7 +966,19 @@ testRun(void)
TEST_ERROR(configParse(strLstSize(argList), strLstPtr(argList), false),
OptionInvalidError,
"option 'pg1-path' cannot have multiple arguments");
"option 'pg1-path' cannot be set multiple times");
// Also test with a boolean option since this gets converted immediately and will blow up if it is multi
storagePutNP(
storageNewWriteNP(storageLocalWrite(), configFile),
bufNewZ(
"[db]\n"
"start-fast=y\n"
"start-fast=n\n"));
TEST_ERROR(configParse(strLstSize(argList), strLstPtr(argList), false),
OptionInvalidError,
"option 'start-fast' cannot be set multiple times");
// Test that log levels are set correctly when reset is enabled, then set them back to harness defaults
// -------------------------------------------------------------------------------------------------------------------------