[pacman-dev] [PATCH 0/2] separate ini parsing from pacman/conf.c
This set of patches separate the ini parsing from pacman/conf.c into its own file to make the code reusable for future additions, like parsing config files for hooks. The config file gets parsed into a list of ini_section_t which contains the name of the section and a list of directives which in turn contain key, value, file and linenum. File and linenum are included to keep the ability to mention the location of errors in pacmans error messages. I realize that a new release is planed for the near future and this patch is not intended to be included in that release, since it adds new strings to be translated and it probably needs more than one review cycle. Best regards, Sascha Kruse
This makes the ini parsing code reusable for future additions like parsing config files for hooks. Signed-off-by: Sascha Kruse <knopwob@googlemail.com> --- 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; + 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; + } + + 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) { + 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; + } + /* 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; + 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; + } + } + break; + } + globfree(&globbuf); + continue; + } + + ini_directive_t *directive = malloc(sizeof(ini_directive_t)); + 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); + 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; + } 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(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(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); +} + +/* 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
On Fri, Mar 15, 2013 at 7:06 AM, Sascha Kruse <knopwob@googlemail.com>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> --- 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
Small typo here 'Recursivly' -> 'Recursively'
+ * @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; + 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; + } + + 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) { + 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; + } + /* 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; + 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; + } + } + break; + } + globfree(&globbuf); + continue; + } + + ini_directive_t *directive = malloc(sizeof(ini_directive_t)); + 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); + 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; + } 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(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(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); +} + +/* 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
I think the other patch should be combined with this one. Since you are moving code out of one file and into another, it's a little confusing if you separate it into two patches. +chirantan
2013/3/16 Chirantan Ekbote <chirantan.ekbote@gmail.com>:
On Fri, Mar 15, 2013 at 7:06 AM, Sascha Kruse <knopwob@googlemail.com>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> ---
... SNIP ...
+#include "util.h" +#include "pacman.h" + +/** Recursivly parse file taking 'Include' directives into account
Small typo here 'Recursivly' -> 'Recursively'
Fixed locally. I'll resubmit with this fix after someone else reviewed this, since I guess there will be more issues that need to be addressed. ... SNIP ...
I think the other patch should be combined with this one. Since you are moving code out of one file and into another, it's a little confusing if you separate it into two patches.
The reasoning for my decision to separate those patches was that everything is still working after the first patch. The first patch adds new functions for config parsing without touching any other code and the second patch replaces the old code with these functions. Greetings, Sascha Kruse
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
Hi Sascha, Are you intending to revise this patch based on the comments that were made? Allan
Hi, On 07/04/2013 04:07 PM, Allan McRae wrote:
Hi Sascha,
Are you intending to revise this patch based on the comments that were made?
I'm sorry. I completely forgot about this patch and I won't have the time to fix this up within the near future. So if you are or some else is interested in fixing this up, feel free to go ahead. -- Best Regards, Sascha kruse
Signed-off-by: Sascha Kruse <knopwob@googlemail.com> --- src/pacman/conf.c | 343 ++++++++++++++++++------------------------------------ 1 file changed, 111 insertions(+), 232 deletions(-) diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 3f1b1c3..06a4612 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -36,6 +36,7 @@ #include "util.h" #include "pacman.h" #include "callback.h" +#include "ini.h" /* global config variable */ config_t *config = NULL; @@ -462,9 +463,21 @@ static void setrepeatingoption(char *ptr, const char *option, } } -static int _parse_options(const char *key, char *value, - const char *file, int linenum) +/** Parse one directive from the 'options' section + * @param directive the directive to parse + * @return 0 on success non-zero otherwise + */ +static int parse_option_directive(ini_directive_t *directive) { + const char *key = directive->key; + char *value = NULL; + const char *file = directive->file; + int linenum = directive->linenum; + + if(directive->value != NULL) { + value = strdup(directive->value); + } + if(value == NULL) { /* options without settings */ if(strcmp(key, "UseSyslog") == 0) { @@ -599,6 +612,24 @@ static int _parse_options(const char *key, char *value, return 0; } +/** Parse the 'options' section from the config file + * @param options the option section to parse_options + * @return 0 on success non-zero otherwise + */ +static int parse_options(ini_section_t *options) +{ + alpm_list_t *iter; + int ret; + + for(iter = options->directives; iter; iter = alpm_list_next(iter)) { + ini_directive_t *directive = (ini_directive_t *) iter->data; + if((ret = parse_option_directive(directive))) { + return ret; + } + } + return 0; +} + static int _add_mirror(alpm_db_t *db, char *value) { const char *dbname = alpm_db_get_name(db); @@ -739,43 +770,55 @@ static int setup_libalpm(void) return 0; } -/** - * Allows parsing in advance of an entire config section before we start - * calling library methods. - */ -struct section_t { - /* useful for all sections */ - char *name; - int is_options; - /* db section option gathering */ - alpm_siglevel_t siglevel; - alpm_list_t *servers; -}; - -/** - * Wrap up a section once we have reached the end of it. This should be called - * when a subsequent section is encountered, or when we have reached the end of - * the root config file. Once called, all existing saved config pieces on the - * section struct are freed. - * @param section the current parsed and saved section data - * @param parse_options whether we are parsing options or repo data - * @return 0 on success, 1 on failure +/** Parse an ini section that describes a repository. + * @param section the ini section. + * @return 0 on success non-zero otherwise */ -static int finish_section(struct section_t *section, int parse_options) +static int parse_repository(ini_section_t *section) { int ret = 0; alpm_list_t *i; alpm_db_t *db; + alpm_siglevel_t siglevel = ALPM_SIG_USE_DEFAULT; + alpm_list_t *servers = NULL; - pm_printf(ALPM_LOG_DEBUG, "config: finish section '%s'\n", section->name); + alpm_list_t *directive_iter; + for(directive_iter = section->directives; directive_iter; + directive_iter = alpm_list_next(directive_iter)) { + ini_directive_t *directive = (ini_directive_t *) directive_iter->data; - /* parsing options (or nothing)- nothing to do except free the pieces */ - if(!section->name || parse_options || section->is_options) { - goto cleanup; + if(strcmp(directive->key, "Server") == 0) { + if(directive->value == NULL || strlen(directive->value) == 0) { + pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: directive '%s' needs a value\n"), + directive->file, directive->linenum, directive->key); + } else { + servers = alpm_list_add(servers, strdup(directive->value)); + } + }else if(strcmp(directive->key, "SigLevel") == 0) { + alpm_list_t *values = NULL; + char *value = strdup(directive->value); + setrepeatingoption(value, "SigLevel", &values); + if(values) { + if(siglevel == ALPM_SIG_USE_DEFAULT) { + siglevel = config->siglevel; + } + if(process_siglevel(values, &siglevel, directive->file, + directive->linenum)) { + FREELIST(values); + ret = 1; + goto cleanup; + } + free(value); + FREELIST(values); + } + } else { + pm_printf(ALPM_LOG_WARNING, + _("config file %s, line %d: directive '%s' in section '%s' not recognized.\n"), + directive->file, directive->linenum, directive->key, section->name); + } } - /* 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, section->name, siglevel); if(db == NULL) { pm_printf(ALPM_LOG_ERROR, _("could not register '%s' database (%s)\n"), section->name, alpm_strerror(alpm_errno(config->handle))); @@ -783,7 +826,7 @@ static int finish_section(struct section_t *section, int parse_options) goto cleanup; } - for(i = section->servers; i; i = alpm_list_next(i)) { + for(i = servers; i; i = alpm_list_next(i)) { char *value = i->data; if(_add_mirror(db, value) != 0) { pm_printf(ALPM_LOG_ERROR, @@ -796,226 +839,62 @@ static int finish_section(struct section_t *section, int parse_options) } cleanup: - alpm_list_free(section->servers); - section->servers = NULL; - section->siglevel = ALPM_SIG_USE_DEFAULT; - free(section->name); - section->name = NULL; + alpm_list_free(servers); return ret; } -/** The "real" parseconfig. Each "Include" directive will recall this method so - * recursion and stack depth are limited to 10 levels. The publicly visible - * parseconfig calls this with a NULL section argument so we can recall from - * within ourself on an include. + +/** Parse a configuration file. * @param file path to the config file - * @param section the current active section - * @param parse_options whether to parse and call methods for the options - * section; if 0, parse and call methods for the repos sections - * @param depth the current recursion depth - * @return 0 on success, 1 on failure + * @return 0 on success, non-zero on error */ -static int _parseconfig(const char *file, struct section_t *section, - int parse_options, int depth) +int parseconfig(const char *file) { - FILE *fp = NULL; - char line[PATH_MAX]; - int linenum = 0; - int ret = 0; - const int max_depth = 10; - - if(depth >= max_depth) { - pm_printf(ALPM_LOG_ERROR, - _("config parsing exceeded max recursion depth of %d.\n"), max_depth); - ret = 1; - goto cleanup; - } + alpm_list_t *sections; + int ret; - 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; + sections = ini_parse_file(file); + if(sections == NULL) { + return 1; } - 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, _("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'; - /* we're at a new section; perform any post-actions for the prior */ - if(finish_section(section, parse_options)) { - ret = 1; - goto cleanup; + /* parse options section */ + pm_printf(ALPM_LOG_DEBUG, "parseconfig: options pass\n"); + alpm_list_t *iter; + for(iter = sections; iter; iter = alpm_list_next(iter)) { + ini_section_t *current = (ini_section_t *) iter->data; + if(strcmp("options", current->name) == 0) { + if((ret = parse_options(current))) { + ini_free(sections); + return ret; } - pm_printf(ALPM_LOG_DEBUG, "config: new section '%s'\n", name); - section->name = name; - section->is_options = (strcmp(name, "options") == 0); - continue; + break; } + } - /* directive */ - /* strsep modifies the 'line' string: 'key \0 value' */ - key = line; - value = line; - strsep(&value, "="); - 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; - } - /* For each directive, compare to the camelcase string. */ - if(section->name == NULL) { - pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: All directives must belong to a section.\n"), - file, linenum); - 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((ret = setup_libalpm())) { + ini_free(sections); + return ret; + } - 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]); - _parseconfig(globbuf.gl_pathv[gindex], section, parse_options, depth + 1); - } - break; - } - globfree(&globbuf); + /* parse the rest (Repositories) */ + pm_printf(ALPM_LOG_DEBUG, "parseconfig: repo pass\n"); + + for (iter = sections; iter; iter = alpm_list_next(iter)) { + ini_section_t *current = (ini_section_t *) iter->data; + if(strcmp("options", current->name) == 0) { + /* skip options */ continue; } - if(parse_options && section->is_options) { - /* we are either in options ... */ - if((ret = _parse_options(key, value, file, linenum)) != 0) { - goto cleanup; - } - } else if(!parse_options && !section->is_options) { - /* ... or in a repo section */ - if(strcmp(key, "Server") == 0) { - 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; - } - section->servers = alpm_list_add(section->servers, strdup(value)); - } else if(strcmp(key, "SigLevel") == 0) { - alpm_list_t *values = NULL; - setrepeatingoption(value, "SigLevel", &values); - if(values) { - if(section->siglevel == ALPM_SIG_USE_DEFAULT) { - section->siglevel = config->siglevel; - } - if(process_siglevel(values, §ion->siglevel, file, linenum)) { - FREELIST(values); - ret = 1; - goto cleanup; - } - FREELIST(values); - } - } else { - pm_printf(ALPM_LOG_WARNING, - _("config file %s, line %d: directive '%s' in section '%s' not recognized.\n"), - file, linenum, key, section->name); - } + if((ret = parse_repository(current))) { + ini_free(sections); + return ret; } } - if(depth == 0) { - ret = finish_section(section, parse_options); - } + ini_free(sections); -cleanup: - if(fp) { - fclose(fp); - } - pm_printf(ALPM_LOG_DEBUG, "config: finished parsing %s\n", file); - return ret; -} - -/** Parse a configuration file. - * @param file path to the config file - * @return 0 on success, non-zero on error - */ -int parseconfig(const char *file) -{ - int ret; - struct section_t section; - memset(§ion, 0, sizeof(struct section_t)); - section.siglevel = ALPM_SIG_USE_DEFAULT; - /* 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"); - if((ret = _parseconfig(file, §ion, 1, 0))) { - return ret; - } - if((ret = setup_libalpm())) { - return ret; - } - /* second pass, repo section parsing */ - pm_printf(ALPM_LOG_DEBUG, "parseconfig: repo pass\n"); - return _parseconfig(file, §ion, 0, 0); + return 0; } /* vim: set ts=2 sw=2 noet: */ -- 1.8.2
participants (4)
-
Allan McRae
-
Andrew Gregory
-
Chirantan Ekbote
-
Sascha Kruse