From 1edcfde93e47c79a8b18b05f7ddb1e9fde29b0ed Mon Sep 17 00:00:00 2001 From: David Steele Date: Tue, 27 Apr 2021 12:12:43 -0400 Subject: [PATCH] Add cfgOptionDisplay()/cfgOptionIdxDisplay(). Centralize the formatting of the configuration value for display to the user or passing on a command line. For the new functions, if the value was set by the user via the command line, config, etc., then that exact value will be displayed. This makes it easier for the user to recognize the value and saves having to format it into something reasonable, especially for time and size option types. Note that cfgOptTypeHash and cfgOptTypeList option types are not supported by these functions, but they are generally not displayed to the user as a whole. This also fixes a bug in config/load.c where time values where not being formatted correctly in an error message. --- doc/xml/release.xml | 10 ++++ src/command/archive/push/push.c | 2 +- src/command/backup/backup.c | 13 +++-- src/command/check/common.c | 2 +- src/command/command.c | 8 +-- src/command/control/common.c | 2 +- src/command/control/start.c | 2 +- src/command/control/stop.c | 2 +- src/command/help/help.c | 6 +-- src/command/local/local.c | 2 +- src/command/remote/remote.c | 2 +- src/command/repo/get.c | 4 +- src/command/restore/restore.c | 8 +-- src/command/stanza/create.c | 4 +- src/command/stanza/delete.c | 6 +-- src/command/stanza/upgrade.c | 4 +- src/config/config.c | 81 ++++++++++++++++++++++++++++++ src/config/config.h | 7 +++ src/config/config.intern.h | 6 +++ src/config/exec.c | 7 +-- src/config/load.c | 10 ++-- src/config/parse.c | 12 ++++- test/src/module/config/loadTest.c | 4 +- test/src/module/config/parseTest.c | 7 +++ 24 files changed, 158 insertions(+), 53 deletions(-) diff --git a/doc/xml/release.xml b/doc/xml/release.xml index c70243a91..0123a3e7a 100644 --- a/doc/xml/release.xml +++ b/doc/xml/release.xml @@ -97,6 +97,16 @@

Add StringId type.

+ + + + + + + + +

Add cfgOptionDisplay()/cfgOptionIdxDisplay().

+ diff --git a/src/command/archive/push/push.c b/src/command/archive/push/push.c index 8226c32dd..c614a0a2b 100644 --- a/src/command/archive/push/push.c +++ b/src/command/archive/push/push.c @@ -390,7 +390,7 @@ cmdArchivePush(void) { THROW_FMT( ArchiveTimeoutError, "unable to push WAL file '%s' to the archive asynchronously after %s second(s)", - strZ(archiveFile), strZ(strNewDbl((double)cfgOptionInt64(cfgOptArchiveTimeout) / MSEC_PER_SEC))); + strZ(archiveFile), strZ(cfgOptionDisplay(cfgOptArchiveTimeout))); } // Log success diff --git a/src/command/backup/backup.c b/src/command/backup/backup.c index 4f900f815..51500556b 100644 --- a/src/command/backup/backup.c +++ b/src/command/backup/backup.c @@ -409,7 +409,7 @@ backupBuildIncrPrior(const InfoBackup *infoBackup) { LOG_WARN_FMT( "%s backup cannot alter " CFGOPT_COMPRESS_TYPE " option to '%s', reset to value in %s", - strZ(cfgOptionStr(cfgOptType)), strZ(cfgOptionStr(cfgOptCompressType)), strZ(backupLabelPrior)); + strZ(cfgOptionDisplay(cfgOptType)), strZ(cfgOptionDisplay(cfgOptCompressType)), strZ(backupLabelPrior)); // Set the compression type back to whatever was in the prior backup. This is not strictly needed since we // could store compression type on a per file basis, but it seems simplest and safest for now. @@ -433,8 +433,7 @@ backupBuildIncrPrior(const InfoBackup *infoBackup) { LOG_WARN_FMT( "%s backup cannot alter hardlink option to '%s', reset to value in %s", - strZ(cfgOptionStr(cfgOptType)), cvtBoolToConstZ(cfgOptionBool(cfgOptRepoHardlink)), - strZ(backupLabelPrior)); + strZ(cfgOptionDisplay(cfgOptType)), strZ(cfgOptionDisplay(cfgOptRepoHardlink)), strZ(backupLabelPrior)); cfgOptionSet(cfgOptRepoHardlink, cfgSourceParam, VARBOOL(manifestPriorData->backupOptionHardLink)); } @@ -455,7 +454,7 @@ backupBuildIncrPrior(const InfoBackup *infoBackup) { LOG_WARN_FMT( "%s backup cannot alter '" CFGOPT_CHECKSUM_PAGE "' option to '%s', reset to '%s' from %s", - strZ(cfgOptionStr(cfgOptType)), cvtBoolToConstZ(cfgOptionBool(cfgOptChecksumPage)), + strZ(cfgOptionDisplay(cfgOptType)), strZ(cfgOptionDisplay(cfgOptChecksumPage)), cvtBoolToConstZ(checksumPagePrior), strZ(manifestData(result)->backupLabel)); } @@ -466,7 +465,7 @@ backupBuildIncrPrior(const InfoBackup *infoBackup) } else { - LOG_WARN_FMT("no prior backup exists, %s backup has been changed to full", strZ(cfgOptionStr(cfgOptType))); + LOG_WARN_FMT("no prior backup exists, %s backup has been changed to full", strZ(cfgOptionDisplay(cfgOptType))); cfgOptionSet(cfgOptType, cfgSourceParam, VARSTR(backupTypeStr(backupTypeFull))); } } @@ -725,7 +724,7 @@ backupResumeFind(const Manifest *manifest, const String *cipherPassBackup) { reason = strNewFmt( "new backup type '%s' does not match resumable backup type '%s'", - strZ(cfgOptionStr(cfgOptType)), strZ(backupTypeStr(manifestResumeData->backupType))); + strZ(cfgOptionDisplay(cfgOptType)), strZ(backupTypeStr(manifestResumeData->backupType))); } // Check prior backup label ??? Do we really care about the prior backup label? else if (!strEq(manifestResumeData->backupLabelPrior, manifestData(manifest)->backupLabelPrior)) @@ -742,7 +741,7 @@ backupResumeFind(const Manifest *manifest, const String *cipherPassBackup) { reason = strNewFmt( "new compression '%s' does not match resumable compression '%s'", - strZ(cfgOptionStr(cfgOptCompressType)), + strZ(cfgOptionDisplay(cfgOptCompressType)), strZ(compressTypeStr(manifestResumeData->backupOptionCompressType))); } else diff --git a/src/command/check/common.c b/src/command/check/common.c index a0f6e8915..7208d4548 100644 --- a/src/command/check/common.c +++ b/src/command/check/common.c @@ -65,7 +65,7 @@ checkDbConfig(const unsigned int pgVersion, const unsigned int pgIdx, const Db * DbMismatchError, "version '%s' and path '%s' queried from cluster do not match version '%s' and '%s' read from '%s/" PG_PATH_GLOBAL "/" PG_FILE_PGCONTROL "'\nHINT: the %s and %s settings likely reference different clusters.", strZ(pgVersionToStr(dbVersion)), strZ(dbPath), strZ(pgVersionToStr(pgVersion)), - strZ(cfgOptionIdxStr(cfgOptPgPath, pgIdx)), strZ(cfgOptionIdxStr(cfgOptPgPath, pgIdx)), + strZ(cfgOptionIdxDisplay(cfgOptPgPath, pgIdx)), strZ(cfgOptionIdxDisplay(cfgOptPgPath, pgIdx)), cfgOptionIdxName(cfgOptPgPath, pgIdx), cfgOptionIdxName(cfgOptPgPort, pgIdx)); } diff --git a/src/command/command.c b/src/command/command.c index d3f06b308..d6fdfb557 100644 --- a/src/command/command.c +++ b/src/command/command.c @@ -126,17 +126,11 @@ cmdOption(void) { valueList = strLstNewVarLst(cfgOptionIdxLst(optionId, optionIdx)); } - // Generate time value - else if (cfgParseOptionType(optionId) == cfgOptTypeTime) - { - valueList = strLstNew(); - strLstAdd(valueList, strNewDbl((double)cfgOptionIdxInt64(optionId, optionIdx) / MSEC_PER_SEC)); - } // Else only one value else { valueList = strLstNew(); - strLstAdd(valueList, varStrForce(cfgOptionIdx(optionId, optionIdx))); + strLstAdd(valueList, cfgOptionIdxDisplay(optionId, optionIdx)); } // Output options and values diff --git a/src/command/control/common.c b/src/command/control/common.c index 1afe27d40..fcf76b584 100644 --- a/src/command/control/common.c +++ b/src/command/control/common.c @@ -33,7 +33,7 @@ lockStopTest(void) if (cfgOptionTest(cfgOptStanza)) { if (storageExistsP(storageLocal(), lockStopFileName(cfgOptionStr(cfgOptStanza)))) - THROW_FMT(StopError, "stop file exists for stanza %s", strZ(cfgOptionStr(cfgOptStanza))); + THROW_FMT(StopError, "stop file exists for stanza %s", strZ(cfgOptionDisplay(cfgOptStanza))); } // Check all stanzas diff --git a/src/command/control/start.c b/src/command/control/start.c index 6c61a28e4..6d7444bf3 100644 --- a/src/command/control/start.c +++ b/src/command/control/start.c @@ -30,7 +30,7 @@ cmdStart(void) { LOG_WARN_FMT( "stop file does not exist%s", - (cfgOptionTest(cfgOptStanza) ? strZ(strNewFmt(" for stanza %s", strZ(cfgOptionStr(cfgOptStanza)))) : "")); + (cfgOptionTest(cfgOptStanza) ? strZ(strNewFmt(" for stanza %s", strZ(cfgOptionDisplay(cfgOptStanza)))) : "")); } } MEM_CONTEXT_TEMP_END(); diff --git a/src/command/control/stop.c b/src/command/control/stop.c index e698766a8..9efaaa557 100644 --- a/src/command/control/stop.c +++ b/src/command/control/stop.c @@ -101,7 +101,7 @@ cmdStop(void) { LOG_WARN_FMT( "stop file already exists for %s", - cfgOptionTest(cfgOptStanza) ? strZ(strNewFmt("stanza %s", strZ(cfgOptionStr(cfgOptStanza)))) : "all stanzas"); + cfgOptionTest(cfgOptStanza) ? strZ(strNewFmt("stanza %s", strZ(cfgOptionDisplay(cfgOptStanza)))) : "all stanzas"); } } MEM_CONTEXT_TEMP_END(); diff --git a/src/command/help/help.c b/src/command/help/help.c index 9ca5e3aa8..746e0a6ad 100644 --- a/src/command/help/help.c +++ b/src/command/help/help.c @@ -12,7 +12,7 @@ Help Command #include "common/io/fdWrite.h" #include "common/memContext.h" #include "common/type/pack.h" -#include "config/config.h" +#include "config/config.intern.h" #include "config/parse.h" #include "version.h" @@ -205,10 +205,8 @@ helpRenderValue(const Variant *value, ConfigOptionType type) result = resultTemp; } - else if (type == cfgOptTypeTime) - result = strNewDbl((double)varInt64(value) / MSEC_PER_SEC); else - result = varStrForce(value); + result = cfgOptionDisplayVar(value, type); } FUNCTION_LOG_RETURN_CONST(STRING, result); diff --git a/src/command/local/local.c b/src/command/local/local.c index d146fe2bf..7579e1f01 100644 --- a/src/command/local/local.c +++ b/src/command/local/local.c @@ -37,7 +37,7 @@ cmdLocal(int fdRead, int fdWrite) MEM_CONTEXT_TEMP_BEGIN() { - String *name = strNewFmt(PROTOCOL_SERVICE_LOCAL "-%u", cfgOptionUInt(cfgOptProcess)); + String *name = strNewFmt(PROTOCOL_SERVICE_LOCAL "-%s", strZ(cfgOptionDisplay(cfgOptProcess))); IoRead *read = ioFdReadNew(name, fdRead, cfgOptionUInt64(cfgOptProtocolTimeout)); ioReadOpen(read); IoWrite *write = ioFdWriteNew(name, fdWrite, cfgOptionUInt64(cfgOptProtocolTimeout)); diff --git a/src/command/remote/remote.c b/src/command/remote/remote.c index 24d5a11c0..b215cc1ed 100644 --- a/src/command/remote/remote.c +++ b/src/command/remote/remote.c @@ -35,7 +35,7 @@ cmdRemote(int fdRead, int fdWrite) MEM_CONTEXT_TEMP_BEGIN() { - String *name = strNewFmt(PROTOCOL_SERVICE_REMOTE "-%u", cfgOptionUInt(cfgOptProcess)); + String *name = strNewFmt(PROTOCOL_SERVICE_REMOTE "-%s", strZ(cfgOptionDisplay(cfgOptProcess))); IoRead *read = ioFdReadNew(name, fdRead, cfgOptionUInt64(cfgOptProtocolTimeout)); ioReadOpen(read); IoWrite *write = ioFdWriteNew(name, fdWrite, cfgOptionUInt64(cfgOptProtocolTimeout)); diff --git a/src/command/repo/get.c b/src/command/repo/get.c index 92967a834..87225df82 100644 --- a/src/command/repo/get.c +++ b/src/command/repo/get.c @@ -48,7 +48,7 @@ storageGetProcess(IoWrite *destination) { THROW_FMT( OptionInvalidValueError, "absolute path '%s' is not in base path '%s'", strZ(file), - strZ(cfgOptionStr(cfgOptRepoPath))); + strZ(cfgOptionDisplay(cfgOptRepoPath))); } // Get the relative part of the file @@ -95,7 +95,7 @@ storageGetProcess(IoWrite *destination) { THROW_FMT( OptionInvalidValueError, "stanza name '%s' given in option doesn't match the given path", - strZ(cfgOptionStr(cfgOptStanza))); + strZ(cfgOptionDisplay(cfgOptStanza))); } // Archive path diff --git a/src/command/restore/restore.c b/src/command/restore/restore.c index e9b77ff80..5d9d17e79 100644 --- a/src/command/restore/restore.c +++ b/src/command/restore/restore.c @@ -82,7 +82,7 @@ restorePathValidate(void) // The PGDATA directory must exist // ??? We should remove this requirement in a separate commit. What's the harm in creating the dir assuming we have perms? if (!storagePathExistsP(storagePg(), NULL)) - THROW_FMT(PathMissingError, "$PGDATA directory '%s' does not exist", strZ(cfgOptionStr(cfgOptPgPath))); + THROW_FMT(PathMissingError, "$PGDATA directory '%s' does not exist", strZ(cfgOptionDisplay(cfgOptPgPath))); // PostgreSQL must not be running if (storageExistsP(storagePg(), PG_FILE_POSTMASTERPID_STR)) @@ -92,7 +92,7 @@ restorePathValidate(void) "unable to restore while PostgreSQL is running\n" "HINT: presence of '" PG_FILE_POSTMASTERPID "' in '%s' indicates PostgreSQL is running.\n" "HINT: remove '" PG_FILE_POSTMASTERPID "' only if PostgreSQL is not running.", - strZ(cfgOptionStr(cfgOptPgPath))); + strZ(cfgOptionDisplay(cfgOptPgPath))); } // If the restore will be destructive attempt to verify that PGDATA looks like a valid PostgreSQL directory @@ -103,7 +103,7 @@ restorePathValidate(void) "--delta or --force specified but unable to find '" PG_FILE_PGVERSION "' or '" BACKUP_MANIFEST_FILE "' in '%s' to" " confirm that this is a valid $PGDATA directory. --delta and --force have been disabled and if any files" " exist in the destination directories the restore will be aborted.", - strZ(cfgOptionStr(cfgOptPgPath))); + strZ(cfgOptionDisplay(cfgOptPgPath))); // Disable delta and force so restore will fail if the directories are not empty cfgOptionSet(cfgOptDelta, cfgSourceDefault, VARBOOL(false)); @@ -391,7 +391,7 @@ restoreBackupSet(void) LOG_WARN_FMT( "unable to find backup set with stop time less than '%s', repo%u: latest backup set will be used", - strZ(cfgOptionStr(cfgOptTarget)), cfgOptionGroupIdxToKey(cfgOptGrpRepo, result.repoIdx)); + strZ(cfgOptionDisplay(cfgOptTarget)), cfgOptionGroupIdxToKey(cfgOptGrpRepo, result.repoIdx)); } else THROW(BackupSetInvalidError, "no backup set found to restore"); diff --git a/src/command/stanza/create.c b/src/command/stanza/create.c index b197abb61..bc955e0b3 100644 --- a/src/command/stanza/create.c +++ b/src/command/stanza/create.c @@ -44,7 +44,7 @@ cmdStanzaCreate(void) for (unsigned int repoIdx = 0; repoIdx < cfgOptionGroupIdxTotal(cfgOptGrpRepo); repoIdx++) { LOG_INFO_FMT( - CFGCMD_STANZA_CREATE " for stanza '%s' on repo%u", strZ(cfgOptionStr(cfgOptStanza)), + CFGCMD_STANZA_CREATE " for stanza '%s' on repo%u", strZ(cfgOptionDisplay(cfgOptStanza)), cfgOptionGroupIdxToKey(cfgOptGrpRepo, repoIdx)); const Storage *storageRepoReadStanza = storageRepoIdx(repoIdx); @@ -133,7 +133,7 @@ cmdStanzaCreate(void) if (sourceFile == NULL) { LOG_INFO_FMT( - "stanza '%s' already exists on repo%u and is valid", strZ(cfgOptionStr(cfgOptStanza)), + "stanza '%s' already exists on repo%u and is valid", strZ(cfgOptionDisplay(cfgOptStanza)), cfgOptionGroupIdxToKey(cfgOptGrpRepo, repoIdx)); } } diff --git a/src/command/stanza/delete.c b/src/command/stanza/delete.c index c0217e8dd..ec0b58d53 100644 --- a/src/command/stanza/delete.c +++ b/src/command/stanza/delete.c @@ -79,7 +79,7 @@ stanzaDelete(const Storage *storageRepoWriteStanza, const StringList *archiveLis THROW_FMT( FileMissingError, "stop file does not exist for stanza '%s'\n" "HINT: has the pgbackrest stop command been run on this server for this stanza?", - strZ(cfgOptionStr(cfgOptStanza))); + strZ(cfgOptionDisplay(cfgOptStanza))); } // If a force has not been issued and Postgres is running, then error @@ -88,9 +88,9 @@ stanzaDelete(const Storage *storageRepoWriteStanza, const StringList *archiveLis THROW_FMT( PgRunningError, PG_FILE_POSTMASTERPID " exists - looks like " PG_NAME " is running. " "To delete stanza '%s' on repo%u, shut down " PG_NAME " for stanza '%s' and try again, or use --force.", - strZ(cfgOptionStr(cfgOptStanza)), + strZ(cfgOptionDisplay(cfgOptStanza)), cfgOptionGroupIdxToKey(cfgOptGrpRepo, cfgOptionGroupIdxDefault(cfgOptGrpRepo)), - strZ(cfgOptionStr(cfgOptStanza))); + strZ(cfgOptionDisplay(cfgOptStanza))); } // Delete the archive info files diff --git a/src/command/stanza/upgrade.c b/src/command/stanza/upgrade.c index 3ba5c4e18..bab7e54e6 100644 --- a/src/command/stanza/upgrade.c +++ b/src/command/stanza/upgrade.c @@ -41,7 +41,7 @@ cmdStanzaUpgrade(void) for (unsigned int repoIdx = 0; repoIdx < cfgOptionGroupIdxTotal(cfgOptGrpRepo); repoIdx++) { LOG_INFO_FMT( - CFGCMD_STANZA_UPGRADE " for stanza '%s' on repo%u", strZ(cfgOptionStr(cfgOptStanza)), + CFGCMD_STANZA_UPGRADE " for stanza '%s' on repo%u", strZ(cfgOptionDisplay(cfgOptStanza)), cfgOptionGroupIdxToKey(cfgOptGrpRepo, repoIdx)); const Storage *storageRepoReadStanza = storageRepoIdx(repoIdx); @@ -100,7 +100,7 @@ cmdStanzaUpgrade(void) if (!(infoArchiveUpgrade || infoBackupUpgrade)) { LOG_INFO_FMT( - "stanza '%s' on repo%u is already up to date", strZ(cfgOptionStr(cfgOptStanza)), + "stanza '%s' on repo%u is already up to date", strZ(cfgOptionDisplay(cfgOptStanza)), cfgOptionGroupIdxToKey(cfgOptGrpRepo, repoIdx)); } } diff --git a/src/config/config.c b/src/config/config.c index 4153e838e..361e40198 100644 --- a/src/config/config.c +++ b/src/config/config.c @@ -584,7 +584,10 @@ cfgOptionDefaultSet(ConfigOption optionId, const Variant *defaultValue) for (unsigned int optionIdx = 0; optionIdx < cfgOptionIdxTotal(optionId); optionIdx++) { if (configLocal->option[optionId].index[optionIdx].source == cfgSourceDefault) + { configLocal->option[optionId].index[optionIdx].value = configLocal->option[optionId].defaultValue; + configLocal->option[optionId].index[optionIdx].display = NULL; + } } } MEM_CONTEXT_END(); @@ -592,6 +595,81 @@ cfgOptionDefaultSet(ConfigOption optionId, const Variant *defaultValue) FUNCTION_TEST_RETURN_VOID(); } + +/**********************************************************************************************************************************/ +const String * +cfgOptionDisplayVar(const Variant *const value, const ConfigOptionType optionType) +{ + FUNCTION_TEST_BEGIN(); + FUNCTION_TEST_PARAM(VARIANT, value); + FUNCTION_TEST_PARAM(UINT, optionType); + FUNCTION_TEST_END(); + + ASSERT(value != NULL); + ASSERT(optionType != cfgOptTypeHash && optionType != cfgOptTypeList); + + if (varType(value) == varTypeString) + { + FUNCTION_TEST_RETURN(varStr(value)); + } + else if (optionType == cfgOptTypeBoolean) + { + FUNCTION_TEST_RETURN(varBool(value) ? TRUE_STR : FALSE_STR); + } + else if (optionType == cfgOptTypeTime) + { + FUNCTION_TEST_RETURN(strNewDbl((double)varInt64(value) / MSEC_PER_SEC)); + } + + FUNCTION_TEST_RETURN(varStrForce(value)); +} + +const String * +cfgOptionIdxDisplay(const ConfigOption optionId, const unsigned int optionIdx) +{ + FUNCTION_TEST_BEGIN(); + FUNCTION_TEST_PARAM(ENUM, optionId); + FUNCTION_TEST_PARAM(UINT, optionIdx); + FUNCTION_TEST_END(); + + ASSERT(optionId < CFG_OPTION_TOTAL); + ASSERT(configLocal != NULL); + ASSERT( + (!configLocal->option[optionId].group && optionIdx == 0) || + (configLocal->option[optionId].group && optionIdx < + configLocal->optionGroup[configLocal->option[optionId].groupId].indexTotal)); + + // Check that the option is valid for the current command + if (!cfgOptionValid(optionId)) + THROW_FMT(AssertError, "option '%s' is not valid for the current command", cfgOptionIdxName(optionId, optionIdx)); + + // If there is already a display value set then return that + ConfigOptionValue *const option = &configLocal->option[optionId].index[optionIdx]; + + if (option->display != NULL) + FUNCTION_TEST_RETURN(option->display); + + // Generate the display value based on the type + MEM_CONTEXT_BEGIN(configLocal->memContext) + { + option->display = cfgOptionDisplayVar(option->value, cfgParseOptionType(optionId)); + } + MEM_CONTEXT_END(); + + + FUNCTION_TEST_RETURN(option->display); +} + +const String * +cfgOptionDisplay(const ConfigOption optionId) +{ + FUNCTION_TEST_BEGIN(); + FUNCTION_TEST_PARAM(ENUM, optionId); + FUNCTION_TEST_END(); + + FUNCTION_TEST_RETURN(cfgOptionIdxDisplay(optionId, cfgOptionIdxDefault(optionId))); +} + /**********************************************************************************************************************************/ String * cfgOptionHostPort(ConfigOption optionId, unsigned int *port) @@ -1183,6 +1261,9 @@ cfgOptionIdxSet(ConfigOption optionId, unsigned int optionIdx, ConfigSource sour } else configLocal->option[optionId].index[optionIdx].value = NULL; + + // Clear the display value, which will be generated when needed + configLocal->option[optionId].index[optionIdx].display = NULL; } MEM_CONTEXT_END(); diff --git a/src/config/config.h b/src/config/config.h index 783680585..3d087a14a 100644 --- a/src/config/config.h +++ b/src/config/config.h @@ -137,6 +137,13 @@ unsigned int cfgOptionIdxUInt(ConfigOption optionId, unsigned int optionIdx); uint64_t cfgOptionUInt64(ConfigOption optionId); uint64_t cfgOptionIdxUInt64(ConfigOption optionId, unsigned int optionIdx); +// Format the configuration value for display to the user or passing on a command line. If the value was set by the user via the +// command line, config, etc., then that exact value will be displayed. This makes it easier for the user to recognize the value and +// saves having to format it into something reasonable, especially for time and size option types. Note that cfgOptTypeHash and +// cfgOptTypeList option types are not supported by these functions, but they are generally not displayed to the user as a whole. +const String *cfgOptionDisplay(const ConfigOption optionId); +const String *cfgOptionIdxDisplay(const ConfigOption optionId, const unsigned int optionIdx); + // Option name by id const char *cfgOptionName(ConfigOption optionId); const char *cfgOptionIdxName(ConfigOption optionId, unsigned int optionIdx); diff --git a/src/config/config.intern.h b/src/config/config.intern.h index eee9196a5..4fe9d4d62 100644 --- a/src/config/config.intern.h +++ b/src/config/config.intern.h @@ -10,6 +10,7 @@ The general-purpose functions for querying the current configuration are found i #define CONFIG_CONFIG_INTERN_H #include "config/config.h" +#include "config/parse.auto.h" /*********************************************************************************************************************************** The maximum number of keys that an indexed option can have, e.g. pg256-path would be the maximum pg-path option @@ -25,6 +26,7 @@ typedef struct ConfigOptionValue bool negate; // Is the option negated? bool reset; // Is the option reset? unsigned int source; // Where the option came from, i.e. ConfigSource enum + const String *display; // Current display value, if any. Used for messages, etc. const Variant *value; // Value } ConfigOptionValue; @@ -87,6 +89,10 @@ unsigned int cfgOptionGroupId(ConfigOption optionId); /*********************************************************************************************************************************** Option Functions ***********************************************************************************************************************************/ +// Format a variant for display using the supplied option type. cfgOptionDisplay()/cfgOptionIdxDisplay() should be used whenever +// possible, but sometimes the variant needs to be manipulated before being formatted. +const String *cfgOptionDisplayVar(const Variant *const value, const ConfigOptionType optionType); + // Convert the key used in the original configuration to a group index. This is used when an option key must be translated into the // local group index, e.g. during parsing or when getting the value of specific options from a remote. unsigned int cfgOptionKeyToIdx(ConfigOption optionId, unsigned int key); diff --git a/src/config/exec.c b/src/config/exec.c index a0d6eccc5..1b3b93a05 100644 --- a/src/config/exec.c +++ b/src/config/exec.c @@ -110,16 +110,11 @@ cfgExecParam(ConfigCommand commandId, ConfigCommandRole commandRoleId, const Key { valueList = strLstNewVarLst(varVarLst(value)); } - else if (cfgParseOptionType(optionId) == cfgOptTypeTime) - { - valueList = strLstNew(); - strLstAdd(valueList, strNewDbl((double)varInt64(value) / MSEC_PER_SEC)); - } // Else only one value else { valueList = strLstNew(); - strLstAdd(valueList, varStrForce(value)); + strLstAdd(valueList, cfgOptionDisplayVar(value, cfgParseOptionType(optionId))); } // Output options and values diff --git a/src/config/load.c b/src/config/load.c index ec6e9bf35..442368f9c 100644 --- a/src/config/load.c +++ b/src/config/load.c @@ -101,7 +101,7 @@ cfgLoadUpdateOption(void) OptionInvalidValueError, "local repo%u and repo%u paths are both '%s' but must be different", cfgOptionGroupIdxToKey(cfgOptGrpRepo, optionIdx), cfgOptionGroupIdxToKey(cfgOptGrpRepo, repoIdx), - strZ(cfgOptionIdxStr(cfgOptRepoPath, repoIdx))); + strZ(cfgOptionIdxDisplay(cfgOptRepoPath, repoIdx))); } } } @@ -145,8 +145,8 @@ cfgLoadUpdateOption(void) OptionInvalidValueError, "'%s' is not valid for '" CFGOPT_PROTOCOL_TIMEOUT "' option\nHINT '" CFGOPT_PROTOCOL_TIMEOUT "' option (%s)" " should be greater than '" CFGOPT_DB_TIMEOUT "' option (%s).", - strZ(varStrForce(cfgOption(cfgOptProtocolTimeout))), strZ(varStrForce(cfgOption(cfgOptProtocolTimeout))), - strZ(varStrForce(cfgOption(cfgOptDbTimeout)))); + strZ(cfgOptionDisplay(cfgOptProtocolTimeout)), strZ(cfgOptionDisplay(cfgOptProtocolTimeout)), + strZ(cfgOptionDisplay(cfgOptDbTimeout))); } } @@ -186,7 +186,7 @@ cfgLoadUpdateOption(void) "option '%s' is not set for '%s=%s', the repository may run out of space" "\nHINT: to retain full backups indefinitely (without warning), set option '%s' to the maximum.", cfgOptionIdxName(cfgOptRepoRetentionFull, optionIdx), cfgOptionIdxName(cfgOptRepoRetentionFullType, optionIdx), - strZ(cfgOptionIdxStr(cfgOptRepoRetentionFullType, optionIdx)), + strZ(cfgOptionIdxDisplay(cfgOptRepoRetentionFullType, optionIdx)), cfgOptionIdxName(cfgOptRepoRetentionFull, optionIdx)); } } @@ -277,7 +277,7 @@ cfgLoadUpdateOption(void) "\nHINT: RFC-2818 forbids dots in wildcard matches." "\nHINT: TLS/SSL verification cannot proceed with this bucket name." "\nHINT: remove dots from the bucket name.", - strZ(cfgOptionIdxStr(cfgOptRepoS3Bucket, repoIdx)), cfgOptionIdxName(cfgOptRepoS3Bucket, repoIdx)); + strZ(cfgOptionIdxDisplay(cfgOptRepoS3Bucket, repoIdx)), cfgOptionIdxName(cfgOptRepoS3Bucket, repoIdx)); } } diff --git a/src/config/parse.c b/src/config/parse.c index 85d015934..2f547bb7c 100644 --- a/src/config/parse.c +++ b/src/config/parse.c @@ -1667,6 +1667,13 @@ configParse(const Storage *storage, unsigned int argListSize, const char *argLis { int64_t valueInt64 = 0; + // Preserve original value to display + MEM_CONTEXT_BEGIN(config->memContext) + { + configOptionValue->display = strDup(value); + } + MEM_CONTEXT_END(); + // Check that the value can be converted TRY_BEGIN() { @@ -1726,7 +1733,7 @@ configParse(const Storage *storage, unsigned int argListSize, const char *argLis cfgParseOptionKeyIdxName(optionId, optionKeyIdx)); } } - // Else if path make sure it is valid + // Else if string make sure it is valid else { // Make sure it is long enough to be a path @@ -1737,6 +1744,7 @@ configParse(const Storage *storage, unsigned int argListSize, const char *argLis cfgParseOptionKeyIdxName(optionId, optionKeyIdx)); } + // If path make sure it is valid if (optionType == cfgOptTypePath) { // Make sure it starts with / @@ -1877,7 +1885,7 @@ configParse(const Storage *storage, unsigned int argListSize, const char *argLis if (index == cfgOptionGroupIdxTotal(groupId)) { THROW_FMT( - OptionInvalidValueError, "key '%u' is not valid for '%s' option", cfgOptionUInt(defaultOptionId), + OptionInvalidValueError, "key '%s' is not valid for '%s' option", strZ(cfgOptionDisplay(defaultOptionId)), cfgOptionName(defaultOptionId)); } diff --git a/test/src/module/config/loadTest.c b/test/src/module/config/loadTest.c index 0152bdc4f..425afe492 100644 --- a/test/src/module/config/loadTest.c +++ b/test/src/module/config/loadTest.c @@ -217,8 +217,8 @@ testRun(void) hrnCfgArgRawZ(argList, cfgOptProtocolTimeout, "50.5"); TEST_ERROR( harnessCfgLoad(cfgCmdCheck, argList), OptionInvalidValueError, - "'50500' is not valid for 'protocol-timeout' option\n" - "HINT 'protocol-timeout' option (50500) should be greater than 'db-timeout' option (100000000)."); + "'50.5' is not valid for 'protocol-timeout' option\n" + "HINT 'protocol-timeout' option (50.5) should be greater than 'db-timeout' option (100000)."); // ------------------------------------------------------------------------------------------------------------------------- TEST_TITLE("very small protocol-timeout triggers db-timeout special handling"); diff --git a/test/src/module/config/parseTest.c b/test/src/module/config/parseTest.c index f38237282..29eb33015 100644 --- a/test/src/module/config/parseTest.c +++ b/test/src/module/config/parseTest.c @@ -1337,6 +1337,7 @@ testRun(void) TEST_RESULT_STR_Z(cfgOptionIdxStr(cfgOptPgSocketPath, 0), "/path/to/socket", " pg1-socket-path is set"); TEST_RESULT_INT(cfgOptionIdxSource(cfgOptPgSocketPath, 0), cfgSourceConfig, " pg1-socket-path is config param"); TEST_RESULT_BOOL(cfgOptionBool(cfgOptOnline), false, " online not is set"); + TEST_RESULT_STR_Z(cfgOptionDisplay(cfgOptOnline), "false", " online display is false"); TEST_RESULT_INT(cfgOptionSource(cfgOptOnline), cfgSourceParam, " online is source param"); TEST_RESULT_BOOL(cfgOptionBool(cfgOptStartFast), false, " start-fast not is set"); TEST_RESULT_INT(cfgOptionSource(cfgOptStartFast), cfgSourceConfig, " start-fast is config param"); @@ -1346,6 +1347,7 @@ testRun(void) TEST_RESULT_BOOL(cfgOptionTest(cfgOptArchiveCheck), false, " archive-check is not set"); TEST_RESULT_BOOL(cfgOptionTest(cfgOptArchiveCopy), false, " archive-copy is not set"); TEST_RESULT_BOOL(cfgOptionBool(cfgOptRepoHardlink), true, " repo-hardlink is set"); + TEST_RESULT_STR_Z(cfgOptionDisplay(cfgOptRepoHardlink), "true", " repo-hardlink display is true"); TEST_RESULT_INT(cfgOptionSource(cfgOptRepoHardlink), cfgSourceConfig, " repo-hardlink is source config"); TEST_RESULT_INT(cfgOptionInt(cfgOptCompressLevel), 3, " compress-level is set"); TEST_RESULT_INT(cfgOptionSource(cfgOptCompressLevel), cfgSourceConfig, " compress-level is source config"); @@ -1357,6 +1359,7 @@ testRun(void) TEST_RESULT_INT(cfgOptionInt64(cfgOptBufferSize), 65536, " buffer-size is set"); TEST_RESULT_INT(cfgOptionSource(cfgOptBufferSize), cfgSourceConfig, " backup-standby is source config"); TEST_RESULT_UINT(cfgOptionUInt64(cfgOptDbTimeout), 1800000, " db-timeout is set"); + TEST_RESULT_STR_Z(cfgOptionDisplay(cfgOptDbTimeout), "1800", " db-timeout display"); TEST_RESULT_UINT(cfgOptionUInt64(cfgOptProtocolTimeout), 3600000, " protocol-timeout is set"); TEST_RESULT_UINT(cfgOptionIdxUInt(cfgOptPgPort, 1), 5432, " pg2-port is set"); TEST_RESULT_UINT(cfgOptionIdxUInt64(cfgOptPgPort, 1), 5432, " pg2-port is set"); @@ -1369,13 +1372,16 @@ testRun(void) TEST_RESULT_PTR(cfgOptionDefault(cfgOptPgHost), NULL, " pg-host default is NULL"); TEST_RESULT_STR_Z(varStr(cfgOptionDefault(cfgOptLogLevelConsole)), "warn", " log-level-console default is warn"); TEST_RESULT_INT(varInt64(cfgOptionDefault(cfgOptPgPort)), 5432, " pg-port default is 5432"); + TEST_RESULT_STR_Z(cfgOptionDisplay(cfgOptPgPort), "5432", " pg-port display is 5432"); TEST_RESULT_INT(varInt64(cfgOptionDefault(cfgOptDbTimeout)), 1800000, " db-timeout default is 1800000"); TEST_RESULT_VOID(cfgOptionDefaultSet(cfgOptPgSocketPath, VARSTRDEF("/default")), " set pg-socket-path default"); TEST_RESULT_STR_Z(cfgOptionIdxStr(cfgOptPgSocketPath, 0), "/path/to/socket", " pg1-socket-path unchanged"); TEST_RESULT_STR_Z(cfgOptionIdxStr(cfgOptPgSocketPath, 1), "/default", " pg2-socket-path is new default"); + TEST_RESULT_STR_Z(cfgOptionIdxDisplay(cfgOptPgSocketPath, 1), "/default", " pg2-socket-path display"); TEST_ERROR(cfgOptionDefaultValue(cfgOptDbInclude), AssertError, "default value not available for option type 3"); + TEST_ERROR(cfgOptionDisplay(cfgOptTarget), AssertError, "option 'target' is not valid for the current command"); TEST_ERROR(cfgOptionLst(cfgOptDbInclude), AssertError, "option 'db-include' is not valid for the current command"); TEST_ERROR(cfgOptionKv(cfgOptPgPath), AssertError, "option 'pg1-path' is type 4 but 3 was requested"); @@ -1423,6 +1429,7 @@ testRun(void) TEST_RESULT_UINT(cfgOptionUInt64(cfgOptArchivePushQueueMax), 4503599627370496, "archive-push-queue-max is set"); TEST_RESULT_INT(cfgOptionSource(cfgOptArchivePushQueueMax), cfgSourceParam, " archive-push-queue-max is source config"); TEST_RESULT_INT(cfgOptionIdxInt64(cfgOptBufferSize, 0), 2097152, "buffer-size is set to bytes from MB"); + TEST_RESULT_STR_Z(cfgOptionDisplay(cfgOptBufferSize), "2MB", "buffer-size display is 2MB"); TEST_RESULT_INT(cfgOptionSource(cfgOptBufferSize), cfgSourceParam, " buffer-size is source config"); TEST_RESULT_PTR(cfgOption(cfgOptSpoolPath), NULL, " spool-path is not set"); TEST_RESULT_INT(cfgOptionSource(cfgOptSpoolPath), cfgSourceDefault, " spool-path is source default");