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

Dan McGee dpmcgee at gmail.com
Mon Nov 17 21:26:08 EST 2008


On Mon, Nov 17, 2008 at 5:33 PM, Nagy Gabor <ngaba at bibl.u-szeged.hu> wrote:
>
>> From 8e11e0c7198f7c7bdeb6ca76d647d4831593b965 Mon Sep 17 00:00:00 2001
>> From: Nagy Gabor <ngaba at bibl.u-szeged.hu>
>> 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 <ngaba at bibl.u-szeged.hu>
>> ---
>>  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



More information about the pacman-dev mailing list