From 5b64c93e8b1de010176b8d3927f80cc8039f4dbc Mon Sep 17 00:00:00 2001 From: David Steele Date: Fri, 20 Sep 2019 17:50:49 -0400 Subject: [PATCH] Add local option for cfgExecParam(). cfgExecParam() was originally written to provide options for remote processes. Remotes processes do not have access to the local config so it was necessary to pass every non-default option. Local processes on the other hand, e.g. archive-get, archive-get-async, archive-push-async, and local, do have access to the local config and therefore don't need every parameter to be passed on the command-line. The previous way was not wrong, but it was overly verbose and did not align with the way Perl had worked. Update cfgExecParam() to accept a local option which excludes options from the command line which can be read from local configs. --- src/command/archive/get/get.c | 2 +- src/command/archive/push/push.c | 2 +- src/config/exec.c | 5 +++-- src/config/exec.h | 2 +- src/protocol/helper.c | 4 ++-- test/src/module/config/execTest.c | 7 +++++-- 6 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/command/archive/get/get.c b/src/command/archive/get/get.c index 7a5c2bf1d..4324aefbc 100644 --- a/src/command/archive/get/get.c +++ b/src/command/archive/get/get.c @@ -216,7 +216,7 @@ cmdArchiveGet(void) kvPut(optionReplace, VARSTR(CFGOPT_LOG_LEVEL_STDERR_STR), VARSTRDEF("off")); // Generate command options - StringList *commandExec = cfgExecParam(cfgCmdArchiveGetAsync, optionReplace); + StringList *commandExec = cfgExecParam(cfgCmdArchiveGetAsync, optionReplace, true); strLstInsert(commandExec, 0, cfgExe()); // Clean the current queue using the list of WAL that we ideally want in the queue. queueNeed() diff --git a/src/command/archive/push/push.c b/src/command/archive/push/push.c index 519ef40a8..277c779c9 100644 --- a/src/command/archive/push/push.c +++ b/src/command/archive/push/push.c @@ -299,7 +299,7 @@ cmdArchivePush(void) kvPut(optionReplace, VARSTR(CFGOPT_LOG_LEVEL_STDERR_STR), VARSTRDEF("off")); // Generate command options - StringList *commandExec = cfgExecParam(cfgCmdArchivePushAsync, optionReplace); + StringList *commandExec = cfgExecParam(cfgCmdArchivePushAsync, optionReplace, true); strLstInsert(commandExec, 0, cfgExe()); strLstAdd(commandExec, strPath(walFile)); diff --git a/src/config/exec.c b/src/config/exec.c index 7110e5a09..564ee51cf 100644 --- a/src/config/exec.c +++ b/src/config/exec.c @@ -13,11 +13,12 @@ Exec Configuration Generate a list of options required for execution of a new command, replacing options as specified in optionReplace ***********************************************************************************************************************************/ StringList * -cfgExecParam(ConfigCommand commandId, const KeyValue *optionReplace) +cfgExecParam(ConfigCommand commandId, const KeyValue *optionReplace, bool local) { FUNCTION_LOG_BEGIN(logLevelTrace); FUNCTION_LOG_PARAM(ENUM, commandId); FUNCTION_LOG_PARAM(KEY_VALUE, optionReplace); + FUNCTION_LOG_PARAM(BOOL, local); // Will the new process be running on the same host? FUNCTION_LOG_END(); StringList *result = NULL; @@ -73,7 +74,7 @@ cfgExecParam(ConfigCommand commandId, const KeyValue *optionReplace) strLstAdd(result, strNewFmt("--reset-%s", cfgOptionName(optionId))); } // Else format the value if found - else if (value != NULL) + else if (value != NULL && (!local || exists || cfgOptionSource(optionId) == cfgSourceParam)) { if (varType(value) == varTypeBool) { diff --git a/src/config/exec.h b/src/config/exec.h index bad4cb1c6..0cc282684 100644 --- a/src/config/exec.h +++ b/src/config/exec.h @@ -10,6 +10,6 @@ Exec Configuration /*********************************************************************************************************************************** Functions ***********************************************************************************************************************************/ -StringList *cfgExecParam(ConfigCommand commandId, const KeyValue *optionReplace); +StringList *cfgExecParam(ConfigCommand commandId, const KeyValue *optionReplace, bool local); #endif diff --git a/src/protocol/helper.c b/src/protocol/helper.c index b9e268837..807a6de55 100644 --- a/src/protocol/helper.c +++ b/src/protocol/helper.c @@ -133,7 +133,7 @@ protocolLocalParam(ProtocolStorageType protocolStorageType, unsigned int protoco // Always output errors on stderr for debugging purposes kvPut(optionReplace, VARSTR(CFGOPT_LOG_LEVEL_STDERR_STR), VARSTRDEF("error")); - result = strLstMove(cfgExecParam(cfgCmdLocal, optionReplace), MEM_CONTEXT_OLD()); + result = strLstMove(cfgExecParam(cfgCmdLocal, optionReplace, true), MEM_CONTEXT_OLD()); } MEM_CONTEXT_TEMP_END(); @@ -300,7 +300,7 @@ protocolRemoteParam(ProtocolStorageType protocolStorageType, unsigned int protoc // Add the type kvPut(optionReplace, VARSTR(CFGOPT_TYPE_STR), isRepo ? VARSTRDEF("backup") : VARSTRDEF("db")); - StringList *commandExec = cfgExecParam(cfgCmdRemote, optionReplace); + StringList *commandExec = cfgExecParam(cfgCmdRemote, optionReplace, false); strLstInsert(commandExec, 0, cfgOptionStr(isRepo ? cfgOptRepoHostCmd : cfgOptPgHostCmd + hostIdx)); strLstAdd(result, strLstJoin(commandExec, " ")); diff --git a/test/src/module/config/execTest.c b/test/src/module/config/execTest.c index 9074c07b4..a3ab99b1e 100644 --- a/test/src/module/config/execTest.c +++ b/test/src/module/config/execTest.c @@ -31,7 +31,7 @@ testRun(void) unsetenv("PGBACKREST_REPO1_CIPHER_PASS"); TEST_RESULT_STR( - strPtr(strLstJoin(cfgExecParam(cfgCmdLocal, NULL), "|")), + strPtr(strLstJoin(cfgExecParam(cfgCmdLocal, NULL, false), "|")), strPtr( strNewFmt( "--no-config|--log-subprocess|--reset-neutral-umask|--pg1-path=\"%s/db path\"|--repo1-path=%s/repo" @@ -50,14 +50,17 @@ testRun(void) strLstAddZ(argList, "--recovery-option=a=b"); strLstAddZ(argList, "--recovery-option=c=d"); strLstAddZ(argList, "restore"); + + setenv("PGBACKREST_REPO1_HOST", "bogus", true); harnessCfgLoad(strLstSize(argList), strLstPtr(argList)); + unsetenv("PGBACKREST_REPO1_HOST"); KeyValue *optionReplace = kvNew(); kvPut(optionReplace, varNewStr(strNew("repo1-path")), varNewStr(strNew("/replace/path"))); kvPut(optionReplace, varNewStr(strNew("stanza")), NULL); TEST_RESULT_STR( - strPtr(strLstJoin(cfgExecParam(cfgCmdRestore, optionReplace), "|")), + strPtr(strLstJoin(cfgExecParam(cfgCmdRestore, optionReplace, true), "|")), strPtr( strNewFmt( "--db-include=1|--db-include=2|--pg1-path=%s/db|--recovery-option=a=b|--recovery-option=c=d"