[pacman-dev] [PATCH] Check return value of rename() calls

[Dave Reisner]

On Mon, Aug 08, 2011 at 12:29:23PM -0500, Dan McGee wrote:
> We did a good job checking this in add.c, but not necessarily anywhere
> else. Fix this up by adding checks into dload.c, remove.c, and conf.c in
> the frontend. Also add loggers where appropriate and make the message
> syntax more consistent.
> 
> Signed-off-by: Dan McGee <dan at archlinux.org>
> ---
> 
> Dave- any memories/reasoning as to why you didn't port this bit over when you
> added the curl backend stuff? (commit 96e458b705eda4ddff)

Laziness? Bad form? I wouldn't mind seeing an explicit comparison to
non-zero for the fail case, but ack otherwise.

d

> The remove.c changes pretty much parallel the checks and logging done in add.c,
> if you're looking to see where the basic stuff from this patch came from.
> 
> conf.c is the only user of rename() in the frontend, so this does add a new
> message there, but it seems like a worthwhile addition.
> 
> -Dan
> 
>  lib/libalpm/dload.c  |    7 +++++--
>  lib/libalpm/remove.c |   24 ++++++++++++++++++------
>  src/pacman/conf.c    |    8 ++++++--
>  3 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index 23b8db7..dc4f91e 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -373,8 +373,11 @@ cleanup:
>  	}
>  
>  	if(ret == 0) {
> -		rename(tempfile, destfile);
> -		if(final_file) {
> +		if(rename(tempfile, destfile)) {
> +			_alpm_log(handle, ALPM_LOG_ERROR, _("could not rename %s to %s (%s)\n"),
> +					tempfile, destfile, strerror(errno));
> +			ret = -1;
> +		} else if(final_file) {
>  			*final_file = strdup(strrchr(destfile, '/') + 1);
>  		}
>  	}
> diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c
> index 83c437f..3c19c90 100644
> --- a/lib/libalpm/remove.c
> +++ b/lib/libalpm/remove.c
> @@ -220,7 +220,7 @@ static int can_remove_file(alpm_handle_t *handle, const alpm_file_t *file,
>  
>  /* Helper function for iterating through a package's file and deleting them
>   * Used by _alpm_remove_commit. */
> -static void unlink_file(alpm_handle_t *handle, alpm_pkg_t *info,
> +static int unlink_file(alpm_handle_t *handle, alpm_pkg_t *info,
>  		const alpm_file_t *fileobj, alpm_list_t *skip_remove, int nosave)
>  {
>  	struct stat buf;
> @@ -234,7 +234,7 @@ static void unlink_file(alpm_handle_t *handle, alpm_pkg_t *info,
>  	if(alpm_list_find_str(skip_remove, fileobj->name)) {
>  		_alpm_log(handle, ALPM_LOG_DEBUG,
>  				"%s is in skip_remove, skipping removal\n", file);
> -		return;
> +		return 1;
>  	}
>  
>  	/* we want to do a lstat here, and not a _alpm_lstat.
> @@ -243,7 +243,7 @@ static void unlink_file(alpm_handle_t *handle, alpm_pkg_t *info,
>  	 * actual symlink */
>  	if(lstat(file, &buf)) {
>  		_alpm_log(handle, ALPM_LOG_DEBUG, "file %s does not exist\n", file);
> -		return;
> +		return 1;
>  	}
>  
>  	if(S_ISDIR(buf.st_mode)) {
> @@ -280,6 +280,7 @@ static void unlink_file(alpm_handle_t *handle, alpm_pkg_t *info,
>  				if(rmdir(file)) {
>  					_alpm_log(handle, ALPM_LOG_DEBUG,
>  							"directory removal of %s failed: %s\n", file, strerror(errno));
> +					return -1;
>  				} else {
>  					_alpm_log(handle, ALPM_LOG_DEBUG,
>  							"removed directory %s (no remaining owners)\n", file);
> @@ -299,10 +300,16 @@ static void unlink_file(alpm_handle_t *handle, alpm_pkg_t *info,
>  				if(cmp != 0) {
>  					char newpath[PATH_MAX];
>  					snprintf(newpath, PATH_MAX, "%s.pacsave", file);
> -					rename(file, newpath);
> +					if(rename(file, newpath)) {
> +						_alpm_log(handle, ALPM_LOG_ERROR, _("could not rename %s to %s (%s)\n"),
> +								file, newpath, strerror(errno));
> +						alpm_logaction(handle, "error: could not rename %s to %s (%s)\n",
> +								file, newpath, strerror(errno));
> +						return -1;
> +					}
>  					_alpm_log(handle, ALPM_LOG_WARNING, _("%s saved as %s\n"), file, newpath);
>  					alpm_logaction(handle, "warning: %s saved as %s\n", file, newpath);
> -					return;
> +					return 0;
>  				}
>  			}
>  		}
> @@ -310,10 +317,14 @@ static void unlink_file(alpm_handle_t *handle, alpm_pkg_t *info,
>  		_alpm_log(handle, ALPM_LOG_DEBUG, "unlinking %s\n", file);
>  
>  		if(unlink(file) == -1) {
> -			_alpm_log(handle, ALPM_LOG_ERROR, _("cannot remove file '%s': %s\n"),
> +			_alpm_log(handle, ALPM_LOG_ERROR, _("cannot remove %s (%s)\n"),
>  					file, strerror(errno));
> +			alpm_logaction(handle, "error: cannot remove %s (%s)\n",
> +					file, strerror(errno));
> +			return -1;
>  		}
>  	}
> +	return 0;
>  }
>  
>  int _alpm_remove_single_package(alpm_handle_t *handle,
> @@ -397,6 +408,7 @@ int _alpm_remove_single_package(alpm_handle_t *handle,
>  	for(i = filelist->count; i > 0; i--) {
>  		alpm_file_t *file = filelist->files + i - 1;
>  		int percent;
> +		/* TODO: check return code and handle accordingly */
>  		unlink_file(handle, oldpkg, file, skip_remove,
>  				handle->trans->flags & ALPM_TRANS_FLAG_NOSAVE);
>  
> diff --git a/src/pacman/conf.c b/src/pacman/conf.c
> index a580406..29e835c 100644
> --- a/src/pacman/conf.c
> +++ b/src/pacman/conf.c
> @@ -184,10 +184,14 @@ static int download_with_xfercommand(const char *url, const char *localpath,
>  		ret = -1;
>  	} else {
>  		/* download was successful */
> +		ret = 0;
>  		if(usepart) {
> -			rename(tempfile, destfile);
> +			if(rename(tempfile, destfile)) {
> +				pm_printf(ALPM_LOG_ERROR, _("could not rename %s to %s (%s)\n"),
> +						tempfile, destfile, strerror(errno));
> +				ret = -1;
> +			}
>  		}
> -		ret = 0;
>  	}
>  
>  cleanup:
> -- 
> 1.7.6
> 
>