[pacman-dev] [PATCH] libalpm: Check return errors while deleting files

[Allan McRae]

On 21/1/20 6:03 am, Hamza Mogni wrote:
> I found this TODO while browsing pacman's source code, tried fixing it
> by checking returned errors while deleting packages files and logging
> accordingly.
> 
> This is my first time editing code and submitting a patch; SORRY if I
> did anything dumb!
> 
> Signed-off-by: Hamza Mogni <hamzamogni5 at gmail.com>

Hi Hazma,

Thanks for your patch and sorry for the delay in review.  Firstly, the
commit message should be used to describe the change you are doing.
What you have written can be added to the patch...

> ---

...  under this line!


>  lib/libalpm/remove.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c
> index 9030bfee..4a19c718 100644
> --- a/lib/libalpm/remove.c
> +++ b/lib/libalpm/remove.c
> @@ -707,7 +707,12 @@ int _alpm_remove_single_package(alpm_handle_t *handle,
>  
>  	if(!(handle->trans->flags & ALPM_TRANS_FLAG_DBONLY)) {
>  		/* TODO check returned errors if any */
> -		remove_package_files(handle, oldpkg, newpkg, targ_count, pkg_count);
> +		int err = remove_package_files(handle, oldpkg, newpkg, targ_count, pkg_count);
> +		if (err == -1) {
> +		    _alpm_log(handle, ALPM_LOG_ERROR, "alpm lacks permission to delete all files");

Good that you identified both error conditions.  This TODO is actually a
lot bigger than it seems!  It was more about what should we do in this
situation.  We have failed to remove the old package, printing an error
message and trying to install the updated version (if any) is not ideal.
 We also should not continue through this function with removing it from
the local package database.

So overall, not an ideal starting place for a first contribution, and a
lot more would need done here.

FYI, output strings need to be translated. So surround in _("output")

> +		} else if (err > 0) {
> +			_alpm_log(handle, ALPM_LOG_ERROR, "failed to delete %d files", err);

This string is more fun...  "1 files" does not make sense, so we can
provide multiple versions of the string.  Look for  _n("one", "many")
style output strings.

> +		}
>  	}
>  
>  	if(!newpkg) {
> 

Cheers,
Allan