Replace getopt_long() with custom implementation.

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.

doc/xml/release.xml

@@ -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"/>
 

src/command/help/help.c

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

src/config/parse.c

@@ -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,203 +925,212 @@ 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)
+            const char *arg = argList[argListIdx];
+
+            // If an option
+            if (arg[0] == '-')
             {
-                // Parse arguments that are not options, i.e. commands and parameters passed to commands
+                // Options must start with --
-                case 1:
+                if (arg[1] != '-')
+                    THROW_FMT(OptionInvalidError, "option '%s' must begin with --", arg);
+
+                // 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)
                 {
-                    // The first argument should be the command
+                    optionName = strNewN(arg, (size_t)(equalPtr - arg));
-                    if (!commandSet)
+                    optionArg = strNew(equalPtr + 1);
+                }
+                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 no argument was found with the option then try the next argument
+                    if (optionArg == NULL)
                     {
-                        const char *command = argList[optind - 1];
+                        // Error if there are no more arguments in the list
+                        if (argListIdx == argListSize - 1)
+                            THROW_FMT(OptionInvalidError, "option '--%s' requires an argument", strZ(optionName));
 
-                        // Try getting the command from the valid command list
+                        optionArg = strNew(argList[++argListIdx]);
-                        config->command = cfgCommandId(command);
-                        config->commandRole = cfgCmdRoleMain;
-
-                        // If not successful then a command role may be appended
-                        if (config->command == cfgCmdNone)
-                        {
-                            const StringList *commandPart = strLstNewSplit(STR(command), 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));
-                            }
-                        }
-
-                        // 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
+                // Else error if an argument was found with the option
-                    {
+                else if (optionArg != NULL)
-                        if (config->paramList == NULL)
+                    THROW_FMT(OptionInvalidError, "option '%s' does not allow an argument", strZ(optionName));
-                        {
-                            MEM_CONTEXT_BEGIN(config->memContext)
-                            {
-                                config->paramList = strLstNew();
-                            }
-                            MEM_CONTEXT_END();
-                        }
 
-                        strLstAdd(config->paramList, strNew(argList[optind - 1]));
+                // Error if this option is secure and cannot be passed on the command line
-                    }
+                if (cfgParseOptionSecure(option.id))
-
+                {
-                    break;
+                    THROW_FMT(
+                        OptionInvalidError,
+                        "option '%s' is not allowed on the command-line\n"
+                        "HINT: this option could expose secrets in the process list.\n"
+                        "HINT: specify the option in a configuration file or an environment variable instead.",
+                        cfgParseOptionKeyIdxName(option.id, option.keyIdx));
                 }
 
-                // If the option is unknown then error
+                // If the option has not been found yet then set it
-                case '?':
+                ParseOptionValue *optionValue = parseOptionIdxValue(parseOptionList, option.id, option.keyIdx);
-                    THROW_FMT(OptionInvalidError, "invalid option '%s'", argList[optind - 1]);
 
-                // If the option is missing an argument then error
+                if (!optionValue->found)
-                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
+                    *optionValue = (ParseOptionValue)
-                    CfgParseOptionResult option = cfgParseOptionInfo(optionValue);
+                    {
+                        .found = true,
+                        .negate = option.negate,
+                        .reset = option.reset,
+                        .source = cfgSourceParam,
+                    };
 
-                    // Make sure the option id is valid
+                    // Only set the argument if the option has one
-                    ASSERT(option.id < CFG_OPTION_TOTAL);
+                    if (optionArg != NULL)
-
+                    {
-                    // Error if this option is secure and cannot be passed on the command line
+                        optionValue->valueList = strLstNew();
-                    if (cfgParseOptionSecure(option.id))
+                        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.
+                    if (optionValue->negate && option.negate)
                     {
                         THROW_FMT(
-                            OptionInvalidError,
+                            OptionInvalidError, "option '%s' is negated multiple times",
-                            "option '%s' is not allowed on the command-line\n"
-                            "HINT: this option could expose secrets in the process list.\n"
-                            "HINT: specify the option in a configuration file or an environment variable instead.",
                             cfgParseOptionKeyIdxName(option.id, option.keyIdx));
                     }
 
-                    // If the option has not been found yet then set it
+                    // Make sure option is not reset more than once. Same justification as negate.
-                    ParseOptionValue *optionValue = parseOptionIdxValue(parseOptionList, option.id, option.keyIdx);
+                    if (optionValue->reset && option.reset)
-
-                    if (!optionValue->found)
                     {
-                        *optionValue = (ParseOptionValue)
+                        THROW_FMT(
-                        {
+                            OptionInvalidError, "option '%s' is reset multiple times",
-                            .found = true,
+                            cfgParseOptionKeyIdxName(option.id, option.keyIdx));
-                            .negate = option.negate,
-                            .reset = option.reset,
-                            .source = cfgSourceParam,
-                        };
-
-                        // Only set the argument if the option requires one
-                        if (optionList[optionListIdx].has_arg == required_argument)
-                        {
-                            optionValue->valueList = strLstNew();
-                            strLstAdd(optionValue->valueList, STR(optarg));
-                        }
                     }
+
+                    // Don't allow an option to be both negated and reset
+                    if ((optionValue->reset && option.negate) || (optionValue->negate && option.reset))
+                    {
+                        THROW_FMT(
+                            OptionInvalidError, "option '%s' cannot be negated and reset",
+                            cfgParseOptionKeyIdxName(option.id, option.keyIdx));
+                    }
+
+                    // Don't allow an option to be both set and negated
+                    if (optionValue->negate != option.negate)
+                    {
+                        THROW_FMT(
+                            OptionInvalidError, "option '%s' cannot be set and negated",
+                            cfgParseOptionKeyIdxName(option.id, option.keyIdx));
+                    }
+
+                    // Don't allow an option to be both set and reset
+                    if (optionValue->reset != option.reset)
+                    {
+                        THROW_FMT(
+                            OptionInvalidError, "option '%s' cannot be set and reset",
+                            cfgParseOptionKeyIdxName(option.id, option.keyIdx));
+                    }
+
+                    // Add the argument
+                    if (optionArg != NULL && parseRuleOption[option.id].multi)
+                    {
+                        strLstAdd(optionValue->valueList, optionArg);
+                    }
+                    // Error if the option does not accept multiple arguments
                     else
                     {
-                        // Make sure option is not negated more than once.  It probably wouldn't hurt anything to accept this case
+                        THROW_FMT(
-                        // but there's no point in allowing the user to be sloppy.
+                            OptionInvalidError, "option '%s' cannot be set multiple times",
-                        if (optionValue->negate && option.negate)
+                            cfgParseOptionKeyIdxName(option.id, option.keyIdx));
-                        {
+                    }
-                            THROW_FMT(
+                }
-                                OptionInvalidError, "option '%s' is negated multiple times",
+            }
-                                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;
 
-                        // Make sure option is not reset more than once.  Same justification as negate.
+                    // If not successful then a command role may be appended
-                        if (optionValue->reset && option.reset)
+                    if (config->command == cfgCmdNone)
-                        {
+                    {
-                            THROW_FMT(
+                        const StringList *commandPart = strLstNewSplit(STR(arg), COLON_STR);
-                                OptionInvalidError, "option '%s' is reset multiple times",
-                                cfgParseOptionKeyIdxName(option.id, option.keyIdx));
-                        }
 
-                        // Don't allow an option to be both negated and reset
+                        if (strLstSize(commandPart) == 2)
-                        if ((optionValue->reset && option.negate) || (optionValue->negate && option.reset))
                         {
-                            THROW_FMT(
+                            // Get command id
-                                OptionInvalidError, "option '%s' cannot be negated and reset",
+                            config->command = cfgCommandId(strZ(strLstGet(commandPart, 0)));
-                                cfgParseOptionKeyIdxName(option.id, option.keyIdx));
-                        }
 
-                        // Don't allow an option to be both set and negated
+                            // If command id is valid then get command role id
-                        if (optionValue->negate != option.negate)
+                            if (config->command != cfgCmdNone)
-                        {
+                                config->commandRole = cfgCommandRoleEnum(strLstGet(commandPart, 1));
-                            THROW_FMT(
-                                OptionInvalidError, "option '%s' cannot be set and negated",
-                                cfgParseOptionKeyIdxName(option.id, option.keyIdx));
-                        }
-
-                        // Don't allow an option to be both set and reset
-                        if (optionValue->reset != option.reset)
-                        {
-                            THROW_FMT(
-                                OptionInvalidError, "option '%s' cannot be set and reset",
-                                cfgParseOptionKeyIdxName(option.id, option.keyIdx));
-                        }
-
-                        // Add the argument
-                        if (optionList[optionListIdx].has_arg == required_argument && parseRuleOption[option.id].multi)
-                        {
-                            strLstAdd(optionValue->valueList, strNew(optarg));
-                        }
-                        // Error if the option does not accept multiple arguments
-                        else
-                        {
-                            THROW_FMT(
-                                OptionInvalidError, "option '%s' cannot be set multiple times",
-                                cfgParseOptionKeyIdxName(option.id, option.keyIdx));
                         }
                     }
 
-                    break;
+                    // 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);
                 }
             }
-
-            // Arg has been found
-            argFound = true;
         }
 
         // 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)

src/config/parse.h

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

src/config/protocol.c

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

test/src/module/config/parseTest.c

@@ -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++)