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

Replace getopt_long() with custom implementation.

Browse Source 
getopt_long() requires an exhaustive list of all possible options that may be found on the command line. Because of the way options are indexed (e.g. repo1-4, pg1-8) optionList[] has 827 entries and we have kept it small by curtailing the maximum indexes very severely. Another issue is that getopt_long() scans the array sequentially so parsing gets slower as the index maximums increase.

Replace getopt_long() with a custom implementation that behaves the same but allows options to be parsed with a function instead of using optionList[]. This commit leaves the list in place in order to focus on the getopt_long() replacement, but cfgParseOption() could be replaced with a more efficient implementation that removes the need for optionList[].

This implementation also fixes an issue where invalid options were misreported in the error message if they only had one dash, e.g. -config. This seems to have been some kind of problem in getopt_long(), but no investigation was done since the new implementation fixes it.

Tests were added at 0825428, 2b8d2da, 34dd663, and 384f247 to check that previously untested getopt_long() behavior doesn't change.
This commit is contained in:
David Steele
2021-05-20 16:02:31 -04:00
committed by GitHub
parent 831ee81466
commit 45a4e801ed
6 changed files with 244 additions and 164 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
doc/xml/release.xml
View File

@ -116,6 +116,24 @@
</release-improvement-list>
<release-development-list>
<release-item>
<commit subject="Add config/parse test where the option/value are not in the same arg."/>
<commit subject="Add config/parse tests for partial options."/>
<commit subject="Add config/parse tests for options and option args with spaces."/>
<commit subject="Add config/parse tests for config/env partial options."/>
<commit subject="Replace getopt_long() with custom implementation.">
<github-pull-request id="1396"/>
</commit>
<release-item-contributor-list>
<release-item-contributor id="david.steele"/>
<release-item-reviewer id="cynthia.shang"/>
<release-item-reviewer id="jan.wieck"/>
</release-item-contributor-list>
<p>Replace <code>getopt_long()</code> with custom implementation.</p>
</release-item>
<release-item>
<github-pull-request id="1377"/>

2
src/command/help/help.c
View File

@ -484,7 +484,7 @@ helpRender(void)
// Ensure the option is valid
const String *optionName = strLstGet(cfgCommandParam(), 0);
CfgParseOptionResult option = cfgParseOption(optionName);
CfgParseOptionResult option = cfgParseOptionP(optionName);
// If the option was not found it might be an indexed option without the index, e.g. repo-host instead of
// repo1-host. This is valid for help even though the parser will reject it.

219
src/config/parse.c
View File

@ -409,15 +409,16 @@ cfgParseOptionInfo(const int info)
}
CfgParseOptionResult
cfgParseOption(const String *optionName)
cfgParseOption(const String *const optionName, const CfgParseOptionParam param)
{
FUNCTION_TEST_BEGIN();
FUNCTION_TEST_PARAM(STRING, optionName);
FUNCTION_TEST_PARAM(BOOL, param.prefixMatch);
FUNCTION_TEST_END();
ASSERT(optionName != NULL);
// Search for the option
// Search for an exact match
unsigned int findIdx = 0;
while (optionList[findIdx].name != NULL)
@ -432,6 +433,29 @@ cfgParseOption(const String *optionName)
if (optionList[findIdx].name != NULL)
FUNCTION_TEST_RETURN(cfgParseOptionInfo(optionList[findIdx].val));
// Search for a single partial match if requested
if (param.prefixMatch)
{
unsigned int findPartialIdx = 0;
unsigned int findPartialTotal = 0;
for (findIdx = 0; findIdx < sizeof(optionList) / sizeof(struct option) - 1; findIdx++)
{
if (strBeginsWith(STR(optionList[findIdx].name), optionName))
{
findPartialIdx = findIdx;
findPartialTotal++;
if (findPartialTotal > 1)
break;
}
}
// If a single partial match was found
if (findPartialTotal == 1)
FUNCTION_TEST_RETURN(cfgParseOptionInfo(optionList[findPartialIdx].val));
}
FUNCTION_TEST_RETURN((CfgParseOptionResult){0});
}
@ -901,101 +925,62 @@ configParse(const Storage *storage, unsigned int argListSize, const char *argLis
// Phase 1: parse command line parameters
// -------------------------------------------------------------------------------------------------------------------------
int optionValue; // Value returned by getopt_long
int optionListIdx; // Index of option in list (if an option was returned)
bool argFound = false; // Track args found to decide on error or help at the end
// Reset optind to 1 in case getopt_long has been called before
optind = 1;
// Don't error automatically on unknown options - they will be processed in the loop below
opterr = false;
// List of parsed options
ParseOption parseOptionList[CFG_OPTION_TOTAL] = {{0}};
// Only the first non-option parameter should be treated as a command so track if the command has been set
bool commandSet = false;
while ((optionValue = getopt_long((int)argListSize, (char **)argList, "-:", optionList, &optionListIdx)) != -1)
for (unsigned int argListIdx = 1; argListIdx < argListSize; argListIdx++)
{
switch (optionValue)
{
// Parse arguments that are not options, i.e. commands and parameters passed to commands
case 1:
{
// The first argument should be the command
if (!commandSet)
{
const char *command = argList[optind - 1];
const char *arg = argList[argListIdx];
// Try getting the command from the valid command list
config->command = cfgCommandId(command);
config->commandRole = cfgCmdRoleMain;
// If not successful then a command role may be appended
if (config->command == cfgCmdNone)
// If an option
if (arg[0] == '-')
{
const StringList *commandPart = strLstNewSplit(STR(command), COLON_STR);
// Options must start with --
if (arg[1] != '-')
THROW_FMT(OptionInvalidError, "option '%s' must begin with --", arg);
if (strLstSize(commandPart) == 2)
// Consume --
arg += 2;
// Get the option name and the value when separated by =
const String *optionName = NULL;
const String *optionArg = NULL;
const char *const equalPtr = strchr(arg, '=');
if (equalPtr)
{
// Get command id
config->command = cfgCommandId(strZ(strLstGet(commandPart, 0)));
// If command id is valid then get command role id
if (config->command != cfgCmdNone)
config->commandRole = cfgCommandRoleEnum(strLstGet(commandPart, 1));
optionName = strNewN(arg, (size_t)(equalPtr - arg));
optionArg = strNew(equalPtr + 1);
}
}
// Error when command does not exist
if (config->command == cfgCmdNone)
THROW_FMT(CommandInvalidError, "invalid command '%s'", command);
// Error when role is not valid for the command
if (!(parseRuleCommand[config->command].commandRoleValid & ((unsigned int)1 << config->commandRole)))
THROW_FMT(CommandInvalidError, "invalid command/role combination '%s'", command);
if (config->command == cfgCmdHelp)
config->help = true;
else
commandSet = true;
}
// Additional arguments are command arguments
else
optionName = strNew(arg);
// Lookup the option name
CfgParseOptionResult option = cfgParseOptionP(optionName, true);
if (!option.found)
THROW_FMT(OptionInvalidError, "invalid option '--%s'", arg);
// If the option requires an argument
if (parseRuleOption[option.id].type != cfgOptTypeBoolean && !option.negate && !option.reset)
{
if (config->paramList == NULL)
// If no argument was found with the option then try the next argument
if (optionArg == NULL)
{
MEM_CONTEXT_BEGIN(config->memContext)
{
config->paramList = strLstNew();
// Error if there are no more arguments in the list
if (argListIdx == argListSize - 1)
THROW_FMT(OptionInvalidError, "option '--%s' requires an argument", strZ(optionName));
optionArg = strNew(argList[++argListIdx]);
}
MEM_CONTEXT_END();
}
strLstAdd(config->paramList, strNew(argList[optind - 1]));
}
break;
}
// If the option is unknown then error
case '?':
THROW_FMT(OptionInvalidError, "invalid option '%s'", argList[optind - 1]);
// If the option is missing an argument then error
case ':':
THROW_FMT(OptionInvalidError, "option '%s' requires an argument", argList[optind - 1]);
// Parse valid option
default:
{
// Get option id and flags from the option code
CfgParseOptionResult option = cfgParseOptionInfo(optionValue);
// Make sure the option id is valid
ASSERT(option.id < CFG_OPTION_TOTAL);
// Else error if an argument was found with the option
else if (optionArg != NULL)
THROW_FMT(OptionInvalidError, "option '%s' does not allow an argument", strZ(optionName));
// Error if this option is secure and cannot be passed on the command line
if (cfgParseOptionSecure(option.id))
@ -1021,17 +1006,17 @@ configParse(const Storage *storage, unsigned int argListSize, const char *argLis
.source = cfgSourceParam,
};
// Only set the argument if the option requires one
if (optionList[optionListIdx].has_arg == required_argument)
// Only set the argument if the option has one
if (optionArg != NULL)
{
optionValue->valueList = strLstNew();
strLstAdd(optionValue->valueList, STR(optarg));
strLstAdd(optionValue->valueList, optionArg);
}
}
else
{
// Make sure option is not negated more than once. It probably wouldn't hurt anything to accept this case
// but there's no point in allowing the user to be sloppy.
// Make sure option is not negated more than once. It probably wouldn't hurt anything to accept this case but
// there's no point in allowing the user to be sloppy.
if (optionValue->negate && option.negate)
{
THROW_FMT(
@ -1072,9 +1057,9 @@ configParse(const Storage *storage, unsigned int argListSize, const char *argLis
}
// Add the argument
if (optionList[optionListIdx].has_arg == required_argument && parseRuleOption[option.id].multi)
if (optionArg != NULL && parseRuleOption[option.id].multi)
{
strLstAdd(optionValue->valueList, strNew(optarg));
strLstAdd(optionValue->valueList, optionArg);
}
// Error if the option does not accept multiple arguments
else
@ -1084,20 +1069,68 @@ configParse(const Storage *storage, unsigned int argListSize, const char *argLis
cfgParseOptionKeyIdxName(option.id, option.keyIdx));
}
}
}
// Else command or parameter
else
{
// The first argument should be the command
if (!commandSet)
{
// Try getting the command from the valid command list
config->command = cfgCommandId(arg);
config->commandRole = cfgCmdRoleMain;
break;
// If not successful then a command role may be appended
if (config->command == cfgCmdNone)
{
const StringList *commandPart = strLstNewSplit(STR(arg), COLON_STR);
if (strLstSize(commandPart) == 2)
{
// Get command id
config->command = cfgCommandId(strZ(strLstGet(commandPart, 0)));
// If command id is valid then get command role id
if (config->command != cfgCmdNone)
config->commandRole = cfgCommandRoleEnum(strLstGet(commandPart, 1));
}
}
// Arg has been found
argFound = true;
// Error when command does not exist
if (config->command == cfgCmdNone)
THROW_FMT(CommandInvalidError, "invalid command '%s'", arg);
// Error when role is not valid for the command
if (!(parseRuleCommand[config->command].commandRoleValid & ((unsigned int)1 << config->commandRole)))
THROW_FMT(CommandInvalidError, "invalid command/role combination '%s'", arg);
if (config->command == cfgCmdHelp)
config->help = true;
else
commandSet = true;
}
// Additional arguments are command arguments
else
{
if (config->paramList == NULL)
{
MEM_CONTEXT_BEGIN(config->memContext)
{
config->paramList = strLstNew();
}
MEM_CONTEXT_END();
}
strLstAddZ(config->paramList, arg);
}
}
}
// Handle command not found
if (!commandSet && !config->help)
{
// If there are args then error
if (argFound)
if (argListSize > 1)
THROW_FMT(CommandRequiredError, "no command found");
// Otherwise set the command to help
@ -1139,7 +1172,7 @@ configParse(const Storage *storage, unsigned int argListSize, const char *argLis
const String *value = STR(equalPtr + 1);
// Find the option
CfgParseOptionResult option = cfgParseOption(key);
CfgParseOptionResult option = cfgParseOptionP(key);
// Warn if the option not found
if (!option.found)
@ -1240,7 +1273,7 @@ configParse(const Storage *storage, unsigned int argListSize, const char *argLis
String *key = strLstGet(keyList, keyIdx);
// Find the optionName in the main list
CfgParseOptionResult option = cfgParseOption(key);
CfgParseOptionResult option = cfgParseOptionP(key);
// Warn if the option not found
if (!option.found)

11
src/config/parse.h
View File

@ -15,6 +15,12 @@ Functions
void configParse(const Storage *storage, unsigned int argListSize, const char *argList[], bool resetLogLevel);
// Parse option name and return option info
typedef struct CfgParseOptionParam
{
VAR_PARAM_HEADER;
bool prefixMatch; // Allow prefix matches, e.g. 'stanz' for 'stanza'
} CfgParseOptionParam;
typedef struct CfgParseOptionResult
{
bool found; // Was the option found?
@ -25,7 +31,10 @@ typedef struct CfgParseOptionResult
bool deprecated; // Is the option deprecated?
} CfgParseOptionResult;
CfgParseOptionResult cfgParseOption(const String *optionName);
#define cfgParseOptionP(optionName, ...) \
cfgParseOption(optionName, (CfgParseOptionParam){VAR_PARAM_INIT, __VA_ARGS__})
CfgParseOptionResult cfgParseOption(const String *const optionName, const CfgParseOptionParam param);
// Default value for the option
const char *cfgParseOptionDefault(ConfigCommand commandId, ConfigOption optionId);

2
src/config/protocol.c
View File

@ -29,7 +29,7 @@ configOptionProtocol(const VariantList *paramList, ProtocolServer *server)
for (unsigned int optionIdx = 0; optionIdx < varLstSize(paramList); optionIdx++)
{
CfgParseOptionResult option = cfgParseOption(varStr(varLstGet(paramList, optionIdx)));
CfgParseOptionResult option = cfgParseOptionP(varStr(varLstGet(paramList, optionIdx)));
CHECK(option.found);
varLstAdd(optionList, varDup(cfgOptionIdx(option.id, cfgOptionKeyToIdx(option.id, option.keyIdx + 1))));

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

@ -21,7 +21,7 @@ Option find test -- this is done a lot in the deprecated tests
static void
testOptionFind(const char *optionName, unsigned int optionId, unsigned int optionKeyIdx, bool negate, bool reset, bool deprecated)
{
CfgParseOptionResult option = cfgParseOption(STR(optionName));
CfgParseOptionResult option = cfgParseOptionP(STR(optionName));
TEST_RESULT_BOOL(option.found, true, "check %s found", optionName);
TEST_RESULT_UINT(option.id, optionId, "check %s id %u", optionName, optionId);
@ -574,6 +574,26 @@ testRun(void)
sizeof(optionResolveOrder) / sizeof(ConfigOption), CFG_OPTION_TOTAL,
"check that the option resolve list contains an entry for every option");
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("error on single - option");
argList = strLstNew();
strLstAddZ(argList, TEST_BACKREST_EXE);
strLstAddZ(argList, "-bogus");
TEST_ERROR(
configParse(storageTest, strLstSize(argList), strLstPtr(argList), false), OptionInvalidError,
"option '-bogus' must begin with --");
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("error when option argument not allowed");
argList = strLstNew();
strLstAddZ(argList, TEST_BACKREST_EXE);
strLstAddZ(argList, "--online=bogus");
TEST_ERROR(
configParse(storageTest, strLstSize(argList), strLstPtr(argList), false), OptionInvalidError,
"option 'online' does not allow an argument");
// -------------------------------------------------------------------------------------------------------------------------
argList = strLstNew();
strLstAdd(argList, strNew(TEST_BACKREST_EXE));
@ -1739,7 +1759,7 @@ testRun(void)
testOptionFind("db-ssh-port", cfgOptPgHostPort, 0, false, false, true);
testOptionFind("db-user", cfgOptPgHostUser, 0, false, false, true);
TEST_RESULT_BOOL(cfgParseOption(STR("no-db-user")).found, false, "no-db-user not found");
TEST_RESULT_BOOL(cfgParseOptionP(STR("no-db-user")).found, false, "no-db-user not found");
// Only check 1-8 since 8 was the max index when these option names were deprecated
for (unsigned int optionIdx = 0; optionIdx < 8; optionIdx++)