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

[Dan McGee]

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)

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