[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(-) -- 1.9.2
Signed-off-by: Andrew Gregory
Hi Andrew, On Sat, 2014-04-26 at 07:56PM -0400, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory
--- 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); 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);
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
--- 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; } 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) {
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
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
--- 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); } } 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,
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; } 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)) {
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
On 27/04/14 09:56, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory
--- 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); 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; }
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
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
Ack.
Signed-off-by: Andrew Gregory
On 27/04/14 09:56, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory
---
This is fine - a note in the commit message about the motivation to do this would be useful. Allan
Signed-off-by: Andrew Gregory
On 27/04/14 09:56, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory
--- 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
On 27/04/14 09:57, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory
---
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
--- 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
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
--- 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 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
--- 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; }
Move the remaining output into conf.c by notifying the callback of read
errors.
Signed-off-by: Andrew Gregory
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
participants (3)
-
Allan McRae
-
Andrew Gregory
-
Sören Brinkmann