[pacman-dev] [PATCH] Remove unnecessary archive_entry_set_pathname() calls
I'm not sure why these were ever here, as by this point we have already extracted the file meaning a call to this function is basically a no-op. Signed-off-by: Dan McGee <dan@archlinux.org> --- lib/libalpm/add.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index ca4cfa9..6795d52 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -500,19 +500,16 @@ static int extract_single_file(struct archive *archive, /* move the existing file to the "pacorig" */ if(rename(filename, newpath)) { - archive_entry_set_pathname(entry, filename); _alpm_log(PM_LOG_ERROR, _("could not rename %s (%s)\n"), filename, strerror(errno)); alpm_logaction("error: could not rename %s (%s)\n", filename, strerror(errno)); errors++; } else { /* copy the tempfile we extracted to the real path */ if(_alpm_copyfile(tempfile, filename)) { - archive_entry_set_pathname(entry, filename); _alpm_log(PM_LOG_ERROR, _("could not copy tempfile to %s (%s)\n"), filename, strerror(errno)); alpm_logaction("error: could not copy tempfile to %s (%s)\n", filename, strerror(errno)); errors++; } else { - archive_entry_set_pathname(entry, filename); _alpm_log(PM_LOG_WARNING, _("%s saved as %s\n"), filename, newpath); alpm_logaction("warning: %s saved as %s\n", filename, newpath); } @@ -531,7 +528,6 @@ static int extract_single_file(struct archive *archive, _alpm_log(PM_LOG_ERROR, _("could not copy tempfile to %s (%s)\n"), filename, strerror(errno)); errors++; } - archive_entry_set_pathname(entry, filename); } else { /* there's no sense in installing the same file twice, install * ONLY is the original and package hashes differ */ -- 1.5.5.1
This should allow some future tests to set modes and ensure they are set after installation. It is also in anticipation of a test for checking permissions on pacnew files. Signed-off-by: Dan McGee <dan@archlinux.org> --- pactest/tests/mode001.py | 2 +- pactest/tests/mode002.py | 12 ++++++++++++ pactest/util.py | 40 +++++++++++++++++++++------------------- 3 files changed, 34 insertions(+), 20 deletions(-) create mode 100644 pactest/tests/mode002.py diff --git a/pactest/tests/mode001.py b/pactest/tests/mode001.py index ff245a2..4ec11e1 100644 --- a/pactest/tests/mode001.py +++ b/pactest/tests/mode001.py @@ -1,7 +1,7 @@ self.description = "Check the mode of default files in a package" p = pmpkg("pkg1") -p.files = ["bin/foo" +p.files = ["bin/foo", "bin/bar"] self.addpkg(p) diff --git a/pactest/tests/mode002.py b/pactest/tests/mode002.py new file mode 100644 index 0000000..cc4a8fe --- /dev/null +++ b/pactest/tests/mode002.py @@ -0,0 +1,12 @@ +self.description = "Check execute mode on files in a package" + +p = pmpkg("pkg1") +p.files = ["bin/foo|755", + "bin/bar|755"] +self.addpkg(p) + +self.args = "-U %s" % p.filename() + +self.addrule("PACMAN_RETCODE=0") +self.addrule("FILE_MODE=bin/foo|755") +self.addrule("FILE_MODE=bin/bar|755") diff --git a/pactest/util.py b/pactest/util.py index 3f0b096..c6d5a59 100755 --- a/pactest/util.py +++ b/pactest/util.py @@ -58,35 +58,35 @@ def vprint(msg): def getfilename(name): """ """ - filename = "" - link = "" - if name.find(" -> ") != -1: - filename, link = name.split(" -> ") - elif name[-1] == "*": - filename = name.rstrip("*") - else: - filename = name + filename = name + extra = "" + if filename[-1] == "*": + filename = filename.rstrip("*") + if filename.find(" -> ") != -1: + filename, extra = filename.split(" -> ") + elif filename.find("|") != -1: + filename, extra = filename.split("|") return filename def mkfile(name, data = ""): """ """ - - isaltered = 0 isdir = 0 islink = 0 + setperms = 0 + filename = name link = "" - filename = "" + perms = "" - if name.find(" -> ") != -1: + if filename[-1] == "*": + filename = filename.rstrip("*") + if filename.find(" -> ") != -1: islink = 1 - filename, link = name.split(" -> ") - elif name[-1] == "*": - isaltered = 1 - filename = name.rstrip("*") - else: - filename = name - if name[-1] == "/": + filename, link = filename.split(" -> ") + elif filename.find("|") != -1: + setperms = 1 + filename, perms = filename.split("|") + if filename[-1] == "/": isdir = 1 if isdir: @@ -114,6 +114,8 @@ def mkfile(name, data = ""): if data[-1] != "\n": fd.write("\n") fd.close() + if setperms: + os.chmod(filename, int(perms, 8)) def mkdescfile(filename, pkg): """ -- 1.5.5.1
We were a bit juryrigged using one call to mkstemp() before rather than extracting the new files side-by-side and doing our comparisons there. We were also facing some permissions issues. Instead, make our life easier by extracting all temp files to a '.paccheck' extension, doing our md5 comparisons, and then taking the correct actions. Still to be done here- a cleanup of the use of PATH_MAX which should not be necessary if we use dynamic allocation on the heap. Signed-off-by: Dan McGee <dan@archlinux.org> --- lib/libalpm/add.c | 64 +++++++++++++++++++++++++--------------------- pactest/tests/mode003.py | 20 ++++++++++++++ 2 files changed, 55 insertions(+), 29 deletions(-) create mode 100644 pactest/tests/mode003.py diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 6795d52..f759e7d 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -434,20 +434,14 @@ static int extract_single_file(struct archive *archive, } if(needbackup) { - char *tempfile; + char checkfile[PATH_MAX]; char *hash_local = NULL, *hash_pkg = NULL; - int fd; int ret; - /* extract the package's version to a temporary file and checksum it */ - STRDUP(tempfile, "/tmp/alpm_XXXXXX", RET_ERR(PM_ERR_MEMORY, -1)); - fd = mkstemp(tempfile); - if(fd == -1) { - RET_ERR(PM_ERR_SYSTEM, -1); - } + snprintf(checkfile, PATH_MAX, "%s.paccheck", filename); + archive_entry_set_pathname(entry, checkfile); - ret = archive_read_data_into_fd(archive, fd); - close(fd); + ret = archive_read_extract(archive, entry, archive_flags); if(ret == ARCHIVE_WARN) { /* operation succeeded but a non-critical error was encountered */ _alpm_log(PM_LOG_DEBUG, "warning extracting %s (%s)\n", @@ -457,14 +451,12 @@ static int extract_single_file(struct archive *archive, entryname, archive_error_string(archive)); alpm_logaction("error: could not extract %s (%s)\n", entryname, archive_error_string(archive)); - unlink(tempfile); - FREE(tempfile); FREE(hash_orig); return(1); } hash_local = alpm_get_md5sum(filename); - hash_pkg = alpm_get_md5sum(tempfile); + hash_pkg = alpm_get_md5sum(checkfile); /* append the new md5 hash to it's respective entry * in newpkg's backup (it will be the new orginal) */ @@ -500,14 +492,18 @@ static int extract_single_file(struct archive *archive, /* move the existing file to the "pacorig" */ if(rename(filename, newpath)) { - _alpm_log(PM_LOG_ERROR, _("could not rename %s (%s)\n"), filename, strerror(errno)); - alpm_logaction("error: could not rename %s (%s)\n", filename, strerror(errno)); + _alpm_log(PM_LOG_ERROR, _("could not rename %s to %s (%s)\n"), + filename, newpath, strerror(errno)); + alpm_logaction("error: could not rename %s to %s (%s)\n", + filename, newpath, strerror(errno)); errors++; } else { - /* copy the tempfile we extracted to the real path */ - if(_alpm_copyfile(tempfile, filename)) { - _alpm_log(PM_LOG_ERROR, _("could not copy tempfile to %s (%s)\n"), filename, strerror(errno)); - alpm_logaction("error: could not copy tempfile to %s (%s)\n", filename, strerror(errno)); + /* rename the file we extracted to the real name */ + if(rename(checkfile, filename)) { + _alpm_log(PM_LOG_ERROR, _("could not rename %s to %s (%s)\n"), + checkfile, filename, strerror(errno)); + alpm_logaction("error: could not rename %s to %s (%s)\n", + checkfile, filename, strerror(errno)); errors++; } else { _alpm_log(PM_LOG_WARNING, _("%s saved as %s\n"), filename, newpath); @@ -524,35 +520,45 @@ static int extract_single_file(struct archive *archive, _alpm_log(PM_LOG_DEBUG, "action: installing new file: %s\n", entryname); - if(_alpm_copyfile(tempfile, filename)) { - _alpm_log(PM_LOG_ERROR, _("could not copy tempfile to %s (%s)\n"), filename, strerror(errno)); + if(rename(checkfile, filename)) { + _alpm_log(PM_LOG_ERROR, _("could not rename %s to %s (%s)\n"), + checkfile, filename, strerror(errno)); + alpm_logaction("error: could not rename %s to %s (%s)\n", + checkfile, filename, strerror(errno)); errors++; } } else { /* there's no sense in installing the same file twice, install * ONLY is the original and package hashes differ */ _alpm_log(PM_LOG_DEBUG, "action: leaving existing file in place\n"); + unlink(checkfile); } } else if(strcmp(hash_orig, hash_pkg) == 0) { /* originally installed file and new file are the same - this * implies the case above failed - i.e. the file was changed by a * user */ _alpm_log(PM_LOG_DEBUG, "action: leaving existing file in place\n"); + unlink(checkfile); } else if(strcmp(hash_local, hash_pkg) == 0) { /* this would be magical. The above two cases failed, but the * user changes just so happened to make the new file exactly the * same as the one in the package... skip it */ _alpm_log(PM_LOG_DEBUG, "action: leaving existing file in place\n"); + unlink(checkfile); } else { char newpath[PATH_MAX]; _alpm_log(PM_LOG_DEBUG, "action: keeping current file and installing new one with .pacnew ending\n"); snprintf(newpath, PATH_MAX, "%s.pacnew", filename); - if(_alpm_copyfile(tempfile, newpath)) { - _alpm_log(PM_LOG_ERROR, _("could not install %s as %s: %s\n"), filename, newpath, strerror(errno)); - alpm_logaction("error: could not install %s as %s: %s\n", filename, newpath, strerror(errno)); + if(rename(checkfile, newpath)) { + _alpm_log(PM_LOG_ERROR, _("could not install %s as %s (%s)\n"), + filename, newpath, strerror(errno)); + alpm_logaction("error: could not install %s as %s (%s)\n", + filename, newpath, strerror(errno)); } else { - _alpm_log(PM_LOG_WARNING, _("%s installed as %s\n"), filename, newpath); - alpm_logaction("warning: %s installed as %s\n", filename, newpath); + _alpm_log(PM_LOG_WARNING, _("%s installed as %s\n"), + filename, newpath); + alpm_logaction("warning: %s installed as %s\n", + filename, newpath); } } } @@ -560,9 +566,9 @@ static int extract_single_file(struct archive *archive, FREE(hash_local); FREE(hash_pkg); FREE(hash_orig); - unlink(tempfile); - FREE(tempfile); } else { + int ret; + /* we didn't need a backup */ if(notouch) { /* change the path to a .pacnew extension */ @@ -583,7 +589,7 @@ static int extract_single_file(struct archive *archive, archive_entry_set_pathname(entry, filename); - int ret = archive_read_extract(archive, entry, archive_flags); + ret = archive_read_extract(archive, entry, archive_flags); if(ret == ARCHIVE_WARN) { /* operation succeeded but a non-critical error was encountered */ _alpm_log(PM_LOG_DEBUG, "warning extracting %s (%s)\n", diff --git a/pactest/tests/mode003.py b/pactest/tests/mode003.py new file mode 100644 index 0000000..1193a5c --- /dev/null +++ b/pactest/tests/mode003.py @@ -0,0 +1,20 @@ +self.description = "Backup file permissions test (same as orig)" + +lp = pmpkg("filesystem") +lp.files = ["etc/profile|666"] +lp.backup = ["etc/profile*"] +self.addpkg2db("local", lp) + +p = pmpkg("filesystem", "1.0-2") +p.files = ["etc/profile|666**"] +p.backup = ["etc/profile"] +self.addpkg(p) + +self.args = "-U %s" % p.filename() + +self.addrule("PACMAN_RETCODE=0") +self.addrule("!FILE_PACSAVE=etc/profile") +self.addrule("FILE_PACNEW=etc/profile") +self.addrule("FILE_EXIST=etc/profile") +self.addrule("FILE_MODE=etc/profile|666") +self.addrule("FILE_MODE=etc/profile.pacnew|666") -- 1.5.5.1
Rework to use a single #define for the buffsize, and in the process clean up some other code and double the default buffer size. Signed-off-by: Dan McGee <dan@archlinux.org> --- lib/libalpm/util.c | 22 ++++++++++++++-------- 1 files changed, 14 insertions(+), 8 deletions(-) diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index a7a6573..92e9991 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -215,27 +215,31 @@ int _alpm_makepath_mode(const char *path, mode_t mode) return(0); } +#define CPBUFSIZE 8 * 1024 + int _alpm_copyfile(const char *src, const char *dest) { FILE *in, *out; size_t len; - char buf[4097]; + char *buf; + int ret = 0; - in = fopen(src, "r"); + in = fopen(src, "rb"); if(in == NULL) { return(1); } - out = fopen(dest, "w"); + out = fopen(dest, "wb"); if(out == NULL) { fclose(in); return(1); } + CALLOC(buf, 1, CPBUFSIZE, ret = 1; goto cleanup;); + /* do the actual file copy */ - while((len = fread(buf, 1, 4096, in))) { + while((len = fread(buf, 1, CPBUFSIZE, in))) { fwrite(buf, 1, len, out); } - fclose(in); /* chmod dest to permissions of src, as long as it is not a symlink */ struct stat statbuf; @@ -245,12 +249,14 @@ int _alpm_copyfile(const char *src, const char *dest) } } else { /* stat was unsuccessful */ - fclose(out); - return(1); + ret = 1; } +cleanup: + fclose(in); fclose(out); - return(0); + FREE(buf); + return(ret); } /* Trim whitespace and newlines from a string -- 1.5.5.1
On Mon, Apr 28, 2008 at 10:34 PM, Dan McGee <dan@archlinux.org> wrote:
We were a bit juryrigged using one call to mkstemp() before rather than extracting the new files side-by-side and doing our comparisons there. We were also facing some permissions issues. Instead, make our life easier by extracting all temp files to a '.paccheck' extension, doing our md5 comparisons, and then taking the correct actions.
Still to be done here- a cleanup of the use of PATH_MAX which should not be necessary if we use dynamic allocation on the heap.
Signed-off-by: Dan McGee <dan@archlinux.org> ---
I posted these patches here because I would love to get some feedback on them from you guys, and they need some real-life testing. The biggest catch with this patch is ensuring we don't leave any paccheck files behind, as we can no longer always unlink the original file since we use rename instead of copy. All of these can be found on my public 'working' branch if you are tracking my GIT repository. I'll hold off a few days before pushing these to master. -Dan
On Tue, Apr 29, 2008 at 5:39 AM, Dan McGee <dan@archlinux.org> wrote:
On Mon, Apr 28, 2008 at 10:34 PM, Dan McGee <dan@archlinux.org> wrote:
We were a bit juryrigged using one call to mkstemp() before rather than extracting the new files side-by-side and doing our comparisons there. We were also facing some permissions issues. Instead, make our life easier by extracting all temp files to a '.paccheck' extension, doing our md5 comparisons, and then taking the correct actions.
Still to be done here- a cleanup of the use of PATH_MAX which should not be necessary if we use dynamic allocation on the heap.
Signed-off-by: Dan McGee <dan@archlinux.org> ---
I posted these patches here because I would love to get some feedback on them from you guys, and they need some real-life testing. The biggest catch with this patch is ensuring we don't leave any paccheck files behind, as we can no longer always unlink the original file since we use rename instead of copy.
All of these can be found on my public 'working' branch if you are tracking my GIT repository. I'll hold off a few days before pushing these to master.
At least it builds fine on cygwin, and make check is fine too :) And reading the code, it looks fine too. Just about this paccheck issue, do we really need to do it that way and have to worry about it? Why can't we just let the unlink call? If the file is here, it will always be removed. If its not here anymore, unlink will fail but that doesn't matter. Otherwise an explicit stat could be made to see if the file is still here. But maybe you find this ugly and prefer handling everything manually, at the risk of leaving some files behind in case we missed something? But if we keep your patch as is, there are some failure cases where you kept the paccheck on purpose, right? Every time the rename call fails, there is no unlink. But I doubt this happens often anyway, so no big problem I guess.
participants (2)
-
Dan McGee
-
Xavier