Split cfgLoad() into multiple functions to make testing easier.

Mainly this helps with unit tests that need to do log expect testing. Add harnessCfgLoad() test function, which allows a new config to be loaded for unit testing without resetting log functions, opening a log file, or taking locks.

doc/xml/release.xml

@@ -47,6 +47,10 @@
                         <p>Storage object improvements.  Convert all functions to variadic functions.  Enforce read-only storage. Add <code>storageLocalWrite()</code> helper function.  Add <code>storageExists()</code>, <code>storagePathCreate()</code>, and <code>storageRemove()</code>.  Add <code>StorageFile</code> object and <code>storageOpenRead()</code>/<code>storageOpenWrite()</code>.  Abstract Posix driver code into a separate module.  Add <code>storagePathRemove()</code> and use it in the Perl Posix driver.</p>
                     </release-item>
 
+                    <release-item>
+                        <p>Split <code>cfgLoad()</code> into multiple functions to make testing easier.  Mainly this helps with unit tests that need to do log expect testing.</p>
+                    </release-item>
+
                     <release-item>
                         <p>Allow <code>MemContext</code> objects to be copied to a new parent.  This makes it easier to create objects and then copy them to another context when they are complete without having to worry about freeing them on error.  Update <code>List</code>, <code>StringList</code>, and <code>Buffer</code> to allow moves.  Update <code>Ini</code> and <code>Storage</code> to take advantage of moves.</p>
                     </release-item>
@@ -128,6 +132,10 @@
                         <p>Move <id>archive-stop</id> and <id>expire</id> tests to the <id>mock</id> module.  These are mock integration tests so they should be grouped with the other mock integration tests.</p>
                     </release-item>
 
+                    <release-item>
+                        <p>Add <code>harnessCfgLoad()</code> test function, which allows a new config to be loaded for unit testing without resetting log functions, opening a log file, or taking locks.</p>
+                    </release-item>
+
                     <release-item>
                         <p>Include VM type in <id>gcov</id> path to avoid conflicts between VMs with different architectures.</p>
                     </release-item>

libc/LibC.xs

@@ -47,6 +47,7 @@ These includes are from the src directory.  There is no Perl-specific code in th
 #include "config/config.h"
 #include "config/define.h"
 #include "config/load.h"
+#include "config/parse.h"
 #include "perl/config.h"
 #include "postgres/pageChecksum.h"
 #include "storage/driver/posix.h"

libc/xs/config/configTest.xs

@@ -19,7 +19,11 @@ CODE:
         // This should run in a temp context but for some reason getopt_long gets upset when if gets called again after the previous
         // arg list being freed.  So, this is a memory leak but it is only used for testing, not production.
         StringList *paramList = strLstNewSplitZ(strCat(strNew("pgbackrest|"), parseParam), "|");
-        cfgLoadParam(strLstSize(paramList), strLstPtr(paramList), strNew(backrestBin));
+
+        // Don't use cfgLoad() because it has a lot of side effects that we don't want
+        configParse(strLstSize(paramList), strLstPtr(paramList));
+        cfgExeSet(strNew(backrestBin));
+        cfgLoadUpdateOption();
 
         String *result = perlOptionJson();
 

src/config/load.c

@@ -46,43 +46,11 @@ cfgLoadLogSetting()
 }
 
 /***********************************************************************************************************************************
-Load the configuration
+Update options that have complex rules
 ***********************************************************************************************************************************/
 void
-cfgLoad(unsigned int argListSize, const char *argList[])
+cfgLoadUpdateOption()
 {
-    cfgLoadParam(argListSize, argList, NULL);
-}
-
-void
-cfgLoadParam(unsigned int argListSize, const char *argList[], String *exe)
-{
-    MEM_CONTEXT_TEMP_BEGIN()
-    {
-        // Parse config from command line and config file
-        configParse(argListSize, argList);
-
-        // Load the log settings
-        cfgLoadLogSetting();
-
-        // Only continue if a command was set.  If no command is set then help will be displayed
-        if (cfgCommand() != cfgCmdNone)
-        {
-            // Open the log file if this command logs to a file
-            if (cfgLogFile())
-            {
-                logFileSet(
-                    strPtr(strNewFmt("%s/%s-%s.log", strPtr(cfgOptionStr(cfgOptLogPath)), strPtr(cfgOptionStr(cfgOptStanza)),
-                    cfgCommandName(cfgCommand()))));
-            }
-
-            // Begin the command
-            cmdBegin(true);
-
-            // If an exe was passed in then use it
-            if (exe != NULL)
-                cfgExeSet(exe);
-
     // Set default for repo-host-cmd
     if (cfgOptionValid(cfgOptRepoHost) && cfgOptionTest(cfgOptRepoHost) &&
         cfgOptionSource(cfgOptRepoHostCmd) == cfgSourceDefault)
@@ -100,14 +68,6 @@ cfgLoadParam(unsigned int argListSize, const char *argList[], String *exe)
         }
     }
 
-            if (cfgLockRequired())
-                lockAcquire(cfgOptionStr(cfgOptLockPath), cfgOptionStr(cfgOptStanza), cfgLockType(), 0, true);
-        }
-
-        // Neutralize the umask to make the repository file/path modes more consistent
-        if (cfgOptionValid(cfgOptNeutralUmask) && cfgOptionBool(cfgOptNeutralUmask))
-            umask(0000);
-
     // Protocol timeout should be greater than db timeout
     if (cfgOptionTest(cfgOptDbTimeout) && cfgOptionTest(cfgOptProtocolTimeout) &&
         cfgOptionDbl(cfgOptProtocolTimeout) <= cfgOptionDbl(cfgOptDbTimeout))
@@ -230,6 +190,47 @@ cfgLoadParam(unsigned int argListSize, const char *argList[], String *exe)
             }
         }
     }
+}
+
+/***********************************************************************************************************************************
+Load the configuration
+***********************************************************************************************************************************/
+void
+cfgLoad(unsigned int argListSize, const char *argList[])
+{
+    MEM_CONTEXT_TEMP_BEGIN()
+    {
+        // Parse config from command line and config file
+        configParse(argListSize, argList);
+
+        // Load the log settings
+        cfgLoadLogSetting();
+
+        // Neutralize the umask to make the repository file/path modes more consistent
+        if (cfgOptionValid(cfgOptNeutralUmask) && cfgOptionBool(cfgOptNeutralUmask))
+            umask(0000);
+
+        // If a command is set
+        if (cfgCommand() != cfgCmdNone)
+        {
+            // Acquire a lock if required
+            if (cfgLockRequired())
+                lockAcquire(cfgOptionStr(cfgOptLockPath), cfgOptionStr(cfgOptStanza), cfgLockType(), 0, true);
+
+            // Open the log file if this command logs to a file
+            if (cfgLogFile())
+            {
+                logFileSet(
+                    strPtr(strNewFmt("%s/%s-%s.log", strPtr(cfgOptionStr(cfgOptLogPath)), strPtr(cfgOptionStr(cfgOptStanza)),
+                    cfgCommandName(cfgCommand()))));
+            }
+
+            // Begin the command
+            cmdBegin(true);
+
+            // Update options that have complex rules
+            cfgLoadUpdateOption();
+        }
     }
     MEM_CONTEXT_TEMP_END();
 }

src/config/load.h

@@ -8,7 +8,7 @@ Configuration Load
 Functions
 ***********************************************************************************************************************************/
 void cfgLoad(unsigned int argListSize, const char *argList[]);
-void cfgLoadParam(unsigned int argListSize, const char *argList[], String *exe);
 void cfgLoadLogSetting();
+void cfgLoadUpdateOption();
 
 #endif

test/lib/pgBackRestTest/Env/ConfigEnvTest.pm

@@ -186,9 +186,6 @@ sub configTestLoad
     $self->testResult(
         sub {configLoad(false, backrestBin(), cfgCommandName($iCommandId), \$strConfigJson)},
         true, 'config load: ' . join(" ", @stryArg));
-
-    # Release any lock that was taken during configLoad.
-    lockRelease(false);
 }
 
 1;

test/src/common/harnessConfig.c

@@ -0,0 +1,21 @@
+/***********************************************************************************************************************************
+Harness for Loading Test Configurations
+***********************************************************************************************************************************/
+#include "common/logTest.h"
+
+#include "config/load.h"
+#include "config/parse.h"
+
+/***********************************************************************************************************************************
+Load a test configuration without any side effects
+
+Log testing requires that log levels be set in a certain way but calls to cfgLoad() will reset that.  Also there's no need to open
+log files, acquire locks, etc.
+***********************************************************************************************************************************/
+void
+harnessCfgLoad(unsigned int argListSize, const char *argList[])
+{
+    configParse(argListSize, argList);
+    logInit(logLevelInfo, logLevelOff, logLevelOff, false);
+    cfgLoadUpdateOption();
+}

test/src/common/harnessConfig.h

@@ -0,0 +1,8 @@
+/***********************************************************************************************************************************
+Harness for Loading Test Configurations
+***********************************************************************************************************************************/
+
+/***********************************************************************************************************************************
+Functions
+***********************************************************************************************************************************/
+void harnessCfgLoad(unsigned int argListSize, const char *argList[]);

test/src/common/logTest.c

@@ -11,6 +11,11 @@ Log Test Harness
 
 #ifndef NO_LOG
 
+/***********************************************************************************************************************************
+Has the log harness been init'd?
+***********************************************************************************************************************************/
+static bool harnessLogInit = false;
+
 /***********************************************************************************************************************************
 Name of file where logs are stored for testing
 ***********************************************************************************************************************************/
@@ -23,6 +28,8 @@ Initialize log for testing
 void
 testLogInit()
 {
+    if (!harnessLogInit)
+    {
         logInit(logLevelInfo, logLevelOff, logLevelOff, false);
 
         stdoutFile = strNewFmt("%s/stdout.log", testPath());
@@ -30,6 +37,9 @@ testLogInit()
 
         stderrFile = strNewFmt("%s/stderr.log", testPath());
         logHandleStdErr = open(strPtr(stderrFile), O_WRONLY | O_CREAT | O_TRUNC, 0640);
+
+        harnessLogInit = true;
+    }
 }
 
 /***********************************************************************************************************************************

test/src/module/archive/pushTest.c

@@ -6,6 +6,8 @@ Test Archive Push Command
 #include "config/load.h"
 #include "version.h"
 
+#include "common/harnessConfig.h"
+
 /***********************************************************************************************************************************
 Test Run
 ***********************************************************************************************************************************/
@@ -22,8 +24,7 @@ testRun()
         strLstAddZ(argList, "--archive-timeout=1");
         strLstAddZ(argList, "--stanza=db");
         strLstAddZ(argList, "archive-push");
-        cfgLoad(strLstSize(argList), strLstPtr(argList));
+        harnessCfgLoad(strLstSize(argList), strLstPtr(argList));
-        logInit(logLevelInfo, logLevelOff, logLevelOff, false);
 
         // -------------------------------------------------------------------------------------------------------------------------
         String *segment = strNew("000000010000000100000001");
@@ -114,8 +115,7 @@ testRun()
         strLstAddZ(argList, "--log-level-console=off");
         strLstAddZ(argList, "--log-level-stderr=off");
         strLstAdd(argList, strNewFmt("--pg1-path=%s/db", testPath()));
-        cfgLoad(strLstSize(argList), strLstPtr(argList));
+        harnessCfgLoad(strLstSize(argList), strLstPtr(argList));
-        logInit(logLevelInfo, logLevelOff, logLevelOff, false);
 
         TEST_ERROR(
             cmdArchivePush(), ArchiveTimeoutError,

test/src/module/config/loadTest.c

@@ -42,11 +42,6 @@ testRun()
         TEST_RESULT_INT(logLevelStdErr, logLevelError, "stderr logging is error");
         TEST_RESULT_INT(logLevelFile, logLevelOff, "file logging is off");
 
-        // -------------------------------------------------------------------------------------------------------------------------
-        TEST_RESULT_VOID(cfgLoadParam(strLstSize(argList), strLstPtr(argList), strNew("pgbackrest2")), "load local config");
-
-        TEST_RESULT_STR(strPtr(cfgExe()), "pgbackrest2", "check exe");
-
         // -------------------------------------------------------------------------------------------------------------------------
         argList = strLstNew();
         strLstAdd(argList, strNew("pgbackrest"));