[pacman-dev] [PATCH 1/3] fix a few warnings reported by clang
- remove unused variables - some more sanity checks - safer printf Signed-off-by: Xavier Chantry <shiningxc@gmail.com> --- lib/libalpm/add.c | 2 ++ lib/libalpm/be_files.c | 2 +- lib/libalpm/conflict.c | 2 +- lib/libalpm/package.c | 3 +-- src/pacman/callback.c | 4 +++- src/pacman/pacman.c | 2 +- src/pacman/util.c | 2 +- 7 files changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index ebcd6a5..dc565ac 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -483,6 +483,8 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count, ALPM_LOG_FUNC; + ASSERT(trans != NULL, RET_ERR(PM_ERR_TRANS_NULL, -1)); + snprintf(scriptlet, PATH_MAX, "%s%s-%s/install", _alpm_db_path(db), alpm_pkg_get_name(newpkg), alpm_pkg_get_version(newpkg)); diff --git a/lib/libalpm/be_files.c b/lib/libalpm/be_files.c index 7fc20b8..f685019 100644 --- a/lib/libalpm/be_files.c +++ b/lib/libalpm/be_files.c @@ -140,8 +140,8 @@ static int dirlist_from_fs(const char *syncdbpath, alpm_list_t **dirlist) entry[len+1] = '\0'; *dirlist = alpm_list_add(*dirlist, entry); } + closedir(dbdir); } - closedir(dbdir); *dirlist = alpm_list_msort(*dirlist, alpm_list_count(*dirlist), _alpm_str_cmp); return(0); diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index e934c01..85db83b 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -408,7 +408,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(pmdb_t *db, pmtrans_t *trans, ALPM_LOG_FUNC; - if(db == NULL || upgrade == NULL) { + if(db == NULL || upgrade == NULL || trans == NULL) { return(NULL); } diff --git a/lib/libalpm/package.c b/lib/libalpm/package.c index 83a2fb8..10220af 100644 --- a/lib/libalpm/package.c +++ b/lib/libalpm/package.c @@ -459,7 +459,6 @@ void SYMEXPORT *alpm_pkg_changelog_open(pmpkg_t *pkg) struct archive *archive = NULL; struct archive_entry *entry; const char *pkgfile = pkg->origin_data.file; - int ret = ARCHIVE_OK; if((archive = archive_read_new()) == NULL) { RET_ERR(PM_ERR_LIBARCHIVE, NULL); @@ -473,7 +472,7 @@ void SYMEXPORT *alpm_pkg_changelog_open(pmpkg_t *pkg) RET_ERR(PM_ERR_PKG_OPEN, NULL); } - while((ret = archive_read_next_header(archive, &entry)) == ARCHIVE_OK) { + while(archive_read_next_header(archive, &entry) == ARCHIVE_OK) { const char *entry_name = archive_entry_pathname(entry); if(strcmp(entry_name, ".CHANGELOG") == 0) { diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 858bfdf..15f5423 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -365,6 +365,8 @@ void cb_trans_progress(pmtransprog_t event, const char *pkgname, int percent, case PM_TRANS_PROGRESS_CONFLICTS_START: opr = _("checking for file conflicts"); break; + default: + return; } /* find # of digits in package counts to scale output */ @@ -504,7 +506,7 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) gettimeofday(&initial_time, NULL); xfered_last = (off_t)0; rate_last = 0.0; - timediff = get_update_timediff(1); + get_update_timediff(1); } } else if(file_xfered == file_total) { /* compute final values */ diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index ff6ef5c..3dccd48 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -217,7 +217,7 @@ static void setarch(const char *arch) static void cleanup(int ret) { /* free alpm library resources */ if(alpm_release() == -1) { - pm_printf(PM_LOG_ERROR, alpm_strerrorlast()); + pm_printf(PM_LOG_ERROR, "%s\n", alpm_strerrorlast()); } /* free memory */ diff --git a/src/pacman/util.c b/src/pacman/util.c index 115b367..d395f3d 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -219,7 +219,7 @@ void indentprint(const char *str, int indent) p = wcstr; cidx = indent; - if(!p) { + if(!p || !len) { return; } -- 1.6.5.4
This function was quite huge (~230 lines) and difficult to parse, now it is slightly better. Signed-off-by: Xavier Chantry <shiningxc@gmail.com> --- src/pacman/pacman.c | 308 ++++++++++++++++++++++++++++----------------------- 1 files changed, 170 insertions(+), 138 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 3dccd48..38fc560 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -713,6 +713,127 @@ cleanup: return(ret); } +static int _parse_options(char *key, char *value) +{ + if(value == NULL) { + /* options without settings */ + if(strcmp(key, "UseSyslog") == 0) { + alpm_option_set_usesyslog(1); + pm_printf(PM_LOG_DEBUG, "config: usesyslog\n"); + } else if(strcmp(key, "ILoveCandy") == 0) { + config->chomp = 1; + pm_printf(PM_LOG_DEBUG, "config: chomp\n"); + } else if(strcmp(key, "ShowSize") == 0) { + config->showsize = 1; + pm_printf(PM_LOG_DEBUG, "config: showsize\n"); + } else if(strcmp(key, "UseDelta") == 0) { + alpm_option_set_usedelta(1); + pm_printf(PM_LOG_DEBUG, "config: usedelta\n"); + } else if(strcmp(key, "TotalDownload") == 0) { + config->totaldownload = 1; + pm_printf(PM_LOG_DEBUG, "config: totaldownload\n"); + } else { + pm_printf(PM_LOG_ERROR, _("directive '%s' without value not recognized\n"), key); + return(1); + } + } else { + /* options with settings */ + if(strcmp(key, "NoUpgrade") == 0) { + setrepeatingoption(value, "NoUpgrade", alpm_option_add_noupgrade); + } else if(strcmp(key, "NoExtract") == 0) { + setrepeatingoption(value, "NoExtract", alpm_option_add_noextract); + } else if(strcmp(key, "IgnorePkg") == 0) { + setrepeatingoption(value, "IgnorePkg", alpm_option_add_ignorepkg); + } else if(strcmp(key, "IgnoreGroup") == 0) { + setrepeatingoption(value, "IgnoreGroup", alpm_option_add_ignoregrp); + } else if(strcmp(key, "HoldPkg") == 0) { + setrepeatingoption(value, "HoldPkg", option_add_holdpkg); + } else if(strcmp(key, "SyncFirst") == 0) { + setrepeatingoption(value, "SyncFirst", option_add_syncfirst); + } else if(strcmp(key, "Architecture") == 0) { + if(!alpm_option_get_arch()) { + setarch(value); + } + } else if(strcmp(key, "DBPath") == 0) { + /* don't overwrite a path specified on the command line */ + if(!config->dbpath) { + config->dbpath = strdup(value); + pm_printf(PM_LOG_DEBUG, "config: dbpath: %s\n", value); + } + } else if(strcmp(key, "CacheDir") == 0) { + if(alpm_option_add_cachedir(value) != 0) { + pm_printf(PM_LOG_ERROR, _("problem adding cachedir '%s' (%s)\n"), + value, alpm_strerrorlast()); + return(1); + } + pm_printf(PM_LOG_DEBUG, "config: cachedir: %s\n", value); + } else if(strcmp(key, "RootDir") == 0) { + /* don't overwrite a path specified on the command line */ + if(!config->rootdir) { + config->rootdir = strdup(value); + pm_printf(PM_LOG_DEBUG, "config: rootdir: %s\n", value); + } + } else if (strcmp(key, "LogFile") == 0) { + if(!config->logfile) { + config->logfile = strdup(value); + pm_printf(PM_LOG_DEBUG, "config: logfile: %s\n", value); + } + } else if (strcmp(key, "XferCommand") == 0) { + config->xfercommand = strdup(value); + alpm_option_set_fetchcb(download_with_xfercommand); + pm_printf(PM_LOG_DEBUG, "config: xfercommand: %s\n", value); + } else if (strcmp(key, "CleanMethod") == 0) { + if (strcmp(value, "KeepInstalled") == 0) { + config->cleanmethod = PM_CLEAN_KEEPINST; + } else if (strcmp(value, "KeepCurrent") == 0) { + config->cleanmethod = PM_CLEAN_KEEPCUR; + } else { + pm_printf(PM_LOG_ERROR, _("invalid value for 'CleanMethod' : '%s'\n"), value); + return(1); + } + pm_printf(PM_LOG_DEBUG, "config: cleanmethod: %s\n", value); + } else { + pm_printf(PM_LOG_ERROR, _("directive '%s' with a value not recognized\n"), key); + return(1); + } + + } + return(0); +} + +static int _add_mirror(pmdb_t *db, char *value) +{ + const char *dbname = alpm_db_get_name(db); + /* let's attempt a replacement for the current repo */ + char *temp = strreplace(value, "$repo", dbname); + /* let's attempt a replacement for the arch */ + const char *arch = alpm_option_get_arch(); + char *server; + if(arch) { + server = strreplace(temp, "$arch", arch); + free(temp); + } else { + if(strstr(temp, "$arch")) { + free(temp); + pm_printf(PM_LOG_ERROR, _("The mirror '%s' contains the $arch" + " variable, but no Architecture is defined.\n"), value); + return(1); + } + server = temp; + } + + if(alpm_db_setserver(db, server) != 0) { + /* pm_errno is set by alpm_db_setserver */ + pm_printf(PM_LOG_ERROR, _("could not add server URL to database '%s': %s (%s)\n"), + dbname, server, alpm_strerrorlast()); + free(server); + return(1); + } + + free(server); + return(0); +} + /* The real parseconfig. Called with a null section argument by the publicly * visible parseconfig so we can recall from within ourself on an include */ static int _parseconfig(const char *file, const char *givensection, @@ -779,159 +900,70 @@ static int _parseconfig(const char *file, const char *givensection, goto cleanup; } } - } else { - /* directive */ - char *key; - /* strsep modifies the 'line' string: 'key \0 ptr' */ - key = line; - ptr = line; - strsep(&ptr, "="); - strtrim(key); - strtrim(ptr); + continue; + } - if(key == NULL) { - pm_printf(PM_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 == NULL) { - pm_printf(PM_LOG_ERROR, _("config file %s, line %d: All directives must belong to a section.\n"), + /* directive */ + char *key, *value; + /* strsep modifies the 'line' string: 'key \0 value' */ + key = line; + value = line; + strsep(&value, "="); + strtrim(key); + strtrim(value); + + if(key == NULL) { + pm_printf(PM_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 == NULL) { + pm_printf(PM_LOG_ERROR, _("config file %s, line %d: All directives must belong to a section.\n"), + file, linenum); + ret = 1; + goto cleanup; + } + if(strcmp(section, "options") == 0) { + if((ret = _parse_options(key, value)) != 0) { + pm_printf(PM_LOG_ERROR, _("config file %s, line %d: problem in options section\n"), file, linenum); ret = 1; goto cleanup; } - if(ptr == NULL && strcmp(section, "options") == 0) { - /* directives without settings, all in [options] */ - if(strcmp(key, "UseSyslog") == 0) { - alpm_option_set_usesyslog(1); - pm_printf(PM_LOG_DEBUG, "config: usesyslog\n"); - } else if(strcmp(key, "ILoveCandy") == 0) { - config->chomp = 1; - pm_printf(PM_LOG_DEBUG, "config: chomp\n"); - } else if(strcmp(key, "ShowSize") == 0) { - config->showsize = 1; - pm_printf(PM_LOG_DEBUG, "config: showsize\n"); - } else if(strcmp(key, "UseDelta") == 0) { - alpm_option_set_usedelta(1); - pm_printf(PM_LOG_DEBUG, "config: usedelta\n"); - } else if(strcmp(key, "TotalDownload") == 0) { - config->totaldownload = 1; - pm_printf(PM_LOG_DEBUG, "config: totaldownload\n"); - } else { - pm_printf(PM_LOG_ERROR, _("config file %s, line %d: directive '%s' not recognized.\n"), + continue; + } else { + /* we are in a repo section */ + if(strcmp(key, "Include") == 0) { + if(value == NULL) { + pm_printf(PM_LOG_ERROR, _("config file %s, line %d: directive %s needs a value\n"), file, linenum, key); ret = 1; goto cleanup; } - } else { - /* directives with settings */ - if(strcmp(key, "Include") == 0) { - pm_printf(PM_LOG_DEBUG, "config: including %s\n", ptr); - _parseconfig(ptr, section, db); - /* Ignore include failures... assume non-critical */ - } else if(strcmp(section, "options") == 0) { - if(strcmp(key, "NoUpgrade") == 0) { - setrepeatingoption(ptr, "NoUpgrade", alpm_option_add_noupgrade); - } else if(strcmp(key, "NoExtract") == 0) { - setrepeatingoption(ptr, "NoExtract", alpm_option_add_noextract); - } else if(strcmp(key, "IgnorePkg") == 0) { - setrepeatingoption(ptr, "IgnorePkg", alpm_option_add_ignorepkg); - } else if(strcmp(key, "IgnoreGroup") == 0) { - setrepeatingoption(ptr, "IgnoreGroup", alpm_option_add_ignoregrp); - } else if(strcmp(key, "HoldPkg") == 0) { - setrepeatingoption(ptr, "HoldPkg", option_add_holdpkg); - } else if(strcmp(key, "SyncFirst") == 0) { - setrepeatingoption(ptr, "SyncFirst", option_add_syncfirst); - } else if(strcmp(key, "Architecture") == 0) { - if(!alpm_option_get_arch()) { - setarch(ptr); - } - } else if(strcmp(key, "DBPath") == 0) { - /* don't overwrite a path specified on the command line */ - if(!config->dbpath) { - config->dbpath = strdup(ptr); - pm_printf(PM_LOG_DEBUG, "config: dbpath: %s\n", ptr); - } - } else if(strcmp(key, "CacheDir") == 0) { - if(alpm_option_add_cachedir(ptr) != 0) { - pm_printf(PM_LOG_ERROR, _("problem adding cachedir '%s' (%s)\n"), - ptr, alpm_strerrorlast()); - ret = 1; - goto cleanup; - } - pm_printf(PM_LOG_DEBUG, "config: cachedir: %s\n", ptr); - } else if(strcmp(key, "RootDir") == 0) { - /* don't overwrite a path specified on the command line */ - if(!config->rootdir) { - config->rootdir = strdup(ptr); - pm_printf(PM_LOG_DEBUG, "config: rootdir: %s\n", ptr); - } - } else if (strcmp(key, "LogFile") == 0) { - if(!config->logfile) { - config->logfile = strdup(ptr); - pm_printf(PM_LOG_DEBUG, "config: logfile: %s\n", ptr); - } - } else if (strcmp(key, "XferCommand") == 0) { - config->xfercommand = strdup(ptr); - alpm_option_set_fetchcb(download_with_xfercommand); - pm_printf(PM_LOG_DEBUG, "config: xfercommand: %s\n", ptr); - } else if (strcmp(key, "CleanMethod") == 0) { - if (strcmp(ptr, "KeepInstalled") == 0) { - config->cleanmethod = PM_CLEAN_KEEPINST; - } else if (strcmp(ptr, "KeepCurrent") == 0) { - config->cleanmethod = PM_CLEAN_KEEPCUR; - } else { - pm_printf(PM_LOG_ERROR, _("invalid value for 'CleanMethod' : '%s'\n"), ptr); - ret = 1; - goto cleanup; - } - 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); - ret = 1; - goto cleanup; - } - } else if(strcmp(key, "Server") == 0) { - /* let's attempt a replacement for the current repo */ - char *temp = strreplace(ptr, "$repo", section); - /* let's attempt a replacement for the arch */ - const char *arch = alpm_option_get_arch(); - char *server; - if(arch) { - server = strreplace(temp, "$arch", arch); - free(temp); - } else { - if(strstr(temp, "$arch")) { - free(temp); - pm_printf(PM_LOG_ERROR, _("The mirror '%s' contains the $arch" - " variable, but no Architecture is defined.\n"), ptr); - ret = 1; - goto cleanup; - } - server = temp; - } - - if(alpm_db_setserver(db, server) != 0) { - /* pm_errno is set by alpm_db_setserver */ - 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 cleanup; - } - - free(server); - } else { - pm_printf(PM_LOG_ERROR, _("config file %s, line %d: directive '%s' not recognized.\n"), + pm_printf(PM_LOG_DEBUG, "config: including %s\n", value); + _parseconfig(value, section, db); + /* Ignore include failures... assume non-critical */ + } else if(strcmp(key, "Server") == 0) { + if(value == NULL) { + pm_printf(PM_LOG_ERROR, _("config file %s, line %d: directive %s needs a value\n"), file, linenum, key); ret = 1; goto cleanup; } + if(_add_mirror(db, value) != 0) { + ret = 1; + goto cleanup; + } + } else { + pm_printf(PM_LOG_ERROR, _("config file %s, line %d: directive '%s' in repository section '%s' not recognized.\n"), + file, linenum, key, section); + ret = 1; + goto cleanup; } } + } cleanup: -- 1.6.5.4
getopt should already ensure that optarg is not NULL when an argument is required, but just be extra safe and double check it before using optarg. To be honest, I only did that to make clang shut up and eliminate the last warnings it reported. Signed-off-by: Xavier Chantry <shiningxc@gmail.com> --- src/pacman/pacman.c | 16 +++++++++++++++- 1 files changed, 15 insertions(+), 1 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 38fc560..522da1b 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -334,6 +334,8 @@ static void setlibpaths(void) } } +#define check_optarg() if(!optarg) { return(1); } + /** Parse command-line arguments for each operation. * @param argc argc * @param argv argv @@ -408,12 +410,14 @@ static int parseargs(int argc, char *argv[]) case 0: break; case OP_NOCONFIRM: config->noconfirm = 1; break; case OP_CONFIG: + check_optarg(); if(config->configfile) { free(config->configfile); } config->configfile = strndup(optarg, PATH_MAX); break; case OP_IGNORE: + check_optarg(); list = strsplit(optarg, ','); for(item = list; item; item = alpm_list_next(item)) { alpm_option_add_ignorepkg((char *)alpm_list_getdata(item)); @@ -445,8 +449,13 @@ static int parseargs(int argc, char *argv[]) break; case OP_NOPROGRESSBAR: config->noprogressbar = 1; break; case OP_NOSCRIPTLET: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; - case OP_ASK: config->noask = 1; config->ask = atoi(optarg); break; + case OP_ASK: + check_optarg(); + config->noask = 1; + config->ask = atoi(optarg); + break; case OP_CACHEDIR: + check_optarg(); if(alpm_option_add_cachedir(optarg) != 0) { pm_printf(PM_LOG_ERROR, _("problem adding cachedir '%s' (%s)\n"), optarg, alpm_strerrorlast()); @@ -457,9 +466,11 @@ static int parseargs(int argc, char *argv[]) config->flags |= PM_TRANS_FLAG_ALLDEPS; break; case OP_LOGFILE: + check_optarg(); config->logfile = strndup(optarg, PATH_MAX); break; case OP_IGNOREGROUP: + check_optarg(); list = strsplit(optarg, ','); for(item = list; item; item = alpm_list_next(item)) { alpm_option_add_ignoregrp((char *)alpm_list_getdata(item)); @@ -471,6 +482,7 @@ static int parseargs(int argc, char *argv[]) config->flags |= PM_TRANS_FLAG_ALLEXPLICIT; break; case OP_ARCH: + check_optarg(); setarch(optarg); break; case 'Q': config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_QUERY); break; @@ -480,6 +492,7 @@ static int parseargs(int argc, char *argv[]) case 'U': config->op = (config->op != PM_OP_MAIN ? 0 : PM_OP_UPGRADE); break; case 'V': config->version = 1; break; case 'b': + check_optarg(); config->dbpath = strdup(optarg); break; case 'c': @@ -516,6 +529,7 @@ static int parseargs(int argc, char *argv[]) config->quiet = 1; break; case 'r': + check_optarg(); config->rootdir = strdup(optarg); break; case 's': -- 1.6.5.4
participants (1)
-
Xavier Chantry