On 03/15/13 at 11:06am, Sascha Kruse wrote:
This makes the ini parsing code reusable for future additions like parsing config files for hooks.
Signed-off-by: Sascha Kruse <knopwob@googlemail.com> ---
Some general notes first: * Always use tabs for indentation. * Returning an empty list isn't a good way to indicate an error. An empty config file is perfectly valid. (`pacman --config /dev/null -Qi pacman` works just fine) More specific comments inlined below.
src/pacman/Makefile.am | 1 + src/pacman/ini.c | 266 +++++++++++++++++++++++++++++++++++++++++++++++++ src/pacman/ini.h | 46 +++++++++ 3 files changed, 313 insertions(+) create mode 100644 src/pacman/ini.c create mode 100644 src/pacman/ini.h
diff --git a/src/pacman/Makefile.am b/src/pacman/Makefile.am index ed51573..3872cc1 100644 --- a/src/pacman/Makefile.am +++ b/src/pacman/Makefile.am @@ -31,6 +31,7 @@ pacman_SOURCES = \ conf.h conf.c \ database.c \ deptest.c \ + ini.c \ package.h package.c \ pacman.h pacman.c \ query.c \ diff --git a/src/pacman/ini.c b/src/pacman/ini.c new file mode 100644 index 0000000..4b52a7f --- /dev/null +++ b/src/pacman/ini.c @@ -0,0 +1,266 @@ +/* + * ini.c + * + * Copyright (c) 2013 Pacman Development Team <pacman-dev@archlinux.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <errno.h> +#include <glob.h> +#include <limits.h> +#include <string.h> /* strdup */ + +#include "ini.h" + +#include "util.h" +#include "pacman.h" + +/** Recursivly parse file taking 'Include' directives into account + * @param file the file to parse + * @param sections the list of sections that have already been parsed + * @param depth depth of include levels + * @return a list of parsed sections + */ +static alpm_list_t *ini_parse_file_r(const char *file, alpm_list_t *sections, + int depth) +{ + FILE *fp = NULL; + char line[PATH_MAX]; + int linenum = 0; + const int max_depth = 10;
I think this type of setting would more traditionally be done as a macro.
+ int failure = 0; + ini_section_t *current_section = NULL; + + if(depth >= max_depth) { + pm_printf(ALPM_LOG_ERROR, + _("ini parsing exceeded max recursion depth of %d.\n"), max_depth); + failure = 1; + goto cleanup; + } + + pm_printf(ALPM_LOG_DEBUG, "ini: attempting to read file %s\n", file); + fp = fopen(file, "r"); + if(fp == NULL) { + pm_printf(ALPM_LOG_ERROR, _("ini file %s could not be read: %s\n"), + file, strerror(errno)); + failure = 1; + goto cleanup; + } + + /* find current section */ + alpm_list_t *last; + if(sections != NULL) { + for (last = sections; last->next; last = alpm_list_next(last)); + current_section = last->data; + }
Use alpm_list_last() for this (it's NULL safe). This needs to be moved to the top of the scope and can be condensed to: alpm_list_t last = alpm_list_last(sections); ini_section_t *current_section = last ? last->data : NULL;
+ + while(fgets(line, PATH_MAX, fp)) { + char *key, *value, *ptr; + size_t line_len; + + linenum++; + + /* ignore whole line and end of line comments */ + if((ptr = strchr(line, '#'))) { + *ptr = '\0'; + } + + line_len = strtrim(line); + + if(line_len == 0) { + continue; + } + + if(line[0] == '[' && line[line_len - 1] == ']') { + char *name; + /* only possibility here is a line == '[]' */ + if(line_len <= 2) { + pm_printf(ALPM_LOG_ERROR, _("ini file %s, line %d: bad section name.\n"), + file, linenum); + failure = 1; + goto cleanup; + } + + + /* create a new section */ + current_section = malloc(sizeof(ini_section_t)); + if(current_section == NULL) { + pm_printf(ALPM_LOG_ERROR, + _("malloc failure: could not allocate %zd bytes\n"), + sizeof(ini_section_t)); + failure = 1; + goto cleanup; + } + sections = alpm_list_add(sections, current_section); + current_section->directives = NULL; + + + /* new config section, skip the '[' */ + name = strdup(line + 1); + name[line_len - 2] = '\0'; + current_section->name = name; + pm_printf(ALPM_LOG_DEBUG, _("ini file %s, line %d: new section: %s.\n"), + file, linenum, name); + continue; + } + + /* directive */ + /* strsep modifies the 'line' string: 'key \0 value' */ + key = line; + value = line; + strsep(&value, "="); + strtrim(key); + strtrim(value); + + if(key == NULL) {
I don't think it's possible for key to be NULL. This should be a check for the empty string instead.
+ pm_printf(ALPM_LOG_ERROR, _("ini file %s, line %d: syntax error in config file- missing key.\n"), + file, linenum); + failure = 1; + goto cleanup; + } + /* For each directive, compare to the camelcase string. */ + if(current_section == NULL) { + pm_printf(ALPM_LOG_ERROR, _("ini file %s, line %d: All directives must belong to a section.\n"), + file, linenum); + failure = 1; + goto cleanup; + }
Let's move this after the "Include" processing. If included files can have their own sections they shouldn't necessarily have to be inside a section already.
+ /* 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, _("ini file %s, line %d: directive '%s' needs a value\n"), + file, linenum, key); + failure = 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, + "ini file %s, line %d: include globbing out of space\n", + file, linenum); + break; + case GLOB_ABORTED: + pm_printf(ALPM_LOG_DEBUG, + "ini file %s, line %d: include globbing read error for %s\n", + file, linenum, value); + break; + case GLOB_NOMATCH: + pm_printf(ALPM_LOG_DEBUG, + "ini file %s, line %d: no include found for %s\n", + file, linenum, value); + break;
Making failed includes non-fatal was added in a4b81387974e6b36c6fbc541b543d6d67882eadd but it's not clear to me why. I would expect a failed include to be fatal, possibly with an exception for wildcards that simply failed to match a path.
+ default: + for(gindex = 0; gindex < globbuf.gl_pathc; gindex++) { + pm_printf(ALPM_LOG_DEBUG, "ini file %s, line %d: including %s\n", + file, linenum, globbuf.gl_pathv[gindex]); + sections = ini_parse_file_r(globbuf.gl_pathv[gindex], sections, depth + 1); + if(sections == NULL) { + globfree(&globbuf); + failure = 1; + goto cleanup; + }
Checking for an empty list isn't necessarily a good way to indicate an error condition. If includes are allowed outside a section, they could validly return an empty list if they don't have any directives.
+ } + break;
All of those breaks should be indented another level.
+ } + globfree(&globbuf); + continue; + } + + ini_directive_t *directive = malloc(sizeof(ini_directive_t));
Declarations need to be at the start of their scope. Put this section in an else block to keep the scope small.
+ if(directive == NULL) { + pm_printf(ALPM_LOG_ERROR, + _("malloc failure: could not allocate %zd bytes\n"), + sizeof(ini_directive_t)); + failure = 1; + goto cleanup; + } + + directive->key = strdup(key); + directive->value = value ? strdup(value) : NULL; + directive->file = strdup(file);
This seems pretty wasteful. Is there a way we can avoid storing a unique string for the filename in every single directive?
+ directive->linenum = linenum; + + current_section->directives = alpm_list_add(current_section->directives, + directive); + + } + +cleanup: + if(fp) { + fclose(fp); + } + pm_printf(ALPM_LOG_DEBUG, "config: finished parsing %s\n", file); + if(failure) { + return NULL;
Memory leak. sections is never free'd.
+ } else { + return sections; + } +} + +/** Parse file into a list of sections + * @param file the file to parse + * @return a list of ini_section_t representing the file + */ +alpm_list_t *ini_parse_file(const char *file) +{ + return ini_parse_file_r(file, NULL, 1); +} + +/** Free a directive + * @param data the directive to free_directive + */ +static void free_directive(void *data) +{ + ini_directive_t *directive = (ini_directive_t *) data; + free(directive->key); + if(directive->value != NULL) { + free(directive->value); + }
free() is NULL safe.
+ free(directive->file); + free(directive); +} + +/** Free a section + * @param data the section to free_section + */ +static void free_section(void *data) +{ + ini_section_t *section = (ini_section_t *) data; + + free(section->name); + alpm_list_free_inner(section->directives, free_directive);
free_inner does not free the list itself.
+ free(section); +} + +/** Free a list of sections. + * This also frees all contained strings. + * @param sections the list of sections to free_section + */ +void ini_free(alpm_list_t *sections) +{ + if(sections == NULL) { + return; + } + + alpm_list_free_inner(sections, free_section);
free_inner does not free the list itself.
+} + +/* vim: set ts=2 sw=2 noet: */ diff --git a/src/pacman/ini.h b/src/pacman/ini.h new file mode 100644 index 0000000..413f8b6 --- /dev/null +++ b/src/pacman/ini.h @@ -0,0 +1,46 @@ +/* + * ini.h + * + * Copyright (c) 2013 Pacman Development Team <pacman-dev@archlinux.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ +#ifndef _PM_INI_H +#define _PM_INI_H + +#include <alpm.h> + +typedef struct _ini_directive_t { + char *key; + char *value; + char *file; + int linenum; +} ini_directive_t; + +typedef struct _ini_section_t { + char *name; + alpm_list_t *directives; +} ini_section_t; + +/* parse file into a list of ini_section_t */ +alpm_list_t *ini_parse_file(const char *file); + +/* free a list of ini_section_t. + * This also frees all strings within. + */ +void ini_free(alpm_list_t *sections); + +#endif /* _PM_INI_H */ + +/* vim: set ts=2 sw=2 noet: */ -- 1.8.2