[pacman-dev] [PATCH 1/2] Introduce ini.{c,h}
Andrew Gregory
andrew.gregory.8 at gmail.com
Tue Apr 2 22:59:17 EDT 2013
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 at 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 at 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 at 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
More information about the pacman-dev
mailing list