On Mon, Jul 18, 2011 at 1:35 AM, Kerrick Staley <mail@kerrickstaley.com> wrote:
Add code to conf.c that parses the new SigLevel directive.
Signed-off-by: Kerrick Staley <mail@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