Re: [pacman-dev] [PATCH] Give an error message on alpm_db_register_sync() error

On Mon, Nov 17, 2008 at 5:33 PM, Nagy Gabor wrote:

...

From 8e11e0c7198f7c7bdeb6ca76d647d4831593b965 Mon Sep 17 00:00:00 2001 From: Nagy Gabor Date: Mon, 17 Nov 2008 17:02:43 +0100 Subject: [PATCH] Give an error message on alpm_db_register_sync() error

This patch slightly modifies pacman.c/_parseconfig():

See FS#12148. Now pacman prints the following error message in that case: "error: could not register 'unstable' database (could not open database)"

I also added an error message for alpm_db_setserver() error.

I changed the "return(1);" scheme to "ret = 1; goto clean_up;" to make sure that we free allocated memory and close open files.

Signed-off-by: Nagy Gabor --- src/pacman/pacman.c | 46 ++++++++++++++++++++++++++++++++++------------ 1 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 3813a16..6d588f8 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -583,6 +583,7 @@ static int _parseconfig(const char *file, const char *givensection, int linenum = 0; char *ptr, *section = NULL; pmdb_t *db = NULL; + int ret = 0;

pm_printf(PM_LOG_DEBUG, "config: attempting to read file %s\n", file); fp = fopen(file, "r"); @@ -625,7 +626,8 @@ static int _parseconfig(const char *file, const char *givensection, if(!strlen(section)) { pm_printf(PM_LOG_ERROR, _("config file %s, line %d: bad section name.\n"), file, linenum); - return(1); + ret = 1; + goto clean_up;

Can we use "cleanup" as the label instead? It is this way every other place in the code, consistency would be a good thing.

...

} /* if we are not looking at the options section, register a db and also * ensure we have set all of our library paths as the library is too stupid @@ -633,6 +635,12 @@ static int _parseconfig(const char *file, const char *givensection, if(strcmp(section, "options") != 0) { setlibpaths(); db = alpm_db_register_sync(section); + if(db == NULL) { + pm_printf(PM_LOG_ERROR, _("could not register '%s' database (%s)\n"), + section, alpm_strerrorlast());

Obviously an error is better than no error at all, but does this have meaning to end users at all? I don't know what else to put however, so for now it is fine.

...

+ ret = 1; + goto clean_up; + } } } else { /* directive */ @@ -647,13 +655,15 @@ static int _parseconfig(const char *file, const char *givensection, if(key == NULL) { pm_printf(PM_LOG_ERROR, _("config file %s, line %d: syntax error in config file- missing key.\n"), file, linenum); - return(1); + ret = 1; + goto clean_up; } /* For each directive, compare to the camelcase string. */ if(section == NULL) { pm_printf(PM_LOG_ERROR, _("config file %s, line %d: All directives must belong to a section.\n"), file, linenum); - return(1); + ret = 1; + goto clean_up; } if(ptr == NULL && strcmp(section, "options") == 0) { /* directives without settings, all in [options] */ @@ -678,7 +688,8 @@ static int _parseconfig(const char *file, const char *givensection, } else { pm_printf(PM_LOG_ERROR, _("config file %s, line %d: directive '%s' not recognized.\n"), file, linenum, key); - return(1); + ret = 1; + goto clean_up; } } else { /* directives with settings */ @@ -709,7 +720,8 @@ static int _parseconfig(const char *file, const char *givensection, if(alpm_option_add_cachedir(ptr) != 0) { pm_printf(PM_LOG_ERROR, _("problem adding cachedir '%s' (%s)\n"), ptr, alpm_strerrorlast()); - return(1); + ret = 1; + goto clean_up; } pm_printf(PM_LOG_DEBUG, "config: cachedir: %s\n", ptr); } else if(strcmp(key, "RootDir") == 0) { @@ -733,13 +745,15 @@ static int _parseconfig(const char *file, const char *givensection, config->cleanmethod = PM_CLEAN_KEEPCUR; } else { pm_printf(PM_LOG_ERROR, _("invalid value for 'CleanMethod' : '%s'\n"), ptr); - return(1); + ret = 1; + goto clean_up; } pm_printf(PM_LOG_DEBUG, "config: cleanmethod: %s\n", ptr); } else { pm_printf(PM_LOG_ERROR, _("config file %s, line %d: directive '%s' not recognized.\n"), file, linenum, key); - return(1); + ret = 1; + goto clean_up; } } else if(strcmp(key, "Server") == 0) { /* let's attempt a replacement for the current repo */ @@ -747,27 +761,35 @@ static int _parseconfig(const char *file, const char *givensection,

if(alpm_db_setserver(db, server) != 0) { /* pm_errno is set by alpm_db_setserver */ - return(1); + pm_printf(PM_LOG_ERROR, _("could not add server URL to database '%s': %s (%s)\n"), + alpm_db_get_name(db), server, alpm_strerrorlast()); + free(server); + ret = 1; + goto clean_up; We didn't do so great with memory leaks before. :)

...

}

free(server); } else { pm_printf(PM_LOG_ERROR, _("config file %s, line %d: directive '%s' not recognized.\n"), file, linenum, key); - return(1); + ret = 1; + goto clean_up; } } } } - fclose(fp); + +clean_up: + if(fp) { + fclose(fp); + } if(section){ free(section); } - /* call setlibpaths here to ensure we have called it at least once */ setlibpaths(); pm_printf(PM_LOG_DEBUG, "config: finished parsing %s\n", file); - return(0); + return(ret); }

/** Parse a configuration file. --

Looks good outside of my main "cleanup" naming comment. I'll pull this into maint once that gets addressed, thanks for the quick fix to the missing message and Flyspray bug. -Dan