[pacman-dev] [PATCH 0/9] refactor ini/config parser
As the first step to back-end hook support, this patchset makes the ini parser suitable for sharing between alpm and pacman by moving all error handling from the ini parser to the _parse_directive callback. This involves:
* moving debug output to the callback * allowing empty section names (alpm already rejects empty db names) * moving Include handling to the callback * notifying the callback of read errors
In order to do that I also refactored the config callback to use only a single pass by storing repo information until alpm is initialized and using bitmasks instead of the ALPM_SIG_*_SET enums to handle siglevel inheritance.
apg
Andrew Gregory (9): conf.c: store repo settings in dedicated struct conf.c: use masks for siglevel inheritance conf.c: parse config in a single pass ini.c: remove final callback call ini.c: move Include parsing to conf.c ini.c: remove unnecessary helper function ini.c: remove empty section name restriction ini.c: remove useless key check ini.c: move error output into conf.c
lib/libalpm/alpm.h | 3 - lib/libalpm/be_sync.c | 2 +- src/pacman/conf.c | 304 ++++++++++++++++++++++++++++---------------------- src/pacman/conf.h | 16 +++ src/pacman/ini.c | 147 ++++-------------------- 5 files changed, 208 insertions(+), 264 deletions(-)
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com --- src/pacman/conf.c | 88 +++++++++++++++++++++++++++++-------------------------- src/pacman/conf.h | 9 ++++++ 2 files changed, 55 insertions(+), 42 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index f75f3a7..bcfa12a 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -151,6 +151,16 @@ int config_free(config_t *oldconfig) return 0; }
+void config_repo_free(config_repo_t *repo) +{ + if(repo == NULL) { + return; + } + free(repo->name); + FREELIST(repo->servers); + free(repo); +} + /** Helper function for download_with_xfercommand() */ static char *get_filename(const char *url) { @@ -750,14 +760,10 @@ static int setup_libalpm(void) * calling library methods. */ struct section_t { - /* useful for all sections */ const char *name; + config_repo_t *repo; int is_options; int parse_options; - /* db section option gathering */ - alpm_list_t *servers; - alpm_siglevel_t siglevel; - alpm_db_usage_t usage; };
static int process_usage(alpm_list_t *values, alpm_db_usage_t *usage, @@ -798,6 +804,7 @@ static int _parse_repo(const char *key, char *value, const char *file, int line, struct section_t *section) { int ret = 0; + config_repo_t *repo = section->repo;
if(strcmp(key, "Server") == 0) { if(!value) { @@ -805,7 +812,7 @@ static int _parse_repo(const char *key, char *value, const char *file, file, line, key); ret = 1; } else { - section->servers = alpm_list_add(section->servers, strdup(value)); + repo->servers = alpm_list_add(repo->servers, strdup(value)); } } else if(strcmp(key, "SigLevel") == 0) { if(!value) { @@ -815,10 +822,10 @@ static int _parse_repo(const char *key, char *value, const char *file, alpm_list_t *values = NULL; setrepeatingoption(value, "SigLevel", &values); if(values) { - if(section->siglevel == ALPM_SIG_USE_DEFAULT) { - section->siglevel = config->siglevel; + if(repo->siglevel == ALPM_SIG_USE_DEFAULT) { + repo->siglevel = config->siglevel; } - ret = process_siglevel(values, §ion->siglevel, file, line); + ret = process_siglevel(values, &repo->siglevel, file, line); FREELIST(values); } } @@ -826,7 +833,7 @@ static int _parse_repo(const char *key, char *value, const char *file, alpm_list_t *values = NULL; setrepeatingoption(value, "Usage", &values); if(values) { - if(process_usage(values, §ion->usage, file, line)) { + if(process_usage(values, &repo->usage, file, line)) { FREELIST(values); return 1; } @@ -835,7 +842,7 @@ static int _parse_repo(const char *key, char *value, const char *file, } else { pm_printf(ALPM_LOG_WARNING, _("config file %s, line %d: directive '%s' in section '%s' not recognized.\n"), - file, line, key, section->name); + file, line, key, repo->name); }
return ret; @@ -852,51 +859,42 @@ static int _parse_repo(const char *key, char *value, const char *file, */ static int finish_section(struct section_t *section) { - int ret = 0; alpm_list_t *i; alpm_db_t *db; + config_repo_t *repo = section->repo;
pm_printf(ALPM_LOG_DEBUG, "config: finish section '%s'\n", section->name);
/* parsing options (or nothing)- nothing to do except free the pieces */ - if(!section->name || section->parse_options || section->is_options) { - goto cleanup; + if(section->parse_options || !section->repo) { + return 0; }
/* if we are not looking at options sections only, register a db */ - db = alpm_register_syncdb(config->handle, section->name, section->siglevel); + db = alpm_register_syncdb(config->handle, repo->name, repo->siglevel); if(db == NULL) { pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"), - section->name, alpm_strerror(alpm_errno(config->handle))); - ret = 1; - goto cleanup; + repo->name, alpm_strerror(alpm_errno(config->handle))); + return 1; }
pm_printf(ALPM_LOG_DEBUG, - "setting usage of %d for %s repoistory\n", - section->usage == 0 ? ALPM_DB_USAGE_ALL : section->usage, - section->name); - alpm_db_set_usage(db, section->usage == 0 ? ALPM_DB_USAGE_ALL : section->usage); + "setting usage of %d for %s repository\n", + repo->usage == 0 ? ALPM_DB_USAGE_ALL : repo->usage, + repo->name); + alpm_db_set_usage(db, repo->usage == 0 ? ALPM_DB_USAGE_ALL : repo->usage);
- for(i = section->servers; i; i = alpm_list_next(i)) { + for(i = repo->servers; i; i = alpm_list_next(i)) { char *value = i->data; if(_add_mirror(db, value) != 0) { pm_printf(ALPM_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; + value, repo->name, alpm_strerror(alpm_errno(config->handle))); + return 1; } - free(value); }
-cleanup: - alpm_list_free(section->servers); - section->servers = NULL; - section->siglevel = ALPM_SIG_USE_DEFAULT; - section->name = NULL; - section->usage = 0; - return ret; + return 0; }
static int _parse_directive(const char *file, int linenum, const char *name, @@ -904,13 +902,21 @@ static int _parse_directive(const char *file, int linenum, const char *name, { struct section_t *section = data; if(!key && !value) { - int ret = finish_section(section); - pm_printf(ALPM_LOG_DEBUG, "config: new section '%s'\n", name); + int ret = finish_section(data); section->name = name; - if(name && strcmp(name, "options") == 0) { + pm_printf(ALPM_LOG_DEBUG, "config: new section '%s'\n", name); + config_repo_free(section->repo); + section->repo = NULL; + section->is_options = 0; + if(!name) { + /* end of file, do nothing */ + } else if(strcmp(name, "options") == 0) { section->is_options = 1; - } else { - section->is_options = 0; + } else if(!section->parse_options) { + section->repo = calloc(sizeof(config_repo_t), 1); + section->repo->name = strdup(name); + section->repo->siglevel = ALPM_SIG_USE_DEFAULT; + section->repo->usage = 0; } return ret; } @@ -924,7 +930,7 @@ static int _parse_directive(const char *file, int linenum, const char *name, if(section->parse_options && section->is_options) { /* we are either in options ... */ return _parse_options(key, value, file, linenum); - } else if(!section->parse_options && !section->is_options) { + } else if(!section->parse_options && section->repo) { /* ... or in a repo section */ return _parse_repo(key, value, file, linenum, section); } @@ -941,8 +947,6 @@ int parseconfig(const char *file) int ret; struct section_t section; memset(§ion, 0, sizeof(struct section_t)); - section.siglevel = ALPM_SIG_USE_DEFAULT; - section.usage = 0; /* 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]. */ diff --git a/src/pacman/conf.h b/src/pacman/conf.h index e8cac50..97f006f 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -34,6 +34,13 @@ typedef struct __colstr_t { const char *nocolor; } colstr_t;
+typedef struct __config_repo_t { + char *name; + alpm_list_t *servers; + alpm_db_usage_t usage; + alpm_siglevel_t siglevel; +} config_repo_t; + typedef struct __config_t { unsigned short op; unsigned short quiet; @@ -205,6 +212,8 @@ void enable_colors(int colors); config_t *config_new(void); int config_free(config_t *oldconfig);
+void config_repo_free(config_repo_t *repo); + int config_set_arch(const char *arch); int parseconfig(const char *file); #endif /* _PM_CONF_H */
Hi Andrew,
On Sat, 2014-04-26 at 07:56PM -0400, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com
src/pacman/conf.c | 88 +++++++++++++++++++++++++++++-------------------------- src/pacman/conf.h | 9 ++++++ 2 files changed, 55 insertions(+), 42 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index f75f3a7..bcfa12a 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -151,6 +151,16 @@ int config_free(config_t *oldconfig) return 0; }
+void config_repo_free(config_repo_t *repo) +{
- if(repo == NULL) {
return;- }
- free(repo->name);
- FREELIST(repo->servers);
- free(repo);
+}
/** Helper function for download_with_xfercommand() */ static char *get_filename(const char *url) { @@ -750,14 +760,10 @@ static int setup_libalpm(void)
- calling library methods.
*/ struct section_t {
- /* useful for all sections */ const char *name;
- config_repo_t *repo;
I haven't looked into the details, but might it make sense to just have the struct here and not a pointer to a struct?
[...]
static int _parse_directive(const char *file, int linenum, const char *name, @@ -904,13 +902,21 @@ static int _parse_directive(const char *file, int linenum, const char *name, { struct section_t *section = data; if(!key && !value) {
int ret = finish_section(section);pm_printf(ALPM_LOG_DEBUG, "config: new section '%s'\n", name);
int ret = finish_section(data);
if(name && strcmp(name, "options") == 0) {
pm_printf(ALPM_LOG_DEBUG, "config: new section '%s'\n", name);config_repo_free(section->repo);section->repo = NULL;section->is_options = 0;if(!name) {/* end of file, do nothing */} else if(strcmp(name, "options") == 0) { section->is_options = 1;
} else {section->is_options = 0;
} else if(!section->parse_options) {section->repo = calloc(sizeof(config_repo_t), 1);
The return value of calloc needs to be checked to be non NULL.
Sören
On 27/04/14 09:56, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com
src/pacman/conf.c | 88 +++++++++++++++++++++++++++++-------------------------- src/pacman/conf.h | 9 ++++++ 2 files changed, 55 insertions(+), 42 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index f75f3a7..bcfa12a 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -151,6 +151,16 @@ int config_free(config_t *oldconfig) return 0; }
+void config_repo_free(config_repo_t *repo) +{
- if(repo == NULL) {
return;- }
- free(repo->name);
- FREELIST(repo->servers);
- free(repo);
+}
OK
/** Helper function for download_with_xfercommand() */ static char *get_filename(const char *url) { @@ -750,14 +760,10 @@ static int setup_libalpm(void)
- calling library methods.
*/ struct section_t {
- /* useful for all sections */ const char *name;
- config_repo_t *repo; int is_options; int parse_options;
- /* db section option gathering */
- alpm_list_t *servers;
- alpm_siglevel_t siglevel;
- alpm_db_usage_t usage;
};
OK
static int process_usage(alpm_list_t *values, alpm_db_usage_t *usage, @@ -798,6 +804,7 @@ static int _parse_repo(const char *key, char *value, const char *file, int line, struct section_t *section) { int ret = 0;
- config_repo_t *repo = section->repo;
OK
if(strcmp(key, "Server") == 0) { if(!value) { @@ -805,7 +812,7 @@ static int _parse_repo(const char *key, char *value, const char *file, file, line, key); ret = 1; } else {
section->servers = alpm_list_add(section->servers, strdup(value));
repo->servers = alpm_list_add(repo->servers, strdup(value));
OK
}} else if(strcmp(key, "SigLevel") == 0) { if(!value) { @@ -815,10 +822,10 @@ static int _parse_repo(const char *key, char *value, const char *file, alpm_list_t *values = NULL; setrepeatingoption(value, "SigLevel", &values); if(values) {
if(section->siglevel == ALPM_SIG_USE_DEFAULT) {section->siglevel = config->siglevel;
if(repo->siglevel == ALPM_SIG_USE_DEFAULT) {repo->siglevel = config->siglevel; }
ret = process_siglevel(values, §ion->siglevel, file, line);
ret = process_siglevel(values, &repo->siglevel, file, line); FREELIST(values); }
OK
@@ -826,7 +833,7 @@ static int _parse_repo(const char *key, char *value, const char *file, alpm_list_t *values = NULL; setrepeatingoption(value, "Usage", &values); if(values) {
if(process_usage(values, §ion->usage, file, line)) {
if(process_usage(values, &repo->usage, file, line)) { FREELIST(values); return 1; }
OK
@@ -835,7 +842,7 @@ static int _parse_repo(const char *key, char *value, const char *file, } else { pm_printf(ALPM_LOG_WARNING, _("config file %s, line %d: directive '%s' in section '%s' not recognized.\n"),
file, line, key, section->name);
file, line, key, repo->name);
OK
return ret; @@ -852,51 +859,42 @@ static int _parse_repo(const char *key, char *value, const char *file, */ static int finish_section(struct section_t *section) {
- int ret = 0; alpm_list_t *i; alpm_db_t *db;
config_repo_t *repo = section->repo;
pm_printf(ALPM_LOG_DEBUG, "config: finish section '%s'\n", section->name);
/* parsing options (or nothing)- nothing to do except free the pieces */
- if(!section->name || section->parse_options || section->is_options) {
goto cleanup;
if(section->parse_options || !section->repo) {
return 0;}
/* if we are not looking at options sections only, register a db */
- db = alpm_register_syncdb(config->handle, section->name, section->siglevel);
- db = alpm_register_syncdb(config->handle, repo->name, repo->siglevel); if(db == NULL) { pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"),
section->name, alpm_strerror(alpm_errno(config->handle)));ret = 1;goto cleanup;
repo->name, alpm_strerror(alpm_errno(config->handle)));return 1;}
pm_printf(ALPM_LOG_DEBUG,
"setting usage of %d for %s repoistory\n",section->usage == 0 ? ALPM_DB_USAGE_ALL : section->usage,section->name);- alpm_db_set_usage(db, section->usage == 0 ? ALPM_DB_USAGE_ALL : section->usage);
"setting usage of %d for %s repository\n",repo->usage == 0 ? ALPM_DB_USAGE_ALL : repo->usage,repo->name);- alpm_db_set_usage(db, repo->usage == 0 ? ALPM_DB_USAGE_ALL : repo->usage);
- for(i = section->servers; i; i = alpm_list_next(i)) {
- for(i = repo->servers; i; i = alpm_list_next(i)) { char *value = i->data; if(_add_mirror(db, value) != 0) { pm_printf(ALPM_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;
value, repo->name, alpm_strerror(alpm_errno(config->handle)));return 1;
free(value);-cleanup:
- alpm_list_free(section->servers);
- section->servers = NULL;
- section->siglevel = ALPM_SIG_USE_DEFAULT;
- section->name = NULL;
- section->usage = 0;
- return ret;
- return 0;
}
static int _parse_directive(const char *file, int linenum, const char *name, @@ -904,13 +902,21 @@ static int _parse_directive(const char *file, int linenum, const char *name, { struct section_t *section = data; if(!key && !value) {
int ret = finish_section(section);pm_printf(ALPM_LOG_DEBUG, "config: new section '%s'\n", name);
int ret = finish_section(data);
Why change "section" to "data" here?
section->name = name;
if(name && strcmp(name, "options") == 0) {
pm_printf(ALPM_LOG_DEBUG, "config: new section '%s'\n", name);config_repo_free(section->repo);section->repo = NULL;section->is_options = 0;if(!name) {/* end of file, do nothing */} else if(strcmp(name, "options") == 0) { section->is_options = 1;
} else {section->is_options = 0;
} else if(!section->parse_options) {section->repo = calloc(sizeof(config_repo_t), 1);section->repo->name = strdup(name);
So section->name and section->repo->name carry two copies of the section name?
section->repo->siglevel = ALPM_SIG_USE_DEFAULT;section->repo->usage = 0;@@ -924,7 +930,7 @@ static int _parse_directive(const char *file, int linenum, const char *name, if(section->parse_options && section->is_options) { /* we are either in options ... */ return _parse_options(key, value, file, linenum);
- } else if(!section->parse_options && !section->is_options) {
- } else if(!section->parse_options && section->repo) {
OK
/* ... or in a repo section */ return _parse_repo(key, value, file, linenum, section);} @@ -941,8 +947,6 @@ int parseconfig(const char *file) int ret; struct section_t section; memset(§ion, 0, sizeof(struct section_t));
- section.siglevel = ALPM_SIG_USE_DEFAULT;
- section.usage = 0;
OK
/* 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]. */ diff --git a/src/pacman/conf.h b/src/pacman/conf.h index e8cac50..97f006f 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -34,6 +34,13 @@ typedef struct __colstr_t { const char *nocolor; } colstr_t;
+typedef struct __config_repo_t {
- char *name;
- alpm_list_t *servers;
- alpm_db_usage_t usage;
- alpm_siglevel_t siglevel;
+} config_repo_t;
OK
typedef struct __config_t { unsigned short op; unsigned short quiet; @@ -205,6 +212,8 @@ void enable_colors(int colors); config_t *config_new(void); int config_free(config_t *oldconfig);
+void config_repo_free(config_repo_t *repo);
int config_set_arch(const char *arch); int parseconfig(const char *file); #endif /* _PM_CONF_H */
This will allow pacman to parse its config file in a single pass and removes the need for the *_SET siglevels in alpm that were only required for pacman's siglevel inheritance.
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com --- lib/libalpm/alpm.h | 3 -- lib/libalpm/be_sync.c | 2 +- src/pacman/conf.c | 93 ++++++++++++++++++++------------------------------- src/pacman/conf.h | 5 +++ 4 files changed, 43 insertions(+), 60 deletions(-)
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index b0adb95..448b8a0 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -115,9 +115,6 @@ typedef enum _alpm_siglevel_t { ALPM_SIG_DATABASE_MARGINAL_OK = (1 << 12), ALPM_SIG_DATABASE_UNKNOWN_OK = (1 << 13),
- ALPM_SIG_PACKAGE_SET = (1 << 27), - ALPM_SIG_PACKAGE_TRUST_SET = (1 << 28), - ALPM_SIG_USE_DEFAULT = (1 << 31) } alpm_siglevel_t;
diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index df0b1b7..0a131d2 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -698,7 +698,7 @@ alpm_db_t *_alpm_db_register_sync(alpm_handle_t *handle, const char *treename, _alpm_log(handle, ALPM_LOG_DEBUG, "registering sync database '%s'\n", treename);
#ifndef HAVE_LIBGPGME - if((level &= ~ALPM_SIG_PACKAGE_SET) != 0 && level != ALPM_SIG_USE_DEFAULT) { + if(level != ALPM_SIG_USE_DEFAULT) { RET_ERR(handle, ALPM_ERR_WRONG_ARGS, NULL); } #endif diff --git a/src/pacman/conf.c b/src/pacman/conf.c index bcfa12a..ab80373 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -319,12 +319,15 @@ int config_set_arch(const char *arch) * @return 0 on success, 1 on any parsing error */ static int process_siglevel(alpm_list_t *values, alpm_siglevel_t *storage, - const char *file, int linenum) + alpm_siglevel_t *storage_mask, const char *file, int linenum) { - alpm_siglevel_t level = *storage; + alpm_siglevel_t level = *storage, mask = *storage_mask; alpm_list_t *i; int ret = 0;
+#define SLSET(sl) do { level |= (sl); mask |= (sl); } while(0) +#define SLUNSET(sl) do { level &= ~(sl); mask |= (sl); } while(0) + /* Collapse the option names into a single bitmasked value */ for(i = values; i; i = alpm_list_next(i)) { const char *original = i->data, *value; @@ -347,51 +350,40 @@ static int process_siglevel(alpm_list_t *values, alpm_siglevel_t *storage, /* now parse out and store actual flag if it is valid */ if(strcmp(value, "Never") == 0) { if(package) { - level &= ~ALPM_SIG_PACKAGE; - level |= ALPM_SIG_PACKAGE_SET; + SLUNSET(ALPM_SIG_PACKAGE); } if(database) { - level &= ~ALPM_SIG_DATABASE; + SLUNSET(ALPM_SIG_DATABASE); } } else if(strcmp(value, "Optional") == 0) { if(package) { - level |= ALPM_SIG_PACKAGE; - level |= ALPM_SIG_PACKAGE_OPTIONAL; - level |= ALPM_SIG_PACKAGE_SET; + SLSET(ALPM_SIG_PACKAGE | ALPM_SIG_PACKAGE_OPTIONAL); } if(database) { - level |= ALPM_SIG_DATABASE; - level |= ALPM_SIG_DATABASE_OPTIONAL; + SLSET(ALPM_SIG_DATABASE | ALPM_SIG_DATABASE_OPTIONAL); } } else if(strcmp(value, "Required") == 0) { if(package) { - level |= ALPM_SIG_PACKAGE; - level &= ~ALPM_SIG_PACKAGE_OPTIONAL; - level |= ALPM_SIG_PACKAGE_SET; + SLSET(ALPM_SIG_PACKAGE); + SLUNSET(ALPM_SIG_PACKAGE_OPTIONAL); } if(database) { - level |= ALPM_SIG_DATABASE; - level &= ~ALPM_SIG_DATABASE_OPTIONAL; + SLSET(ALPM_SIG_DATABASE); + SLUNSET(ALPM_SIG_DATABASE_OPTIONAL); } } else if(strcmp(value, "TrustedOnly") == 0) { if(package) { - level &= ~ALPM_SIG_PACKAGE_MARGINAL_OK; - level &= ~ALPM_SIG_PACKAGE_UNKNOWN_OK; - level |= ALPM_SIG_PACKAGE_TRUST_SET; + SLUNSET(ALPM_SIG_PACKAGE_MARGINAL_OK | ALPM_SIG_PACKAGE_UNKNOWN_OK); } if(database) { - level &= ~ALPM_SIG_DATABASE_MARGINAL_OK; - level &= ~ALPM_SIG_DATABASE_UNKNOWN_OK; + SLUNSET(ALPM_SIG_DATABASE_MARGINAL_OK | ALPM_SIG_DATABASE_UNKNOWN_OK); } } else if(strcmp(value, "TrustAll") == 0) { if(package) { - level |= ALPM_SIG_PACKAGE_MARGINAL_OK; - level |= ALPM_SIG_PACKAGE_UNKNOWN_OK; - level |= ALPM_SIG_PACKAGE_TRUST_SET; + SLSET(ALPM_SIG_PACKAGE_MARGINAL_OK | ALPM_SIG_PACKAGE_UNKNOWN_OK); } if(database) { - level |= ALPM_SIG_DATABASE_MARGINAL_OK; - level |= ALPM_SIG_DATABASE_UNKNOWN_OK; + SLSET(ALPM_SIG_DATABASE_MARGINAL_OK | ALPM_SIG_DATABASE_UNKNOWN_OK); } } else { pm_printf(ALPM_LOG_ERROR, @@ -402,6 +394,9 @@ static int process_siglevel(alpm_list_t *values, alpm_siglevel_t *storage, level &= ~ALPM_SIG_USE_DEFAULT; }
+#undef SLSET +#undef SLUNSET + /* ensure we have sig checking ability and are actually turning it on */ if(!(alpm_capabilities() & ALPM_CAPABILITY_SIGNATURES) && level & (ALPM_SIG_PACKAGE | ALPM_SIG_DATABASE)) { @@ -413,32 +408,11 @@ static int process_siglevel(alpm_list_t *values, alpm_siglevel_t *storage,
if(!ret) { *storage = level; + *storage_mask = mask; } return ret; }
-/** - * Merge the package entires of two signature verification levels. - * @param base initial siglevel - * @param over overridden siglevel, derived value is stored here - */ -static void merge_siglevel(alpm_siglevel_t *base, alpm_siglevel_t *over) -{ - alpm_siglevel_t level = *over; - if(!(level & ALPM_SIG_USE_DEFAULT)) { - if(!(level & ALPM_SIG_PACKAGE_SET)) { - level |= *base & ALPM_SIG_PACKAGE; - level |= *base & ALPM_SIG_PACKAGE_OPTIONAL; - } - if(!(level & ALPM_SIG_PACKAGE_TRUST_SET)) { - level |= *base & ALPM_SIG_PACKAGE_MARGINAL_OK; - level |= *base & ALPM_SIG_PACKAGE_UNKNOWN_OK; - } - } - - *over = level; -} - static int process_cleanmethods(alpm_list_t *values, const char *file, int linenum) { @@ -585,7 +559,8 @@ static int _parse_options(const char *key, char *value, } else if(strcmp(key, "SigLevel") == 0) { alpm_list_t *values = NULL; setrepeatingoption(value, "SigLevel", &values); - if(process_siglevel(values, &config->siglevel, file, linenum)) { + if(process_siglevel(values, &config->siglevel, + &config->siglevel_mask, file, linenum)) { FREELIST(values); return 1; } @@ -593,7 +568,8 @@ static int _parse_options(const char *key, char *value, } else if(strcmp(key, "LocalFileSigLevel") == 0) { alpm_list_t *values = NULL; setrepeatingoption(value, "LocalFileSigLevel", &values); - if(process_siglevel(values, &config->localfilesiglevel, file, linenum)) { + if(process_siglevel(values, &config->localfilesiglevel, + &config->localfilesiglevel_mask, file, linenum)) { FREELIST(values); return 1; } @@ -601,7 +577,8 @@ static int _parse_options(const char *key, char *value, } else if(strcmp(key, "RemoteFileSigLevel") == 0) { alpm_list_t *values = NULL; setrepeatingoption(value, "RemoteFileSigLevel", &values); - if(process_siglevel(values, &config->remotefilesiglevel, file, linenum)) { + if(process_siglevel(values, &config->remotefilesiglevel, + &config->remotefilesiglevel_mask, file, linenum)) { FREELIST(values); return 1; } @@ -727,8 +704,11 @@ static int setup_libalpm(void)
alpm_option_set_default_siglevel(handle, config->siglevel);
- merge_siglevel(&config->siglevel, &config->localfilesiglevel); - merge_siglevel(&config->siglevel, &config->remotefilesiglevel); +#define SLMERGE(l, m) if(m) { l = (l & (m)) | (config->siglevel & ~(m)); } + SLMERGE(config->localfilesiglevel, config->localfilesiglevel_mask); + SLMERGE(config->remotefilesiglevel, config->remotefilesiglevel_mask); +#undef SLMERGE + alpm_option_set_local_file_siglevel(handle, config->localfilesiglevel); alpm_option_set_remote_file_siglevel(handle, config->remotefilesiglevel);
@@ -822,10 +802,8 @@ static int _parse_repo(const char *key, char *value, const char *file, alpm_list_t *values = NULL; setrepeatingoption(value, "SigLevel", &values); if(values) { - if(repo->siglevel == ALPM_SIG_USE_DEFAULT) { - repo->siglevel = config->siglevel; - } - ret = process_siglevel(values, &repo->siglevel, file, line); + ret = process_siglevel(values, &repo->siglevel, + &repo->siglevel_mask, file, line); FREELIST(values); } } @@ -871,6 +849,9 @@ static int finish_section(struct section_t *section) }
/* if we are not looking at options sections only, register a db */ +#define SLMERGE(l, m) if(m) { l = (l & (m)) | (config->siglevel & ~(m)); } + SLMERGE(repo->siglevel, repo->siglevel_mask); +#undef SLMERGE db = alpm_register_syncdb(config->handle, repo->name, repo->siglevel); if(db == NULL) { pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"), diff --git a/src/pacman/conf.h b/src/pacman/conf.h index 97f006f..42484fb 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -39,6 +39,7 @@ typedef struct __config_repo_t { alpm_list_t *servers; alpm_db_usage_t usage; alpm_siglevel_t siglevel; + alpm_siglevel_t siglevel_mask; } config_repo_t;
typedef struct __config_t { @@ -95,6 +96,10 @@ typedef struct __config_t { alpm_siglevel_t localfilesiglevel; alpm_siglevel_t remotefilesiglevel;
+ alpm_siglevel_t siglevel_mask; + alpm_siglevel_t localfilesiglevel_mask; + alpm_siglevel_t remotefilesiglevel_mask; + /* conf file options */ /* I Love Candy! */ unsigned short chomp;
On 27/04/14 09:56, Andrew Gregory wrote:
This will allow pacman to parse its config file in a single pass and removes the need for the *_SET siglevels in alpm that were only required for pacman's siglevel inheritance.
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com
lib/libalpm/alpm.h | 3 -- lib/libalpm/be_sync.c | 2 +- src/pacman/conf.c | 93 ++++++++++++++++++++------------------------------- src/pacman/conf.h | 5 +++ 4 files changed, 43 insertions(+), 60 deletions(-)
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index b0adb95..448b8a0 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -115,9 +115,6 @@ typedef enum _alpm_siglevel_t { ALPM_SIG_DATABASE_MARGINAL_OK = (1 << 12), ALPM_SIG_DATABASE_UNKNOWN_OK = (1 << 13),
- ALPM_SIG_PACKAGE_SET = (1 << 27),
- ALPM_SIG_PACKAGE_TRUST_SET = (1 << 28),
- ALPM_SIG_USE_DEFAULT = (1 << 31)
} alpm_siglevel_t;
OK
diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index df0b1b7..0a131d2 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -698,7 +698,7 @@ alpm_db_t *_alpm_db_register_sync(alpm_handle_t *handle, const char *treename, _alpm_log(handle, ALPM_LOG_DEBUG, "registering sync database '%s'\n", treename);
#ifndef HAVE_LIBGPGME
- if((level &= ~ALPM_SIG_PACKAGE_SET) != 0 && level != ALPM_SIG_USE_DEFAULT) {
- if(level != ALPM_SIG_USE_DEFAULT) { RET_ERR(handle, ALPM_ERR_WRONG_ARGS, NULL); }
#endif
OK
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index bcfa12a..ab80373 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -319,12 +319,15 @@ int config_set_arch(const char *arch)
- @return 0 on success, 1 on any parsing error
*/ static int process_siglevel(alpm_list_t *values, alpm_siglevel_t *storage,
const char *file, int linenum)
alpm_siglevel_t *storage_mask, const char *file, int linenum){
- alpm_siglevel_t level = *storage;
- alpm_siglevel_t level = *storage, mask = *storage_mask; alpm_list_t *i; int ret = 0;
+#define SLSET(sl) do { level |= (sl); mask |= (sl); } while(0) +#define SLUNSET(sl) do { level &= ~(sl); mask |= (sl); } while(0)
- /* Collapse the option names into a single bitmasked value */ for(i = values; i; i = alpm_list_next(i)) { const char *original = i->data, *value;
OK
@@ -347,51 +350,40 @@ static int process_siglevel(alpm_list_t *values, alpm_siglevel_t *storage, /* now parse out and store actual flag if it is valid */ if(strcmp(value, "Never") == 0) { if(package) {
level &= ~ALPM_SIG_PACKAGE;level |= ALPM_SIG_PACKAGE_SET;
SLUNSET(ALPM_SIG_PACKAGE); } if(database) {
level &= ~ALPM_SIG_DATABASE;
SLUNSET(ALPM_SIG_DATABASE); }
level |= ALPM_SIG_PACKAGE;level |= ALPM_SIG_PACKAGE_OPTIONAL;level |= ALPM_SIG_PACKAGE_SET;
SLSET(ALPM_SIG_PACKAGE | ALPM_SIG_PACKAGE_OPTIONAL); } if(database) {
level |= ALPM_SIG_DATABASE;level |= ALPM_SIG_DATABASE_OPTIONAL;
SLSET(ALPM_SIG_DATABASE | ALPM_SIG_DATABASE_OPTIONAL); }
level |= ALPM_SIG_PACKAGE;level &= ~ALPM_SIG_PACKAGE_OPTIONAL;level |= ALPM_SIG_PACKAGE_SET;
SLSET(ALPM_SIG_PACKAGE);SLUNSET(ALPM_SIG_PACKAGE_OPTIONAL); } if(database) {
level |= ALPM_SIG_DATABASE;level &= ~ALPM_SIG_DATABASE_OPTIONAL;
SLSET(ALPM_SIG_DATABASE);SLUNSET(ALPM_SIG_DATABASE_OPTIONAL); }
level &= ~ALPM_SIG_PACKAGE_MARGINAL_OK;level &= ~ALPM_SIG_PACKAGE_UNKNOWN_OK;level |= ALPM_SIG_PACKAGE_TRUST_SET;
SLUNSET(ALPM_SIG_PACKAGE_MARGINAL_OK | ALPM_SIG_PACKAGE_UNKNOWN_OK); } if(database) {
level &= ~ALPM_SIG_DATABASE_MARGINAL_OK;level &= ~ALPM_SIG_DATABASE_UNKNOWN_OK;
SLUNSET(ALPM_SIG_DATABASE_MARGINAL_OK | ALPM_SIG_DATABASE_UNKNOWN_OK); }
level |= ALPM_SIG_PACKAGE_MARGINAL_OK;level |= ALPM_SIG_PACKAGE_UNKNOWN_OK;level |= ALPM_SIG_PACKAGE_TRUST_SET;
SLSET(ALPM_SIG_PACKAGE_MARGINAL_OK | ALPM_SIG_PACKAGE_UNKNOWN_OK); } if(database) {
level |= ALPM_SIG_DATABASE_MARGINAL_OK;level |= ALPM_SIG_DATABASE_UNKNOWN_OK;
SLSET(ALPM_SIG_DATABASE_MARGINAL_OK | ALPM_SIG_DATABASE_UNKNOWN_OK); }
OK
@@ -402,6 +394,9 @@ static int process_siglevel(alpm_list_t *values, alpm_siglevel_t *storage, level &= ~ALPM_SIG_USE_DEFAULT; }
+#undef SLSET +#undef SLUNSET
- /* ensure we have sig checking ability and are actually turning it on */ if(!(alpm_capabilities() & ALPM_CAPABILITY_SIGNATURES) && level & (ALPM_SIG_PACKAGE | ALPM_SIG_DATABASE)) {
@@ -413,32 +408,11 @@ static int process_siglevel(alpm_list_t *values, alpm_siglevel_t *storage,
if(!ret) { *storage = level;
*storage_mask = mask;}
-/**
- Merge the package entires of two signature verification levels.
- @param base initial siglevel
- @param over overridden siglevel, derived value is stored here
- */
-static void merge_siglevel(alpm_siglevel_t *base, alpm_siglevel_t *over) -{
- alpm_siglevel_t level = *over;
- if(!(level & ALPM_SIG_USE_DEFAULT)) {
if(!(level & ALPM_SIG_PACKAGE_SET)) {level |= *base & ALPM_SIG_PACKAGE;level |= *base & ALPM_SIG_PACKAGE_OPTIONAL;}if(!(level & ALPM_SIG_PACKAGE_TRUST_SET)) {level |= *base & ALPM_SIG_PACKAGE_MARGINAL_OK;level |= *base & ALPM_SIG_PACKAGE_UNKNOWN_OK;}- }
- *over = level;
-}
static int process_cleanmethods(alpm_list_t *values, const char *file, int linenum) { @@ -585,7 +559,8 @@ static int _parse_options(const char *key, char *value, } else if(strcmp(key, "SigLevel") == 0) { alpm_list_t *values = NULL; setrepeatingoption(value, "SigLevel", &values);
if(process_siglevel(values, &config->siglevel, file, linenum)) {
if(process_siglevel(values, &config->siglevel,&config->siglevel_mask, file, linenum)) {
OK
FREELIST(values); return 1; }@@ -593,7 +568,8 @@ static int _parse_options(const char *key, char *value, } else if(strcmp(key, "LocalFileSigLevel") == 0) { alpm_list_t *values = NULL; setrepeatingoption(value, "LocalFileSigLevel", &values);
if(process_siglevel(values, &config->localfilesiglevel, file, linenum)) {
if(process_siglevel(values, &config->localfilesiglevel,&config->localfilesiglevel_mask, file, linenum)) { FREELIST(values); return 1; }@@ -601,7 +577,8 @@ static int _parse_options(const char *key, char *value, } else if(strcmp(key, "RemoteFileSigLevel") == 0) { alpm_list_t *values = NULL; setrepeatingoption(value, "RemoteFileSigLevel", &values);
if(process_siglevel(values, &config->remotefilesiglevel, file, linenum)) {
if(process_siglevel(values, &config->remotefilesiglevel,&config->remotefilesiglevel_mask, file, linenum)) { FREELIST(values); return 1; }
OK
@@ -727,8 +704,11 @@ static int setup_libalpm(void)
alpm_option_set_default_siglevel(handle, config->siglevel);
- merge_siglevel(&config->siglevel, &config->localfilesiglevel);
- merge_siglevel(&config->siglevel, &config->remotefilesiglevel);
+#define SLMERGE(l, m) if(m) { l = (l & (m)) | (config->siglevel & ~(m)); }
- SLMERGE(config->localfilesiglevel, config->localfilesiglevel_mask);
- SLMERGE(config->remotefilesiglevel, config->remotefilesiglevel_mask);
+#undef SLMERGE
- alpm_option_set_local_file_siglevel(handle, config->localfilesiglevel); alpm_option_set_remote_file_siglevel(handle, config->remotefilesiglevel);
@@ -822,10 +802,8 @@ static int _parse_repo(const char *key, char *value, const char *file, alpm_list_t *values = NULL; setrepeatingoption(value, "SigLevel", &values); if(values) {
if(repo->siglevel == ALPM_SIG_USE_DEFAULT) {repo->siglevel = config->siglevel;}ret = process_siglevel(values, &repo->siglevel, file, line);
ret = process_siglevel(values, &repo->siglevel,&repo->siglevel_mask, file, line); FREELIST(values);
OK
} }@@ -871,6 +849,9 @@ static int finish_section(struct section_t *section) }
/* if we are not looking at options sections only, register a db */ +#define SLMERGE(l, m) if(m) { l = (l & (m)) | (config->siglevel & ~(m)); }
- SLMERGE(repo->siglevel, repo->siglevel_mask);
+#undef SLMERGE
Should this be a function rather than using the same define twice?
db = alpm_register_syncdb(config->handle, repo->name, repo->siglevel); if(db == NULL) { pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"), diff --git a/src/pacman/conf.h b/src/pacman/conf.h index 97f006f..42484fb 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -39,6 +39,7 @@ typedef struct __config_repo_t { alpm_list_t *servers; alpm_db_usage_t usage; alpm_siglevel_t siglevel;
- alpm_siglevel_t siglevel_mask;
} config_repo_t;
typedef struct __config_t { @@ -95,6 +96,10 @@ typedef struct __config_t { alpm_siglevel_t localfilesiglevel; alpm_siglevel_t remotefilesiglevel;
- alpm_siglevel_t siglevel_mask;
- alpm_siglevel_t localfilesiglevel_mask;
- alpm_siglevel_t remotefilesiglevel_mask;
- /* conf file options */ /* I Love Candy! */ unsigned short chomp;
OK
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com --- src/pacman/conf.c | 130 ++++++++++++++++++++++-------------------------------- src/pacman/conf.h | 2 + 2 files changed, 54 insertions(+), 78 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index ab80373..8b2078b 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -132,6 +132,9 @@ int config_free(config_t *oldconfig) alpm_list_free(oldconfig->explicit_adds); alpm_list_free(oldconfig->explicit_removes);
+ alpm_list_free_inner(config->repos, (alpm_list_fn_free) config_repo_free); + alpm_list_free(config->repos); + FREELIST(oldconfig->holdpkg); FREELIST(oldconfig->ignorepkg); FREELIST(oldconfig->ignoregrp); @@ -627,6 +630,37 @@ static int _add_mirror(alpm_db_t *db, char *value) return 0; }
+static int register_repo(config_repo_t *repo) +{ + alpm_list_t *i; + alpm_db_t *db; + + db = alpm_register_syncdb(config->handle, repo->name, repo->siglevel); + if(db == NULL) { + pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"), + repo->name, alpm_strerror(alpm_errno(config->handle))); + return 1; + } + + pm_printf(ALPM_LOG_DEBUG, + "setting usage of %d for %s repository\n", + repo->usage == 0 ? ALPM_DB_USAGE_ALL : repo->usage, + repo->name); + alpm_db_set_usage(db, repo->usage == 0 ? ALPM_DB_USAGE_ALL : repo->usage); + + for(i = repo->servers; i; i = alpm_list_next(i)) { + char *value = i->data; + if(_add_mirror(db, value) != 0) { + pm_printf(ALPM_LOG_ERROR, + _("could not add mirror '%s' to database '%s' (%s)\n"), + value, repo->name, alpm_strerror(alpm_errno(config->handle))); + return 1; + } + } + + return 0; +} + /** Sets up libalpm global stuff in one go. Called after the command line * and initial config file parsing. Once this is complete, we can see if any * paths were defined. If a rootdir was defined and nothing else, we want all @@ -638,6 +672,7 @@ static int setup_libalpm(void) int ret = 0; alpm_errno_t err; alpm_handle_t *handle; + alpm_list_t *i;
pm_printf(ALPM_LOG_DEBUG, "setup_libalpm called\n");
@@ -707,11 +742,16 @@ static int setup_libalpm(void) #define SLMERGE(l, m) if(m) { l = (l & (m)) | (config->siglevel & ~(m)); } SLMERGE(config->localfilesiglevel, config->localfilesiglevel_mask); SLMERGE(config->remotefilesiglevel, config->remotefilesiglevel_mask); -#undef SLMERGE - alpm_option_set_local_file_siglevel(handle, config->localfilesiglevel); alpm_option_set_remote_file_siglevel(handle, config->remotefilesiglevel);
+ for(i = config->repos; i; i = alpm_list_next(i)) { + config_repo_t *repo = i->data; + SLMERGE(repo->siglevel, repo->siglevel_mask); + register_repo(repo); + } +#undef SLMERGE + if(config->xfercommand) { alpm_option_set_fetchcb(handle, download_with_xfercommand); } else if(!(alpm_capabilities() & ALPM_CAPABILITY_DOWNLOADER)) { @@ -742,8 +782,6 @@ static int setup_libalpm(void) struct section_t { const char *name; config_repo_t *repo; - int is_options; - int parse_options; };
static int process_usage(alpm_list_t *values, alpm_db_usage_t *usage, @@ -826,80 +864,25 @@ static int _parse_repo(const char *key, char *value, const char *file, return ret; }
-/** - * 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) -{ - alpm_list_t *i; - alpm_db_t *db; - config_repo_t *repo = section->repo; - - pm_printf(ALPM_LOG_DEBUG, "config: finish section '%s'\n", section->name); - - /* parsing options (or nothing)- nothing to do except free the pieces */ - if(section->parse_options || !section->repo) { - return 0; - } - - /* if we are not looking at options sections only, register a db */ -#define SLMERGE(l, m) if(m) { l = (l & (m)) | (config->siglevel & ~(m)); } - SLMERGE(repo->siglevel, repo->siglevel_mask); -#undef SLMERGE - db = alpm_register_syncdb(config->handle, repo->name, repo->siglevel); - if(db == NULL) { - pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"), - repo->name, alpm_strerror(alpm_errno(config->handle))); - return 1; - } - - pm_printf(ALPM_LOG_DEBUG, - "setting usage of %d for %s repository\n", - repo->usage == 0 ? ALPM_DB_USAGE_ALL : repo->usage, - repo->name); - alpm_db_set_usage(db, repo->usage == 0 ? ALPM_DB_USAGE_ALL : repo->usage); - - for(i = repo->servers; i; i = alpm_list_next(i)) { - char *value = i->data; - if(_add_mirror(db, value) != 0) { - pm_printf(ALPM_LOG_ERROR, - _("could not add mirror '%s' to database '%s' (%s)\n"), - value, repo->name, alpm_strerror(alpm_errno(config->handle))); - return 1; - } - } - - return 0; -} - static int _parse_directive(const char *file, int linenum, const char *name, char *key, char *value, void *data) { struct section_t *section = data; if(!key && !value) { - int ret = finish_section(data); section->name = name; pm_printf(ALPM_LOG_DEBUG, "config: new section '%s'\n", name); - config_repo_free(section->repo); - section->repo = NULL; - section->is_options = 0; if(!name) { /* end of file, do nothing */ } else if(strcmp(name, "options") == 0) { - section->is_options = 1; - } else if(!section->parse_options) { + section->repo = NULL; + } else { section->repo = calloc(sizeof(config_repo_t), 1); section->repo->name = strdup(name); section->repo->siglevel = ALPM_SIG_USE_DEFAULT; section->repo->usage = 0; + config->repos = alpm_list_add(config->repos, section->repo); } - return ret; + return 0; }
if(section->name == NULL) { @@ -908,15 +891,13 @@ static int _parse_directive(const char *file, int linenum, const char *name, return 1; }
- if(section->parse_options && section->is_options) { + if(!section->repo) { /* we are either in options ... */ return _parse_options(key, value, file, linenum); - } else if(!section->parse_options && section->repo) { + } else { /* ... or in a repo section */ return _parse_repo(key, value, file, linenum, section); } - - return 0; }
/** Parse a configuration file. @@ -928,23 +909,16 @@ int parseconfig(const char *file) int ret; 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(ALPM_LOG_DEBUG, "parseconfig: options pass\n"); - section.parse_options = 1; if((ret = parse_ini(file, _parse_directive, §ion))) { return ret; } if((ret = setup_libalpm())) { return ret; } - /* second pass, repo section parsing */ - pm_printf(ALPM_LOG_DEBUG, "parseconfig: repo pass\n"); - section.parse_options = 0; - return parse_ini(file, _parse_directive, §ion); + alpm_list_free_inner(config->repos, (alpm_list_fn_free) config_repo_free); + alpm_list_free(config->repos); + config->repos = NULL; + return ret; }
/* vim: set noet: */ diff --git a/src/pacman/conf.h b/src/pacman/conf.h index 42484fb..95a940e 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -125,6 +125,8 @@ typedef struct __config_t {
/* Color strings for output */ colstr_t colstr; + + alpm_list_t *repos; } config_t;
/* Operations */
On 27/04/14 09:56, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com
src/pacman/conf.c | 130 ++++++++++++++++++++++-------------------------------- src/pacman/conf.h | 2 + 2 files changed, 54 insertions(+), 78 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index ab80373..8b2078b 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -132,6 +132,9 @@ int config_free(config_t *oldconfig) alpm_list_free(oldconfig->explicit_adds); alpm_list_free(oldconfig->explicit_removes);
- alpm_list_free_inner(config->repos, (alpm_list_fn_free) config_repo_free);
- alpm_list_free(config->repos);
- FREELIST(oldconfig->holdpkg); FREELIST(oldconfig->ignorepkg); FREELIST(oldconfig->ignoregrp);
OK
@@ -627,6 +630,37 @@ static int _add_mirror(alpm_db_t *db, char *value) return 0; }
+static int register_repo(config_repo_t *repo) +{
- alpm_list_t *i;
- alpm_db_t *db;
- db = alpm_register_syncdb(config->handle, repo->name, repo->siglevel);
- if(db == NULL) {
pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"),repo->name, alpm_strerror(alpm_errno(config->handle)));return 1;- }
- pm_printf(ALPM_LOG_DEBUG,
"setting usage of %d for %s repository\n",repo->usage == 0 ? ALPM_DB_USAGE_ALL : repo->usage,repo->name);- alpm_db_set_usage(db, repo->usage == 0 ? ALPM_DB_USAGE_ALL : repo->usage);
- for(i = repo->servers; i; i = alpm_list_next(i)) {
char *value = i->data;if(_add_mirror(db, value) != 0) {pm_printf(ALPM_LOG_ERROR,_("could not add mirror '%s' to database '%s' (%s)\n"),value, repo->name, alpm_strerror(alpm_errno(config->handle)));return 1;}- }
- return 0;
+}
OK
/** Sets up libalpm global stuff in one go. Called after the command line
- and initial config file parsing. Once this is complete, we can see if any
- paths were defined. If a rootdir was defined and nothing else, we want all
@@ -638,6 +672,7 @@ static int setup_libalpm(void) int ret = 0; alpm_errno_t err; alpm_handle_t *handle;
alpm_list_t *i;
pm_printf(ALPM_LOG_DEBUG, "setup_libalpm called\n");
@@ -707,11 +742,16 @@ static int setup_libalpm(void) #define SLMERGE(l, m) if(m) { l = (l & (m)) | (config->siglevel & ~(m)); } SLMERGE(config->localfilesiglevel, config->localfilesiglevel_mask); SLMERGE(config->remotefilesiglevel, config->remotefilesiglevel_mask); -#undef SLMERGE
- alpm_option_set_local_file_siglevel(handle, config->localfilesiglevel); alpm_option_set_remote_file_siglevel(handle, config->remotefilesiglevel);
- for(i = config->repos; i; i = alpm_list_next(i)) {
config_repo_t *repo = i->data;SLMERGE(repo->siglevel, repo->siglevel_mask);
I'd expect this to be done in register_repo. That fits with the idea of making SLMERGE a function.
register_repo(repo);- }
+#undef SLMERGE
OK
if(config->xfercommand) { alpm_option_set_fetchcb(handle, download_with_xfercommand); } else if(!(alpm_capabilities() & ALPM_CAPABILITY_DOWNLOADER)) { @@ -742,8 +782,6 @@ static int setup_libalpm(void) struct section_t { const char *name; config_repo_t *repo;
- int is_options;
- int parse_options;
};
OK
static int process_usage(alpm_list_t *values, alpm_db_usage_t *usage, @@ -826,80 +864,25 @@ static int _parse_repo(const char *key, char *value, const char *file, return ret; }
-/**
- 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) -{
- alpm_list_t *i;
- alpm_db_t *db;
- config_repo_t *repo = section->repo;
- pm_printf(ALPM_LOG_DEBUG, "config: finish section '%s'\n", section->name);
- /* parsing options (or nothing)- nothing to do except free the pieces */
- if(section->parse_options || !section->repo) {
return 0;- }
- /* if we are not looking at options sections only, register a db */
-#define SLMERGE(l, m) if(m) { l = (l & (m)) | (config->siglevel & ~(m)); }
- SLMERGE(repo->siglevel, repo->siglevel_mask);
-#undef SLMERGE
- db = alpm_register_syncdb(config->handle, repo->name, repo->siglevel);
- if(db == NULL) {
pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"),repo->name, alpm_strerror(alpm_errno(config->handle)));return 1;- }
- pm_printf(ALPM_LOG_DEBUG,
"setting usage of %d for %s repository\n",repo->usage == 0 ? ALPM_DB_USAGE_ALL : repo->usage,repo->name);- alpm_db_set_usage(db, repo->usage == 0 ? ALPM_DB_USAGE_ALL : repo->usage);
- for(i = repo->servers; i; i = alpm_list_next(i)) {
char *value = i->data;if(_add_mirror(db, value) != 0) {pm_printf(ALPM_LOG_ERROR,_("could not add mirror '%s' to database '%s' (%s)\n"),value, repo->name, alpm_strerror(alpm_errno(config->handle)));return 1;}- }
- return 0;
-}
!!
static int _parse_directive(const char *file, int linenum, const char *name, char *key, char *value, void *data) { struct section_t *section = data; if(!key && !value) {
int ret = finish_section(data);config_repo_free(section->repo);section->repo = NULL;section->is_options = 0;section->is_options = 1;} else if(!section->parse_options) {
section->repo = NULL;} else { section->repo = calloc(sizeof(config_repo_t), 1); section->repo->name = strdup(name); section->repo->siglevel = ALPM_SIG_USE_DEFAULT; section->repo->usage = 0;config->repos = alpm_list_add(config->repos, section->repo);
return ret;
return 0;
OK
if(section->name == NULL) { @@ -908,15 +891,13 @@ static int _parse_directive(const char *file, int linenum, const char *name, return 1; }
- if(section->parse_options && section->is_options) {
- if(!section->repo) { /* we are either in options ... */ return _parse_options(key, value, file, linenum);
- } else if(!section->parse_options && section->repo) {
- } else { /* ... or in a repo section */ return _parse_repo(key, value, file, linenum, section); }
- return 0;
}
/** Parse a configuration file. @@ -928,23 +909,16 @@ int parseconfig(const char *file) int ret; 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(ALPM_LOG_DEBUG, "parseconfig: options pass\n");
- section.parse_options = 1; if((ret = parse_ini(file, _parse_directive, §ion))) { return ret; } if((ret = setup_libalpm())) { return ret; }
- /* second pass, repo section parsing */
- pm_printf(ALPM_LOG_DEBUG, "parseconfig: repo pass\n");
- section.parse_options = 0;
- return parse_ini(file, _parse_directive, §ion);
- alpm_list_free_inner(config->repos, (alpm_list_fn_free) config_repo_free);
- alpm_list_free(config->repos);
- config->repos = NULL;
- return ret;
}
OK
/* vim: set noet: */ diff --git a/src/pacman/conf.h b/src/pacman/conf.h index 42484fb..95a940e 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -125,6 +125,8 @@ typedef struct __config_t {
/* Color strings for output */ colstr_t colstr;
- alpm_list_t *repos;
OK
} config_t;
/* Operations */
Storing repo information removes the need for the final callback. This allows the call signature to be re-purposed for indicating read errors.
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com --- src/pacman/conf.c | 4 +--- src/pacman/ini.c | 4 ---- 2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 8b2078b..6eb197a 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -871,9 +871,7 @@ static int _parse_directive(const char *file, int linenum, const char *name, if(!key && !value) { section->name = name; pm_printf(ALPM_LOG_DEBUG, "config: new section '%s'\n", name); - if(!name) { - /* end of file, do nothing */ - } else if(strcmp(name, "options") == 0) { + if(strcmp(name, "options") == 0) { section->repo = NULL; } else { section->repo = calloc(sizeof(config_repo_t), 1); diff --git a/src/pacman/ini.c b/src/pacman/ini.c index 2a3ef0e..fdc7642 100644 --- a/src/pacman/ini.c +++ b/src/pacman/ini.c @@ -172,10 +172,6 @@ static int _parse_ini(const char *file, ini_parser_fn cb, void *data, } }
- if(depth == 0) { - ret = cb(NULL, 0, NULL, NULL, NULL, data); - } - cleanup: if(fp) { fclose(fp);
On 27/04/14 09:56, Andrew Gregory wrote:
Storing repo information removes the need for the final callback. This allows the call signature to be re-purposed for indicating read errors.
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com
Ack.
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com --- src/pacman/conf.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/pacman/ini.c | 67 +++-------------------------------------------------- 2 files changed, 72 insertions(+), 64 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 6eb197a..97cbf53 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -22,6 +22,7 @@ #include <limits.h> #include <locale.h> /* setlocale */ #include <fcntl.h> /* open */ +#include <glob.h> #include <stdlib.h> #include <stdio.h> #include <string.h> /* strdup */ @@ -782,6 +783,7 @@ static int setup_libalpm(void) struct section_t { const char *name; config_repo_t *repo; + int depth; };
static int process_usage(alpm_list_t *values, alpm_db_usage_t *usage, @@ -865,6 +867,69 @@ static int _parse_repo(const char *key, char *value, const char *file, }
static int _parse_directive(const char *file, int linenum, const char *name, + char *key, char *value, void *data); + +static int process_include(const char *value, void *data, + const char *file, int linenum) +{ + glob_t globbuf; + int globret, ret = 0; + size_t gindex; + struct section_t *section = data; + static const int config_max_recursion = 10; + + if(value == NULL) { + pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: directive '%s' needs a value\n"), + file, linenum, "Include"); + return 1; + } + + if(section->depth >= config_max_recursion) { + pm_printf(ALPM_LOG_ERROR, + _("config parsing exceeded max recursion depth of %d.\n"), + config_max_recursion); + return 1; + } + + section->depth++; + + /* Ignore include failures... assume non-critical */ + globret = glob(value, GLOB_NOCHECK, NULL, &globbuf); + switch(globret) { + case GLOB_NOSPACE: + pm_printf(ALPM_LOG_DEBUG, + "config file %s, line %d: include globbing out of space\n", + file, linenum); + break; + case GLOB_ABORTED: + pm_printf(ALPM_LOG_DEBUG, + "config file %s, line %d: include globbing read error for %s\n", + file, linenum, value); + break; + case GLOB_NOMATCH: + pm_printf(ALPM_LOG_DEBUG, + "config file %s, line %d: no include found for %s\n", + file, linenum, value); + break; + default: + for(gindex = 0; gindex < globbuf.gl_pathc; gindex++) { + pm_printf(ALPM_LOG_DEBUG, "config file %s, line %d: including %s\n", + file, linenum, globbuf.gl_pathv[gindex]); + ret = parse_ini(globbuf.gl_pathv[gindex], _parse_directive, data); + if(ret) { + goto cleanup; + } + } + break; + } + +cleanup: + section->depth--; + globfree(&globbuf); + return ret; +} + +static int _parse_directive(const char *file, int linenum, const char *name, char *key, char *value, void *data) { struct section_t *section = data; @@ -883,6 +948,10 @@ static int _parse_directive(const char *file, int linenum, const char *name, return 0; }
+ if(strcmp(key, "Include") == 0) { + return process_include(value, data, file, linenum); + } + if(section->name == NULL) { pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: All directives must belong to a section.\n"), file, linenum); diff --git a/src/pacman/ini.c b/src/pacman/ini.c index fdc7642..dc1fb7a 100644 --- a/src/pacman/ini.c +++ b/src/pacman/ini.c @@ -18,7 +18,6 @@ */
#include <errno.h> -#include <glob.h> #include <limits.h> #include <string.h> /* strdup */
@@ -27,8 +26,6 @@ #include "ini.h" #include "util.h"
-static const int ini_max_recursion = 10; - /** * @brief INI parser backend. * @@ -37,26 +34,17 @@ static const int ini_max_recursion = 10; * @param data caller defined data to be passed to the callback * @param section_name the name of the current section * @param line buffer to read into, must be at least PATH_MAX long - * @param depth recursion depth, should initially be 0 * * @return 0 on success, 1 on parsing errors, the callback return value * otherwise */ static int _parse_ini(const char *file, ini_parser_fn cb, void *data, - char **section_name, char *line, int depth) + char **section_name, char *line) { FILE *fp = NULL; int linenum = 0; int ret = 0;
- if(depth >= ini_max_recursion) { - pm_printf(ALPM_LOG_ERROR, - _("config parsing exceeded max recursion depth of %d.\n"), - ini_max_recursion); - ret = 1; - goto cleanup; - } - pm_printf(ALPM_LOG_DEBUG, "config: attempting to read file %s\n", file); fp = fopen(file, "r"); if(fp == NULL) { @@ -121,52 +109,6 @@ static int _parse_ini(const char *file, ini_parser_fn cb, void *data, ret = 1; goto cleanup; } - /* Include is allowed in both options and repo sections */ - if(strcmp(key, "Include") == 0) { - glob_t globbuf; - int globret; - size_t gindex; - - if(value == NULL) { - pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: directive '%s' needs a value\n"), - file, linenum, key); - ret = 1; - goto cleanup; - } - /* Ignore include failures... assume non-critical */ - globret = glob(value, GLOB_NOCHECK, NULL, &globbuf); - switch(globret) { - case GLOB_NOSPACE: - pm_printf(ALPM_LOG_DEBUG, - "config file %s, line %d: include globbing out of space\n", - file, linenum); - break; - case GLOB_ABORTED: - pm_printf(ALPM_LOG_DEBUG, - "config file %s, line %d: include globbing read error for %s\n", - file, linenum, value); - break; - case GLOB_NOMATCH: - pm_printf(ALPM_LOG_DEBUG, - "config file %s, line %d: no include found for %s\n", - file, linenum, value); - break; - default: - for(gindex = 0; gindex < globbuf.gl_pathc; gindex++) { - pm_printf(ALPM_LOG_DEBUG, "config file %s, line %d: including %s\n", - file, linenum, globbuf.gl_pathv[gindex]); - ret =_parse_ini(globbuf.gl_pathv[gindex], cb, data, - section_name, line, depth + 1); - if(ret) { - globfree(&globbuf); - goto cleanup; - } - } - break; - } - globfree(&globbuf); - continue; - } if((ret = cb(file, linenum, *section_name, key, value, data)) != 0) { goto cleanup; } @@ -176,10 +118,7 @@ cleanup: if(fp) { fclose(fp); } - if(depth == 0) { - free(*section_name); - *section_name = NULL; - } + free(*section_name); pm_printf(ALPM_LOG_DEBUG, "config: finished parsing %s\n", file); return ret; } @@ -207,7 +146,7 @@ cleanup: int parse_ini(const char *file, ini_parser_fn cb, void *data) { char *section_name = NULL, line[PATH_MAX]; - return _parse_ini(file, cb, data, §ion_name, line, 0); + return _parse_ini(file, cb, data, §ion_name, line); }
/* vim: set noet: */
On 27/04/14 09:56, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com
This is fine - a note in the commit message about the motivation to do this would be useful.
Allan
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com --- src/pacman/ini.c | 52 +++++++++++++++++----------------------------------- 1 file changed, 17 insertions(+), 35 deletions(-)
diff --git a/src/pacman/ini.c b/src/pacman/ini.c index dc1fb7a..5c5232d 100644 --- a/src/pacman/ini.c +++ b/src/pacman/ini.c @@ -27,20 +27,28 @@ #include "util.h"
/** - * @brief INI parser backend. + * @brief Parse a pacman-style INI config file. * * @param file path to the config file * @param cb callback for key/value pairs * @param data caller defined data to be passed to the callback - * @param section_name the name of the current section - * @param line buffer to read into, must be at least PATH_MAX long * * @return 0 on success, 1 on parsing errors, the callback return value * otherwise + * + * @note The callback will be called at the beginning of each section with an + * empty key and value, for each key/value pair, and when parsing is complete + * with all arguments except @a data empty. + * + * @note The @a key and @a value passed to @ cb will be overwritten between + * calls. The section name will remain valid until after @a cb is called to + * begin a new section. + * + * @note Parsing will immediately stop if the callback returns non-zero. */ -static int _parse_ini(const char *file, ini_parser_fn cb, void *data, - char **section_name, char *line) +int parse_ini(const char *file, ini_parser_fn cb, void *data) { + char line[PATH_MAX], *section_name = NULL; FILE *fp = NULL; int linenum = 0; int ret = 0; @@ -85,8 +93,8 @@ static int _parse_ini(const char *file, ini_parser_fn cb, void *data, name[line_len - 2] = '\0';
ret = cb(file, linenum, name, NULL, NULL, data); - free(*section_name); - *section_name = name; + free(section_name); + section_name = name;
/* we're at a new section; perform any post-actions for the prior */ if(ret) { @@ -109,7 +117,7 @@ static int _parse_ini(const char *file, ini_parser_fn cb, void *data, ret = 1; goto cleanup; } - if((ret = cb(file, linenum, *section_name, key, value, data)) != 0) { + if((ret = cb(file, linenum, section_name, key, value, data)) != 0) { goto cleanup; } } @@ -118,35 +126,9 @@ cleanup: if(fp) { fclose(fp); } - free(*section_name); + free(section_name); pm_printf(ALPM_LOG_DEBUG, "config: finished parsing %s\n", file); return ret; }
-/** - * @brief Parse a pacman-style INI config file. - * - * @param file path to the config file - * @param cb callback for key/value pairs - * @param data caller defined data to be passed to the callback - * - * @return 0 on success, 1 on parsing errors, the callback return value - * otherwise - * - * @note The callback will be called at the beginning of each section with an - * empty key and value, for each key/value pair, and when parsing is complete - * with all arguments except @a data empty. - * - * @note The @a key and @a value passed to @ cb will be overwritten between - * calls. The section name will remain valid until after @a cb is called to - * begin a new section. - * - * @note Parsing will immediately stop if the callback returns non-zero. - */ -int parse_ini(const char *file, ini_parser_fn cb, void *data) -{ - char *section_name = NULL, line[PATH_MAX]; - return _parse_ini(file, cb, data, §ion_name, line); -} - /* vim: set noet: */
On 27/04/14 09:56, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com
src/pacman/ini.c | 52 +++++++++++++++++----------------------------------- 1 file changed, 17 insertions(+), 35 deletions(-)
diff --git a/src/pacman/ini.c b/src/pacman/ini.c index dc1fb7a..5c5232d 100644 --- a/src/pacman/ini.c +++ b/src/pacman/ini.c @@ -27,20 +27,28 @@ #include "util.h"
/**
- @brief INI parser backend.
- @brief Parse a pacman-style INI config file.
- @param file path to the config file
- @param cb callback for key/value pairs
- @param data caller defined data to be passed to the callback
- @param section_name the name of the current section
- @param line buffer to read into, must be at least PATH_MAX long
- @return 0 on success, 1 on parsing errors, the callback return value
- otherwise
- @note The callback will be called at the beginning of each section with an
- empty key and value, for each key/value pair, and when parsing is complete
- with all arguments except @a data empty.
You removed the final callback in patch 4/9. The documentation should have been updated there.
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com --- src/pacman/ini.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/src/pacman/ini.c b/src/pacman/ini.c index 5c5232d..a8c4c04 100644 --- a/src/pacman/ini.c +++ b/src/pacman/ini.c @@ -81,13 +81,6 @@ int parse_ini(const char *file, ini_parser_fn cb, void *data)
if(line[0] == '[' && line[line_len - 1] == ']') { char *name; - /* only possibility here is a line == '[]' */ - if(line_len <= 2) { - pm_printf(ALPM_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';
On 27/04/14 09:57, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com
Why? This seems a reasonable restriction.
src/pacman/ini.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/src/pacman/ini.c b/src/pacman/ini.c index 5c5232d..a8c4c04 100644 --- a/src/pacman/ini.c +++ b/src/pacman/ini.c @@ -81,13 +81,6 @@ int parse_ini(const char *file, ini_parser_fn cb, void *data)
if(line[0] == '[' && line[line_len - 1] == ']') { char *name;
/* only possibility here is a line == '[]' */if(line_len <= 2) {pm_printf(ALPM_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';
On 12/29/14 at 02:07pm, Allan McRae wrote:
On 27/04/14 09:57, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com
Why? This seems a reasonable restriction.
To move the error handling out of the ini parser. alpm will already reject an empty db name. I'll add a commit message to that effect.
src/pacman/ini.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/src/pacman/ini.c b/src/pacman/ini.c index 5c5232d..a8c4c04 100644 --- a/src/pacman/ini.c +++ b/src/pacman/ini.c @@ -81,13 +81,6 @@ int parse_ini(const char *file, ini_parser_fn cb, void *data)
if(line[0] == '[' && line[line_len - 1] == ']') { char *name;
/* only possibility here is a line == '[]' */if(line_len <= 2) {pm_printf(ALPM_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';
key points to a statically allocated string, it can't be NULL.
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com --- src/pacman/ini.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/src/pacman/ini.c b/src/pacman/ini.c index a8c4c04..dec3eb0 100644 --- a/src/pacman/ini.c +++ b/src/pacman/ini.c @@ -104,12 +104,6 @@ int parse_ini(const char *file, ini_parser_fn cb, void *data) strtrim(key); strtrim(value);
- if(key == NULL) { - pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: syntax error in config file- missing key.\n"), - file, linenum); - ret = 1; - goto cleanup; - } if((ret = cb(file, linenum, section_name, key, value, data)) != 0) { goto cleanup; }
On 27/04/14 09:57, Andrew Gregory wrote:
key points to a statically allocated string, it can't be NULL.
It looks like this is supposed to lines like: [ ]
in the config file.
Should the test be key == '\0'?
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com
src/pacman/ini.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/src/pacman/ini.c b/src/pacman/ini.c index a8c4c04..dec3eb0 100644 --- a/src/pacman/ini.c +++ b/src/pacman/ini.c @@ -104,12 +104,6 @@ int parse_ini(const char *file, ini_parser_fn cb, void *data) strtrim(key); strtrim(value);
if(key == NULL) {pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: syntax error in config file- missing key.\n"),file, linenum);ret = 1;goto cleanup;}
On 12/29/14 at 02:13pm, Allan McRae wrote:
On 27/04/14 09:57, Andrew Gregory wrote:
key points to a statically allocated string, it can't be NULL.
It looks like this is supposed to lines like: [ ]
in the config file.
Should the test be key == '\0'?
No check is necessary, the callback will reject empty keys. I'll add a note to the commit message.
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com
src/pacman/ini.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/src/pacman/ini.c b/src/pacman/ini.c index a8c4c04..dec3eb0 100644 --- a/src/pacman/ini.c +++ b/src/pacman/ini.c @@ -104,12 +104,6 @@ int parse_ini(const char *file, ini_parser_fn cb, void *data) strtrim(key); strtrim(value);
if(key == NULL) {pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: syntax error in config file- missing key.\n"),file, linenum);ret = 1;goto cleanup;}
Move the remaining output into conf.c by notifying the callback of read errors.
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com --- src/pacman/conf.c | 8 +++++++- src/pacman/ini.c | 21 ++++++++------------- 2 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 97cbf53..84bdac8 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -933,7 +933,11 @@ static int _parse_directive(const char *file, int linenum, const char *name, char *key, char *value, void *data) { struct section_t *section = data; - if(!key && !value) { + if(!name && !key && !value) { + pm_printf(ALPM_LOG_ERROR, _("config file %s could not be read: %s\n"), + file, strerror(errno)); + return 1; + } else if(!key && !value) { section->name = name; pm_printf(ALPM_LOG_DEBUG, "config: new section '%s'\n", name); if(strcmp(name, "options") == 0) { @@ -976,9 +980,11 @@ int parseconfig(const char *file) int ret; struct section_t section; memset(§ion, 0, sizeof(struct section_t)); + pm_printf(ALPM_LOG_DEBUG, "config: attempting to read file %s\n", file); if((ret = parse_ini(file, _parse_directive, §ion))) { return ret; } + pm_printf(ALPM_LOG_DEBUG, "config: finished parsing %s\n", file); if((ret = setup_libalpm())) { return ret; } diff --git a/src/pacman/ini.c b/src/pacman/ini.c index dec3eb0..495e6c0 100644 --- a/src/pacman/ini.c +++ b/src/pacman/ini.c @@ -33,12 +33,14 @@ * @param cb callback for key/value pairs * @param data caller defined data to be passed to the callback * - * @return 0 on success, 1 on parsing errors, the callback return value - * otherwise + * @return the callback return value * * @note The callback will be called at the beginning of each section with an - * empty key and value, for each key/value pair, and when parsing is complete - * with all arguments except @a data empty. + * empty key and value and for each key/value pair. + * + * @note If the parser encounters an error the callback will be called with + * section, key, and value set to NULL and errno set by fopen, fgets, or + * strdup. * * @note The @a key and @a value passed to @ cb will be overwritten between * calls. The section name will remain valid until after @a cb is called to @@ -53,13 +55,9 @@ int parse_ini(const char *file, ini_parser_fn cb, void *data) int linenum = 0; int ret = 0;
- pm_printf(ALPM_LOG_DEBUG, "config: attempting to read file %s\n", file); fp = fopen(file, "r"); if(fp == NULL) { - pm_printf(ALPM_LOG_ERROR, _("config file %s could not be read: %s\n"), - file, strerror(errno)); - ret = 1; - goto cleanup; + return cb(file, 0, NULL, NULL, NULL, data); }
while(fgets(line, PATH_MAX, fp)) { @@ -110,11 +108,8 @@ int parse_ini(const char *file, ini_parser_fn cb, void *data) }
cleanup: - if(fp) { - fclose(fp); - } + fclose(fp); free(section_name); - pm_printf(ALPM_LOG_DEBUG, "config: finished parsing %s\n", file); return ret; }
On 27/04/14 09:57, Andrew Gregory wrote:
Move the remaining output into conf.c by notifying the callback of read errors.
OK.
Just a quick-n-dirty test to demonstrate the reworked parser. Obviously not suitable to be committed.
Only modified conf.c to allow parsing the config without also initializing alpm.
Can be run with `make check` in test/unit/pacman/. --- src/pacman/conf.c | 81 +++++++++++---------- test/unit/pacman/config.c | 174 ++++++++++++++++++++++++++++++++++++++++++++++ test/unit/pacman/tap.h | 123 ++++++++++++++++++++++++++++++++ 3 files changed, 342 insertions(+), 36 deletions(-) create mode 100644 test/unit/pacman/config.c create mode 100644 test/unit/pacman/tap.h
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 5449b92..09daeb4 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -679,6 +679,7 @@ static int register_repo(config_repo_t *repo) */ static int setup_libalpm(void) { + setup_libalpm(); int ret = 0; alpm_errno_t err; alpm_handle_t *handle; @@ -686,25 +687,6 @@ static int setup_libalpm(void)
pm_printf(ALPM_LOG_DEBUG, "setup_libalpm called\n");
- /* Configure root path first. If it is set and dbpath/logfile were not - * set, then set those as well to reside under the root. */ - if(config->rootdir) { - char path[PATH_MAX]; - if(!config->dbpath) { - snprintf(path, PATH_MAX, "%s/%s", config->rootdir, DBPATH + 1); - config->dbpath = strdup(path); - } - if(!config->logfile) { - snprintf(path, PATH_MAX, "%s/%s", config->rootdir, LOGFILE + 1); - config->logfile = strdup(path); - } - } else { - config->rootdir = strdup(ROOTDIR); - if(!config->dbpath) { - config->dbpath = strdup(DBPATH); - } - } - /* initialize library */ handle = alpm_initialize(config->rootdir, config->dbpath, &err); if(!handle) { @@ -722,7 +704,6 @@ static int setup_libalpm(void) alpm_option_set_questioncb(handle, cb_question); alpm_option_set_progresscb(handle, cb_progress);
- config->logfile = config->logfile ? config->logfile : strdup(LOGFILE); ret = alpm_option_set_logfile(handle, config->logfile); if(ret != 0) { pm_printf(ALPM_LOG_ERROR, _("problem setting logfile '%s' (%s)\n"), @@ -730,9 +711,6 @@ static int setup_libalpm(void) return ret; }
- /* Set GnuPG's home directory. This is not relative to rootdir, even if - * rootdir is defined. Reasoning: gpgdir contains configuration data. */ - config->gpgdir = config->gpgdir ? config->gpgdir : strdup(GPGDIR); ret = alpm_option_set_gpgdir(handle, config->gpgdir); if(ret != 0) { pm_printf(ALPM_LOG_ERROR, _("problem setting gpgdir '%s' (%s)\n"), @@ -749,18 +727,12 @@ static int setup_libalpm(void)
alpm_option_set_default_siglevel(handle, config->siglevel);
-#define SLMERGE(l, m) if(m) { l = (l & (m)) | (config->siglevel & ~(m)); } - SLMERGE(config->localfilesiglevel, config->localfilesiglevel_mask); - SLMERGE(config->remotefilesiglevel, config->remotefilesiglevel_mask); alpm_option_set_local_file_siglevel(handle, config->localfilesiglevel); alpm_option_set_remote_file_siglevel(handle, config->remotefilesiglevel);
for(i = config->repos; i; i = alpm_list_next(i)) { - config_repo_t *repo = i->data; - SLMERGE(repo->siglevel, repo->siglevel_mask); - register_repo(repo); + register_repo(i->data); } -#undef SLMERGE
if(config->xfercommand) { alpm_option_set_fetchcb(handle, download_with_xfercommand); @@ -980,6 +952,42 @@ static int _parse_directive(const char *file, int linenum, const char *name, } }
+static void finalize_config(void) +{ + /* Configure root path first. If it is set and dbpath/logfile were not + * set, then set those as well to reside under the root. */ + if(config->rootdir) { + char path[PATH_MAX]; + if(!config->dbpath) { + snprintf(path, PATH_MAX, "%s/%s", config->rootdir, DBPATH + 1); + config->dbpath = strdup(path); + } + if(!config->logfile) { + snprintf(path, PATH_MAX, "%s/%s", config->rootdir, LOGFILE + 1); + config->logfile = strdup(path); + } + } else { + config->rootdir = strdup(ROOTDIR); + if(!config->dbpath) { + config->dbpath = strdup(DBPATH); + } + } + config->logfile = config->logfile ? config->logfile : strdup(LOGFILE); + /* Set GnuPG's home directory. This is not relative to rootdir, even if + * rootdir is defined. Reasoning: gpgdir contains configuration data. */ + config->gpgdir = config->gpgdir ? config->gpgdir : strdup(GPGDIR); + +#define SLMERGE(l, m) if(m) { l = (l & (m)) | (config->siglevel & ~(m)); } + SLMERGE(config->localfilesiglevel, config->localfilesiglevel_mask); + SLMERGE(config->remotefilesiglevel, config->remotefilesiglevel_mask); + alpm_list_t *i; + for(i = config->repos; i; i = alpm_list_next(i)) { + config_repo_t *repo = i->data; + SLMERGE(repo->siglevel, repo->siglevel_mask); + } +#undef SLMERGE +} + /** Parse a configuration file. * @param file path to the config file * @return 0 on success, non-zero on error @@ -994,12 +1002,13 @@ int parseconfig(const char *file) return ret; } pm_printf(ALPM_LOG_DEBUG, "config: finished parsing %s\n", file); - if((ret = setup_libalpm())) { - return ret; - } - alpm_list_free_inner(config->repos, (alpm_list_fn_free) config_repo_free); - alpm_list_free(config->repos); - config->repos = NULL; + finalize_config(); + /*if((ret = setup_libalpm())) {*/ + /*return ret;*/ + /*}*/ + /*alpm_list_free_inner(config->repos, (alpm_list_fn_free) config_repo_free);*/ + /*alpm_list_free(config->repos);*/ + /*config->repos = NULL;*/ return ret; }
diff --git a/test/unit/pacman/config.c b/test/unit/pacman/config.c new file mode 100644 index 0000000..9f5aa54 --- /dev/null +++ b/test/unit/pacman/config.c @@ -0,0 +1,174 @@ +#include <string.h> +#include <stdlib.h> + +#define CONFFILE "/etc/pacman.conf" +#define DBPATH "/var/lib/pacman/" +#define LOGFILE "/var/log/pacman.log" +#define ROOTDIR "/" +#define GPGDIR "/etc/pacman.d/gnupg/" +#define CACHEDIR "/var/cache/pacman/pkg" + +#include "pacman/conf.c" +#include "pacman/ini.c" +#include "pacman/util.c" +#include "pacman/callback.c" + +#include "tap.h" + +int main(int argc, char **argv) { + alpm_list_t *i; + char temp_path1[] = "pu_config_test_XXXXXX"; + char temp_path2[] = "pu_config_test_XXXXXX"; + int fd1 = mkstemp(temp_path1); + int fd2 = mkstemp(temp_path2); + FILE *stream1 = fdopen(fd1, "r+"); + FILE *stream2 = fdopen(fd2, "r+"); + fprintf(stream2, + "\n" + "SigLevel = DatabaseNever\n" + ""); + fprintf(stream1, + "\n" + "[options]\n" + "#RootDir = /root1\n" + "RootDir = /root2\n" + "DBPath = /dbpath1/ \n" + "DBPath = /dbpath2/ \n" + "CacheDir=/cachedir\n" + "GPGDir =gpgdir\n" + "LogFile= /logfile #ignore me\n" + " HoldPkg = holdpkga holdpkgb \n" + "IgnorePkg = ignorepkga\n" + "IgnorePkg = ignorepkgb\n" + " IgnoreGroup = ignoregroupa ignoregroupb \n" + "Architecture = i686\n" + "NoUpgrade = /tmp/noupgrade*\n" + "NoExtract = /tmp/noextract*\n" + "CleanMethod = KeepInstalled KeepCurrent\n" + "UseSyslog\n" + "Color\n" + "UseDelta\n" + "TotalDownload\n" + "CheckSpace\n" + "VerbosePkgLists\n" + "XferCommand = xfercommand\n" + + "LocalFileSigLevel = PackageNever\n" + "SigLevel = PackageOptional DatabaseRequired PackageTrustedOnly\n" + "RemoteFileSigLevel = PackageTrustAll\n" + + "[core]\n" + "SigLevel = PackageRequired\n" + "Server = $repo:$arch\n" + + "[extra]\n" + "Include = %s\n" + "", + temp_path2); + + fclose(stream1); + fclose(stream2); + + tap_plan(37); + + if((config = config_new()) == NULL) { + tap_bail("config_new failed"); + } + parseconfig(temp_path1); + + tap_is_str(config->rootdir, "/root2", "RootDir"); + tap_is_str(config->dbpath, "/dbpath1/", "DBPath"); + tap_is_str(config->gpgdir, "gpgdir", "DPGDir"); + tap_is_str(config->logfile, "/logfile", "LogFile"); + tap_is_str(config->arch, "i686", "Arch"); + tap_is_str(config->xfercommand, "xfercommand", "Include XferCommand"); + + tap_ok(config->usesyslog, "UseSyslog"); + tap_ok(config->color, "Color"); + tap_ok(config->totaldownload, "TotalDownload"); + tap_ok(config->checkspace, "CheckSpace"); + tap_ok(config->verbosepkglists, "VerbosePkgLists"); + +#define is_list_exhausted(l, name) do { \ + tap_ok(l == NULL, name " exhausted"); \ + if(l) { \ + tap_diag("remaining elements:"); \ + while(l) { \ + tap_diag(l->data); \ + l = alpm_list_next(l); \ + } \ + } \ + } while(0) + + +#define is_str_list(l, str, desc) do { \ + if(l) { \ + tap_is_str(l->data, str, desc); \ + l = alpm_list_next(l); \ + } else { \ + tap_ok(l != NULL, desc); \ + } \ + } while(0) + + i = config->ignorepkg; + is_str_list(i, "ignorepkga", "IgnorePkg a"); + is_str_list(i, "ignorepkgb", "IgnorePkg b"); + is_list_exhausted(i, "ignorepkg"); + + i = config->ignoregrp; + is_str_list(i, "ignoregroupa", "IgnoreGroup a"); + is_str_list(i, "ignoregroupb", "IgnoreGroup b"); + is_list_exhausted(i, "ignoregroup"); + + i = config->noupgrade; + is_str_list(i, "/tmp/noupgrade*", "NoUpgrade"); + is_list_exhausted(i, "noupgrade"); + + i = config->noextract; + is_str_list(i, "/tmp/noextract*", "NoExtract"); + is_list_exhausted(i, "noextract"); + + i = config->cachedirs; + is_str_list(i, "/cachedir", "CacheDir"); + is_list_exhausted(i, "cachedir"); + + i = config->holdpkg; + is_str_list(i, "holdpkga", "HoldPkg a"); + is_str_list(i, "holdpkgb", "HoldPkg b"); + is_list_exhausted(i, "holdpkgs"); +#undef tap_is_str_list + + tap_is_float(config->deltaratio, 0.7, 0.0001, "UseDelta (default)"); + + alpm_siglevel_t base = ALPM_SIG_PACKAGE + | ALPM_SIG_PACKAGE_OPTIONAL + | ALPM_SIG_DATABASE; + alpm_siglevel_t local = base & ~(ALPM_SIG_PACKAGE); + alpm_siglevel_t remote = base + | ALPM_SIG_PACKAGE_MARGINAL_OK + | ALPM_SIG_PACKAGE_UNKNOWN_OK; + alpm_siglevel_t core = base & ~(ALPM_SIG_PACKAGE_OPTIONAL); + alpm_siglevel_t extra = base & ~(ALPM_SIG_DATABASE); + + tap_is_int(config->siglevel, base, "SigLevel"); + tap_is_int(config->localfilesiglevel, local, "LocalFileSigLevel"); + tap_is_int(config->remotefilesiglevel, remote, "RemoteFileSigLevel"); + + i = config->repos; + config_repo_t *repo = i->data; + tap_is_str(repo->name, "core", "core name"); + tap_is_int(repo->siglevel, core, "core siglevel"); + tap_is_str(repo->servers->data, "$repo:$arch", "core server"); + tap_ok(repo->servers->next == NULL, "core server count"); + + i = alpm_list_next(i); + repo = i->data; + tap_is_str(repo->name, "extra", "extra name"); + tap_is_int(repo->siglevel, extra, "extra siglevel"); + tap_ok(repo->servers == NULL, "extra server count"); + + unlink(temp_path1); + unlink(temp_path2); + + return tap_tests_failed(); +} diff --git a/test/unit/pacman/tap.h b/test/unit/pacman/tap.h new file mode 100644 index 0000000..e670909 --- /dev/null +++ b/test/unit/pacman/tap.h @@ -0,0 +1,123 @@ +#include <math.h> +#include <stdarg.h> + +int _tap_tests_run = 0; +int _tap_tests_failed = 0; + +const char *_tap_todo = NULL; + +void tap_todo(const char *reason) +{ + _tap_todo = reason; +} + +void tap_ok(int success, const char *description, ...) +{ + const char *result; + if(success) { + result = "ok"; + if(_tap_todo) ++_tap_tests_failed; + } else { + result = "not ok"; + if(!_tap_todo) ++_tap_tests_failed; + } + + printf("%s %d", result, ++_tap_tests_run); + + if(description) { + va_list args; + fputs(" - ", stdout); + va_start(args, description); + vprintf(description, args); + va_end(args); + } + + if(_tap_todo) { + fputs(" # TODO", stdout); + if(*_tap_todo) { + fputc(' ', stdout); + fputs(_tap_todo, stdout); + } + } + + fputc('\n', stdout); +} + +void tap_bail(const char *format, ...) +{ + va_list args; + fputs("Bail out! ", stdout); + va_start(args, format); + vprintf(format, args); + va_end(args); + fputc('\n', stdout); +} + +void tap_diag(const char *format, ...) +{ + va_list args; + fputs(" # ", stdout); + va_start(args, format); + vprintf(format, args); + va_end(args); + fputc('\n', stdout); +} + +void tap_is_float(float got, float expected, float delta, + const char *description) +{ + float diff = fabs(expected - got); + int match = diff < delta; + tap_ok(match, description); + if(!match) { + tap_diag("expected '%f'", expected); + tap_diag("got '%f'", got); + tap_diag("delta '%f'", diff); + tap_diag("allowed '%f'", delta); + } +} + +void tap_is_int(int got, int expected, const char *description) +{ + int match = got == expected; + tap_ok(match, description); + if(!match) { + tap_diag("expected '%d'", expected); + tap_diag("got '%d'", got); + } +} + +void tap_is_str(const char *got, const char *expected, const char *description) +{ + int match; + if(got && expected) { + match = (strcmp(got, expected) == 0); + } else { + match = (got == expected); + } + tap_ok(match, description); + if(!match) { + tap_diag("expected '%s'", expected); + tap_diag("got '%s'", got); + } +} + +void tap_plan(int test_count) +{ + printf("1..%d\n", test_count); +} + +void tap_done_testing(void) +{ + tap_plan(_tap_tests_run); +} + +int tap_tests_run(void) +{ + return _tap_tests_run; +} + +int tap_tests_failed(void) +{ + return _tap_tests_failed; +}
participants (3)
-
Allan McRae
-
Andrew Gregory
-
Sören Brinkmann