[pacman-dev] [PATCH 1/2] Fix warnings not usually emitted (CFLAGS not in configure.ac)
-Wbad-function-cast: * casting long int to pmpkgreason_t unecessarily * casting pmpkgreason_t to long unecessarily * casting off_t (signed integral) to float before division unecessarily -Wshadow: * shadowing "handle" variables renamed to "new_handle" and "local_handle" shadowing "filestr" renamed to "pkgfilestr" * shadowing "remove" renamed to "remove_pkgs" * shadowing "sync" renamed to "syncpkg" * Removed redundant declarations * shadowing "prefix" renamed to "entry_prefix" * shadowing "pipe" renamed to "pipe_handle" -Wconversion: * explicitely cast nread to size_t from ssize_t in dload.c (guaranteed not to be negative at this point) * use size_t, as opposed to int, for string length and byte sizes * use signed int in alpm_list_count as advertised by API (should probably be changed to usngiend int or size_t, but requires an API change) Signed-off-by: Sebastian Nowicki <sebnow@gmail.com> --- Some of these warnings are quite pedantic and wouldn't really cause issues, so they might not be desired. The change in alpm_list_count is backwards in my opinion - the API should be changed to use unsigned int (or size_t). I didn't want to change the API though. This doesn't fix all the warnings. There are some where I had doubts about the semantics of the code, and some where the warnings would effectively just be suppressed (mostly regarding casting off_t to double/float for printing file sizes). lib/libalpm/alpm_list.c | 2 +- lib/libalpm/be_files.c | 2 +- lib/libalpm/conflict.c | 8 +++--- lib/libalpm/db.c | 2 +- lib/libalpm/deps.c | 12 +++++----- lib/libalpm/dload.c | 2 +- lib/libalpm/handle.c | 48 +++++++++++++++++++++++----------------------- lib/libalpm/sync.c | 21 +++++++++---------- lib/libalpm/util.c | 24 +++++++++++----------- src/pacman/package.c | 4 +- src/pacman/query.c | 5 +-- src/pacman/sync.c | 4 +-- src/pacman/upgrade.c | 1 - 13 files changed, 65 insertions(+), 70 deletions(-) diff --git a/lib/libalpm/alpm_list.c b/lib/libalpm/alpm_list.c index 80ba1ee..9afa950 100644 --- a/lib/libalpm/alpm_list.c +++ b/lib/libalpm/alpm_list.c @@ -576,7 +576,7 @@ void SYMEXPORT *alpm_list_getdata(const alpm_list_t *node) */ int SYMEXPORT alpm_list_count(const alpm_list_t *list) { - unsigned int i = 0; + int i = 0; const alpm_list_t *lp = list; while(lp) { ++i; diff --git a/lib/libalpm/be_files.c b/lib/libalpm/be_files.c index 4432171..477b1d0 100644 --- a/lib/libalpm/be_files.c +++ b/lib/libalpm/be_files.c @@ -582,7 +582,7 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq) if(fgets(line, sizeof(line), fp) == NULL) { goto error; } - info->reason = (pmpkgreason_t)atol(_alpm_strtrim(line)); + info->reason = atol(_alpm_strtrim(line)); } else if(strcmp(line, "%SIZE%") == 0 || strcmp(line, "%CSIZE%") == 0) { /* NOTE: the CSIZE and SIZE fields both share the "size" field * in the pkginfo_t struct. This can be done b/c CSIZE diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 694c38d..9d6616a 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -400,7 +400,7 @@ static int dir_belongsto_pkg(char *dirpath, pmpkg_t *pkg) * 1: check every target against every target * 2: check every target against the filesystem */ alpm_list_t *_alpm_db_find_fileconflicts(pmdb_t *db, pmtrans_t *trans, - alpm_list_t *upgrade, alpm_list_t *remove) + alpm_list_t *upgrade, alpm_list_t *remove_pkgs) { alpm_list_t *i, *j, *conflicts = NULL; int numtargs = alpm_list_count(upgrade); @@ -495,7 +495,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(pmdb_t *db, pmtrans_t *trans, int resolved_conflict = 0; /* have we acted on this conflict? */ /* Check remove list (will we remove the conflicting local file?) */ - for(k = remove; k && !resolved_conflict; k = k->next) { + for(k = remove_pkgs; k && !resolved_conflict; k = k->next) { pmpkg_t *rempkg = k->data; if(rempkg && alpm_list_find_str(alpm_pkg_get_files(rempkg), filestr)) { _alpm_log(PM_LOG_DEBUG, "local file will be removed, not a conflict: %s\n", filestr); @@ -540,8 +540,8 @@ alpm_list_t *_alpm_db_find_fileconflicts(pmdb_t *db, pmtrans_t *trans, FREE(rpath); continue; } - char *filestr = rpath + strlen(handle->root); - if(alpm_list_find_str(alpm_pkg_get_files(dbpkg),filestr)) { + char *pkgfilestr = rpath + strlen(handle->root); + if(alpm_list_find_str(alpm_pkg_get_files(dbpkg),pkgfilestr)) { resolved_conflict = 1; } free(rpath); diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c index c8a91a2..de297de 100644 --- a/lib/libalpm/db.c +++ b/lib/libalpm/db.c @@ -168,7 +168,7 @@ int SYMEXPORT alpm_db_setserver(pmdb_t *db, const char *url) alpm_list_t *i; int found = 0; char *newurl; - int len = 0; + size_t len = 0; ALPM_LOG_FUNC; diff --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c index fd893a6..ff7af14 100644 --- a/lib/libalpm/deps.c +++ b/lib/libalpm/deps.c @@ -232,12 +232,12 @@ alpm_list_t SYMEXPORT *alpm_deptest(pmdb_t *db, alpm_list_t *targets) * Dependencies can include versions with depmod operators. * @param pkglist the list of local packages * @param reversedeps handles the backward dependencies - * @param remove an alpm_list_t* of packages to be removed + * @param remove_pkgs an alpm_list_t* of packages to be removed * @param upgrade an alpm_list_t* of packages to be upgraded (remove-then-upgrade) * @return an alpm_list_t* of pmpkg_t* of missing_t pointers. */ alpm_list_t SYMEXPORT *alpm_checkdeps(alpm_list_t *pkglist, int reversedeps, - alpm_list_t *remove, alpm_list_t *upgrade) + alpm_list_t *remove_pkgs, alpm_list_t *upgrade) { alpm_list_t *i, *j; alpm_list_t *targets, *dblist = NULL, *modified = NULL; @@ -246,7 +246,7 @@ alpm_list_t SYMEXPORT *alpm_checkdeps(alpm_list_t *pkglist, int reversedeps, ALPM_LOG_FUNC; - targets = alpm_list_join(alpm_list_copy(remove), alpm_list_copy(upgrade)); + targets = alpm_list_join(alpm_list_copy(remove_pkgs), alpm_list_copy(upgrade)); for(i = pkglist; i; i = i->next) { void *pkg = i->data; if(alpm_list_find(targets, pkg, _alpm_pkg_cmp)) { @@ -585,7 +585,7 @@ pmpkg_t *_alpm_resolvedep(pmdepend_t *dep, alpm_list_t *dbs, * searched first for any dependency packages needed to complete the * resolve, and to which will be added any [pkg] and all of its * dependencies not already on the list - * @param remove is the set of packages which will be removed in this + * @param remove_pkgs is the set of packages which will be removed in this * transaction * @param data returns the dependency which could not be satisfied in the * event of an error @@ -596,7 +596,7 @@ pmpkg_t *_alpm_resolvedep(pmdepend_t *dep, alpm_list_t *dbs, */ int _alpm_resolvedeps(alpm_list_t *localpkgs, alpm_list_t *dbs_sync, pmpkg_t *pkg, alpm_list_t *preferred, alpm_list_t **packages, - alpm_list_t *remove, alpm_list_t **data) + alpm_list_t *remove_pkgs, alpm_list_t **data) { alpm_list_t *i, *j; alpm_list_t *targ; @@ -620,7 +620,7 @@ int _alpm_resolvedeps(alpm_list_t *localpkgs, alpm_list_t *dbs_sync, pmpkg_t *pk for(i = alpm_list_last(*packages); i; i = i->next) { pmpkg_t *tpkg = i->data; targ = alpm_list_add(NULL, tpkg); - deps = alpm_checkdeps(localpkgs, 0, remove, targ); + deps = alpm_checkdeps(localpkgs, 0, remove_pkgs, targ); alpm_list_free(targ); for(j = deps; j; j = j->next) { pmdepmissing_t *miss = j->data; diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 9b59f52..7d5a904 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -250,7 +250,7 @@ static int download_internal(const char *url, const char *localpath, while((nread = fetchIO_read(dlf, buffer, PM_DLBUF_LEN)) > 0) { check_stop(); size_t nwritten = 0; - nwritten = fwrite(buffer, 1, nread, localf); + nwritten = fwrite(buffer, 1, (size_t)nread, localf); if((nwritten != (size_t)nread) || ferror(localf)) { pm_errno = PM_ERR_RETRIEVE; _alpm_log(PM_LOG_ERROR, _("error writing to file '%s': %s\n"), diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index aa34cf4..6434044 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -44,48 +44,48 @@ pmhandle_t *handle = NULL; pmhandle_t *_alpm_handle_new() { - pmhandle_t *handle; + pmhandle_t *new_handle; ALPM_LOG_FUNC; - CALLOC(handle, 1, sizeof(pmhandle_t), RET_ERR(PM_ERR_MEMORY, NULL)); - handle->lckfd = -1; + CALLOC(new_handle, 1, sizeof(pmhandle_t), RET_ERR(PM_ERR_MEMORY, NULL)); + new_handle->lckfd = -1; - return(handle); + return(new_handle); } -void _alpm_handle_free(pmhandle_t *handle) +void _alpm_handle_free(pmhandle_t *local_handle) { ALPM_LOG_FUNC; - if(handle == NULL) { + if(local_handle == NULL) { return; } /* close logfile */ - if(handle->logstream) { - fclose(handle->logstream); - handle->logstream= NULL; + if(local_handle->logstream) { + fclose(local_handle->logstream); + local_handle->logstream= NULL; } - if(handle->usesyslog) { - handle->usesyslog = 0; + if(local_handle->usesyslog) { + local_handle->usesyslog = 0; closelog(); } /* free memory */ - _alpm_trans_free(handle->trans); - FREE(handle->root); - FREE(handle->dbpath); - FREELIST(handle->cachedirs); - FREE(handle->logfile); - FREE(handle->lockfile); - FREE(handle->arch); - FREELIST(handle->dbs_sync); - FREELIST(handle->noupgrade); - FREELIST(handle->noextract); - FREELIST(handle->ignorepkg); - FREELIST(handle->ignoregrp); - FREE(handle); + _alpm_trans_free(local_handle->trans); + FREE(local_handle->root); + FREE(local_handle->dbpath); + FREELIST(local_handle->cachedirs); + FREE(local_handle->logfile); + FREE(local_handle->lockfile); + FREE(local_handle->arch); + FREELIST(local_handle->dbs_sync); + FREELIST(local_handle->noupgrade); + FREELIST(local_handle->noextract); + FREELIST(local_handle->ignorepkg); + FREELIST(local_handle->ignoregrp); + FREE(local_handle); } alpm_cb_log SYMEXPORT alpm_option_get_logcb() diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index f819396..fab67c9 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -411,7 +411,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync alpm_list_t *deps = NULL; alpm_list_t *unresolvable = NULL; alpm_list_t *i, *j; - alpm_list_t *remove = NULL; + alpm_list_t *remove_pkgs = NULL; int ret = 0; ALPM_LOG_FUNC; @@ -435,7 +435,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync for(i = trans->add; i; i = i->next) { pmpkg_t *spkg = i->data; for(j = spkg->removes; j; j = j->next) { - remove = alpm_list_add(remove, j->data); + remove_pkgs = alpm_list_add(remove_pkgs, j->data); } } @@ -447,7 +447,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync for(i = trans->add; i; i = i->next) { pmpkg_t *pkg = i->data; if(_alpm_resolvedeps(localpkgs, dbs_sync, pkg, trans->add, - &resolved, remove, data) == -1) { + &resolved, remove_pkgs, data) == -1) { unresolvable = alpm_list_add(unresolvable, pkg); } /* Else, [resolved] now additionally contains [pkg] and all of its @@ -512,7 +512,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync for(i = deps; i; i = i->next) { pmconflict_t *conflict = i->data; - pmpkg_t *rsync, *sync, *sync1, *sync2; + pmpkg_t *rsync, *syncpkg, *sync1, *sync2; /* have we already removed one of the conflicting targets? */ sync1 = _alpm_pkg_find(trans->add, conflict->package1); @@ -529,10 +529,10 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync pmdepend_t *dep2 = _alpm_splitdep(conflict->package2); if(alpm_depcmp(sync1, dep2)) { rsync = sync2; - sync = sync1; + syncpkg = sync1; } else if(alpm_depcmp(sync2, dep1)) { rsync = sync1; - sync = sync2; + syncpkg = sync2; } else { _alpm_log(PM_LOG_ERROR, _("unresolvable package conflicts detected\n")); pm_errno = PM_ERR_CONFLICTING_DEPS; @@ -555,7 +555,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync /* Prints warning */ _alpm_log(PM_LOG_WARNING, _("removing '%s' from target list because it conflicts with '%s'\n"), - rsync->name, sync->name); + rsync->name, syncpkg->name); trans->add = alpm_list_remove(trans->add, rsync, _alpm_pkg_cmp, NULL); _alpm_pkg_free_trans(rsync); /* rsync is not transaction target anymore */ continue; @@ -588,7 +588,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync _alpm_log(PM_LOG_DEBUG, "package '%s' conflicts with '%s'\n", conflict->package1, conflict->package2); - pmpkg_t *sync = _alpm_pkg_find(trans->add, conflict->package1); + pmpkg_t *syncpkg = _alpm_pkg_find(trans->add, conflict->package1); pmpkg_t *local = _alpm_db_get_pkgfromcache(db_local, conflict->package2); int doremove = 0; QUESTION(trans, PM_TRANS_CONV_CONFLICT_PKG, conflict->package1, @@ -596,7 +596,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync if(doremove) { /* append to the removes list */ _alpm_log(PM_LOG_DEBUG, "electing '%s' for removal\n", conflict->package2); - sync->removes = alpm_list_add(sync->removes, local); + syncpkg->removes = alpm_list_add(syncpkg->removes, local); } else { /* abort */ _alpm_log(PM_LOG_ERROR, _("unresolvable package conflicts detected\n")); pm_errno = PM_ERR_CONFLICTING_DEPS; @@ -651,7 +651,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync cleanup: alpm_list_free(unresolvable); - alpm_list_free(remove); + alpm_list_free(remove_pkgs); return(ret); } @@ -894,7 +894,6 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data) /* if we have deltas to work with */ if(handle->usedelta && deltas) { - int ret = 0; errors = 0; /* Check integrity of deltas */ EVENT(trans, PM_TRANS_EVT_DELTA_INTEGRITY_START, NULL, NULL); diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 5bf4ef1..c74649e 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -338,13 +338,13 @@ int _alpm_unpack(const char *archive, const char *prefix, alpm_list_t *list, int /* If specific files were requested, skip entries that don't match. */ if(list) { - char *prefix = strdup(entryname); - char *p = strstr(prefix,"/"); + char *entry_prefix = strdup(entryname); + char *p = strstr(entry_prefix,"/"); if(p) { *(p+1) = '\0'; } - char *found = alpm_list_find_str(list, prefix); - free(prefix); + char *found = alpm_list_find_str(list, entry_prefix); + free(entry_prefix); if(!found) { if (archive_read_data_skip(_archive) != ARCHIVE_OK) { ret = 1; @@ -524,22 +524,22 @@ int _alpm_run_chroot(const char *root, const char *path, char *const argv[]) } else { /* this code runs for the parent only (wait on the child) */ int status; - FILE *pipe; + FILE *pipe_handle; close(pipefd[1]); - pipe = fdopen(pipefd[0], "r"); - if(pipe == NULL) { + pipe_handle = fdopen(pipefd[0], "r"); + if(pipe_handle == NULL) { close(pipefd[0]); retval = 1; } else { - while(!feof(pipe)) { + while(!feof(pipe_handle)) { char line[PATH_MAX]; - if(fgets(line, PATH_MAX, pipe) == NULL) + if(fgets(line, PATH_MAX, pipe_handle) == NULL) break; alpm_logaction("%s", line); EVENT(handle->trans, PM_TRANS_EVT_SCRIPTLET_INFO, line, NULL); } - fclose(pipe); + fclose(pipe_handle); } while(waitpid(pid, &status, 0) == -1) { @@ -668,7 +668,7 @@ int _alpm_lstat(const char *path, struct stat *buf) { int ret; char *newpath = strdup(path); - int len = strlen(newpath); + size_t len = strlen(newpath); /* strip the trailing slash if one exists */ if(len != 0 && newpath[len - 1] == '/') { @@ -781,7 +781,7 @@ char *_alpm_archive_fgets(char *line, size_t size, struct archive *a) char *i; for(i = line; i < last; i++) { - int ret = archive_read_data(a, i, 1); + ssize_t ret = archive_read_data(a, i, 1); /* special check for first read- if null, return null, * this indicates EOF */ if(i == line && (ret <= 0 || *i == '\0')) { diff --git a/src/pacman/package.c b/src/pacman/package.c index ac84a0c..2b1faae 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -67,7 +67,7 @@ void dump_pkg_full(pmpkg_t *pkg, int level) strftime(idatestr, 50, "%c", localtime(&idate)); } - switch((long)alpm_pkg_get_reason(pkg)) { + switch(alpm_pkg_get_reason(pkg)) { case PM_PKG_REASON_EXPLICIT: reason = _("Explicitly installed"); break; @@ -242,7 +242,7 @@ void dump_pkg_changelog(pmpkg_t *pkg) } else { /* allocate a buffer to get the changelog back in chunks */ char buf[CLBUF_SIZE]; - int ret = 0; + size_t ret = 0; while((ret = alpm_pkg_changelog_read(buf, CLBUF_SIZE, pkg, fp))) { if(ret < CLBUF_SIZE) { /* if we hit the end of the file, we need to add a null terminator */ diff --git a/src/pacman/query.c b/src/pacman/query.c index 5538e81..740da82 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -266,7 +266,6 @@ static int query_search(alpm_list_t *targets) static int query_group(alpm_list_t *targets) { alpm_list_t *i, *j; - char *grpname = NULL; int ret = 0; if(targets == NULL) { for(j = alpm_db_get_grpcache(db_local); j; j = alpm_list_next(j)) { @@ -283,8 +282,8 @@ static int query_group(alpm_list_t *targets) } } else { for(i = targets; i; i = alpm_list_next(i)) { + char *grpname = alpm_list_getdata(i); pmgrp_t *grp; - grpname = alpm_list_getdata(i); grp = alpm_db_readgrp(db_local, grpname); if(grp) { const alpm_list_t *p, *packages = alpm_grp_get_pkgs(grp); @@ -369,7 +368,7 @@ static int check(pmpkg_t *pkg) { alpm_list_t *i; const char *root; - int allfiles = 0, errors = 0; + unsigned int allfiles = 0, errors = 0; size_t rootlen; char f[PATH_MAX]; diff --git a/src/pacman/sync.c b/src/pacman/sync.c index b9497d6..8970b5d 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -468,7 +468,7 @@ static int sync_info(alpm_list_t *syncs, alpm_list_t *targets) pkgstr = target; for(j = syncs; j; j = alpm_list_next(j)) { - pmdb_t *db = alpm_list_getdata(j); + db = alpm_list_getdata(j); for(k = alpm_db_get_pkgcache(db); k; k = alpm_list_next(k)) { pmpkg_t *pkg = alpm_list_getdata(k); @@ -642,7 +642,6 @@ static int sync_trans(alpm_list_t *targets) pm_fprintf(stderr, PM_LOG_ERROR, _("failed to prepare transaction (%s)\n"), alpm_strerrorlast()); switch(pm_errno) { - alpm_list_t *i; case PM_ERR_PKG_INVALID_ARCH: for(i = data; i; i = alpm_list_next(i)) { char *pkg = alpm_list_getdata(i); @@ -711,7 +710,6 @@ static int sync_trans(alpm_list_t *targets) pm_fprintf(stderr, PM_LOG_ERROR, _("failed to commit transaction (%s)\n"), alpm_strerrorlast()); switch(pm_errno) { - alpm_list_t *i; case PM_ERR_FILE_CONFLICTS: for(i = data; i; i = alpm_list_next(i)) { pmfileconflict_t *conflict = alpm_list_getdata(i); diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index c9c8301..fcd1295 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -155,7 +155,6 @@ int pacman_upgrade(alpm_list_t *targets) pm_fprintf(stderr, PM_LOG_ERROR, _("failed to commit transaction (%s)\n"), alpm_strerrorlast()); switch(pm_errno) { - alpm_list_t *i; case PM_ERR_FILE_CONFLICTS: for(i = data; i; i = alpm_list_next(i)) { pmfileconflict_t *conflict = alpm_list_getdata(i); -- 1.7.2.2
Signed-off-by: Sebastian Nowicki <sebnow@gmail.com> --- lib/libalpm/package.c | 16 +++++++++++++--- 1 files changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/libalpm/package.c b/lib/libalpm/package.c index 0060300..717d32c 100644 --- a/lib/libalpm/package.c +++ b/lib/libalpm/package.c @@ -488,12 +488,15 @@ void SYMEXPORT *alpm_pkg_changelog_open(pmpkg_t *pkg) /** * Read data from an open changelog 'file stream'. Similar to fread in - * functionality, this function takes a buffer and amount of data to read. + * functionality, this function takes a buffer and amount of data to read. If an + * error occurs pm_errno will be set. + * * @param ptr a buffer to fill with raw changelog data * @param size the size of the buffer * @param pkg the package that the changelog is being read from * @param fp a 'file stream' to the package changelog - * @return the number of characters read, or 0 if there is no more data + * @return the number of characters read, or 0 if there is no more data or an + * error occurred. */ size_t SYMEXPORT alpm_pkg_changelog_read(void *ptr, size_t size, const pmpkg_t *pkg, const void *fp) @@ -502,7 +505,14 @@ size_t SYMEXPORT alpm_pkg_changelog_read(void *ptr, size_t size, if(pkg->origin == PKG_FROM_CACHE) { ret = fread(ptr, 1, size, (FILE*)fp); } else if(pkg->origin == PKG_FROM_FILE) { - ret = archive_read_data((struct archive*)fp, ptr, size); + ssize_t sret = archive_read_data((struct archive*)fp, ptr, size); + /* Report error (negative values) */ + if(sret < 0) { + pm_errno = PM_ERR_LIBARCHIVE; + ret = 0; + } else { + ret = (size_t)sret; + } } return(ret); } -- 1.7.2.2
On Sun, Sep 12, 2010 at 9:01 AM, Sebastian Nowicki <sebnow@gmail.com> wrote:
Signed-off-by: Sebastian Nowicki <sebnow@gmail.com>
Looks good, thanks!
--- lib/libalpm/package.c | 16 +++++++++++++--- 1 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/lib/libalpm/package.c b/lib/libalpm/package.c index 0060300..717d32c 100644 --- a/lib/libalpm/package.c +++ b/lib/libalpm/package.c @@ -488,12 +488,15 @@ void SYMEXPORT *alpm_pkg_changelog_open(pmpkg_t *pkg)
/** * Read data from an open changelog 'file stream'. Similar to fread in - * functionality, this function takes a buffer and amount of data to read. + * functionality, this function takes a buffer and amount of data to read. If an + * error occurs pm_errno will be set. + * * @param ptr a buffer to fill with raw changelog data * @param size the size of the buffer * @param pkg the package that the changelog is being read from * @param fp a 'file stream' to the package changelog - * @return the number of characters read, or 0 if there is no more data + * @return the number of characters read, or 0 if there is no more data or an + * error occurred. */ size_t SYMEXPORT alpm_pkg_changelog_read(void *ptr, size_t size, const pmpkg_t *pkg, const void *fp) @@ -502,7 +505,14 @@ size_t SYMEXPORT alpm_pkg_changelog_read(void *ptr, size_t size, if(pkg->origin == PKG_FROM_CACHE) { ret = fread(ptr, 1, size, (FILE*)fp); } else if(pkg->origin == PKG_FROM_FILE) { - ret = archive_read_data((struct archive*)fp, ptr, size); + ssize_t sret = archive_read_data((struct archive*)fp, ptr, size); + /* Report error (negative values) */ + if(sret < 0) { + pm_errno = PM_ERR_LIBARCHIVE; + ret = 0; + } else { + ret = (size_t)sret; + } } return(ret); } -- 1.7.2.2
On 13/09/10 00:01, Sebastian Nowicki wrote:
-Wbad-function-cast: * casting long int to pmpkgreason_t unecessarily * casting pmpkgreason_t to long unecessarily * casting off_t (signed integral) to float before division unecessarily
-Wshadow: * shadowing "handle" variables renamed to "new_handle" and "local_handle" shadowing "filestr" renamed to "pkgfilestr" * shadowing "remove" renamed to "remove_pkgs" * shadowing "sync" renamed to "syncpkg" * Removed redundant declarations * shadowing "prefix" renamed to "entry_prefix" * shadowing "pipe" renamed to "pipe_handle"
-Wconversion: * explicitely cast nread to size_t from ssize_t in dload.c (guaranteed not to be negative at this point) * use size_t, as opposed to int, for string length and byte sizes * use signed int in alpm_list_count as advertised by API (should probably be changed to usngiend int or size_t, but requires an API change)
Signed-off-by: Sebastian Nowicki<sebnow@gmail.com>
I tried having a look at this but kept getting lost in what part of the patch fixed what warnings. e.g. I can not find the casting off_t to float change for -Wbad-function-cast. I think it would be much better to split these up a bit more. Splitting by error type would be fine for me.
--- Some of these warnings are quite pedantic and wouldn't really cause issues, so they might not be desired.
The change in alpm_list_count is backwards in my opinion - the API should be changed to use unsigned int (or size_t). I didn't want to change the API though.
I can not see why this ever returned a signed int. Allan
On Wed, Sep 15, 2010 at 8:07 AM, Allan McRae <allan@archlinux.org> wrote:
On 13/09/10 00:01, Sebastian Nowicki wrote:
-Wbad-function-cast: * casting long int to pmpkgreason_t unecessarily * casting pmpkgreason_t to long unecessarily * casting off_t (signed integral) to float before division unecessarily
-Wshadow: * shadowing "handle" variables renamed to "new_handle" and "local_handle" shadowing "filestr" renamed to "pkgfilestr" * shadowing "remove" renamed to "remove_pkgs" * shadowing "sync" renamed to "syncpkg" * Removed redundant declarations * shadowing "prefix" renamed to "entry_prefix" * shadowing "pipe" renamed to "pipe_handle"
-Wconversion: * explicitely cast nread to size_t from ssize_t in dload.c (guaranteed not to be negative at this point) * use size_t, as opposed to int, for string length and byte sizes * use signed int in alpm_list_count as advertised by API (should probably be changed to usngiend int or size_t, but requires an API change)
Signed-off-by: Sebastian Nowicki<sebnow@gmail.com>
I tried having a look at this but kept getting lost in what part of the patch fixed what warnings. e.g. I can not find the casting off_t to float change for -Wbad-function-cast.
I think it would be much better to split these up a bit more. Splitting by error type would be fine for me.
+1; if you could do one patch for each warning that would be awesome. I know it is a bit more work but it makes it a lot easier to see what you've done (as well as prevent it from sneaking in again in the future).
--- Some of these warnings are quite pedantic and wouldn't really cause issues, so they might not be desired.
The change in alpm_list_count is backwards in my opinion - the API should be changed to use unsigned int (or size_t). I didn't want to change the API though.
I can not see why this ever returned a signed int.
This should really be size_t, just like strlen(), etc. If we want to change it in a major version release, we haven't shied away from it in the past, so I'd be for it. In most cases it wouldn't break code or built applications anyway. -Dan
The following have been renamed to avoid shadowing: * conflict.c - filestr -> pkgfilestr - remove -> remove_pkgs * deps.c - index -> pkg_index - remove -> remove_pkgs - time -> epoch * diskspace.c - abort -> should_abort * handle.c - handle -> local_handle / new_handle * package.c - i -> deps * sync.c (alpm) - current -> current_pkg - remove -> remove_pkgs - ret -> gpg_ret - sync -> syncpkg * trans.c - handle -> local_handle * util.c (alpm) - pipe -> pipefh - prefix -> entry_prefix * callback.c - p -> ptr * util.c (pacman) - dup -> p In some instance duplicate declarations were removed. Signed-off-by: Sebastian Nowicki <sebnow@gmail.com> --- This is a follow up for an earlier patch I sent back in December. Apologies for not following up sooner. The feedback was to split the patch up. This is the first patch from the split. The API for alpm_checkdeps is slightly modified (a parameter is renamed). I'm not sure whether this is a backwards incompatible change or not. Sorry for the size, I don't think it can be split up much further without it turning into a dozen patches. This patch was generated against the master branch. lib/libalpm/Makefile.am | 2 +- lib/libalpm/alpm.h | 2 +- lib/libalpm/conflict.c | 8 +++--- lib/libalpm/deps.c | 18 ++++++++-------- lib/libalpm/diskspace.c | 8 +++--- lib/libalpm/dload.c | 6 ++-- lib/libalpm/handle.c | 50 +++++++++++++++++++++++----------------------- lib/libalpm/package.c | 6 ++-- lib/libalpm/sync.c | 37 ++++++++++++++++----------------- lib/libalpm/trans.c | 18 ++++++++-------- lib/libalpm/util.c | 20 +++++++++--------- src/pacman/Makefile.am | 2 +- src/pacman/callback.c | 10 ++++---- src/pacman/query.c | 2 +- src/pacman/sync.c | 4 +-- src/pacman/upgrade.c | 1 - src/pacman/util.c | 14 ++++++------ 17 files changed, 102 insertions(+), 106 deletions(-) diff --git a/lib/libalpm/Makefile.am b/lib/libalpm/Makefile.am index fb224a5..21f7057 100644 --- a/lib/libalpm/Makefile.am +++ b/lib/libalpm/Makefile.am @@ -7,7 +7,7 @@ include_HEADERS = alpm_list.h alpm.h DEFS = -DLOCALEDIR=\"@localedir@\" @DEFS@ -AM_CFLAGS = -pedantic -D_GNU_SOURCE +AM_CFLAGS = -pedantic -Wshadow -D_GNU_SOURCE if ENABLE_VISIBILITY_CC if DARWIN diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index b08191d..1b08fca 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -457,7 +457,7 @@ typedef enum _pmdepmod_t { } pmdepmod_t; alpm_list_t *alpm_checkdeps(alpm_list_t *pkglist, int reversedeps, - alpm_list_t *remove, alpm_list_t *upgrade); + alpm_list_t *remove_pkgs, alpm_list_t *upgrade); pmpkg_t *alpm_find_satisfier(alpm_list_t *pkgs, const char *depstring); pmpkg_t *alpm_find_dbs_satisfier(alpm_list_t *dbs, const char *depstring); diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index b4ecd65..f18e3d4 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -382,7 +382,7 @@ static int dir_belongsto_pkg(char *dirpath, pmpkg_t *pkg) * 1: check every target against every target * 2: check every target against the filesystem */ alpm_list_t *_alpm_db_find_fileconflicts(pmdb_t *db, pmtrans_t *trans, - alpm_list_t *upgrade, alpm_list_t *remove) + alpm_list_t *upgrade, alpm_list_t *remove_pkgs) { alpm_list_t *i, *j, *conflicts = NULL; size_t numtargs = alpm_list_count(upgrade); @@ -477,7 +477,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(pmdb_t *db, pmtrans_t *trans, int resolved_conflict = 0; /* have we acted on this conflict? */ /* Check remove list (will we remove the conflicting local file?) */ - for(k = remove; k && !resolved_conflict; k = k->next) { + for(k = remove_pkgs; k && !resolved_conflict; k = k->next) { pmpkg_t *rempkg = k->data; if(rempkg && alpm_list_find_str(alpm_pkg_get_files(rempkg), filestr)) { _alpm_log(PM_LOG_DEBUG, "local file will be removed, not a conflict: %s\n", filestr); @@ -522,8 +522,8 @@ alpm_list_t *_alpm_db_find_fileconflicts(pmdb_t *db, pmtrans_t *trans, FREE(rpath); continue; } - char *filestr = rpath + strlen(handle->root); - if(alpm_list_find_str(alpm_pkg_get_files(dbpkg),filestr)) { + char *pkgfilestr = rpath + strlen(handle->root); + if(alpm_list_find_str(alpm_pkg_get_files(dbpkg), pkgfilestr)) { resolved_conflict = 1; } free(rpath); diff --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c index ee95c89..1d5eb0c 100644 --- a/lib/libalpm/deps.c +++ b/lib/libalpm/deps.c @@ -259,12 +259,12 @@ pmpkg_t SYMEXPORT *alpm_find_satisfier(alpm_list_t *pkgs, const char *depstring) * Dependencies can include versions with depmod operators. * @param pkglist the list of local packages * @param reversedeps handles the backward dependencies - * @param remove an alpm_list_t* of packages to be removed + * @param remove_pkgs an alpm_list_t* of packages to be removed * @param upgrade an alpm_list_t* of packages to be upgraded (remove-then-upgrade) * @return an alpm_list_t* of pmpkg_t* of pmdepmissing_t pointers. */ alpm_list_t SYMEXPORT *alpm_checkdeps(alpm_list_t *pkglist, int reversedeps, - alpm_list_t *remove, alpm_list_t *upgrade) + alpm_list_t *remove_pkgs, alpm_list_t *upgrade) { alpm_list_t *i, *j; alpm_list_t *targets, *dblist = NULL, *modified = NULL; @@ -273,7 +273,7 @@ alpm_list_t SYMEXPORT *alpm_checkdeps(alpm_list_t *pkglist, int reversedeps, ALPM_LOG_FUNC; - targets = alpm_list_join(alpm_list_copy(remove), alpm_list_copy(upgrade)); + targets = alpm_list_join(alpm_list_copy(remove_pkgs), alpm_list_copy(upgrade)); for(i = pkglist; i; i = i->next) { pmpkg_t *pkg = i->data; if(_alpm_pkg_find(targets, pkg->name)) { @@ -621,14 +621,14 @@ static pmpkg_t *resolvedep(pmdepend_t *dep, alpm_list_t *dbs, count = alpm_list_count(providers); if (count >= 1) { /* default to first provider if there is no QUESTION callback */ - int index = 0; + int pkg_index = 0; if(count > 1) { /* if there is more than one provider, we ask the user */ QUESTION(handle->trans, PM_TRANS_CONV_SELECT_PROVIDER, - providers, dep, NULL, &index); + providers, dep, NULL, &pkg_index); } - if(index >= 0 && index < count) { - pmpkg_t *pkg = alpm_list_getdata(alpm_list_nth(providers, index)); + if(pkg_index >= 0 && pkg_index < count) { + pmpkg_t *pkg = alpm_list_getdata(alpm_list_nth(providers, pkg_index)); alpm_list_free(providers); return pkg; } @@ -688,7 +688,7 @@ pmpkg_t SYMEXPORT *alpm_find_dbs_satisfier(alpm_list_t *dbs, const char *depstri */ int _alpm_resolvedeps(alpm_list_t *localpkgs, alpm_list_t *dbs_sync, pmpkg_t *pkg, alpm_list_t *preferred, alpm_list_t **packages, - alpm_list_t *remove, alpm_list_t **data) + alpm_list_t *remove_pkgs, alpm_list_t **data) { int ret = 0; alpm_list_t *i, *j; @@ -713,7 +713,7 @@ int _alpm_resolvedeps(alpm_list_t *localpkgs, alpm_list_t *dbs_sync, pmpkg_t *pk for(i = alpm_list_last(*packages); i; i = i->next) { pmpkg_t *tpkg = i->data; targ = alpm_list_add(NULL, tpkg); - deps = alpm_checkdeps(localpkgs, 0, remove, targ); + deps = alpm_checkdeps(localpkgs, 0, remove_pkgs, targ); alpm_list_free(targ); for(j = deps; j; j = j->next) { diff --git a/lib/libalpm/diskspace.c b/lib/libalpm/diskspace.c index 983a3ac..8141d3e 100644 --- a/lib/libalpm/diskspace.c +++ b/lib/libalpm/diskspace.c @@ -262,7 +262,7 @@ int _alpm_check_diskspace(pmtrans_t *trans, pmdb_t *db_local) alpm_list_t *mount_points, *i; alpm_mountpoint_t *root_mp; size_t replaces = 0, current = 0, numtargs; - int abort = 0; + int should_abort = 0; alpm_list_t *targ; numtargs = alpm_list_count(trans->add); @@ -322,7 +322,7 @@ int _alpm_check_diskspace(pmtrans_t *trans, pmdb_t *db_local) if(data->used && data->read_only) { _alpm_log(PM_LOG_ERROR, _("Partition %s is mounted read only\n"), data->mount_dir); - abort = 1; + should_abort = 1; } else if(data->used & USED_INSTALL) { /* cushion is roughly min(5% capacity, 20MiB) */ long fivepc = ((long)data->fsp.f_blocks / 20) + 1; @@ -337,7 +337,7 @@ int _alpm_check_diskspace(pmtrans_t *trans, pmdb_t *db_local) _alpm_log(PM_LOG_ERROR, _("Partition %s too full: %ld blocks needed, %ld blocks free\n"), data->mount_dir, data->max_blocks_needed + cushion, (unsigned long)data->fsp.f_bfree); - abort = 1; + should_abort = 1; } } } @@ -348,7 +348,7 @@ int _alpm_check_diskspace(pmtrans_t *trans, pmdb_t *db_local) } FREELIST(mount_points); - if(abort) { + if(should_abort) { RET_ERR(PM_ERR_DISK_SPACE, -1); } diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index d9e9488..a8d7ec0 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -142,12 +142,12 @@ static int curl_gethost(const char *url, char *buffer) return 0; } -static int utimes_long(const char *path, long time) +static int utimes_long(const char *path, long epoch) { - if(time != -1) { + if(epoch != -1) { struct timeval tv[2]; memset(&tv, 0, sizeof(tv)); - tv[0].tv_sec = tv[1].tv_sec = time; + tv[0].tv_sec = tv[1].tv_sec = epoch; return utimes(path, tv); } return 0; diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index fd40f19..0a5d031 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -44,53 +44,53 @@ pmhandle_t *handle = NULL; pmhandle_t *_alpm_handle_new() { - pmhandle_t *handle; + pmhandle_t *new_handle; ALPM_LOG_FUNC; - CALLOC(handle, 1, sizeof(pmhandle_t), RET_ERR(PM_ERR_MEMORY, NULL)); + CALLOC(new_handle, 1, sizeof(pmhandle_t), RET_ERR(PM_ERR_MEMORY, NULL)); - return handle; + return new_handle; } -void _alpm_handle_free(pmhandle_t *handle) +void _alpm_handle_free(pmhandle_t *local_handle) { ALPM_LOG_FUNC; - if(handle == NULL) { + if(local_handle == NULL) { return; } /* close logfile */ - if(handle->logstream) { - fclose(handle->logstream); - handle->logstream= NULL; + if(local_handle->logstream) { + fclose(local_handle->logstream); + local_handle->logstream= NULL; } - if(handle->usesyslog) { - handle->usesyslog = 0; + if(local_handle->usesyslog) { + local_handle->usesyslog = 0; closelog(); } #ifdef HAVE_LIBCURL /* release curl handle */ - curl_easy_cleanup(handle->curl); + curl_easy_cleanup(local_handle->curl); #endif /* free memory */ - _alpm_trans_free(handle->trans); - FREE(handle->root); - FREE(handle->dbpath); - FREELIST(handle->cachedirs); - FREE(handle->logfile); - FREE(handle->lockfile); - FREE(handle->arch); - FREE(handle->signaturedir); - FREELIST(handle->dbs_sync); - FREELIST(handle->noupgrade); - FREELIST(handle->noextract); - FREELIST(handle->ignorepkg); - FREELIST(handle->ignoregrp); - FREE(handle); + _alpm_trans_free(local_handle->trans); + FREE(local_handle->root); + FREE(local_handle->dbpath); + FREELIST(local_handle->cachedirs); + FREE(local_handle->logfile); + FREE(local_handle->lockfile); + FREE(local_handle->arch); + FREE(local_handle->signaturedir); + FREELIST(local_handle->dbs_sync); + FREELIST(local_handle->noupgrade); + FREELIST(local_handle->noextract); + FREELIST(local_handle->ignorepkg); + FREELIST(local_handle->ignoregrp); + FREE(local_handle); } diff --git a/lib/libalpm/package.c b/lib/libalpm/package.c index 69c2b85..3da34c3 100644 --- a/lib/libalpm/package.c +++ b/lib/libalpm/package.c @@ -377,9 +377,9 @@ static void find_requiredby(pmpkg_t *pkg, pmdb_t *db, alpm_list_t **reqs) const alpm_list_t *i; for(i = _alpm_db_get_pkgcache(db); i; i = i->next) { pmpkg_t *cachepkg = i->data; - alpm_list_t *i; - for(i = alpm_pkg_get_depends(cachepkg); i; i = i->next) { - if(_alpm_depcmp(pkg, i->data)) { + alpm_list_t *deps; + for(deps = alpm_pkg_get_depends(cachepkg); deps; deps = deps->next) { + if(_alpm_depcmp(pkg, deps->data)) { const char *cachepkgname = cachepkg->name; if(alpm_list_find_str(*reqs, cachepkgname) == NULL) { *reqs = alpm_list_add(*reqs, strdup(cachepkgname)); diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 5428e40..1ddc8e0 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -304,7 +304,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync alpm_list_t *deps = NULL; alpm_list_t *unresolvable = NULL; alpm_list_t *i, *j; - alpm_list_t *remove = NULL; + alpm_list_t *remove_pkgs = NULL; int ret = 0; ALPM_LOG_FUNC; @@ -328,7 +328,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync for(i = trans->add; i; i = i->next) { pmpkg_t *spkg = i->data; for(j = spkg->removes; j; j = j->next) { - remove = alpm_list_add(remove, j->data); + remove_pkgs = alpm_list_add(remove_pkgs, j->data); } } @@ -342,7 +342,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync for(i = trans->add; i; i = i->next) { pmpkg_t *pkg = i->data; if(_alpm_resolvedeps(localpkgs, dbs_sync, pkg, trans->add, - &resolved, remove, data) == -1) { + &resolved, remove_pkgs, data) == -1) { unresolvable = alpm_list_add(unresolvable, pkg); } /* Else, [resolved] now additionally contains [pkg] and all of its @@ -407,7 +407,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync for(i = deps; i; i = i->next) { pmconflict_t *conflict = i->data; - pmpkg_t *rsync, *sync, *sync1, *sync2; + pmpkg_t *rsync, *syncpkg, *sync1, *sync2; /* have we already removed one of the conflicting targets? */ sync1 = _alpm_pkg_find(trans->add, conflict->package1); @@ -424,10 +424,10 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync pmdepend_t *dep2 = _alpm_splitdep(conflict->package2); if(_alpm_depcmp(sync1, dep2)) { rsync = sync2; - sync = sync1; + syncpkg = sync1; } else if(_alpm_depcmp(sync2, dep1)) { rsync = sync1; - sync = sync2; + syncpkg = sync2; } else { _alpm_log(PM_LOG_ERROR, _("unresolvable package conflicts detected\n")); pm_errno = PM_ERR_CONFLICTING_DEPS; @@ -450,7 +450,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync /* Prints warning */ _alpm_log(PM_LOG_WARNING, _("removing '%s' from target list because it conflicts with '%s'\n"), - rsync->name, sync->name); + rsync->name, syncpkg->name); trans->add = alpm_list_remove(trans->add, rsync, _alpm_pkg_cmp, NULL); _alpm_pkg_free_trans(rsync); /* rsync is not transaction target anymore */ continue; @@ -483,7 +483,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync _alpm_log(PM_LOG_DEBUG, "package '%s' conflicts with '%s'\n", conflict->package1, conflict->package2); - pmpkg_t *sync = _alpm_pkg_find(trans->add, conflict->package1); + pmpkg_t *syncpkg = _alpm_pkg_find(trans->add, conflict->package1); pmpkg_t *local = _alpm_db_get_pkgfromcache(db_local, conflict->package2); int doremove = 0; QUESTION(trans, PM_TRANS_CONV_CONFLICT_PKG, conflict->package1, @@ -491,7 +491,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync if(doremove) { /* append to the removes list */ _alpm_log(PM_LOG_DEBUG, "electing '%s' for removal\n", conflict->package2); - sync->removes = alpm_list_add(sync->removes, local); + syncpkg->removes = alpm_list_add(syncpkg->removes, local); } else { /* abort */ _alpm_log(PM_LOG_ERROR, _("unresolvable package conflicts detected\n")); pm_errno = PM_ERR_CONFLICTING_DEPS; @@ -550,7 +550,7 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync cleanup: alpm_list_free(unresolvable); - alpm_list_free(remove); + alpm_list_free(remove_pkgs); return ret; } @@ -718,12 +718,12 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data) /* group sync records by repository and download */ for(i = handle->dbs_sync; i; i = i->next) { - pmdb_t *current = i->data; + pmdb_t *current_pkg = i->data; for(j = trans->add; j; j = j->next) { pmpkg_t *spkg = j->data; - if(spkg->origin != PKG_FROM_FILE && current == spkg->origin_data.db) { + if(spkg->origin != PKG_FROM_FILE && current_pkg == spkg->origin_data.db) { const char *fname = NULL; fname = alpm_pkg_get_filename(spkg); @@ -757,12 +757,12 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data) } if(files) { - EVENT(trans, PM_TRANS_EVT_RETRIEVE_START, current->treename, NULL); - errors = _alpm_download_files(files, current->servers, cachedir); + EVENT(trans, PM_TRANS_EVT_RETRIEVE_START, current_pkg->treename, NULL); + errors = _alpm_download_files(files, current_pkg->servers, cachedir); if (errors) { _alpm_log(PM_LOG_WARNING, _("failed to retrieve some files from %s\n"), - current->treename); + current_pkg->treename); if(pm_errno == 0) { pm_errno = PM_ERR_RETRIEVE; } @@ -785,7 +785,6 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data) /* if we have deltas to work with */ if(handle->usedelta && deltas) { - int ret = 0; errors = 0; /* Check integrity of deltas */ EVENT(trans, PM_TRANS_EVT_DELTA_INTEGRITY_START, NULL, NULL); @@ -850,9 +849,9 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data) pmdb_t *sdb = alpm_pkg_get_db(spkg); if(sdb->pgp_verify != PM_PGP_VERIFY_NEVER) { - int ret = _alpm_gpgme_checksig(filepath, pgpsig); - if((sdb->pgp_verify == PM_PGP_VERIFY_ALWAYS && ret != 0) || - (sdb->pgp_verify == PM_PGP_VERIFY_OPTIONAL && ret == 1)) { + int gpg_ret = _alpm_gpgme_checksig(filepath, pgpsig); + if((sdb->pgp_verify == PM_PGP_VERIFY_ALWAYS && gpg_ret != 0) || + (sdb->pgp_verify == PM_PGP_VERIFY_OPTIONAL && gpg_ret == 1)) { errors++; *data = alpm_list_add(*data, strdup(filename)); FREE(filepath); diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index 8125419..0390732 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -53,13 +53,13 @@ */ /* Create a lock file */ -static int make_lock(pmhandle_t *handle) +static int make_lock(pmhandle_t *local_handle) { int fd; char *dir, *ptr; /* create the dir of the lockfile first */ - dir = strdup(handle->lockfile); + dir = strdup(local_handle->lockfile); ptr = strrchr(dir, '/'); if(ptr) { *ptr = '\0'; @@ -71,27 +71,27 @@ static int make_lock(pmhandle_t *handle) FREE(dir); do { - fd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL, 0000); + fd = open(local_handle->lockfile, O_WRONLY | O_CREAT | O_EXCL, 0000); } while (fd == -1 && errno == EINTR); if(fd > 0) { FILE *f = fdopen(fd, "w"); fprintf(f, "%ld\n", (long)getpid()); fflush(f); fsync(fd); - handle->lckstream = f; + local_handle->lckstream = f; return 0; } return -1; } /* Remove a lock file */ -static int remove_lock(pmhandle_t *handle) +static int remove_lock(pmhandle_t *local_handle) { - if(handle->lckstream != NULL) { - fclose(handle->lckstream); - handle->lckstream = NULL; + if(local_handle->lckstream != NULL) { + fclose(local_handle->lckstream); + local_handle->lckstream = NULL; } - if(unlink(handle->lockfile) == -1 && errno != ENOENT) { + if(unlink(local_handle->lockfile) == -1 && errno != ENOENT) { return -1; } return 0; diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 660d4eb..86e31b6 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -298,13 +298,13 @@ int _alpm_unpack(const char *archive, const char *prefix, alpm_list_t *list, int /* If specific files were requested, skip entries that don't match. */ if(list) { - char *prefix = strdup(entryname); - char *p = strstr(prefix,"/"); + char *entry_prefix = strdup(entryname); + char *p = strstr(entry_prefix,"/"); if(p) { *(p+1) = '\0'; } - char *found = alpm_list_find_str(list, prefix); - free(prefix); + char *found = alpm_list_find_str(list, entry_prefix); + free(entry_prefix); if(!found) { if (archive_read_data_skip(_archive) != ARCHIVE_OK) { ret = 1; @@ -485,22 +485,22 @@ int _alpm_run_chroot(const char *root, const char *path, char *const argv[]) } else { /* this code runs for the parent only (wait on the child) */ int status; - FILE *pipe; + FILE *pipefh; close(pipefd[1]); - pipe = fdopen(pipefd[0], "r"); - if(pipe == NULL) { + pipefh = fdopen(pipefd[0], "r"); + if(pipefh == NULL) { close(pipefd[0]); retval = 1; } else { - while(!feof(pipe)) { + while(!feof(pipefh)) { char line[PATH_MAX]; - if(fgets(line, PATH_MAX, pipe) == NULL) + if(fgets(line, PATH_MAX, pipefh) == NULL) break; alpm_logaction("%s", line); EVENT(handle->trans, PM_TRANS_EVT_SCRIPTLET_INFO, line, NULL); } - fclose(pipe); + fclose(pipefh); } while(waitpid(pid, &status, 0) == -1) { diff --git a/src/pacman/Makefile.am b/src/pacman/Makefile.am index 333b819..25e8fae 100644 --- a/src/pacman/Makefile.am +++ b/src/pacman/Makefile.am @@ -17,7 +17,7 @@ DEFS = -DLOCALEDIR=\"@localedir@\" \ @DEFS@ INCLUDES = -I$(top_srcdir)/lib/libalpm -AM_CFLAGS = -pedantic -D_GNU_SOURCE +AM_CFLAGS = -pedantic -Wshadow -D_GNU_SOURCE if USE_GIT_VERSION GIT_VERSION := $(shell sh -c 'git describe --abbrev=4 --dirty | sed s/^v//') diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 08c1cf3..8e52eac 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -614,14 +614,14 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) /* if padwid is < 0, we need to trim the string so padwid = 0 */ if(padwid < 0) { int i = filenamelen - 3; - wchar_t *p = wcfname; + wchar_t *ptr = wcfname; /* grab the max number of char columns we can fill */ - while(i > 0 && wcwidth(*p) < i) { - i -= wcwidth(*p); - p++; + while(i > 0 && wcwidth(*ptr) < i) { + i -= wcwidth(*ptr); + ptr++; } /* then add the ellipsis and fill out any extra padding */ - wcscpy(p, L"..."); + wcscpy(ptr, L"..."); padwid = i; } diff --git a/src/pacman/query.c b/src/pacman/query.c index d2bfe69..2bc3373 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -286,7 +286,6 @@ static int query_search(alpm_list_t *targets) static int query_group(alpm_list_t *targets) { alpm_list_t *i, *j; - char *grpname = NULL; int ret = 0; pmdb_t *db_local = alpm_option_get_localdb(); @@ -305,6 +304,7 @@ static int query_group(alpm_list_t *targets) } } else { for(i = targets; i; i = alpm_list_next(i)) { + char *grpname; pmgrp_t *grp; grpname = alpm_list_getdata(i); grp = alpm_db_readgrp(db_local, grpname); diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 0b34f04..ea73668 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -488,7 +488,7 @@ static int sync_info(alpm_list_t *syncs, alpm_list_t *targets) pkgstr = target; for(j = syncs; j; j = alpm_list_next(j)) { - pmdb_t *db = alpm_list_getdata(j); + db = alpm_list_getdata(j); for(k = alpm_db_get_pkgcache(db); k; k = alpm_list_next(k)) { pmpkg_t *pkg = alpm_list_getdata(k); @@ -761,7 +761,6 @@ static int sync_trans(alpm_list_t *targets) pm_fprintf(stderr, PM_LOG_ERROR, _("failed to prepare transaction (%s)\n"), alpm_strerrorlast()); switch(pm_errno) { - alpm_list_t *i; case PM_ERR_PKG_INVALID_ARCH: for(i = data; i; i = alpm_list_next(i)) { char *pkg = alpm_list_getdata(i); @@ -830,7 +829,6 @@ static int sync_trans(alpm_list_t *targets) pm_fprintf(stderr, PM_LOG_ERROR, _("failed to commit transaction (%s)\n"), alpm_strerrorlast()); switch(pm_errno) { - alpm_list_t *i; case PM_ERR_FILE_CONFLICTS: for(i = data; i; i = alpm_list_next(i)) { pmfileconflict_t *conflict = alpm_list_getdata(i); diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index 5b89400..28491b4 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -166,7 +166,6 @@ int pacman_upgrade(alpm_list_t *targets) pm_fprintf(stderr, PM_LOG_ERROR, _("failed to commit transaction (%s)\n"), alpm_strerrorlast()); switch(pm_errno) { - alpm_list_t *i; case PM_ERR_FILE_CONFLICTS: for(i = data; i; i = alpm_list_next(i)) { pmfileconflict_t *conflict = alpm_list_getdata(i); diff --git a/src/pacman/util.c b/src/pacman/util.c index 51bb052..1e53f80 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -398,24 +398,24 @@ alpm_list_t *strsplit(const char *str, const char splitchar) { alpm_list_t *list = NULL; const char *prev = str; - char *dup = NULL; + char *p = NULL; while((str = strchr(str, splitchar))) { - dup = strndup(prev, (size_t)(str - prev)); - if(dup == NULL) { + p = strndup(prev, (size_t)(str - prev)); + if(p == NULL) { return NULL; } - list = alpm_list_add(list, dup); + list = alpm_list_add(list, p); str++; prev = str; } - dup = strdup(prev); - if(dup == NULL) { + p = strdup(prev); + if(p == NULL) { return NULL; } - list = alpm_list_add(list, dup); + list = alpm_list_add(list, p); return list; } -- 1.7.4.2
On 03/04/11 20:49, Sebastian Nowicki wrote:
The following have been renamed to avoid shadowing:
* conflict.c - filestr -> pkgfilestr - remove -> remove_pkgs * deps.c - index -> pkg_index - remove -> remove_pkgs - time -> epoch * diskspace.c - abort -> should_abort * handle.c - handle -> local_handle / new_handle * package.c - i -> deps * sync.c (alpm) - current -> current_pkg - remove -> remove_pkgs - ret -> gpg_ret - sync -> syncpkg * trans.c - handle -> local_handle * util.c (alpm) - pipe -> pipefh - prefix -> entry_prefix * callback.c - p -> ptr * util.c (pacman) - dup -> p
In some instance duplicate declarations were removed.
Signed-off-by: Sebastian Nowicki<sebnow@gmail.com> --- This is a follow up for an earlier patch I sent back in December. Apologies for not following up sooner. The feedback was to split the patch up. This is the first patch from the split.
The API for alpm_checkdeps is slightly modified (a parameter is renamed). I'm not sure whether this is a backwards incompatible change or not.
Sorry for the size, I don't think it can be split up much further without it turning into a dozen patches.
This patch was generated against the master branch.
lib/libalpm/Makefile.am | 2 +- lib/libalpm/alpm.h | 2 +- lib/libalpm/conflict.c | 8 +++--- lib/libalpm/deps.c | 18 ++++++++-------- lib/libalpm/diskspace.c | 8 +++--- lib/libalpm/dload.c | 6 ++-- lib/libalpm/handle.c | 50 +++++++++++++++++++++++----------------------- lib/libalpm/package.c | 6 ++-- lib/libalpm/sync.c | 37 ++++++++++++++++----------------- lib/libalpm/trans.c | 18 ++++++++-------- lib/libalpm/util.c | 20 +++++++++--------- src/pacman/Makefile.am | 2 +- src/pacman/callback.c | 10 ++++---- src/pacman/query.c | 2 +- src/pacman/sync.c | 4 +-- src/pacman/upgrade.c | 1 - src/pacman/util.c | 14 ++++++------ 17 files changed, 102 insertions(+), 106 deletions(-)
diff --git a/lib/libalpm/Makefile.am b/lib/libalpm/Makefile.am index fb224a5..21f7057 100644 --- a/lib/libalpm/Makefile.am +++ b/lib/libalpm/Makefile.am @@ -7,7 +7,7 @@ include_HEADERS = alpm_list.h alpm.h
DEFS = -DLOCALEDIR=\"@localedir@\" @DEFS@
-AM_CFLAGS = -pedantic -D_GNU_SOURCE +AM_CFLAGS = -pedantic -Wshadow -D_GNU_SOURCE
I have not gone through all of these changes... I think adding extra warnings etc should be done in configure.ac and only when --enable-debug is specified (as is currently done with stack protector, -Werror etc). But do we really want to add all these warnings in there? I use all of these: -Wclobbered -Wempty-body -Wfloat-equal -Wignored-qualifiers -Wmissing-declarations -Wmissing-parameter-type -Wmissing-prototypes -Wold-style-declaration -Woverride-init -Wsign-compare -Wstrict-prototypes -Wtype-limits -Wuninitialized I'm not sure anybody wants that... Allan
On Mon, Apr 4, 2011 at 7:26 AM, Allan McRae <allan@archlinux.org> wrote:
On 03/04/11 20:49, Sebastian Nowicki wrote:
The following have been renamed to avoid shadowing:
* conflict.c - filestr -> pkgfilestr - remove -> remove_pkgs * deps.c - index -> pkg_index - remove -> remove_pkgs - time -> epoch * diskspace.c - abort -> should_abort * handle.c - handle -> local_handle / new_handle * package.c - i -> deps * sync.c (alpm) - current -> current_pkg - remove -> remove_pkgs - ret -> gpg_ret - sync -> syncpkg * trans.c - handle -> local_handle * util.c (alpm) - pipe -> pipefh - prefix -> entry_prefix * callback.c - p -> ptr * util.c (pacman) - dup -> p
In some instance duplicate declarations were removed.
Signed-off-by: Sebastian Nowicki<sebnow@gmail.com> --- This is a follow up for an earlier patch I sent back in December. Apologies for not following up sooner. The feedback was to split the patch up. This is the first patch from the split.
The API for alpm_checkdeps is slightly modified (a parameter is renamed). I'm not sure whether this is a backwards incompatible change or not.
Sorry for the size, I don't think it can be split up much further without it turning into a dozen patches.
This patch was generated against the master branch.
lib/libalpm/Makefile.am | 2 +- lib/libalpm/alpm.h | 2 +- lib/libalpm/conflict.c | 8 +++--- lib/libalpm/deps.c | 18 ++++++++-------- lib/libalpm/diskspace.c | 8 +++--- lib/libalpm/dload.c | 6 ++-- lib/libalpm/handle.c | 50 +++++++++++++++++++++++----------------------- lib/libalpm/package.c | 6 ++-- lib/libalpm/sync.c | 37 ++++++++++++++++----------------- lib/libalpm/trans.c | 18 ++++++++-------- lib/libalpm/util.c | 20 +++++++++--------- src/pacman/Makefile.am | 2 +- src/pacman/callback.c | 10 ++++---- src/pacman/query.c | 2 +- src/pacman/sync.c | 4 +-- src/pacman/upgrade.c | 1 - src/pacman/util.c | 14 ++++++------ 17 files changed, 102 insertions(+), 106 deletions(-)
diff --git a/lib/libalpm/Makefile.am b/lib/libalpm/Makefile.am index fb224a5..21f7057 100644 --- a/lib/libalpm/Makefile.am +++ b/lib/libalpm/Makefile.am @@ -7,7 +7,7 @@ include_HEADERS = alpm_list.h alpm.h
DEFS = -DLOCALEDIR=\"@localedir@\" @DEFS@
-AM_CFLAGS = -pedantic -D_GNU_SOURCE +AM_CFLAGS = -pedantic -Wshadow -D_GNU_SOURCE
I have not gone through all of these changes... I think adding extra warnings etc should be done in configure.ac and only when --enable-debug is specified (as is currently done with stack protector, -Werror etc).
But do we really want to add all these warnings in there? I use all of these:
-Wclobbered -Wempty-body -Wfloat-equal -Wignored-qualifiers -Wmissing-declarations -Wmissing-parameter-type -Wmissing-prototypes -Wold-style-declaration -Woverride-init -Wsign-compare -Wstrict-prototypes -Wtype-limits -Wuninitialized
I'm not sure anybody wants that...
Allan
I wasn't going to add it initially, but thought this would prevent shadowing sneaking back in again. I don't really mind either way though.
Allan McRae wrote:
But do we really want to add all these warnings in there? I use all of these:
-Wclobbered -Wempty-body -Wfloat-equal -Wignored-qualifiers -Wmissing-declarations -Wmissing-parameter-type -Wmissing-prototypes -Wold-style-declaration -Woverride-init -Wsign-compare -Wstrict-prototypes -Wtype-limits -Wuninitialized
I'm not sure anybody wants that...
If they pass (or close to), why not ? Developers should follow the same rules to not break something that was respected before and to keep a consistent codebase.
On Sun, Apr 3, 2011 at 6:26 PM, Allan McRae <allan@archlinux.org> wrote:
On 03/04/11 20:49, Sebastian Nowicki wrote:
The following have been renamed to avoid shadowing:
* conflict.c - filestr -> pkgfilestr - remove -> remove_pkgs * deps.c - index -> pkg_index - remove -> remove_pkgs - time -> epoch * diskspace.c - abort -> should_abort * handle.c - handle -> local_handle / new_handle * package.c - i -> deps * sync.c (alpm) - current -> current_pkg - remove -> remove_pkgs - ret -> gpg_ret - sync -> syncpkg * trans.c - handle -> local_handle * util.c (alpm) - pipe -> pipefh - prefix -> entry_prefix * callback.c - p -> ptr * util.c (pacman) - dup -> p
In some instance duplicate declarations were removed.
Signed-off-by: Sebastian Nowicki<sebnow@gmail.com> ---
diff --git a/lib/libalpm/Makefile.am b/lib/libalpm/Makefile.am index fb224a5..21f7057 100644 --- a/lib/libalpm/Makefile.am +++ b/lib/libalpm/Makefile.am @@ -7,7 +7,7 @@ include_HEADERS = alpm_list.h alpm.h
DEFS = -DLOCALEDIR=\"@localedir@\" @DEFS@
-AM_CFLAGS = -pedantic -D_GNU_SOURCE +AM_CFLAGS = -pedantic -Wshadow -D_GNU_SOURCE
I have not gone through all of these changes... I think adding extra warnings etc should be done in configure.ac and only when --enable-debug is specified (as is currently done with stack protector, -Werror etc). Yes, this is the only place they belong, definitely not in the Makefile (and only in the libalpm Makefile, why?).
But do we really want to add all these warnings in there? I use all of these:
-Wclobbered -Wempty-body -Wfloat-equal -Wignored-qualifiers -Wmissing-declarations -Wmissing-parameter-type -Wmissing-prototypes -Wold-style-declaration -Woverride-init -Wsign-compare -Wstrict-prototypes -Wtype-limits -Wuninitialized
I'm not sure anybody wants that...
I'm not against it, provided: 1. When it is checked in, the compile doesn't break. 2. When someone uses "exotic" compiliers such as icc, gcc 3.x, whatever Cygwin has, and clang, we don't blow up or make the output that much worse for them. -Dan
On Tue, Apr 5, 2011 at 1:37 PM, Dan McGee <dpmcgee@gmail.com> wrote:
On Sun, Apr 3, 2011 at 6:26 PM, Allan McRae <allan@archlinux.org> wrote:
On 03/04/11 20:49, Sebastian Nowicki wrote:
diff --git a/lib/libalpm/Makefile.am b/lib/libalpm/Makefile.am index fb224a5..21f7057 100644 --- a/lib/libalpm/Makefile.am +++ b/lib/libalpm/Makefile.am @@ -7,7 +7,7 @@ include_HEADERS = alpm_list.h alpm.h
DEFS = -DLOCALEDIR=\"@localedir@\" @DEFS@
-AM_CFLAGS = -pedantic -D_GNU_SOURCE +AM_CFLAGS = -pedantic -Wshadow -D_GNU_SOURCE
I have not gone through all of these changes... I think adding extra warnings etc should be done in configure.ac and only when --enable-debug is specified (as is currently done with stack protector, -Werror etc). Yes, this is the only place they belong, definitely not in the Makefile (and only in the libalpm Makefile, why?).
It was added to the pacman Makefile as well (later down in the patch), wasn't really sure where to put it since I couldn't find other warning flags (aside from -Wall). I guess I'll just put it in the configure.ac script for now. It can always be removed later if it causes issues. Might even check if icc produces different results.
participants (4)
-
Allan McRae
-
Dan McGee
-
Sebastian Nowicki
-
Xavier