Re: [pacman-dev] [PATCH] Implement two TODOs about missing error handling

On Thu, 25 Oct 2012 16:57:37 +0200 Alexander Rødseth wrote:

Signed-off-by: Alexander Rødseth ---

The commit message could be more descriptive of what this actually does. Maybe something like 'report errors from _alpm_remove_single_package'.

lib/libalpm/remove.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index f189e30..370ec99 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -501,7 +501,7 @@ static int remove_package_files(alpm_handle_t *handle, * @param targ_count current index within the transaction (1-based) * @param pkg_count the number of packages affected by the transaction * - * @return 0 + * @return 0 on success, -1 if errors occurred */ int _alpm_remove_single_package(alpm_handle_t *handle, alpm_pkg_t *oldpkg, alpm_pkg_t *newpkg, @@ -509,6 +509,7 @@ int _alpm_remove_single_package(alpm_handle_t *handle, { const char *pkgname = oldpkg->name; const char *pkgver = oldpkg->version; + int ret = 0;

if(newpkg) { _alpm_log(handle, ALPM_LOG_DEBUG, "removing old package first (%s-%s)\n", @@ -528,9 +529,13 @@ int _alpm_remove_single_package(alpm_handle_t *handle, } }

+ /* remove the package files */ if(!(handle->trans->flags & ALPM_TRANS_FLAG_DBONLY)) { - /* TODO check returned errors if any */ - remove_package_files(handle, oldpkg, newpkg, targ_count, pkg_count); + if(remove_package_files(handle, oldpkg, newpkg, targ_count, pkg_count)) { + _alpm_log(handle, ALPM_LOG_ERROR, _("could not remove package files for %s\n"), + pkgname); + ret = -1; + } }

/* run the post-remove script if it exists */ @@ -551,15 +556,16 @@ int _alpm_remove_single_package(alpm_handle_t *handle, if(_alpm_local_db_remove(handle->db_local, oldpkg) == -1) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not remove database entry %s-%s\n"), pkgname, pkgver); + ret = -1; } + /* remove the package from the cache */ if(_alpm_db_remove_pkgfromcache(handle->db_local, oldpkg) == -1) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not remove entry '%s' from cache\n"), pkgname);

I think the package cache being in an incorrect state is worth returning an error too.

}

- /* TODO: useful return values */ - return 0; + return ret; }

/**

This return value is used to avoid running ldconfig when it could break the system. Is it worth differentiating error types so that we can still run ldconfig if possible?