[pacman-dev] [PATCH 3/3] Implement parsing of the new SigLevel directive
Dan McGee
dpmcgee at gmail.com
Mon Jul 18 20:25:45 EDT 2011
On Mon, Jul 18, 2011 at 1:35 AM, Kerrick Staley <mail at kerrickstaley.com> wrote:
> Add code to conf.c that parses the new SigLevel directive.
>
> Signed-off-by: Kerrick Staley <mail at kerrickstaley.com>
> ---
> src/pacman/conf.c | 97 +++++++++++++++++++++++++++++++++++++++++++---------
> 1 files changed, 80 insertions(+), 17 deletions(-)
>
> diff --git a/src/pacman/conf.c b/src/pacman/conf.c
> index fac6da3..de5b463 100644
> --- a/src/pacman/conf.c
> +++ b/src/pacman/conf.c
> @@ -222,20 +222,83 @@ int config_set_arch(const char *arch)
> return 0;
> }
>
> -static alpm_siglevel_t option_verifysig(const char *value)
> +/**
> + * Parse a signature verification level. Destroys the passed string.
> + * @param str the string listing the siglevel options
> + * @return -1 if value was empty, -2 if value contained unknown
Shouldn't 0 be empty? Considering this method wouldn't even be called
if you never had a LHS option key of SigLevel.
> + * options, -3 if some of the options were contradictory, the siglevel
> + * value otherwise.
> + */
> +static alpm_siglevel_t option_siglevel(char *str)
> {
> - alpm_siglevel_t level;
> - if(strcmp(value, "Always") == 0) {
> - level = ALPM_SIG_PACKAGE | ALPM_SIG_DATABASE;
> - } else if(strcmp(value, "Optional") == 0) {
> - level = ALPM_SIG_PACKAGE | ALPM_SIG_PACKAGE_OPTIONAL |
> - ALPM_SIG_DATABASE | ALPM_SIG_DATABASE_OPTIONAL;
> - } else if(strcmp(value, "Never") == 0) {
> - level = 0;
> - } else {
> + alpm_siglevel_t level = 0;
> +
> + char *strtok_state;
> + char *tok = strtok_r(str, " \t", &strtok_state);
If we're taking tabs here, then we should take them everywhere, and it
should be done in a separate patch. See the strchr() call in
setrepeatingoption().
I'd rather not reinvent the tokenization and parsing wheel here.
Please use setrepeatingoption() and refactor this method to be more
like process_cleanmethods().
> +
> + if (!tok) {
> return -1;
> }
> - pm_printf(ALPM_LOG_DEBUG, "config: VerifySig = %s (%d)\n", value, level);
> +
> + do {
> + if(strcmp(tok, "Never") == 0) {
> + level = level ? -3 : 0;
> + break;
Why in the heck are you breaking out of the loop? This means
SigLevel = Never asdlfs;aiophgewioyh432i89 fkhg qiew5 o089a wl;
is now valid, no?
> + } else if(strcmp(tok, "Optional") == 0) {
> + level = level ? -3 : ALPM_SIG_PACKAGE | ALPM_SIG_PACKAGE_OPTIONAL |
> + ALPM_SIG_DATABASE | ALPM_SIG_DATABASE_OPTIONAL;
> + break;
> + } else if(strcmp(tok, "Required") == 0) {
> + level = level ? -3 : ALPM_SIG_PACKAGE | ALPM_SIG_DATABASE;
> + break;
I disagree with every one of these breaks. Always finish parsing what you start.
> + } else if(strcmp(tok, "PackageHash") == 0) {
> + if (level & ALPM_SIG_DATABASE_OPTIONAL) {
> + level = -3;
> + break;
> + }
> + level |= ALPM_SIG_PACKAGE_HASH_OK;
Kill this block, out of scope here.
> + } else if(strcmp(tok, "PackageRequired") == 0) {
> + if (level & ALPM_SIG_PACKAGE_OPTIONAL) {
> + level = -3;
> + break;
Also disagreeing with these breaks. If we are going to have the
non-package and non-database options, allowing one to override makes
perfect sense to me and should not be rejected.
SigLevel = Always PackageOptional
should work fine. I see it is getting rejected below. You realize
PACKAGE and PACKAGE_OPTIONAL are not mutually exclusive, right?
> + }
> + level |= ALPM_SIG_PACKAGE;
> + } else if(strcmp(tok, "DatabaseRequired") == 0) {
> + if (level & ALPM_SIG_DATABASE_OPTIONAL) {
> + level = -3;
> + break;
> + }
> + level |= ALPM_SIG_DATABASE;
> + } else if(strcmp(tok, "PackageOptional") == 0) {
> + if (level & ALPM_SIG_PACKAGE) {
> + level = -3;
> + break;
> + }
> + level |= ALPM_SIG_PACKAGE_OPTIONAL;
> + } else if(strcmp(tok, "DatabaseOptional") == 0) {
> + if (level & ALPM_SIG_PACKAGE) {
> + level = -3;
> + break;
> + }
> + level |= ALPM_SIG_DATABASE_OPTIONAL;
> + } else if(strcmp(tok, "PackageAllowMarginal") == 0) {
> + level |= ALPM_SIG_PACKAGE_MARGINAL_OK;
> + } else if(strcmp(tok, "DatabaseAllowMarginal") == 0) {
> + level |= ALPM_SIG_DATABASE_MARGINAL_OK;
> + } else if(strcmp(tok, "PackageAllowUnknown") == 0) {
> + level |= ALPM_SIG_PACKAGE_UNKNOWN_OK;
> + } else if(strcmp(tok, "DatabaseAllowUnknown") == 0) {
> + level |= ALPM_SIG_DATABASE_UNKNOWN_OK;
> + } else {
> + level = -2;
> + break;
> + }
> + } while ((tok = strtok_r(NULL, " \t", &strtok_state)));
> +
> + if (level >= 0) {
> + pm_printf(ALPM_LOG_DEBUG, "config: SigLevel = %s (%d)\n", str, level);
> + }
No need to restrict this printout as it is a debug message.
> +
> return level;
> }
>
> @@ -359,9 +422,9 @@ static int _parse_options(const char *key, char *value,
> return 1;
> }
> FREELIST(methods);
> - } else if(strcmp(key, "VerifySig") == 0) {
> - alpm_siglevel_t level = option_verifysig(value);
> - if(level != -1) {
> + } else if(strcmp(key, "SigLevel") == 0) {
> + alpm_siglevel_t level = option_siglevel(value);
> + if(level >= 0) {
> config->siglevel = level;
> } else {
> pm_printf(ALPM_LOG_ERROR,
This error message won't work at all as you just finished trashing the
contents of value.
> @@ -726,9 +789,9 @@ static int _parseconfig(const char *file, struct section_t *section,
> goto cleanup;
> }
> section->servers = alpm_list_add(section->servers, strdup(value));
> - } else if(strcmp(key, "VerifySig") == 0) {
> - alpm_siglevel_t level = option_verifysig(value);
> - if(level != -1) {
> + } else if(strcmp(key, "SigLevel") == 0) {
> + alpm_siglevel_t level = option_siglevel(value);
> + if(level >= 0) {
This now rejects a 'VerifySig = ' line, which perhaps is odd, but I'm
not sure is correct (and I don't think we did it before?). One would
assume this would use the default value or something.
> section->siglevel = level;
> } else {
> pm_printf(ALPM_LOG_ERROR,
> --
> 1.7.6
More information about the pacman-dev
mailing list