Validate configuration options in a single pass.

By pre-calculating and storing the option dependencies in parse.auto.c validation can be completed in a single pass, which is both simpler and faster.

build/lib/pgBackRestBuild/Config/BuildParse.pm

@@ -28,6 +28,7 @@ use pgBackRestBuild::Config::Data;
 use constant BLDLCL_FILE_DEFINE                                     => 'parse';
 
 use constant BLDLCL_DATA_OPTION                                     => '01-dataOption';
+use constant BLDLCL_DATA_OPTION_RESOLVE                             => '01-dataOptionResolve';
 
 ####################################################################################################################################
 # Definitions for constants and data to build
@@ -48,6 +49,11 @@ my $rhBuild =
                 {
                     &BLD_SUMMARY => 'Option parse data',
                 },
+
+                &BLDLCL_DATA_OPTION_RESOLVE =>
+                {
+                    &BLD_SUMMARY => 'Order for option parse resolution',
+                },
             },
         },
     },
@@ -171,6 +177,75 @@ sub buildConfigParse
 
     $rhBuild->{&BLD_FILE}{&BLDLCL_FILE_DEFINE}{&BLD_DATA}{&BLDLCL_DATA_OPTION}{&BLD_SOURCE} = $strBuildSource;
 
+    # Build option resolve order list.  This allows the option validation in C to take place in a single pass.
+    #
+    # Always process the stanza option first since confusing error message are produced if it is missing.
+    #-------------------------------------------------------------------------------------------------------------------------------
+    my @stryResolveOrder = (buildConfigOptionEnum(CFGOPT_STANZA));
+    my $rhResolved = {&CFGOPT_STANZA => true};
+    my $bAllResolved;
+
+    do
+    {
+        # Assume that we will finish on this loop
+        $bAllResolved = true;
+
+        # Loop through all options
+        foreach my $strOption (sort(keys(%{$rhConfigDefine})))
+        {
+            my $bSkip = false;
+
+            # Check the default depend
+            my $strOptionDepend =
+                ref($rhConfigDefine->{$strOption}{&CFGDEF_DEPEND}) eq 'HASH' ?
+                    $rhConfigDefine->{$strOption}{&CFGDEF_DEPEND}{&CFGDEF_DEPEND_OPTION} :
+                    $rhConfigDefine->{$strOption}{&CFGDEF_DEPEND};
+
+            if (defined($strOptionDepend) && !$rhResolved->{$strOptionDepend})
+            {
+                # &log(WARN, "$strOptionDepend is not resolved");
+                $bSkip = true;
+            }
+
+            # Check the command depends
+            foreach my $strCommand (sort(keys(%{$rhConfigDefine->{$strOption}{&CFGDEF_COMMAND}})))
+            {
+                my $strOptionDepend =
+                    ref($rhConfigDefine->{$strOption}{&CFGDEF_COMMAND}{$strCommand}{&CFGDEF_DEPEND}) eq 'HASH' ?
+                        $rhConfigDefine->{$strOption}{&CFGDEF_COMMAND}{$strCommand}{&CFGDEF_DEPEND}{&CFGDEF_DEPEND_OPTION} :
+                        $rhConfigDefine->{$strOption}{&CFGDEF_COMMAND}{$strCommand}{&CFGDEF_DEPEND};
+
+                if (defined($strOptionDepend) && !$rhResolved->{$strOptionDepend})
+                {
+                    $bSkip = true;
+                }
+            }
+
+            # If dependency was not found try again on the next loop
+            if ($bSkip)
+            {
+                $bAllResolved = false;
+            }
+            # Else add option to resolve order list
+            elsif (!$rhResolved->{$strOption})
+            {
+                $rhResolved->{$strOption} = true;
+
+                for (my $iIndex = 0; $iIndex < $rhConfigDefine->{$strOption}{&CFGDEF_INDEX_TOTAL}; $iIndex++)
+                {
+                    push(@stryResolveOrder, buildConfigOptionEnum($strOption) . ($iIndex == 0 ? '' : " + $iIndex"));
+                }
+            }
+        }
+    }
+    while (!$bAllResolved);
+
+    $rhBuild->{&BLD_FILE}{&BLDLCL_FILE_DEFINE}{&BLD_DATA}{&BLDLCL_DATA_OPTION_RESOLVE}{&BLD_SOURCE} =
+        "static const ConfigOption optionResolveOrder[CFG_OPTION_TOTAL] =\n" .
+        "{\n" .
+        "    " . join(",\n    ", @stryResolveOrder) . ",\n" .
+        "};\n";
+
     return $rhBuild;
 }
 

doc/xml/release.xml

@@ -45,6 +45,10 @@
                 </release-improvement-list>
 
                 <release-development-list>
+                    <release-item>
+                        <p>Validate configuration options in a single pass.  By pre-calculating and storing the option dependencies in <file>parse.auto.c</file> validation can be completed in a single pass, which is both simpler and faster.</p>
+                    </release-item>
+
                     <release-item>
                         <release-item-contributor-list>
                             <release-item-ideator id="frost.stephen"/>

src/config/parse.auto.c

@@ -2190,3 +2190,171 @@ static const struct option optionList[] =
         .name = NULL
     }
 };
+
+/***********************************************************************************************************************************
+Order for option parse resolution
+***********************************************************************************************************************************/
+static const ConfigOption optionResolveOrder[CFG_OPTION_TOTAL] =
+{
+    cfgOptStanza,
+    cfgOptArchiveAsync,
+    cfgOptArchiveGetQueueMax,
+    cfgOptArchivePushQueueMax,
+    cfgOptArchiveTimeout,
+    cfgOptBackupStandby,
+    cfgOptBufferSize,
+    cfgOptChecksumPage,
+    cfgOptCmdSsh,
+    cfgOptCommand,
+    cfgOptCompress,
+    cfgOptCompressLevel,
+    cfgOptCompressLevelNetwork,
+    cfgOptConfig,
+    cfgOptConfigIncludePath,
+    cfgOptConfigPath,
+    cfgOptDbInclude,
+    cfgOptDbTimeout,
+    cfgOptDelta,
+    cfgOptHostId,
+    cfgOptLinkAll,
+    cfgOptLinkMap,
+    cfgOptLockPath,
+    cfgOptLogLevelConsole,
+    cfgOptLogLevelFile,
+    cfgOptLogLevelStderr,
+    cfgOptLogPath,
+    cfgOptLogTimestamp,
+    cfgOptManifestSaveThreshold,
+    cfgOptNeutralUmask,
+    cfgOptOnline,
+    cfgOptOutput,
+    cfgOptPerlOption,
+    cfgOptPgHost,
+    cfgOptPgHost + 1,
+    cfgOptPgHost + 2,
+    cfgOptPgHost + 3,
+    cfgOptPgHost + 4,
+    cfgOptPgHost + 5,
+    cfgOptPgHost + 6,
+    cfgOptPgHost + 7,
+    cfgOptPgHostCmd,
+    cfgOptPgHostCmd + 1,
+    cfgOptPgHostCmd + 2,
+    cfgOptPgHostCmd + 3,
+    cfgOptPgHostCmd + 4,
+    cfgOptPgHostCmd + 5,
+    cfgOptPgHostCmd + 6,
+    cfgOptPgHostCmd + 7,
+    cfgOptPgHostConfig,
+    cfgOptPgHostConfig + 1,
+    cfgOptPgHostConfig + 2,
+    cfgOptPgHostConfig + 3,
+    cfgOptPgHostConfig + 4,
+    cfgOptPgHostConfig + 5,
+    cfgOptPgHostConfig + 6,
+    cfgOptPgHostConfig + 7,
+    cfgOptPgHostConfigIncludePath,
+    cfgOptPgHostConfigIncludePath + 1,
+    cfgOptPgHostConfigIncludePath + 2,
+    cfgOptPgHostConfigIncludePath + 3,
+    cfgOptPgHostConfigIncludePath + 4,
+    cfgOptPgHostConfigIncludePath + 5,
+    cfgOptPgHostConfigIncludePath + 6,
+    cfgOptPgHostConfigIncludePath + 7,
+    cfgOptPgHostConfigPath,
+    cfgOptPgHostConfigPath + 1,
+    cfgOptPgHostConfigPath + 2,
+    cfgOptPgHostConfigPath + 3,
+    cfgOptPgHostConfigPath + 4,
+    cfgOptPgHostConfigPath + 5,
+    cfgOptPgHostConfigPath + 6,
+    cfgOptPgHostConfigPath + 7,
+    cfgOptPgHostPort,
+    cfgOptPgHostPort + 1,
+    cfgOptPgHostPort + 2,
+    cfgOptPgHostPort + 3,
+    cfgOptPgHostPort + 4,
+    cfgOptPgHostPort + 5,
+    cfgOptPgHostPort + 6,
+    cfgOptPgHostPort + 7,
+    cfgOptPgHostUser,
+    cfgOptPgHostUser + 1,
+    cfgOptPgHostUser + 2,
+    cfgOptPgHostUser + 3,
+    cfgOptPgHostUser + 4,
+    cfgOptPgHostUser + 5,
+    cfgOptPgHostUser + 6,
+    cfgOptPgHostUser + 7,
+    cfgOptPgPath,
+    cfgOptPgPath + 1,
+    cfgOptPgPath + 2,
+    cfgOptPgPath + 3,
+    cfgOptPgPath + 4,
+    cfgOptPgPath + 5,
+    cfgOptPgPath + 6,
+    cfgOptPgPath + 7,
+    cfgOptPgPort,
+    cfgOptPgPort + 1,
+    cfgOptPgPort + 2,
+    cfgOptPgPort + 3,
+    cfgOptPgPort + 4,
+    cfgOptPgPort + 5,
+    cfgOptPgPort + 6,
+    cfgOptPgPort + 7,
+    cfgOptPgSocketPath,
+    cfgOptPgSocketPath + 1,
+    cfgOptPgSocketPath + 2,
+    cfgOptPgSocketPath + 3,
+    cfgOptPgSocketPath + 4,
+    cfgOptPgSocketPath + 5,
+    cfgOptPgSocketPath + 6,
+    cfgOptPgSocketPath + 7,
+    cfgOptProcess,
+    cfgOptProcessMax,
+    cfgOptProtocolTimeout,
+    cfgOptRepoCipherType,
+    cfgOptRepoHardlink,
+    cfgOptRepoHost,
+    cfgOptRepoHostCmd,
+    cfgOptRepoHostConfig,
+    cfgOptRepoHostConfigIncludePath,
+    cfgOptRepoHostConfigPath,
+    cfgOptRepoHostPort,
+    cfgOptRepoHostUser,
+    cfgOptRepoPath,
+    cfgOptRepoRetentionArchive,
+    cfgOptRepoRetentionArchiveType,
+    cfgOptRepoRetentionDiff,
+    cfgOptRepoRetentionFull,
+    cfgOptRepoType,
+    cfgOptResume,
+    cfgOptSet,
+    cfgOptSpoolPath,
+    cfgOptStartFast,
+    cfgOptStopAuto,
+    cfgOptTablespaceMap,
+    cfgOptTablespaceMapAll,
+    cfgOptTest,
+    cfgOptTestDelay,
+    cfgOptTestPoint,
+    cfgOptType,
+    cfgOptArchiveCheck,
+    cfgOptArchiveCopy,
+    cfgOptForce,
+    cfgOptRecoveryOption,
+    cfgOptRepoCipherPass,
+    cfgOptRepoS3Bucket,
+    cfgOptRepoS3CaFile,
+    cfgOptRepoS3CaPath,
+    cfgOptRepoS3Endpoint,
+    cfgOptRepoS3Host,
+    cfgOptRepoS3Key,
+    cfgOptRepoS3KeySecret,
+    cfgOptRepoS3Region,
+    cfgOptRepoS3Token,
+    cfgOptRepoS3VerifySsl,
+    cfgOptTarget,
+    cfgOptTargetAction,
+    cfgOptTargetExclusive,
+    cfgOptTargetTimeline,
+};

src/config/parse.c

@@ -674,27 +674,11 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
 
             // Phase 3: validate option definitions and load into configuration
             // ---------------------------------------------------------------------------------------------------------------------
-            bool allResolved = false;
+            for (unsigned int optionOrderIdx = 0; optionOrderIdx < CFG_OPTION_TOTAL; optionOrderIdx++)
-            bool optionResolved[CFG_OPTION_TOTAL] = {false};
-            ConfigOption optionId = 0;
-
-            // If stanza is required but not set then many other options will likely not be set unless they were specified on the
-            // command line, which leads to confusing errors like 'command requires option: pg1-path' when what is really missing is
-            // the stanza.  In this case start start the loop at the stanza option so the user gets a sensible error.
-            if (!parseOptionList[cfgOptStanza].found && cfgDefOptionRequired(commandDefId, cfgDefOptStanza))
-                optionId = cfgOptStanza;
-
-            do
             {
-                // If first loop starts partway through we know that all options cannot possibly be resolved on the first loop.
+                // Validate options based on the option resolve order.  This allows resolving all options in a single pass.
-                // After that we'll assume that all options will be resolved in the next loop and set allResolved = false if we find
+                ConfigOption optionId = optionResolveOrder[optionOrderIdx];
-                // something that can't be resolved yet.
-                if (optionId == 0)
-                    allResolved = true;
 
-                // Loop through all options
-                for (; optionId < CFG_OPTION_TOTAL; optionId++)
-                {
                 // Get the option data parsed from the command-line
                 ParseOption *parseOption = &parseOptionList[optionId];
 
@@ -702,10 +686,6 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
                 ConfigDefineOption optionDefId = cfgOptionDefIdFromId(optionId);
                 ConfigDefineOptionType optionDefType = cfgDefOptionType(optionDefId);
 
-                    // Skip this option if it has already been resolved
-                    if (optionResolved[optionId])
-                        continue;
-
                 // Error if the option is not valid for this command
                 if (parseOption->found && !cfgDefOptionValid(commandDefId, optionDefId))
                 {
@@ -733,14 +713,11 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
                     THROW_FMT(OptionInvalidError, "option '%s' cannot have multiple arguments", cfgOptionName(optionId));
                 }
 
-                    // Is the option valid for this command?  If not, mark it as resolved since there is nothing more to do.
+                // Is the option valid for this command?  If not, there is nothing more to do.
                 cfgOptionValidSet(optionId, cfgDefOptionValid(commandDefId, optionDefId));
 
                 if (!cfgOptionValid(optionId))
-                    {
-                        optionResolved[optionId] = true;
                     continue;
-                    }
 
                 // Is the value set for this option?
                 bool optionSet =
@@ -763,13 +740,6 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
                     ConfigDefineOption dependOptionDefId = cfgOptionDefIdFromId(dependOptionId);
                     ConfigDefineOptionType dependOptionDefType = cfgDefOptionType(dependOptionDefId);
 
-                        // Make sure the depend option has been resolved, otherwise skip this option for now
-                        if (!optionResolved[dependOptionId])
-                        {
-                            allResolved = false;
-                            continue;
-                        }
-
                     // Get the depend option value
                     const Variant *dependValue = cfgOption(dependOptionId);
 
@@ -789,7 +759,8 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
                     {
                         dependResolved = false;
 
-                            // if (optionSet)
+                        // If depend not resolved and option value is set on the command-line then error.  See unresolved list
+                        // depend below for a detailed explanation.
                         if (optionSet && parseOption->source == cfgSourceParam)
                         {
                             THROW_FMT(
@@ -870,8 +841,11 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
                     }
                 }
 
-                    // Is the option defined?
+                // Is the option resolved?
-                    if (optionSet && dependResolved)
+                if (dependResolved)
+                {
+                    // Is the option set?
+                    if (optionSet)
                     {
                         if (optionDefType == cfgDefOptTypeBoolean)
                         {
@@ -962,10 +936,10 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
                             cfgOptionSet(optionId, parseOption->source, varNewStr(value));
                         }
                     }
-                    else if (dependResolved && parseOption->negate)
+                    else if (parseOption->negate)
                         cfgOptionSet(optionId, parseOption->source, NULL);
                     // Else try to set a default
-                    else if (dependResolved)
+                    else
                     {
                         // Get the default value for this option
                         const char *value = cfgDefOptionDefault(commandDefId, optionDefId);
@@ -985,15 +959,8 @@ configParse(unsigned int argListSize, const char *argList[], bool resetLogLevel)
                                 cfgOptionName(optionId), hint);
                         }
                     }
-
-                    // Option is now resolved
-                    optionResolved[optionId] = true;
                 }
-
-                // Restart option loop
-                optionId = 0;
             }
-            while (!allResolved);
         }
     }
     MEM_CONTEXT_TEMP_END();

test/src/module/config/parseTest.c

@@ -552,6 +552,10 @@ testRun(void)
         StringList *argList = NULL;
         String *configFile = strNewFmt("%s/test.config", testPath());
 
+        TEST_RESULT_INT(
+            sizeof(optionResolveOrder) / sizeof(ConfigOption), CFG_OPTION_TOTAL,
+            "check that the option resolve list contains an entry for every option");
+
         // -------------------------------------------------------------------------------------------------------------------------
         argList = strLstNew();
         strLstAdd(argList, strNew(TEST_BACKREST_EXE));
@@ -718,6 +722,8 @@ testRun(void)
         strLstAdd(argList, strNew("--stanza=db"));
         strLstAdd(argList, strNew("--repo1-type=s3"));
         strLstAdd(argList, strNew("--repo1-s3-key=xxx"));
+        strLstAdd(argList, strNew("--repo1-s3-bucket=xxx"));
+        strLstAdd(argList, strNew("--repo1-s3-endpoint=xxx"));
         strLstAdd(argList, strNew(TEST_COMMAND_BACKUP));
         TEST_ERROR(
             configParse(strLstSize(argList), strLstPtr(argList), false), OptionInvalidError,