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

[Andrew Gregory]

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?