db/pgbackrest
1
0
Fork 0
mirror of https://github.com/pgbackrest/pgbackrest.git synced 2026-05-22 10:15:16 +02:00
Code Issues Releases Activity

Centralize error handling for unsupported features.

Browse Source 
Some features are conditionally compiled into pgBackRest (e.g. lz4). Previously checking to see if the feature existed was the responsibility of the feature's module.

Centralize this logic in the config/parse module to make the errors more detailed and consistent.

This also fixes the assert that is thrown when SFTP storage is specified but SFTP support is not compiled into pgBackRest.
This commit is contained in:
David Steele
2023-05-24 14:17:07 +03:00
committed by GitHub
parent de46276bf6
commit 36ff81dc6f
13 changed files with 210 additions and 43 deletions
Download Patch File Download Diff File Expand all files Collapse all files
doc/xml/release.xml
+17
View File
@@ -15,6 +15,23 @@
<release-list>
<release date="XXXX-XX-XX" version="2.47dev" title="UNDER DEVELOPMENT">
<release-core-list>
<release-improvement-list>
<release-item>
<commit subject="Refactor allow list processing in config/parse module."/>
<commit subject="Centralize error handling for unsupported features.">
<github-pull-request id="2075"/>
</commit>
<release-item-contributor-list>
<release-item-contributor id="david.steele"/>
<release-item-reviewer id="stefan.fercot"/>
</release-item-contributor-list>
<p>Centralize error handling for unsupported features.</p>
</release-item>
</release-improvement-list>
</release-core-list>
</release>
<release date="2023-05-22" version="2.46" title="Block Incremental Backup and SFTP Storage">
src/build/config/config.yaml
+3 -3
View File
@@ -763,8 +763,8 @@ option:
- none
- bz2
- gz
- lz4
- zst
- lz4: HAVE_LIBLZ4
- zst: HAVE_LIBZST
command: compress
command-role:
async: {}
@@ -1735,7 +1735,7 @@ option:
- gcs
- posix
- s3
- sftp
- sftp: HAVE_LIBSSH2
command:
annotate:
command-role:
src/build/config/parse.c
+52 -11
View File
@@ -288,7 +288,7 @@ typedef struct BldCfgOptionCommandRaw
const Variant *required;
const String *defaultValue;
const BldCfgOptionDependRaw *depend;
const StringList *allowList;
const List *allowList;
const StringList *roleList;
} BldCfgOptionCommandRaw;
@@ -307,7 +307,7 @@ typedef struct BldCfgOptionRaw
const String *group;
bool secure;
const BldCfgOptionDependRaw *depend;
const StringList *allowList;
const List *allowList;
const String *allowRangeMin;
const String *allowRangeMax;
const List *cmdList;
@@ -316,10 +316,35 @@ typedef struct BldCfgOptionRaw
} BldCfgOptionRaw;
// Helper to parse allow list
static const StringList *
static List *
bldCfgParseAllowListDup(const List *const allowList)
{
List *result = NULL;
if (allowList != NULL)
{
result = lstNewP(sizeof(BldCfgOptionValue));
for (unsigned int valueIdx = 0; valueIdx < lstSize(allowList); valueIdx++)
{
const BldCfgOptionValue *const bldCfgOptionValue = lstGet(allowList, valueIdx);
const BldCfgOptionValue bldCfgOptionValueCopy =
{
.value = strDup(bldCfgOptionValue->value),
.condition = strDup(bldCfgOptionValue->condition),
};
lstAdd(result, &bldCfgOptionValueCopy);
}
}
return result;
}
static const List *
bldCfgParseAllowList(Yaml *const yaml, const List *const optList)
{
StringList *result = NULL;
List *result = NULL;
MEM_CONTEXT_TEMP_BEGIN()
{
@@ -332,14 +357,30 @@ bldCfgParseAllowList(Yaml *const yaml, const List *const optList)
MEM_CONTEXT_PRIOR_BEGIN()
{
result = strLstNew();
result = lstNewP(sizeof(BldCfgOptionValue));
}
MEM_CONTEXT_PRIOR_END();
do
{
yamlEventCheck(allowListVal, yamlEventTypeScalar);
strLstAdd(result, allowListVal.value);
BldCfgOptionValue bldCfgOptionValue;
if (allowListVal.type == yamlEventTypeScalar)
{
bldCfgOptionValue.value = allowListVal.value;
bldCfgOptionValue.condition = NULL;
}
else
{
yamlEventCheck(allowListVal, yamlEventTypeMapBegin);
bldCfgOptionValue.value = yamlEventNextCheck(yaml, yamlEventTypeScalar).value;
bldCfgOptionValue.condition = yamlEventNextCheck(yaml, yamlEventTypeScalar).value;
yamlEventNextCheck(yaml, yamlEventTypeMapEnd);
}
lstAdd(result, &bldCfgOptionValue);
allowListVal = yamlEventNext(yaml);
}
@@ -356,7 +397,7 @@ bldCfgParseAllowList(Yaml *const yaml, const List *const optList)
MEM_CONTEXT_PRIOR_BEGIN()
{
result = strLstDup(optInherit->allowList);
result = bldCfgParseAllowListDup(optInherit->allowList);
}
MEM_CONTEXT_PRIOR_END();
}
@@ -687,7 +728,7 @@ bldCfgParseOptionCommandList(Yaml *const yaml, const List *const optList)
.required = varDup(optCmdRaw.required),
.defaultValue = strDup(optCmdRaw.defaultValue),
.depend = optCmdRaw.depend,
.allowList = strLstDup(optCmdRaw.allowList),
.allowList = bldCfgParseAllowListDup(optCmdRaw.allowList),
.roleList = strLstDup(optCmdRaw.roleList),
};
@@ -918,7 +959,7 @@ bldCfgParseOptionList(Yaml *const yaml, const List *const cmdList, const List *c
.defaultLiteral = optRaw->defaultLiteral,
.group = strDup(optRaw->group),
.secure = optRaw->secure,
.allowList = strLstDup(optRaw->allowList),
.allowList = bldCfgParseAllowListDup(optRaw->allowList),
.allowRangeMin = strDup(optRaw->allowRangeMin),
.allowRangeMax = strDup(optRaw->allowRangeMax),
.deprecateList = bldCfgParseOptionDeprecateReconcile(optRaw->deprecateList),
@@ -993,7 +1034,7 @@ bldCfgParseOptionList(Yaml *const yaml, const List *const cmdList, const List *c
.required = varBool(optCmd.required),
.defaultValue = strDup(optCmd.defaultValue),
.depend = bldCfgParseDependReconcile(optRaw, optCmd.depend, result),
.allowList = strLstDup(optCmd.allowList),
.allowList = bldCfgParseAllowListDup(optCmd.allowList),
.roleList = strLstDup(optCmd.roleList),
};
src/build/config/parse.h
+8 -2
View File
@@ -110,10 +110,16 @@ typedef struct BldCfgOptionCommand
bool required; // Is the option required?
const String *defaultValue; // Default value, if any
const BldCfgOptionDepend *depend; // Dependency, if any
const StringList *allowList; // Allowed value list
const List *allowList; // Allowed value list
const StringList *roleList; // Roles valid for the command
} BldCfgOptionCommand;
typedef struct BldCfgOptionValue
{
const String *value; // Option value
const String *condition; // Is the option conditionally compiled?
} BldCfgOptionValue;
struct BldCfgOption
{
const String *name; // Name
@@ -129,7 +135,7 @@ struct BldCfgOption
const String *group; // Option group, if any
bool secure; // Does the option contain a secret?
const BldCfgOptionDepend *depend; // Dependency, if any
const StringList *allowList; // Allowed value list
const List *allowList; // Allowed value list
const String *allowRangeMin; // Allow range min, if any
const String *allowRangeMax; // Allow range max, if any
const List *cmdList; // Command override list
src/build/config/render.c
+28 -12
View File
@@ -134,8 +134,8 @@ bldCfgRenderConfigAutoH(const Storage *const storageRepo, const BldCfg bldCfg)
if (opt->allowList != NULL)
{
for (unsigned int allowListIdx = 0; allowListIdx < strLstSize(opt->allowList); allowListIdx++)
strLstAddIfMissing(allowList, strLstGet(opt->allowList, allowListIdx));
for (unsigned int allowListIdx = 0; allowListIdx < lstSize(opt->allowList); allowListIdx++)
strLstAddIfMissing(allowList, ((const BldCfgOptionValue *)lstGet(opt->allowList, allowListIdx))->value);
}
for (unsigned int optCmdListIdx = 0; optCmdListIdx < lstSize(opt->cmdList); optCmdListIdx++)
@@ -144,8 +144,8 @@ bldCfgRenderConfigAutoH(const Storage *const storageRepo, const BldCfg bldCfg)
if (optCmd->allowList != NULL)
{
for (unsigned int allowListIdx = 0; allowListIdx < strLstSize(optCmd->allowList); allowListIdx++)
strLstAddIfMissing(allowList, strLstGet(optCmd->allowList, allowListIdx));
for (unsigned int allowListIdx = 0; allowListIdx < lstSize(optCmd->allowList); allowListIdx++)
strLstAddIfMissing(allowList, ((const BldCfgOptionValue *)lstGet(optCmd->allowList, allowListIdx))->value);
}
}
@@ -466,7 +466,7 @@ bldCfgRenderAllowRange(const String *const allowRangeMin, const String *const al
// Helper to render allow list
static String *
bldCfgRenderAllowList(const StringList *const allowList, const String *const optType)
bldCfgRenderAllowList(const List *const allowList, const String *const optType)
{
ASSERT(allowList != NULL);
ASSERT(optType != NULL);
@@ -478,11 +478,21 @@ bldCfgRenderAllowList(const StringList *const allowList, const String *const opt
" PARSE_RULE_OPTIONAL_ALLOW_LIST\n"
" (\n");
for (unsigned int allowIdx = 0; allowIdx < strLstSize(allowList); allowIdx++)
for (unsigned int allowIdx = 0; allowIdx < lstSize(allowList); allowIdx++)
{
const String *const value = strLstGet(allowList, allowIdx);
const BldCfgOptionValue *const allow = lstGet(allowList, allowIdx);
strCatFmt(result, " %s,\n", strZ(bldCfgRenderScalar(value, optType)));
strCatFmt(result, " %s,\n", strZ(bldCfgRenderScalar(allow->value, optType)));
if (allow->condition != NULL)
{
strCatFmt(
result,
"#ifndef %s\n"
" PARSE_RULE_BOOL_FALSE,\n"
"#endif\n",
strZ(allow->condition));
}
}
strCatZ(result, " )");
@@ -814,8 +824,11 @@ bldCfgRenderParseAutoC(const Storage *const storageRepo, const BldCfg bldCfg, co
{
kvAdd(optionalDefaultRule, ruleAllowList, VARSTR(bldCfgRenderAllowList(opt->allowList, opt->type)));
for (unsigned int allowIdx = 0; allowIdx < strLstSize(opt->allowList); allowIdx++)
bldCfgRenderValueAdd(opt->type, strLstGet(opt->allowList, allowIdx), ruleDataList, NULL);
for (unsigned int allowIdx = 0; allowIdx < lstSize(opt->allowList); allowIdx++)
{
bldCfgRenderValueAdd(
opt->type, ((const BldCfgOptionValue *)lstGet(opt->allowList, allowIdx))->value, ruleDataList, NULL);
}
}
if (opt->defaultValue != NULL)
@@ -843,8 +856,11 @@ bldCfgRenderParseAutoC(const Storage *const storageRepo, const BldCfg bldCfg, co
{
kvAdd(optionalCmdRuleType, ruleAllowList, VARSTR(bldCfgRenderAllowList(optCmd->allowList, opt->type)));
for (unsigned int allowIdx = 0; allowIdx < strLstSize(optCmd->allowList); allowIdx++)
bldCfgRenderValueAdd(opt->type, strLstGet(optCmd->allowList, allowIdx), ruleDataList, NULL);
for (unsigned int allowIdx = 0; allowIdx < lstSize(optCmd->allowList); allowIdx++)
{
bldCfgRenderValueAdd(
opt->type, ((const BldCfgOptionValue *)lstGet(optCmd->allowList, allowIdx))->value, ruleDataList, NULL);
}
}
// Defaults
src/common/compress/helper.c
+2 -2
View File
@@ -136,7 +136,7 @@ compressTypeEnum(const StringId type)
}
/**********************************************************************************************************************************/
FN_EXTERN void
static void
compressTypePresent(CompressType type)
{
FUNCTION_TEST_BEGIN();
@@ -146,7 +146,7 @@ compressTypePresent(CompressType type)
ASSERT(type < LENGTH_OF(compressHelperLocal));
if (type != compressTypeNone && compressHelperLocal[type].compressNew == NULL)
THROW_FMT(OptionInvalidValueError, PROJECT_NAME " not compiled with %s support", strZ(compressHelperLocal[type].type));
THROW_FMT(OptionInvalidValueError, PROJECT_NAME " not built with %s support", strZ(compressHelperLocal[type].type));
FUNCTION_TEST_RETURN_VOID();
}
src/common/compress/helper.h
-3
View File
@@ -42,9 +42,6 @@ Functions
// Get enum from a compression type string
FN_EXTERN CompressType compressTypeEnum(StringId type);
// Check that a valid compress type is compiled into this binary. Errors when the compress type is not present.
FN_EXTERN void compressTypePresent(CompressType type);
// Get string representation of a compression type. This is the extension without the period.
FN_EXTERN const String *compressTypeStr(CompressType type);
src/config/load.c
-4
View File
@@ -358,10 +358,6 @@ cfgLoadUpdateOption(void)
cfgOptionSet(cfgOptCompress, cfgSourceDefault, NULL);
}
// Check that selected compress type has been compiled into this binary
if (cfgOptionValid(cfgOptCompressType))
compressTypePresent(compressTypeEnum(cfgOptionStrId(cfgOptCompressType)));
// Update compress-level default based on the compression type. Also check that level range is valid per compression type.
if (cfgOptionValid(cfgOptCompressLevel))
{
src/config/parse.auto.c.inc
+9
View File
@@ -1582,7 +1582,13 @@ static const ParseRuleOption parseRuleOption[CFG_OPTION_TOTAL] =
PARSE_RULE_VAL_STRID(parseRuleValStrIdBz2), // opt/compress-type
PARSE_RULE_VAL_STRID(parseRuleValStrIdGz), // opt/compress-type
PARSE_RULE_VAL_STRID(parseRuleValStrIdLz4), // opt/compress-type
#ifndef HAVE_LIBLZ4 // opt/compress-type
PARSE_RULE_BOOL_FALSE, // opt/compress-type
#endif // opt/compress-type
PARSE_RULE_VAL_STRID(parseRuleValStrIdZst), // opt/compress-type
#ifndef HAVE_LIBZST // opt/compress-type
PARSE_RULE_BOOL_FALSE, // opt/compress-type
#endif // opt/compress-type
), // opt/compress-type
// opt/compress-type
PARSE_RULE_OPTIONAL_DEFAULT // opt/compress-type
@@ -9123,6 +9129,9 @@ static const ParseRuleOption parseRuleOption[CFG_OPTION_TOTAL] =
PARSE_RULE_VAL_STRID(parseRuleValStrIdPosix), // opt/repo-type
PARSE_RULE_VAL_STRID(parseRuleValStrIdS3), // opt/repo-type
PARSE_RULE_VAL_STRID(parseRuleValStrIdSftp), // opt/repo-type
#ifndef HAVE_LIBSSH2 // opt/repo-type
PARSE_RULE_BOOL_FALSE, // opt/repo-type
#endif // opt/repo-type
), // opt/repo-type
// opt/repo-type
PARSE_RULE_OPTIONAL_DEFAULT // opt/repo-type
src/config/parse.c
+47 -2
View File
@@ -1420,6 +1420,46 @@ cfgFileLoad(
??? Add validation of section names and check all sections for invalid options in the check command. It's too expensive to add the
logic to this critical path code.
***********************************************************************************************************************************/
// Helper to check that option values are valid for conditional builds. This is a bit tricky since the distros used for unit testing
// have all possible features enabled, so this is split out to allow it to be tested independently. The loop variable is
// intentionally integrated into this function to make it obvious if it is omitted from the caller.
FN_EXTERN bool
cfgParseOptionValueCondition(
bool more, PackRead *const allowList, const bool allowListFound, const unsigned int optionId, const unsigned int optionKeyIdx,
const String *const valueAllow)
{
FUNCTION_TEST_BEGIN();
FUNCTION_TEST_PARAM(BOOL, more);
FUNCTION_TEST_PARAM(PACK_READ, allowList);
FUNCTION_TEST_PARAM(BOOL, allowListFound);
FUNCTION_TEST_PARAM(UINT, optionId);
FUNCTION_TEST_PARAM(UINT, optionKeyIdx);
FUNCTION_TEST_PARAM(STRING, valueAllow);
FUNCTION_TEST_END();
ASSERT(allowList != NULL);
ASSERT(valueAllow != NULL);
if (more && pckReadType(allowList) == pckTypeBool)
{
pckReadBoolP(allowList);
if (allowListFound)
{
THROW_FMT(
OptionInvalidValueError,
PROJECT_NAME " not built with '%s=%s' support\n"
"HINT: if " PROJECT_NAME " was installed from a package, does the package support this feature?\n"
"HINT: if " PROJECT_NAME " was built from source, were the required development packages installed?",
cfgParseOptionKeyIdxName(optionId, optionKeyIdx), strZ(valueAllow));
}
more = pckReadNext(allowList);
}
FUNCTION_TEST_RETURN(BOOL, more);
}
FN_EXTERN void
cfgParse(const Storage *const storage, const unsigned int argListSize, const char *argList[], const CfgParseParam param)
{
@@ -2373,9 +2413,14 @@ cfgParse(const Storage *const storage, const unsigned int argListSize, const cha
}
}
// Stop when value is found or allow list is exhausted
if (allowListFound || !pckReadNext(allowList))
// Stop when allow list is exhausted or value is found
if (!cfgParseOptionValueCondition(
pckReadNext(allowList), allowList, allowListFound, optionId, optionKeyIdx,
valueAllow) ||
allowListFound)
{
break;
}
}
pckReadFree(allowList);
test/src/module/build/configTest.c
+4 -1
View File
@@ -285,7 +285,7 @@ testRun(void)
" - 32768\n"
" allow-list:\n"
" - 8192\n"
" - 16384\n"
" - 16384: HAVE_LARGE_MEM\n"
" command-role:\n"
" main: {}\n"
"\n"
@@ -831,6 +831,9 @@ testRun(void)
" (\n"
" PARSE_RULE_VAL_INT(parseRuleValInt8192),\n"
" PARSE_RULE_VAL_INT(parseRuleValInt16384),\n"
"#ifndef HAVE_LARGE_MEM\n"
" PARSE_RULE_BOOL_FALSE,\n"
"#endif\n"
" ),\n"
" ),\n"
" ),\n"
test/src/module/common/compressTest.c
+3 -3
View File
@@ -352,7 +352,7 @@ testRun(void)
TEST_RESULT_VOID(FUNCTION_LOG_OBJECT_FORMAT(decompress, lz4DecompressToLog, buffer, sizeof(buffer)), "lz4DecompressToLog");
TEST_RESULT_Z(buffer, "{inputSame: true, inputOffset: 999, frameDone false, done: true}", "check log");
#else
TEST_ERROR(compressTypePresent(compressTypeLz4), OptionInvalidValueError, "pgBackRest not compiled with lz4 support");
TEST_ERROR(compressTypePresent(compressTypeLz4), OptionInvalidValueError, "pgBackRest not built with lz4 support");
#endif // HAVE_LIBLZ4
}
@@ -399,7 +399,7 @@ testRun(void)
TEST_RESULT_VOID(FUNCTION_LOG_OBJECT_FORMAT(decompress, zstDecompressToLog, buffer, sizeof(buffer)), "zstDecompressToLog");
TEST_RESULT_Z(buffer, "{inputSame: true, inputOffset: 999, frameDone false, done: true}", "check log");
#else
TEST_ERROR(compressTypePresent(compressTypeZst), OptionInvalidValueError, "pgBackRest not compiled with zst support");
TEST_ERROR(compressTypePresent(compressTypeZst), OptionInvalidValueError, "pgBackRest not built with zst support");
#endif // HAVE_LIBZST
}
@@ -422,7 +422,7 @@ testRun(void)
TEST_TITLE("compressTypePresent()");
TEST_RESULT_VOID(compressTypePresent(compressTypeNone), "type none always present");
TEST_ERROR(compressTypePresent(compressTypeXz), OptionInvalidValueError, "pgBackRest not compiled with xz support");
TEST_ERROR(compressTypePresent(compressTypeXz), OptionInvalidValueError, "pgBackRest not built with xz support");
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("compressTypeFromName()");
test/src/module/config/parseTest.c
+37
View File
@@ -2123,6 +2123,43 @@ testRun(void)
TEST_RESULT_UINT(cfgOptionGroupIdxToKey(cfgOptGrpRepo, 0), 1, "check repo1 key");
TEST_RESULT_Z(cfgOptionGroupName(cfgOptGrpRepo, 0), "repo1", "check repo1 name");
TEST_RESULT_STR_Z(cfgOptionIdxStr(cfgOptRepoPath, 0), "/var/lib/pgbackrest", "check repo1-path");
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("conditional option value -- value not found and no next value");
{
PackWrite *packWrite = pckWriteNewP();
pckWriteBoolP(packWrite, false, .defaultWrite = true);
pckWriteEndP(packWrite);
PackRead *packRead = pckReadNew(pckWriteResult(packWrite));
pckReadNext(packRead);
TEST_RESULT_BOOL(cfgParseOptionValueCondition(true, packRead, false, 0, 0, STRDEF("")), false, "check condition");
}
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("conditional option value -- value found");
{
argList = strLstNew();
hrnCfgArgRawZ(argList, cfgOptStanza, "test");
hrnCfgArgKeyRawZ(argList, cfgOptPgPath, 1, "/pg1");
hrnCfgEnvKeyRawZ(cfgOptRepoRetentionFull, 1, "1");
hrnCfgEnvRawZ(cfgOptCompressType, "lz4");
HRN_CFG_LOAD(cfgCmdBackup, argList);
PackWrite *packWrite = pckWriteNewP();
pckWriteBoolP(packWrite, false, .defaultWrite = true);
pckWriteEndP(packWrite);
PackRead *packRead = pckReadNew(pckWriteResult(packWrite));
pckReadNext(packRead);
TEST_ERROR(
cfgParseOptionValueCondition(true, packRead, true, cfgOptCompressType, 0, STRDEF("lz4")), OptionInvalidValueError,
"pgBackRest not built with 'compress-type=lz4' support\n"
"HINT: if pgBackRest was installed from a package, does the package support this feature?\n"
"HINT: if pgBackRest was built from source, were the required development packages installed?");
}
}
// *****************************************************************************************************************************