[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
Signed-off-by: Sascha Kruse
On Fri, Mar 15, 2013 at 7:06 AM, Sascha Kruse
This makes the ini parsing code reusable for future additions like parsing config files for hooks.
Signed-off-by: Sascha Kruse
--- 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
+ * + * 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 +#include +#include +#include /* 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
+ * + * 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 + +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
On Fri, Mar 15, 2013 at 7:06 AM, Sascha Kruse
wrote: This makes the ini parsing code reusable for future additions like parsing config files for hooks.
Signed-off-by: Sascha Kruse
---
... 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
---
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
+ * + * 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 +#include +#include +#include /* 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
+ * + * 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 + +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
participants (4)
-
Allan McRae
-
Andrew Gregory
-
Chirantan Ekbote
-
Sascha Kruse