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

Andrew Gregory andrew.gregory.8 at gmail.com
Thu Jan 31 00:01:00 EST 2013


On Thu, 25 Oct 2012 16:57:37 +0200
Alexander Rødseth <rodseth at gmail.com> wrote:

> Signed-off-by: Alexander Rødseth <rodseth at gmail.com>
> ---

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?


More information about the pacman-dev mailing list