[pacman-dev] [PATCH 1/9] conf: _parseconfig() cleanups and documentation
* Function doxygen documentation * Reuse a single strlen() call * Prevent infinite recursion (limit to 10 levels) * Other small cleanups Signed-off-by: Dan McGee <dan@archlinux.org> --- src/pacman/conf.c | 59 +++++++++++++++++++++++++++++++++++------------------ 1 files changed, 39 insertions(+), 20 deletions(-) diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 97e6497..68fe59c 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -508,32 +508,52 @@ static int setup_libalpm(void) } -/* The real parseconfig. Called with a null section argument by the publicly - * visible parseconfig so we can recall from within ourself on an include */ -static int _parseconfig(const char *file, int parse_options, - char **section, pmdb_t *db) +/** The "real" parseconfig. Each "Include" directive will recall this method so + * recursion and stack depth are limited to 10 levels. The publicly visible + * parseconfig calls this with a NULL section argument so we can recall from + * within ourself on an include. + * @param file path to the config file + * @param section the current active section name; should be freed after all + * parsing is complete + * @param db the current active alpm database object + * @param parse_options whether to parse and call methods for the options + * section; if 0, parse and call methods for the repos sections + * @param depth the current recursion depth + **/ +static int _parseconfig(const char *file, char **section, pmdb_t *db, + int parse_options, int depth) { FILE *fp = NULL; - char line[PATH_MAX+1]; + char line[PATH_MAX]; int linenum = 0; - char *ptr; int ret = 0; + const int max_depth = 10; + + if(depth >= max_depth) { + pm_printf(PM_LOG_ERROR, + _("config parsing exceeded max recursion depth of %d.\n"), max_depth); + ret = 1; + goto cleanup; + } pm_printf(PM_LOG_DEBUG, "config: attempting to read file %s\n", file); fp = fopen(file, "r"); if(fp == NULL) { pm_printf(PM_LOG_ERROR, _("config file %s could not be read.\n"), file); - return 1; + ret = 1; + goto cleanup; } while(fgets(line, PATH_MAX, fp)) { - char *key, *value; + char *key, *value, *ptr; + size_t line_len; linenum++; strtrim(line); + line_len = strlen(line); /* ignore whole line and end of line comments */ - if(strlen(line) == 0 || line[0] == '#') { + if(line_len == 0 || line[0] == '#') { continue; } if((ptr = strchr(line, '#'))) { @@ -548,19 +568,18 @@ static int _parseconfig(const char *file, int parse_options, goto cleanup; } - if(line[0] == '[' && line[strlen(line)-1] == ']') { + if(line[0] == '[' && line[line_len - 1] == ']') { char *name; - /* new config section, skip the '[' */ - ptr = line; - ptr++; - name = strdup(ptr); - name[strlen(name)-1] = '\0'; - if(!strlen(name)) { + /* only possibility here is a line == '[]' */ + if(line_len <= 2) { pm_printf(PM_LOG_ERROR, _("config file %s, line %d: bad section name.\n"), file, linenum); ret = 1; goto cleanup; } + /* new config section, skip the '[' */ + name = strdup(line + 1); + name[line_len - 2] = '\0'; pm_printf(PM_LOG_DEBUG, "config: new section '%s'\n", name); /* if we are not looking at the options section, register a db */ if(!parse_options && strcmp(name, "options") != 0) { @@ -634,7 +653,7 @@ static int _parseconfig(const char *file, int parse_options, for(gindex = 0; gindex < globbuf.gl_pathc; gindex++) { pm_printf(PM_LOG_DEBUG, "config file %s, line %d: including %s\n", file, linenum, globbuf.gl_pathv[gindex]); - _parseconfig(globbuf.gl_pathv[gindex], parse_options, section, db); + _parseconfig(globbuf.gl_pathv[gindex], section, db, parse_options, depth++); } break; } @@ -690,7 +709,7 @@ cleanup: } /** Parse a configuration file. - * @param file path to the config file. + * @param file path to the config file * @return 0 on success, non-zero on error */ int parseconfig(const char *file) @@ -703,7 +722,7 @@ int parseconfig(const char *file) /* call the real parseconfig function with a null section & db argument */ pm_printf(PM_LOG_DEBUG, "parseconfig: options pass\n"); - if((ret = _parseconfig(file, 1, §ion, NULL))) { + if((ret = _parseconfig(file, §ion, NULL, 1, 0))) { free(section); return ret; } @@ -714,7 +733,7 @@ int parseconfig(const char *file) /* second pass, repo section parsing */ section = NULL; pm_printf(PM_LOG_DEBUG, "parseconfig: repo pass\n"); - return _parseconfig(file, 0, §ion, NULL); + return _parseconfig(file, §ion, NULL, 0, 0); free(section); } -- 1.7.5.2
We now parse an entire repo section and store all information about it. When the next section is encountered or the end of the root config file is reached, we will then process the stored information. Signed-off-by: Dan McGee <dan@archlinux.org> --- src/pacman/conf.c | 149 +++++++++++++++++++++++++++++++++++----------------- 1 files changed, 100 insertions(+), 49 deletions(-) diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 68fe59c..a5bc148 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -507,20 +507,93 @@ static int setup_libalpm(void) return 0; } +/** + * Allows parsing in advance of an entire config section before we start + * calling library methods. + */ +struct section_t { + /* useful for all sections */ + char *name; + int is_options; + /* db section option gathering */ + pgp_verify_t sigverify; + alpm_list_t *servers; +}; + +/** + * Wrap up a section once we have reached the end of it. This should be called + * when a subsequent section is encountered, or when we have reached the end of + * the root config file. Once called, all existing saved config pieces on the + * section struct are freed. + * @param section the current parsed and saved section data + * @param parse_options whether we are parsing options or repo data + * @return 0 on success, 1 on failure + */ +static int finish_section(struct section_t *section, int parse_options) +{ + int ret = 0; + alpm_list_t *i; + pmdb_t *db; + + pm_printf(PM_LOG_DEBUG, "config: finish section '%s'\n", section->name); + + /* parsing options (or nothing)- nothing to do except free the pieces */ + if(!section->name || parse_options || section->is_options) { + goto cleanup; + } + + /* if we are not looking at options sections only, register a db */ + db = alpm_db_register_sync(config->handle, section->name); + if(db == NULL) { + pm_printf(PM_LOG_ERROR, _("could not register '%s' database (%s)\n"), + section->name, alpm_strerror(alpm_errno(config->handle))); + ret = 1; + goto cleanup; + } + + if(section->sigverify) { + if(alpm_db_set_pgp_verify(db, section->sigverify)) { + pm_printf(PM_LOG_ERROR, + _("could not set verify option for database '%s' (%s)\n"), + section->name, alpm_strerror(alpm_errno(config->handle))); + ret = 1; + goto cleanup; + } + } + + for(i = section->servers; i; i = alpm_list_next(i)) { + char *value = alpm_list_getdata(i); + if(_add_mirror(db, value) != 0) { + pm_printf(PM_LOG_ERROR, + _("could not add mirror '%s' to database '%s' (%s)\n"), + value, section->name, alpm_strerror(alpm_errno(config->handle))); + ret = 1; + goto cleanup; + } + free(value); + } + +cleanup: + alpm_list_free(section->servers); + section->servers = NULL; + section->sigverify = 0; + free(section->name); + section->name = NULL; + return ret; +} /** The "real" parseconfig. Each "Include" directive will recall this method so * recursion and stack depth are limited to 10 levels. The publicly visible * parseconfig calls this with a NULL section argument so we can recall from * within ourself on an include. * @param file path to the config file - * @param section the current active section name; should be freed after all - * parsing is complete - * @param db the current active alpm database object + * @param section the current active section * @param parse_options whether to parse and call methods for the options * section; if 0, parse and call methods for the repos sections * @param depth the current recursion depth - **/ -static int _parseconfig(const char *file, char **section, pmdb_t *db, + * @return 0 on success, 1 on failure + */ +static int _parseconfig(const char *file, struct section_t *section, int parse_options, int depth) { FILE *fp = NULL; @@ -560,14 +633,6 @@ static int _parseconfig(const char *file, char **section, pmdb_t *db, *ptr = '\0'; } - /* sanity check */ - if(parse_options && db) { - pm_printf(PM_LOG_ERROR, _("config file %s, line %d: parsing options but have a database.\n"), - file, linenum); - ret = 1; - goto cleanup; - } - if(line[0] == '[' && line[line_len - 1] == ']') { char *name; /* only possibility here is a line == '[]' */ @@ -580,21 +645,14 @@ static int _parseconfig(const char *file, char **section, pmdb_t *db, /* new config section, skip the '[' */ name = strdup(line + 1); name[line_len - 2] = '\0'; - pm_printf(PM_LOG_DEBUG, "config: new section '%s'\n", name); - /* if we are not looking at the options section, register a db */ - if(!parse_options && strcmp(name, "options") != 0) { - db = alpm_db_register_sync(config->handle, name); - if(db == NULL) { - pm_printf(PM_LOG_ERROR, _("could not register '%s' database (%s)\n"), - name, alpm_strerror(alpm_errno(config->handle))); - ret = 1; - goto cleanup; - } - } - if(*section) { - free(*section); + /* we're at a new section; perform any post-actions for the prior */ + if(finish_section(section, parse_options)) { + ret = 1; + goto cleanup; } - *section = name; + pm_printf(PM_LOG_DEBUG, "config: new section '%s'\n", name); + section->name = name; + section->is_options = (strcmp(name, "options") == 0); continue; } @@ -613,7 +671,7 @@ static int _parseconfig(const char *file, char **section, pmdb_t *db, goto cleanup; } /* For each directive, compare to the camelcase string. */ - if(*section == NULL) { + if(section->name == NULL) { pm_printf(PM_LOG_ERROR, _("config file %s, line %d: All directives must belong to a section.\n"), file, linenum); ret = 1; @@ -653,19 +711,19 @@ static int _parseconfig(const char *file, char **section, pmdb_t *db, for(gindex = 0; gindex < globbuf.gl_pathc; gindex++) { pm_printf(PM_LOG_DEBUG, "config file %s, line %d: including %s\n", file, linenum, globbuf.gl_pathv[gindex]); - _parseconfig(globbuf.gl_pathv[gindex], section, db, parse_options, depth++); + _parseconfig(globbuf.gl_pathv[gindex], section, parse_options, depth + 1); } break; } globfree(&globbuf); continue; } - if(parse_options && strcmp(*section, "options") == 0) { + if(parse_options && section->is_options) { /* we are either in options ... */ if((ret = _parse_options(key, value, file, linenum)) != 0) { goto cleanup; } - } else if (!parse_options && strcmp(*section, "options") != 0) { + } else if (!parse_options && !section->is_options) { /* ... or in a repo section */ if(strcmp(key, "Server") == 0) { if(value == NULL) { @@ -674,19 +732,11 @@ static int _parseconfig(const char *file, char **section, pmdb_t *db, ret = 1; goto cleanup; } - if(_add_mirror(db, value) != 0) { - ret = 1; - goto cleanup; - } + section->servers = alpm_list_add(section->servers, strdup(value)); } else if(strcmp(key, "VerifySig") == 0) { pgp_verify_t level = option_verifysig(value); if(level != PM_PGP_VERIFY_UNKNOWN) { - ret = alpm_db_set_pgp_verify(db, level); - if(ret != 0) { - pm_printf(PM_LOG_ERROR, _("could not add set verify option for database '%s': %s (%s)\n"), - alpm_db_get_name(db), value, alpm_strerror(alpm_errno(config->handle))); - goto cleanup; - } + section->sigverify = level; } else { pm_printf(PM_LOG_ERROR, _("config file %s, line %d: directive '%s' has invalid value '%s'\n"), @@ -697,11 +747,15 @@ static int _parseconfig(const char *file, char **section, pmdb_t *db, } else { pm_printf(PM_LOG_WARNING, _("config file %s, line %d: directive '%s' in section '%s' not recognized.\n"), - file, linenum, key, *section); + file, linenum, key, section->name); } } } + if(depth == 0) { + ret = finish_section(section, parse_options); + } + cleanup: fclose(fp); pm_printf(PM_LOG_DEBUG, "config: finished parsing %s\n", file); @@ -715,26 +769,23 @@ cleanup: int parseconfig(const char *file) { int ret; - char *section = NULL; + struct section_t section; + memset(§ion, 0, sizeof(struct section_t)); /* the config parse is a two-pass affair. We first parse the entire thing for * the [options] section so we can get all default and path options set. * Next, we go back and parse everything but [options]. */ /* call the real parseconfig function with a null section & db argument */ pm_printf(PM_LOG_DEBUG, "parseconfig: options pass\n"); - if((ret = _parseconfig(file, §ion, NULL, 1, 0))) { - free(section); + if((ret = _parseconfig(file, §ion, 1, 0))) { return ret; } - free(section); if((ret = setup_libalpm())) { return ret; } /* second pass, repo section parsing */ - section = NULL; pm_printf(PM_LOG_DEBUG, "parseconfig: repo pass\n"); - return _parseconfig(file, §ion, NULL, 0, 0); - free(section); + return _parseconfig(file, §ion, 0, 0); } /* vim: set ts=2 sw=2 noet: */ -- 1.7.5.2
This was a lot of stuff that can stand by itself for the most part. Signed-off-by: Dan McGee <dan@archlinux.org> --- lib/libalpm/be_sync.c | 53 ++++++++++++++++++++++++++++-------------------- 1 files changed, 31 insertions(+), 22 deletions(-) diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 18e8ded..ccc8fdb 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -37,6 +37,34 @@ #include "deps.h" #include "dload.h" +static char *get_sync_dir(pmhandle_t *handle) +{ + const char *dbpath = alpm_option_get_dbpath(handle); + size_t len = strlen(dbpath) + 6; + char *syncpath; + struct stat buf; + + MALLOC(syncpath, len, RET_ERR(handle, PM_ERR_MEMORY, NULL)); + sprintf(syncpath, "%s%s", dbpath, "sync/"); + + if(stat(syncpath, &buf) != 0) { + _alpm_log(handle, PM_LOG_DEBUG, "database dir '%s' does not exist, creating it\n", + syncpath); + if(_alpm_makepath(syncpath) != 0) { + free(syncpath); + RET_ERR(handle, PM_ERR_SYSTEM, NULL); + } + } else if(!S_ISDIR(buf.st_mode)) { + _alpm_log(handle, PM_LOG_WARNING, _("removing invalid file: %s\n"), syncpath); + if(unlink(syncpath) != 0 || _alpm_makepath(syncpath) != 0) { + free(syncpath); + RET_ERR(handle, PM_ERR_SYSTEM, NULL); + } + } + + return syncpath; +} + /** Update a package database * * An update of the package database \a db will be attempted. Unless @@ -79,10 +107,7 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db) { char *syncpath; - const char *dbpath; alpm_list_t *i; - struct stat buf; - size_t len; int ret = -1; mode_t oldmask; pmhandle_t *handle; @@ -94,29 +119,13 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db) ASSERT(db != handle->db_local, RET_ERR(handle, PM_ERR_WRONG_ARGS, -1)); ASSERT(db->servers != NULL, RET_ERR(handle, PM_ERR_SERVER_NONE, -1)); - dbpath = alpm_option_get_dbpath(handle); - len = strlen(dbpath) + 6; - MALLOC(syncpath, len, RET_ERR(handle, PM_ERR_MEMORY, -1)); - sprintf(syncpath, "%s%s", dbpath, "sync/"); - /* make sure we have a sane umask */ oldmask = umask(0022); - if(stat(syncpath, &buf) != 0) { - _alpm_log(handle, PM_LOG_DEBUG, "database dir '%s' does not exist, creating it\n", - syncpath); - if(_alpm_makepath(syncpath) != 0) { - free(syncpath); - RET_ERR(handle, PM_ERR_SYSTEM, -1); - } - } else if(!S_ISDIR(buf.st_mode)) { - _alpm_log(handle, PM_LOG_WARNING, _("removing invalid file: %s\n"), syncpath); - if(unlink(syncpath) != 0 || _alpm_makepath(syncpath) != 0) { - free(syncpath); - RET_ERR(handle, PM_ERR_SYSTEM, -1); - } + syncpath = get_sync_dir(handle); + if(!syncpath) { + return -1; } - check_sig = _alpm_db_get_sigverify_level(db); for(i = db->servers; i; i = i->next) { -- 1.7.5.2
Note that is a bit different than the normal _alpm_db_path() method; the caller is expected to free the result. Signed-off-by: Dan McGee <dan@archlinux.org> --- lib/libalpm/db.c | 14 ++++++++++++++ lib/libalpm/db.h | 1 + 2 files changed, 15 insertions(+), 0 deletions(-) diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c index b2f3762..f26eaae 100644 --- a/lib/libalpm/db.c +++ b/lib/libalpm/db.c @@ -378,6 +378,20 @@ int _alpm_db_version(pmdb_t *db) return db->ops->version(db); } +char *_alpm_db_sig_path(pmdb_t *db) +{ + char *sigpath; + size_t len; + const char *dbfile = _alpm_db_path(db); + if(!db || !dbfile) { + return NULL; + } + len = strlen(dbfile) + strlen(".sig") + 1; + CALLOC(sigpath, len, sizeof(char), RET_ERR(db->handle, PM_ERR_MEMORY, NULL)); + sprintf(sigpath, "%s.sig", dbfile); + return sigpath; +} + int _alpm_db_cmp(const void *d1, const void *d2) { pmdb_t *db1 = (pmdb_t *)d1; diff --git a/lib/libalpm/db.h b/lib/libalpm/db.h index 9ecf31f..e3faeeb 100644 --- a/lib/libalpm/db.h +++ b/lib/libalpm/db.h @@ -72,6 +72,7 @@ struct __pmdb_t { pmdb_t *_alpm_db_new(const char *treename, int is_local); void _alpm_db_free(pmdb_t *db); const char *_alpm_db_path(pmdb_t *db); +char *_alpm_db_sig_path(pmdb_t *db); int _alpm_db_version(pmdb_t *db); int _alpm_db_cmp(const void *d1, const void *d2); alpm_list_t *_alpm_db_search(pmdb_t *db, const alpm_list_t *needles); -- 1.7.5.2
On 08/06/11 17:51, Dan McGee wrote:
Note that is a bit different than the normal _alpm_db_path() method; the caller is expected to free the result.
Signed-off-by: Dan McGee<dan@archlinux.org> --- lib/libalpm/db.c | 14 ++++++++++++++ lib/libalpm/db.h | 1 + 2 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c index b2f3762..f26eaae 100644 --- a/lib/libalpm/db.c +++ b/lib/libalpm/db.c @@ -378,6 +378,20 @@ int _alpm_db_version(pmdb_t *db) return db->ops->version(db); }
+char *_alpm_db_sig_path(pmdb_t *db) +{ + char *sigpath; + size_t len; + const char *dbfile = _alpm_db_path(db); + if(!db || !dbfile) { + return NULL; + } + len = strlen(dbfile) + strlen(".sig") + 1;
Note that most other places we just do "+ 5" rather than the strlen(".sig"). Not that it matters given it will be optimised out...
+ CALLOC(sigpath, len, sizeof(char), RET_ERR(db->handle, PM_ERR_MEMORY, NULL)); + sprintf(sigpath, "%s.sig", dbfile); + return sigpath; +} + int _alpm_db_cmp(const void *d1, const void *d2) { pmdb_t *db1 = (pmdb_t *)d1; diff --git a/lib/libalpm/db.h b/lib/libalpm/db.h index 9ecf31f..e3faeeb 100644 --- a/lib/libalpm/db.h +++ b/lib/libalpm/db.h @@ -72,6 +72,7 @@ struct __pmdb_t { pmdb_t *_alpm_db_new(const char *treename, int is_local); void _alpm_db_free(pmdb_t *db); const char *_alpm_db_path(pmdb_t *db); +char *_alpm_db_sig_path(pmdb_t *db); int _alpm_db_version(pmdb_t *db); int _alpm_db_cmp(const void *d1, const void *d2); alpm_list_t *_alpm_db_search(pmdb_t *db, const alpm_list_t *needles);
On Wed, Jun 8, 2011 at 6:27 AM, Allan McRae <allan@archlinux.org> wrote:
On 08/06/11 17:51, Dan McGee wrote:
Note that is a bit different than the normal _alpm_db_path() method; the caller is expected to free the result.
Signed-off-by: Dan McGee<dan@archlinux.org> --- lib/libalpm/db.c | 14 ++++++++++++++ lib/libalpm/db.h | 1 + 2 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c index b2f3762..f26eaae 100644 --- a/lib/libalpm/db.c +++ b/lib/libalpm/db.c @@ -378,6 +378,20 @@ int _alpm_db_version(pmdb_t *db) return db->ops->version(db); }
+char *_alpm_db_sig_path(pmdb_t *db) +{ + char *sigpath; + size_t len; + const char *dbfile = _alpm_db_path(db); + if(!db || !dbfile) { + return NULL; + } + len = strlen(dbfile) + strlen(".sig") + 1;
Note that most other places we just do "+ 5" rather than the strlen(".sig"). Not that it matters given it will be optimised out... Yeah, at least here clarity won the battle for me of how to write it. I'm fine with doing it the strlen(static string) way as far as coding standards goes. It doesn't look like there are all that many of these anyway.
-Dan $ git grep 'len =.*+.*[0-9]*[02-9];' | cat lib/libalpm/add.c: size_t backup_len = strlen(oldbackup) + 34; lib/libalpm/add.c: size_t backup_len = strlen(oldbackup) + 34; lib/libalpm/be_local.c: len = strlen(dbpath) + strlen(info->name) + strlen(info->version) + 3; lib/libalpm/be_sync.c: size_t len = strlen(dbpath) + 6; lib/libalpm/be_sync.c: len = strlen(server) + strlen(db->treename) + 9; lib/libalpm/dload.c: len = strlen(url) + 5; lib/libalpm/signing.c: size_t len = strlen(path) + 5; lib/libalpm/sync.c: len = strlen(cachedir) + strlen(d->from) + 2; lib/libalpm/sync.c: len = strlen(cachedir) + strlen(d->to) + 2; lib/libalpm/sync.c: len = strlen(server_url) + strlen(filename) + 2; lib/libalpm/trans.c: size_t len = strlen(pkgname) + strlen(pkgver) + strlen(pkgarch) + 3; src/pacman/callback.c: len = strlen(opr) + ((pkgname) ? strlen(pkgname) : 0) + 2; src/pacman/conf.c: size_t len = strlen(path) + strlen(filename) + 6;
This is the ideal place to do it as all clients should be checking the return value and ensuring there are no errors. This is similar to pkg_load(). We also add an additional step of validation after we download a new database; a subsequent '-y' operation can potentially invalidate the original check at registration time. Signed-off-by: Dan McGee <dan@archlinux.org> --- lib/libalpm/alpm.h | 5 +++- lib/libalpm/be_sync.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++- lib/libalpm/db.c | 5 ++- lib/libalpm/db.h | 3 +- src/pacman/conf.c | 16 ++----------- src/util/cleanupdelta.c | 2 +- src/util/testdb.c | 2 +- test/pacman/pactest.py | 7 ++--- 8 files changed, 72 insertions(+), 24 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index d7ee7d1..1e1bfdd 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -276,9 +276,12 @@ alpm_list_t *alpm_option_get_syncdbs(pmhandle_t *handle); /** Register a sync database of packages. * @param handle the context handle * @param treename the name of the sync repository + * @param check_sig what level of signature checking to perform on the + * database; note that this must be a '.sig' file type verification * @return a pmdb_t* on success (the value), NULL on error */ -pmdb_t *alpm_db_register_sync(pmhandle_t *handle, const char *treename); +pmdb_t *alpm_db_register_sync(pmhandle_t *handle, const char *treename, + pgp_verify_t check_sig); /** Unregister a package database. * @param db pointer to the package database to unregister diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index ccc8fdb..c6fee84 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -20,7 +20,9 @@ #include "config.h" +#include <errno.h> #include <sys/stat.h> +#include <unistd.h> /* libarchive */ #include <archive.h> @@ -65,6 +67,37 @@ static char *get_sync_dir(pmhandle_t *handle) return syncpath; } +static int sync_db_validate(pmdb_t *db) +{ + /* this takes into account the default verification level if UNKNOWN + * was assigned to this db */ + pgp_verify_t check_sig = _alpm_db_get_sigverify_level(db); + + if(check_sig != PM_PGP_VERIFY_NEVER) { + int ret; + const char *dbpath = _alpm_db_path(db); + if(!dbpath) { + /* pm_errno set in _alpm_db_path() */ + return -1; + } + + /* we can skip any validation if the database doesn't exist */ + if(access(dbpath, R_OK) != 0 && errno == ENOENT) { + return 0; + } + + _alpm_log(db->handle, PM_LOG_DEBUG, "checking signature for %s\n", + db->treename); + ret = _alpm_gpgme_checksig(db->handle, dbpath, NULL); + if((check_sig == PM_PGP_VERIFY_ALWAYS && ret != 0) || + (check_sig == PM_PGP_VERIFY_OPTIONAL && ret == 1)) { + RET_ERR(db->handle, PM_ERR_SIG_INVALID, -1); + } + } + + return 0; +} + /** Update a package database * * An update of the package database \a db will be attempted. Unless @@ -143,6 +176,15 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db) if(ret == 0 && (check_sig == PM_PGP_VERIFY_ALWAYS || check_sig == PM_PGP_VERIFY_OPTIONAL)) { + /* an existing sig file is no good at this point */ + char *sigpath = _alpm_db_sig_path(db); + if(!sigpath) { + ret = -1; + break; + } + unlink(sigpath); + free(sigpath); + int errors_ok = (check_sig == PM_PGP_VERIFY_OPTIONAL); /* if we downloaded a DB, we want the .sig from the same server */ snprintf(fileurl, len, "%s/%s.db.sig", server, db->treename); @@ -172,6 +214,11 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db) /* Cache needs to be rebuilt */ _alpm_db_free_pkgcache(db); + if(sync_db_validate(db)) { + /* pm_errno should be set */ + ret = -1; + } + cleanup: free(syncpath); @@ -524,7 +571,8 @@ struct db_operations sync_db_ops = { .version = sync_db_version, }; -pmdb_t *_alpm_db_register_sync(pmhandle_t *handle, const char *treename) +pmdb_t *_alpm_db_register_sync(pmhandle_t *handle, const char *treename, + pgp_verify_t level) { pmdb_t *db; @@ -536,6 +584,12 @@ pmdb_t *_alpm_db_register_sync(pmhandle_t *handle, const char *treename) } db->ops = &sync_db_ops; db->handle = handle; + db->pgp_verify = level; + + if(sync_db_validate(db)) { + _alpm_db_free(db); + return NULL; + } handle->dbs_sync = alpm_list_add(handle->dbs_sync, db); return db; diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c index f26eaae..9c974f5 100644 --- a/lib/libalpm/db.c +++ b/lib/libalpm/db.c @@ -45,7 +45,8 @@ */ /** Register a sync database of packages. */ -pmdb_t SYMEXPORT *alpm_db_register_sync(pmhandle_t *handle, const char *treename) +pmdb_t SYMEXPORT *alpm_db_register_sync(pmhandle_t *handle, const char *treename, + pgp_verify_t check_sig) { /* Sanity checks */ ASSERT(handle != NULL, return NULL); @@ -54,7 +55,7 @@ pmdb_t SYMEXPORT *alpm_db_register_sync(pmhandle_t *handle, const char *treename /* Do not register a database if a transaction is on-going */ ASSERT(handle->trans == NULL, RET_ERR(handle, PM_ERR_TRANS_NOT_NULL, NULL)); - return _alpm_db_register_sync(handle, treename); + return _alpm_db_register_sync(handle, treename, check_sig); } /* Helper function for alpm_db_unregister{_all} */ diff --git a/lib/libalpm/db.h b/lib/libalpm/db.h index e3faeeb..c5fcd5f 100644 --- a/lib/libalpm/db.h +++ b/lib/libalpm/db.h @@ -77,7 +77,8 @@ int _alpm_db_version(pmdb_t *db); int _alpm_db_cmp(const void *d1, const void *d2); alpm_list_t *_alpm_db_search(pmdb_t *db, const alpm_list_t *needles); pmdb_t *_alpm_db_register_local(pmhandle_t *handle); -pmdb_t *_alpm_db_register_sync(pmhandle_t *handle, const char *treename); +pmdb_t *_alpm_db_register_sync(pmhandle_t *handle, const char *treename, + pgp_verify_t level); void _alpm_db_unregister(pmdb_t *db); /* be_*.c, backend specific calls */ diff --git a/src/pacman/conf.c b/src/pacman/conf.c index a5bc148..33ac0b8 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -460,7 +460,7 @@ static int setup_libalpm(void) ret = alpm_option_set_logfile(handle, config->logfile); if(ret != 0) { pm_printf(PM_LOG_ERROR, _("problem setting logfile '%s' (%s)\n"), - config->logfile, alpm_strerror(alpm_errno(config->handle))); + config->logfile, alpm_strerror(alpm_errno(handle))); return ret; } @@ -470,7 +470,7 @@ static int setup_libalpm(void) ret = alpm_option_set_signaturedir(handle, config->gpgdir); if(ret != 0) { pm_printf(PM_LOG_ERROR, _("problem setting gpgdir '%s' (%s)\n"), - config->gpgdir, alpm_strerror(alpm_errno(config->handle))); + config->gpgdir, alpm_strerror(alpm_errno(handle))); return ret; } @@ -543,7 +543,7 @@ static int finish_section(struct section_t *section, int parse_options) } /* if we are not looking at options sections only, register a db */ - db = alpm_db_register_sync(config->handle, section->name); + db = alpm_db_register_sync(config->handle, section->name, section->sigverify); if(db == NULL) { pm_printf(PM_LOG_ERROR, _("could not register '%s' database (%s)\n"), section->name, alpm_strerror(alpm_errno(config->handle))); @@ -551,16 +551,6 @@ static int finish_section(struct section_t *section, int parse_options) goto cleanup; } - if(section->sigverify) { - if(alpm_db_set_pgp_verify(db, section->sigverify)) { - pm_printf(PM_LOG_ERROR, - _("could not set verify option for database '%s' (%s)\n"), - section->name, alpm_strerror(alpm_errno(config->handle))); - ret = 1; - goto cleanup; - } - } - for(i = section->servers; i; i = alpm_list_next(i)) { char *value = alpm_list_getdata(i); if(_add_mirror(db, value) != 0) { diff --git a/src/util/cleanupdelta.c b/src/util/cleanupdelta.c index 9829170..5ee59db 100644 --- a/src/util/cleanupdelta.c +++ b/src/util/cleanupdelta.c @@ -75,7 +75,7 @@ static void checkdbs(const char *dbpath, alpm_list_t *dbnames) { for(i = dbnames; i; i = alpm_list_next(i)) { char *dbname = alpm_list_getdata(i); snprintf(syncdbpath, PATH_MAX, "%s/sync/%s", dbpath, dbname); - db = alpm_db_register_sync(handle, dbname); + db = alpm_db_register_sync(handle, dbname, PM_PGP_VERIFY_OPTIONAL); if(db == NULL) { fprintf(stderr, "error: could not register sync database (%s)\n", alpm_strerror(alpm_errno(handle))); diff --git a/src/util/testdb.c b/src/util/testdb.c index 0bd7820..f45b4a9 100644 --- a/src/util/testdb.c +++ b/src/util/testdb.c @@ -152,7 +152,7 @@ static int check_syncdbs(alpm_list_t *dbnames) { for(i = dbnames; i; i = alpm_list_next(i)) { char *dbname = alpm_list_getdata(i); - db = alpm_db_register_sync(handle, dbname); + db = alpm_db_register_sync(handle, dbname, PM_PGP_VERIFY_OPTIONAL); if(db == NULL) { fprintf(stderr, "error: could not register sync database (%s)\n", alpm_strerror(alpm_errno(handle))); diff --git a/test/pacman/pactest.py b/test/pacman/pactest.py index 77f87da..deb338f 100755 --- a/test/pacman/pactest.py +++ b/test/pacman/pactest.py @@ -113,13 +113,12 @@ def createOptParser(): env.run() env.results() - if env.failed > 0: - print "pacman testing root saved: %s" % root_path - sys.exit(1) - if not opts.keeproot: shutil.rmtree(root_path) else: print "pacman testing root saved: %s" % root_path + if env.failed > 0: + sys.exit(1) + # vim: set ts=4 sw=4 et: -- 1.7.5.2
On Wed, Jun 8, 2011 at 2:51 AM, Dan McGee <dan@archlinux.org> wrote:
This is the ideal place to do it as all clients should be checking the return value and ensuring there are no errors. This is similar to pkg_load().
We also add an additional step of validation after we download a new database; a subsequent '-y' operation can potentially invalidate the original check at registration time.
This patch does break the current implementation of sign001.py; we will need to beef up the test system a bit more to support DB sigs in addition to package ones. -Dan
This will let us keep track if the database itself has been marked valid in whatever fashion. For local databases at the moment we ensure there are no depends files; for sync databases we ensure the PGP signature is valid if required/requested. The loading of the pkgcache is prohibited if the database is invalid. Signed-off-by: Dan McGee <dan@archlinux.org> --- lib/libalpm/alpm.c | 2 +- lib/libalpm/be_local.c | 25 +++++++++++++++++-------- lib/libalpm/be_sync.c | 20 +++++++++++++------- lib/libalpm/db.c | 18 +++++------------- lib/libalpm/db.h | 4 ++-- lib/libalpm/trans.c | 12 ------------ src/pacman/conf.c | 3 +++ src/pacman/util.c | 4 ---- 8 files changed, 41 insertions(+), 47 deletions(-) diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c index 5d475ce..f3f83c6 100644 --- a/lib/libalpm/alpm.c +++ b/lib/libalpm/alpm.c @@ -70,7 +70,7 @@ pmhandle_t SYMEXPORT *alpm_initialize(const char *root, const char *dbpath, snprintf(myhandle->lockfile, lockfilelen, "%s%s", myhandle->dbpath, lf); if(_alpm_db_register_local(myhandle) == NULL) { - myerr = PM_ERR_DB_CREATE; + myerr = myhandle->pm_errno; goto cleanup; } diff --git a/lib/libalpm/be_local.c b/lib/libalpm/be_local.c index d9a76cc..a37357a 100644 --- a/lib/libalpm/be_local.c +++ b/lib/libalpm/be_local.c @@ -863,12 +863,15 @@ int _alpm_local_db_remove(pmdb_t *db, pmpkg_t *info) return ret; } -static int local_db_version(pmdb_t *db) +static int local_db_validate(pmdb_t *db) { struct dirent *ent = NULL; const char *dbpath; DIR *dbdir; - int version; + + if(db->valid) { + return 0; + } dbpath = _alpm_db_path(db); if(dbpath == NULL) { @@ -878,7 +881,7 @@ static int local_db_version(pmdb_t *db) if(dbdir == NULL) { if(errno == ENOENT) { /* database dir doesn't exist yet */ - version = 2; + db->valid = 1; goto done; } else { RET_ERR(db->handle, PM_ERR_DB_OPEN, -1); @@ -899,26 +902,26 @@ static int local_db_version(pmdb_t *db) snprintf(path, PATH_MAX, "%s%s/depends", dbpath, name); if(access(path, F_OK) == 0) { /* we found a depends file- bail */ - version = 1; + db->handle->pm_errno = PM_ERR_DB_VERSION; + db->valid = 0; goto done; } } /* we found no depends file after full scan */ - version = 2; + db->valid = 1; done: if(dbdir) { closedir(dbdir); } - _alpm_log(db->handle, PM_LOG_DEBUG, "local database version %d\n", version); - return version; + return !db->valid; } struct db_operations local_db_ops = { .populate = local_db_populate, .unregister = _alpm_db_unregister, - .version = local_db_version, + .validate = local_db_validate, }; pmdb_t *_alpm_db_register_local(pmhandle_t *handle) @@ -929,11 +932,17 @@ pmdb_t *_alpm_db_register_local(pmhandle_t *handle) db = _alpm_db_new("local", 1); if(db == NULL) { + handle->pm_errno = PM_ERR_DB_CREATE; return NULL; } db->ops = &local_db_ops; db->handle = handle; + if(local_db_validate(db)) { + _alpm_db_free(db); + return NULL; + } + handle->db_local = db; return db; } diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index c6fee84..6f4c36c 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -69,20 +69,28 @@ static char *get_sync_dir(pmhandle_t *handle) static int sync_db_validate(pmdb_t *db) { + pgp_verify_t check_sig; + + if(db->valid) { + return 0; + } + /* this takes into account the default verification level if UNKNOWN * was assigned to this db */ - pgp_verify_t check_sig = _alpm_db_get_sigverify_level(db); + check_sig = _alpm_db_get_sigverify_level(db); if(check_sig != PM_PGP_VERIFY_NEVER) { int ret; const char *dbpath = _alpm_db_path(db); if(!dbpath) { /* pm_errno set in _alpm_db_path() */ + db->valid = 0; return -1; } /* we can skip any validation if the database doesn't exist */ if(access(dbpath, R_OK) != 0 && errno == ENOENT) { + db->valid = 1; return 0; } @@ -91,10 +99,12 @@ static int sync_db_validate(pmdb_t *db) ret = _alpm_gpgme_checksig(db->handle, dbpath, NULL); if((check_sig == PM_PGP_VERIFY_ALWAYS && ret != 0) || (check_sig == PM_PGP_VERIFY_OPTIONAL && ret == 1)) { + db->valid = 0; RET_ERR(db->handle, PM_ERR_SIG_INVALID, -1); } } + db->valid = 1; return 0; } @@ -214,6 +224,7 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db) /* Cache needs to be rebuilt */ _alpm_db_free_pkgcache(db); + db->valid = 0; if(sync_db_validate(db)) { /* pm_errno should be set */ ret = -1; @@ -560,15 +571,10 @@ error: return -1; } -static int sync_db_version(pmdb_t UNUSED *db) -{ - return 2; -} - struct db_operations sync_db_ops = { .populate = sync_db_populate, .unregister = _alpm_db_unregister, - .version = sync_db_version, + .validate = sync_db_validate, }; pmdb_t *_alpm_db_register_sync(pmhandle_t *handle, const char *treename, diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c index 9c974f5..508870f 100644 --- a/lib/libalpm/db.c +++ b/lib/libalpm/db.c @@ -371,14 +371,6 @@ const char *_alpm_db_path(pmdb_t *db) return db->_path; } -int _alpm_db_version(pmdb_t *db) -{ - if(!db) { - return -1; - } - return db->ops->version(db); -} - char *_alpm_db_sig_path(pmdb_t *db) { char *sigpath; @@ -477,11 +469,8 @@ alpm_list_t *_alpm_db_search(pmdb_t *db, const alpm_list_t *needles) /* Returns a new package cache from db. * It frees the cache if it already exists. */ -int _alpm_db_load_pkgcache(pmdb_t *db) +static int load_pkgcache(pmdb_t *db) { - if(db == NULL) { - return -1; - } _alpm_db_free_pkgcache(db); _alpm_log(db->handle, PM_LOG_DEBUG, "loading package cache for repository '%s'\n", @@ -520,7 +509,10 @@ pmpkghash_t *_alpm_db_get_pkgcache_hash(pmdb_t *db) } if(!db->pkgcache_loaded) { - _alpm_db_load_pkgcache(db); + if(db->ops->validate(db)) { + return NULL; + } + load_pkgcache(db); } /* hmmm, still NULL ?*/ diff --git a/lib/libalpm/db.h b/lib/libalpm/db.h index c5fcd5f..5e9c9c1 100644 --- a/lib/libalpm/db.h +++ b/lib/libalpm/db.h @@ -46,7 +46,7 @@ typedef enum _pmdbinfrq_t { struct db_operations { int (*populate) (pmdb_t *); void (*unregister) (pmdb_t *); - int (*version) (pmdb_t *); + int (*validate) (pmdb_t *); }; /* Database */ @@ -55,6 +55,7 @@ struct __pmdb_t { char *treename; /* do not access directly, use _alpm_db_path(db) for lazy access */ char *_path; + int valid; int pkgcache_loaded; int grpcache_loaded; /* also indicates whether we are RO or RW */ @@ -73,7 +74,6 @@ pmdb_t *_alpm_db_new(const char *treename, int is_local); void _alpm_db_free(pmdb_t *db); const char *_alpm_db_path(pmdb_t *db); char *_alpm_db_sig_path(pmdb_t *db); -int _alpm_db_version(pmdb_t *db); int _alpm_db_cmp(const void *d1, const void *d2); alpm_list_t *_alpm_db_search(pmdb_t *db, const alpm_list_t *needles); pmdb_t *_alpm_db_register_local(pmhandle_t *handle); diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index a95280c..7e1c092 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -101,8 +101,6 @@ int SYMEXPORT alpm_trans_init(pmhandle_t *handle, pmtransflag_t flags, alpm_trans_cb_progress progress) { pmtrans_t *trans; - const int required_db_version = 2; - int db_version; /* Sanity checks */ ASSERT(handle != NULL, return -1); @@ -122,16 +120,6 @@ int SYMEXPORT alpm_trans_init(pmhandle_t *handle, pmtransflag_t flags, trans->cb_progress = progress; trans->state = STATE_INITIALIZED; - /* check database version */ - db_version = _alpm_db_version(handle->db_local); - if(db_version < required_db_version) { - _alpm_log(handle, PM_LOG_ERROR, - _("%s database version is too old\n"), handle->db_local->treename); - remove_lock(handle); - _alpm_trans_free(trans); - RET_ERR(handle, PM_ERR_DB_VERSION, -1); - } - handle->trans = trans; return 0; diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 33ac0b8..1c2f91c 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -449,6 +449,9 @@ static int setup_libalpm(void) if(!handle) { pm_printf(PM_LOG_ERROR, _("failed to initialize alpm library (%s)\n"), alpm_strerror(err)); + if(err == PM_ERR_DB_VERSION) { + pm_printf(PM_LOG_ERROR, _(" try running pacman-db-upgrade\n")); + } return -1; } config->handle = handle; diff --git a/src/pacman/util.c b/src/pacman/util.c index 71acd6f..2388e5c 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -68,10 +68,6 @@ int trans_init(pmtransflag_t flags) " running, you can remove %s\n"), alpm_option_get_lockfile(config->handle)); } - else if(err == PM_ERR_DB_VERSION) { - fprintf(stderr, _(" try running pacman-db-upgrade\n")); - } - return -1; } return 0; -- 1.7.5.2
Signed-off-by: Dan McGee <dan@archlinux.org> --- lib/libalpm/alpm.h | 8 ++++++++ lib/libalpm/db.c | 7 +++++++ 2 files changed, 15 insertions(+), 0 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 1e1bfdd..329ffd7 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -307,6 +307,14 @@ const char *alpm_db_get_name(const pmdb_t *db); */ const char *alpm_db_get_url(const pmdb_t *db); +/** Check the validity of a database. + * This is most useful for sync databases and verifying signature status. + * If invalid, the handle error code will be set accordingly. + * @param db pointer to the package database + * @return 0 if valid, -1 if invalid (pm_errno is set accordingly) + */ +int alpm_db_valid(pmdb_t *db); + /** @name Accessors to the list of servers for a database. * @{ */ diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c index 508870f..e740acb 100644 --- a/lib/libalpm/db.c +++ b/lib/libalpm/db.c @@ -245,6 +245,13 @@ const char SYMEXPORT *alpm_db_get_url(const pmdb_t *db) return url; } +/** Check the validity of a database. */ +int SYMEXPORT alpm_db_valid(pmdb_t *db) +{ + ASSERT(db != NULL, return -1); + db->handle->pm_errno = 0; + return db->ops->validate(db); +} /** Get a package entry from a package database. */ pmpkg_t SYMEXPORT *alpm_db_get_pkg(pmdb_t *db, const char *name) -- 1.7.5.2
They are placeholders, but important for things like trying to re-sync a database missing a signature. By using the alpm_db_valid() method at the right time, a client can take the appropriate action with these invalid databases as necessary. In pacman's case, we disallow just about anything that involves looking at a sync database outside of an '-Sy' operation (although we do check the validity immediately after). A few operations are still permitted- '-Q' ops that don't touch sync databases as well as '-R'. Signed-off-by: Dan McGee <dan@archlinux.org> --- lib/libalpm/be_sync.c | 6 +----- src/pacman/database.c | 2 +- src/pacman/query.c | 7 ++----- src/pacman/remove.c | 2 +- src/pacman/sync.c | 21 ++++++++++----------- src/pacman/upgrade.c | 2 +- src/pacman/util.c | 27 ++++++++++++++++++++++++++- src/pacman/util.h | 3 ++- 8 files changed, 44 insertions(+), 26 deletions(-) diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 6f4c36c..69c7e21 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -592,14 +592,10 @@ pmdb_t *_alpm_db_register_sync(pmhandle_t *handle, const char *treename, db->handle = handle; db->pgp_verify = level; - if(sync_db_validate(db)) { - _alpm_db_free(db); - return NULL; - } + sync_db_validate(db); handle->dbs_sync = alpm_list_add(handle->dbs_sync, db); return db; } - /* vim: set ts=2 sw=2 noet: */ diff --git a/src/pacman/database.c b/src/pacman/database.c index 292fa54..5a5c398 100644 --- a/src/pacman/database.c +++ b/src/pacman/database.c @@ -59,7 +59,7 @@ int pacman_database(alpm_list_t *targets) } /* Lock database */ - if(trans_init(0) == -1) { + if(trans_init(0, 0) == -1) { return 1; } diff --git a/src/pacman/query.c b/src/pacman/query.c index 76d9d73..938fff6 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -502,11 +502,8 @@ int pacman_query(alpm_list_t *targets) return ret; } - if(config->op_q_foreign) { - /* ensure we have at least one valid sync db set up */ - alpm_list_t *sync_dbs = alpm_option_get_syncdbs(config->handle); - if(sync_dbs == NULL || alpm_list_count(sync_dbs) == 0) { - pm_printf(PM_LOG_ERROR, _("no usable package repositories configured.\n")); + if(config->op_q_foreign || config->op_q_upgrade) { + if(check_syncdbs(1, 1)) { return 1; } } diff --git a/src/pacman/remove.c b/src/pacman/remove.c index 59698e7..595263a 100644 --- a/src/pacman/remove.c +++ b/src/pacman/remove.c @@ -81,7 +81,7 @@ int pacman_remove(alpm_list_t *targets) } /* Step 0: create a new transaction */ - if(trans_init(config->flags) == -1) { + if(trans_init(config->flags, 0) == -1) { return 1; } diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 6d224e4..36c7799 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -283,7 +283,7 @@ static int sync_synctree(int level, alpm_list_t *syncs) alpm_list_t *i; int success = 0, ret; - if(trans_init(0) == -1) { + if(trans_init(0, 0) == -1) { return 0; } @@ -305,10 +305,6 @@ static int sync_synctree(int level, alpm_list_t *syncs) if(trans_release() == -1) { return 0; } - /* We should always succeed if at least one DB was upgraded - we may possibly - * fail later with unresolved deps, but that should be rare, and would be - * expected - */ if(!success) { pm_fprintf(stderr, PM_LOG_ERROR, _("failed to synchronize any databases\n")); } @@ -746,7 +742,7 @@ static int sync_trans(alpm_list_t *targets) alpm_list_t *i; /* Step 1: create a new transaction... */ - if(trans_init(config->flags) == -1) { + if(trans_init(config->flags, 1) == -1) { return 1; } @@ -900,7 +896,7 @@ int pacman_sync(alpm_list_t *targets) if(config->op_s_clean) { int ret = 0; - if(trans_init(0) == -1) { + if(trans_init(0, 0) == -1) { return 1; } @@ -915,13 +911,12 @@ int pacman_sync(alpm_list_t *targets) return ret; } - /* ensure we have at least one valid sync db set up */ - sync_dbs = alpm_option_get_syncdbs(config->handle); - if(sync_dbs == NULL || alpm_list_count(sync_dbs) == 0) { - pm_printf(PM_LOG_ERROR, _("no usable package repositories configured.\n")); + if(check_syncdbs(1, 0)) { return 1; } + sync_dbs = alpm_option_get_syncdbs(config->handle); + if(config->op_s_sync) { /* grab a fresh package list */ printf(_(":: Synchronizing package databases...\n")); @@ -931,6 +926,10 @@ int pacman_sync(alpm_list_t *targets) } } + if(check_syncdbs(1, 1)) { + return 1; + } + /* search for a package */ if(config->op_s_search) { return sync_search(sync_dbs, targets); diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index cdbbffe..ef273cf 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -67,7 +67,7 @@ int pacman_upgrade(alpm_list_t *targets) } /* Step 1: create a new transaction */ - if(trans_init(config->flags) == -1) { + if(trans_init(config->flags, 1) == -1) { return 1; } diff --git a/src/pacman/util.c b/src/pacman/util.c index 2388e5c..ce305a4 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -49,9 +49,12 @@ #include "callback.h" -int trans_init(pmtransflag_t flags) +int trans_init(pmtransflag_t flags, int check_valid) { int ret; + + check_syncdbs(0, check_valid); + if(config->print) { ret = alpm_trans_init(config->handle, flags, NULL, NULL, NULL); } else { @@ -100,6 +103,28 @@ int needs_root(void) } } +int check_syncdbs(int min_count, int check_valid) +{ + alpm_list_t *i, *sync_dbs; + sync_dbs = alpm_option_get_syncdbs(config->handle); + if(sync_dbs == NULL || alpm_list_count(sync_dbs) < min_count) { + pm_printf(PM_LOG_ERROR, _("no usable package repositories configured.\n")); + return 1; + } + if(check_valid) { + /* ensure all known dbs are valid */ + for(i = sync_dbs; i; i = alpm_list_next(i)) { + pmdb_t *db = i->data; + if(alpm_db_valid(db)) { + pm_printf(PM_LOG_ERROR, _("database '%s' is not valid (%s)\n"), + alpm_db_get_name(db), alpm_strerror(alpm_errno(config->handle))); + return 1; + } + } + } + return 0; +} + /* discard unhandled input on the terminal's input buffer */ static int flush_term_input(void) { #ifdef HAVE_TCFLUSH diff --git a/src/pacman/util.h b/src/pacman/util.h index 95c1ce9..a35f715 100644 --- a/src/pacman/util.h +++ b/src/pacman/util.h @@ -39,9 +39,10 @@ /* update speed for the fill_progress based functions */ #define UPDATE_SPEED_SEC 0.2f -int trans_init(pmtransflag_t flags); +int trans_init(pmtransflag_t flags, int check_valid); int trans_release(void); int needs_root(void); +int check_syncdbs(int min_count, int check_valid); int getcols(int def); int rmrf(const char *path); const char *mbasename(const char *path); -- 1.7.5.2
On 08/06/11 17:51, Dan McGee wrote:
They are placeholders, but important for things like trying to re-sync a database missing a signature. By using the alpm_db_valid() method at the right time, a client can take the appropriate action with these invalid databases as necessary.
In pacman's case, we disallow just about anything that involves looking at a sync database outside of an '-Sy' operation (although we do check the validity immediately after). A few operations are still permitted- '-Q' ops that don't touch sync databases as well as '-R'.
Signed-off-by: Dan McGee<dan@archlinux.org> --- lib/libalpm/be_sync.c | 6 +----- src/pacman/database.c | 2 +- src/pacman/query.c | 7 ++----- src/pacman/remove.c | 2 +-
<snip>
diff --git a/src/pacman/util.c b/src/pacman/util.c index 2388e5c..ce305a4 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c <snip> @@ -100,6 +103,28 @@ int needs_root(void) } }
+int check_syncdbs(int min_count, int check_valid)
unsigned int min_count?
+{ + alpm_list_t *i, *sync_dbs; + sync_dbs = alpm_option_get_syncdbs(config->handle); + if(sync_dbs == NULL || alpm_list_count(sync_dbs)< min_count) {
otherwise we are doing a comparison between signed and unsigned integer expressions here. Allan
On 08/06/11 17:51, Dan McGee wrote:
They are placeholders, but important for things like trying to re-sync a database missing a signature. By using the alpm_db_valid() method at the right time, a client can take the appropriate action with these invalid databases as necessary.
In pacman's case, we disallow just about anything that involves looking at a sync database outside of an '-Sy' operation (although we do check the validity immediately after). A few operations are still permitted- '-Q' ops that don't touch sync databases as well as '-R'.
Signed-off-by: Dan McGee<dan@archlinux.org> --- lib/libalpm/be_sync.c | 6 +----- src/pacman/database.c | 2 +- src/pacman/query.c | 7 ++----- src/pacman/remove.c | 2 +- src/pacman/sync.c | 21 ++++++++++----------- src/pacman/upgrade.c | 2 +- src/pacman/util.c | 27 ++++++++++++++++++++++++++- src/pacman/util.h | 3 ++- 8 files changed, 44 insertions(+), 26 deletions(-)
diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 6f4c36c..69c7e21 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -592,14 +592,10 @@ pmdb_t *_alpm_db_register_sync(pmhandle_t *handle, const char *treename, db->handle = handle; db->pgp_verify = level;
- if(sync_db_validate(db)) { - _alpm_db_free(db); - return NULL; - } + sync_db_validate(db);
handle->dbs_sync = alpm_list_add(handle->dbs_sync, db); return db; }
- /* vim: set ts=2 sw=2 noet: */ diff --git a/src/pacman/database.c b/src/pacman/database.c index 292fa54..5a5c398 100644 --- a/src/pacman/database.c +++ b/src/pacman/database.c @@ -59,7 +59,7 @@ int pacman_database(alpm_list_t *targets) }
/* Lock database */ - if(trans_init(0) == -1) { + if(trans_init(0, 0) == -1) { return 1; }
diff --git a/src/pacman/query.c b/src/pacman/query.c index 76d9d73..938fff6 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -502,11 +502,8 @@ int pacman_query(alpm_list_t *targets) return ret; }
- if(config->op_q_foreign) { - /* ensure we have at least one valid sync db set up */ - alpm_list_t *sync_dbs = alpm_option_get_syncdbs(config->handle); - if(sync_dbs == NULL || alpm_list_count(sync_dbs) == 0) { - pm_printf(PM_LOG_ERROR, _("no usable package repositories configured.\n")); + if(config->op_q_foreign || config->op_q_upgrade) { + if(check_syncdbs(1, 1)) { return 1; } } diff --git a/src/pacman/remove.c b/src/pacman/remove.c index 59698e7..595263a 100644 --- a/src/pacman/remove.c +++ b/src/pacman/remove.c @@ -81,7 +81,7 @@ int pacman_remove(alpm_list_t *targets) }
/* Step 0: create a new transaction */ - if(trans_init(config->flags) == -1) { + if(trans_init(config->flags, 0) == -1) { return 1; }
diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 6d224e4..36c7799 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -283,7 +283,7 @@ static int sync_synctree(int level, alpm_list_t *syncs) alpm_list_t *i; int success = 0, ret;
- if(trans_init(0) == -1) { + if(trans_init(0, 0) == -1) { return 0; }
@@ -305,10 +305,6 @@ static int sync_synctree(int level, alpm_list_t *syncs) if(trans_release() == -1) { return 0; } - /* We should always succeed if at least one DB was upgraded - we may possibly - * fail later with unresolved deps, but that should be rare, and would be - * expected - */ if(!success) { pm_fprintf(stderr, PM_LOG_ERROR, _("failed to synchronize any databases\n")); } @@ -746,7 +742,7 @@ static int sync_trans(alpm_list_t *targets) alpm_list_t *i;
/* Step 1: create a new transaction... */ - if(trans_init(config->flags) == -1) { + if(trans_init(config->flags, 1) == -1) { return 1; }
@@ -900,7 +896,7 @@ int pacman_sync(alpm_list_t *targets) if(config->op_s_clean) { int ret = 0;
- if(trans_init(0) == -1) { + if(trans_init(0, 0) == -1) { return 1; }
@@ -915,13 +911,12 @@ int pacman_sync(alpm_list_t *targets) return ret; }
- /* ensure we have at least one valid sync db set up */ - sync_dbs = alpm_option_get_syncdbs(config->handle); - if(sync_dbs == NULL || alpm_list_count(sync_dbs) == 0) { - pm_printf(PM_LOG_ERROR, _("no usable package repositories configured.\n")); + if(check_syncdbs(1, 0)) { return 1; }
+ sync_dbs = alpm_option_get_syncdbs(config->handle); + if(config->op_s_sync) { /* grab a fresh package list */ printf(_(":: Synchronizing package databases...\n")); @@ -931,6 +926,10 @@ int pacman_sync(alpm_list_t *targets) } }
+ if(check_syncdbs(1, 1)) { + return 1; + } + /* search for a package */ if(config->op_s_search) { return sync_search(sync_dbs, targets); diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index cdbbffe..ef273cf 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -67,7 +67,7 @@ int pacman_upgrade(alpm_list_t *targets) }
/* Step 1: create a new transaction */ - if(trans_init(config->flags) == -1) { + if(trans_init(config->flags, 1) == -1) { return 1; }
diff --git a/src/pacman/util.c b/src/pacman/util.c index 2388e5c..ce305a4 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -49,9 +49,12 @@ #include "callback.h"
-int trans_init(pmtransflag_t flags) +int trans_init(pmtransflag_t flags, int check_valid) { int ret; + + check_syncdbs(0, check_valid); + if(config->print) { ret = alpm_trans_init(config->handle, flags, NULL, NULL, NULL); } else { @@ -100,6 +103,28 @@ int needs_root(void) } }
+int check_syncdbs(int min_count, int check_valid) +{ + alpm_list_t *i, *sync_dbs; + sync_dbs = alpm_option_get_syncdbs(config->handle); + if(sync_dbs == NULL || alpm_list_count(sync_dbs)< min_count) { + pm_printf(PM_LOG_ERROR, _("no usable package repositories configured.\n")); + return 1; + } + if(check_valid) { + /* ensure all known dbs are valid */ + for(i = sync_dbs; i; i = alpm_list_next(i)) { + pmdb_t *db = i->data; + if(alpm_db_valid(db)) { + pm_printf(PM_LOG_ERROR, _("database '%s' is not valid (%s)\n"), + alpm_db_get_name(db), alpm_strerror(alpm_errno(config->handle))); + return 1;
Instead of returning immediately here, would it be good to finish checking all databases for validity?
On Wed, Jun 8, 2011 at 6:38 AM, Allan McRae <allan@archlinux.org> wrote:
On 08/06/11 17:51, Dan McGee wrote:
@@ -100,6 +103,28 @@ int needs_root(void) } }
+int check_syncdbs(int min_count, int check_valid) +{ + alpm_list_t *i, *sync_dbs; + sync_dbs = alpm_option_get_syncdbs(config->handle); + if(sync_dbs == NULL || alpm_list_count(sync_dbs)< min_count) { + pm_printf(PM_LOG_ERROR, _("no usable package repositories configured.\n")); + return 1; + } + if(check_valid) { + /* ensure all known dbs are valid */ + for(i = sync_dbs; i; i = alpm_list_next(i)) { + pmdb_t *db = i->data; + if(alpm_db_valid(db)) { + pm_printf(PM_LOG_ERROR, _("database '%s' is not valid (%s)\n"), + alpm_db_get_name(db), alpm_strerror(alpm_errno(config->handle))); + return 1;
Instead of returning immediately here, would it be good to finish checking all databases for validity?
I thought about that too when doing it and I was on the fence- I think that is a good idea, as then you would know in one shot what all is wrong. Just set error = 1 if we ever are invalid in the loop and then return that value... -Dan
On Wed, Jun 8, 2011 at 7:18 AM, Dan McGee <dpmcgee@gmail.com> wrote:
On Wed, Jun 8, 2011 at 6:38 AM, Allan McRae <allan@archlinux.org> wrote:
On 08/06/11 17:51, Dan McGee wrote:
@@ -100,6 +103,28 @@ int needs_root(void) } }
+int check_syncdbs(int min_count, int check_valid) +{ + alpm_list_t *i, *sync_dbs; + sync_dbs = alpm_option_get_syncdbs(config->handle); + if(sync_dbs == NULL || alpm_list_count(sync_dbs)< min_count) { + pm_printf(PM_LOG_ERROR, _("no usable package repositories configured.\n")); + return 1; + } + if(check_valid) { + /* ensure all known dbs are valid */ + for(i = sync_dbs; i; i = alpm_list_next(i)) { + pmdb_t *db = i->data; + if(alpm_db_valid(db)) { + pm_printf(PM_LOG_ERROR, _("database '%s' is not valid (%s)\n"), + alpm_db_get_name(db), alpm_strerror(alpm_errno(config->handle))); + return 1;
Instead of returning immediately here, would it be good to finish checking all databases for validity?
I thought about that too when doing it and I was on the fence- I think that is a good idea, as then you would know in one shot what all is wrong. Just set error = 1 if we ever are invalid in the loop and then return that value...
Updated to fix this; example output: dmcgee@galway ~/projects/pacman (working) $ ./src/pacman/pacman -Ss masdfasd error: database 'core' is not valid (invalid PGP signature) error: database 'extra' is not valid (invalid PGP signature) error: database 'community-testing' is not valid (invalid PGP signature) error: database 'multilib' is not valid (invalid PGP signature) error: database 'community' is not valid (invalid PGP signature)
This method is old, it doesn't adequately check for a NULL server list, and can easily be done using better API method we provide these days. Signed-off-by: Dan McGee <dan@archlinux.org> --- lib/libalpm/alpm.h | 6 ------ lib/libalpm/db.c | 13 ------------- src/pacman/util.c | 17 +++++++---------- 3 files changed, 7 insertions(+), 29 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 329ffd7..37bea6d 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -301,12 +301,6 @@ int alpm_db_unregister_all(pmhandle_t *handle); */ const char *alpm_db_get_name(const pmdb_t *db); -/** Get a download URL for the package database. - * @param db pointer to the package database - * @return a fully-specified download URL, NULL on error - */ -const char *alpm_db_get_url(const pmdb_t *db); - /** Check the validity of a database. * This is most useful for sync databases and verifying signature status. * If invalid, the handle error code will be set accordingly. diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c index e740acb..4a2cbab 100644 --- a/lib/libalpm/db.c +++ b/lib/libalpm/db.c @@ -232,19 +232,6 @@ const char SYMEXPORT *alpm_db_get_name(const pmdb_t *db) return db->treename; } -/** Get a download URL for the package database. */ -const char SYMEXPORT *alpm_db_get_url(const pmdb_t *db) -{ - char *url; - - ASSERT(db != NULL, return NULL); - ASSERT(db->servers != NULL, return NULL); - - url = (char *)db->servers->data; - - return url; -} - /** Check the validity of a database. */ int SYMEXPORT alpm_db_valid(pmdb_t *db) { diff --git a/src/pacman/util.c b/src/pacman/util.c index ce305a4..21e3ad8 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -804,22 +804,19 @@ static off_t pkg_get_size(pmpkg_t *pkg) static char *pkg_get_location(pmpkg_t *pkg) { - pmdb_t *db; - const char *dburl; - char *string; + alpm_list_t *servers; + char *string = NULL; switch(config->op) { case PM_OP_SYNC: - db = alpm_pkg_get_db(pkg); - dburl = alpm_db_get_url(db); - if(dburl) { - char *pkgurl = NULL; - pm_asprintf(&pkgurl, "%s/%s", dburl, alpm_pkg_get_filename(pkg)); - return pkgurl; + servers = alpm_db_get_servers(alpm_pkg_get_db(pkg)); + if(servers) { + pm_asprintf(&string, "%s/%s", alpm_list_getdata(servers), + alpm_pkg_get_filename(pkg)); + return string; } case PM_OP_UPGRADE: return strdup(alpm_pkg_get_filename(pkg)); default: - string = NULL; pm_asprintf(&string, "%s-%s", alpm_pkg_get_name(pkg), alpm_pkg_get_version(pkg)); return string; } -- 1.7.5.2
participants (3)
-
Allan McRae
-
Dan McGee
-
Dan McGee