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

Message ID 20200120200302.573195-1-hamzamogni5@gmail.com
State Rejected, archived
Headers show
Series [pacman-dev] libalpm: Check return errors while deleting files | expand

Commit Message

Hamza Mogni Jan. 20, 2020, 8:03 p.m. UTC
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@gmail.com>
---
 lib/libalpm/remove.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Allan McRae Jan. 31, 2020, 5:18 a.m. UTC | #1
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@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

Patch

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");
+		} else if (err > 0) {
+			_alpm_log(handle, ALPM_LOG_ERROR, "failed to delete %d files", err);
+		}
 	}
 
 	if(!newpkg) {