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 <[email protected]> > ---
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 <[email protected]> > + * > + * 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 <[email protected]> > + * > + * 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
