- Pacman-dev - lists.archlinux.org

[pacman-dev] [PATCH 1/2] Add missing header
by Allan McRae 10 Jul '13

10 Jul '13

Exposed when building with --without-libcurl Signed-off-by: Allan McRae <allan(a)archlinux.org> --- lib/libalpm/be_sync.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index feda6f5..bd68739 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -22,6 +22,7 @@ #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> +#include <limits.h> #include <unistd.h> /* libarchive */ -- 1.8.3.2

1 1

[pacman-dev] [PATCH] be_sync: avoid crashing on files in the root of a DB
by Dave Reisner 09 Jul '13

09 Jul '13

As seen: https://bbs.archlinux.org/viewtopic.php?pid=1297766 Signed-off-by: Dave Reisner <dreisner(a)archlinux.org> --- lib/libalpm/be_sync.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index feda6f5..f3e0a33 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -561,6 +561,13 @@ static int sync_db_read(alpm_db_t *db, struct archive *archive, return -1; } + if(filename == NULL) { + /* A file exists outside of a subdirectory. This isn't a read error, so return + * success and try to continue on. */ + _alpm_log(db->handle, ALPM_LOG_DEBUG, "unknown database file: %s\n", filename); + return 0; + } + if(strcmp(filename, "desc") == 0 || strcmp(filename, "depends") == 0 || (strcmp(filename, "deltas") == 0 && db->handle->deltaratio > 0.0) ) { int ret; -- 1.8.3.2

4 6

[pacman-dev] [PATCH 0/2] separate ini parsing from pacman/conf.c
by Sascha Kruse 06 Jul '13

06 Jul '13

This set of patches separate the ini parsing from pacman/conf.c into its own file to make the code reusable for future additions, like parsing config files for hooks. The config file gets parsed into a list of ini_section_t which contains the name of the section and a list of directives which in turn contain key, value, file and linenum. File and linenum are included to keep the ability to mention the location of errors in pacmans error messages. I realize that a new release is planed for the near future and this patch is not intended to be included in that release, since it adds new strings to be translated and it probably needs more than one review cycle. Best regards, Sascha Kruse

4 7

[pacman-dev] (no subject)
by Jonathan Frazier 05 Jul '13

05 Jul '13

Subject: re: [pacman-dev] [PATCH] pacdiff using pacman db v2 Reply-To: In-Reply-To: 51D1468A.3030209(a)xinu.at On 07/01/13 at 11:06am, Florian Pritz wrote: > Subject: [pacman-dev] [PATCH] pacdiff using pacman db v2 > > Please move that v2 into the PATCH tag like so: [PATCH v2]. Otherwise > it sounds like you use "version 2 of the pacman db" and it might end > up in the final commit. Ok will do. > > I'd also suggest to change the subject to something like "pacdiff: use > pacman db for lookups to increase performance" so it's more obvious > what this patch does and why. Still missing the "why is using that > faster/better than locate", but that can be in the commit message. > > On 01.07.2013 02:00, Jonathan Frazier wrote: > > Implement the default search using the local pacman db. > > I'm not sure the default should switch to an inferior method (inferior > because it doesn't find everything). updatedb might be too slow on > certain hardware or whatever, but I think that's a minority so we > should stick with locate. Also that way long time users won't have > left over .pacsaves. > First let me say that I am not stuck on this being the default option, but it is the one I will use. I could say locate is inferior because it finds pacsaves not in use. giving more noise and less signal. My rational is more that it finds the active configs more throughly. Speed is not the only factor. I will try and add more descriptive commit msgs. > > Use @sysconfdir@/pacman.conf to find the local db Search for > > pacsave.0 files. > > You also search for .pacsave, .pacsave.1, ..., .pacnew, .pacorig. No > need to mention this explicitly here. Also the comment about > pacman.conf doesn't belong here unless you explain why you do it this > way and you have a good reason not to do anything else. > > Actually none of the 3 lines explain why any of this is done. > > I know these are the changes you made compared to the previous patch, > but such changes belong to the comment section below (between the > "---" marker and the list of changed files) not into the commit > message. > Adding support for these extra pacsave files is a new feature and worth noting in the changelog. this feature was requested in another thread. I can make it a separate patch if the rest of this is going to take longer. > All I remember (now) is that you do this because updatedb is slow on > some low end hardware, but that should really be mentioned here > because I might forget (or am I already wrong? I honestly don't know) > and others won't even have known in the first place and someone might > care when they look through the commit log in a few years to find why > a certain feature appeared (been there, done that). > I will try to be more descriptive in my changlog. > > > > Signed-off-by: Jonathan Frazier <eyeswide(a)gmail.com> --- > > contrib/pacdiff.sh.in | 86 > > ++++++++++++++++++++++++++++++++++++++------------- 1 file changed, > > 64 insertions(+), 22 deletions(-) > > > > diff --git a/contrib/pacdiff.sh.in b/contrib/pacdiff.sh.in index > > 47779d6..ee80c9c 100644 --- a/contrib/pacdiff.sh.in +++ > > b/contrib/pacdiff.sh.in @@ -21,9 +21,8 @@ declare -r > > myname='pacdiff' declare -r myver='@PACKAGE_VERSION@' > > > > -diffprog=${DIFFPROG:-vimdiff} > > -diffsearchpath=${DIFFSEARCHPATH:-/etc} -locate=0 > > +diffprog=${DIFF_PROG:-vimdiff} > > +difffindpath="${DIFF_FIND_PATH:-/etc}" > > Out of scope of this patch. > DIFFSEARCHPATH is misleading and I find it difficult to read. the old name implies it works for all search types, when it only applied to find. Since nobody is defending find as the default, it should be changed along with the default. Adding underscores was just cause I am fixing it. Is DIFFFINDPATH more acceptable? > > USE_COLOR='y' > > > > m4_include(../scripts/library/output_format.sh) @@ -32,11 +31,19 @@ > > m4_include(../scripts/library/term_colors.sh) > > > > usage() { echo "$myname : a simple pacnew/pacorig/pacsave updater" > > - echo "Usage : $myname [-l]" - echo " -l/--locate makes $myname > > use locate rather than find" - echo " DIFFPROG variable allows to > > override the default vimdiff" - echo " DIFFSEARCHPATH allows to > > override the default /etc path" - echo "Example : DIFFPROG=meld > > DIFFSEARCHPATH=\"/boot /etc /usr\" $myname" + echo "" + echo > > "Usage : $myname [-lf]" + echo " -l/--locate makes $myname search > > using locate" + echo " -f/--find makes $myname search using > > find" > > Add an option for your pacsave method. Maybe -p. > Does it make sense to have an option for the default? I don't think so. Whatever the default ends up being it doesn't need a option. > > + echo "" + echo "Note: the default search looks for backup files in > > the local pacman db" + echo " this generally will not find > > *.pacsave files" + echo "" + echo " DIFF_PROG variable will > > override the default editor: vimdiff" + echo " DIFF_FIND_PATH will > > override the default path when using --find" + echo "" + echo > > "Example : DIFF_PROG=meld $myname" + echo "Example : > > DIFF_FIND_PATH=\"/boot /etc /usr\" $myname -f" > > Out of scope. > > > } > > > > version() { @@ -45,27 +52,61 @@ version() { echo 'Copyright (C) > > 2013 Pacman Development Team <pacman-dev(a)archlinux.org>' } > > > > -cmd() { - if [ $locate -eq 1 ]; then - locate -0 -e -b > > \*.pacnew \*.pacorig \*.pacsave - else - find $diffsearchpath $ > > -name \*.pacnew -o -name \*.pacorig -o -name \*.pacsave $ -print0 - > > fi -} - > > Any reason you moved cmd()? Makes reviewing the changes harder. > I was concerned about the scope of the variables it contains. as none are not defined at that time. I don't remember if it was necessary or just my correcting "bad" style from writing in other languages. > > if [ $# -gt 0 ]; then case $1 in -l|--locate) - locate=1;; + > > locate=$(type -P locate);; + -f|--find) + find=$(type -P > > find);; > > Why did you change that to do a path lookup and not exit if you can't > find it? > I didn't think to test if find or locate could not be found. I can add that. The idea was to only define a variable for the active search type and use that to define the mode later. using the path is convenient. > > -V|--version) > > - version; exit 0;; > > + version; exit 0;; > > -h|--help) > > - usage; exit 0;; > > + usage; exit 0;; > > *) > > - usage; exit 1;; > > + usage; exit 1;; > > esac > > fi > > > > +if [[ ! -n $locate && ! -n $find ]]; then > > Don't do that. Someone might add a 4th option in the future and then > they have to adjust everything again. > > Just use a $mode variable and set it to a default or to the last > specified option. I'd also use strings here for easier understanding > when you use them in conditions ("find", "locate", "pacsave"). > I will add a var for just the pacman db search. which will replace this. > > + if [[ ! -r @sysconfdir@/pacman.conf ]]; then > > + error "unable to read @sysconfdir@/pacman.conf" > > + usage; exit 1 > > + fi > > + > > + eval $(awk '/DBPath/ {print $1$2$3}' @sysconfdir@/pacman.conf) > > Make sysconfdir a variable like myver, don't hardcode it directly. (same > for localstatedir below) > This code is what is used in bacman and pacscripts as others requested I use as an example. What is the reason to make it a variable? The only reason I could come up with for making it variable was to support running in a alternate root. Adding such support seemed overly complex given the current state of pacdiff. I was thinking of adding this feature after converting the option handling to allow options with associated strings. I don't see it as a reason to hold up the rest of this code. > > + pac_db="${DBPath:-@localstatedir@/lib/pacman/}local" > > + if [[ ! -d "${pac_db}" ]]; then > > + error "unable to read pacman db %s". "${pac_db}" > > + usage; exit 1 > > + fi > > +fi > > + > > +check_backup() { > > + [[ -f "$1" ]] && printf "$1"'\0' > > +} > > + > > +cmd() { > > + if [[ -n $locate ]]; then > > + locate -0 -e -b \*.pacnew \*.pacorig \*.pacsave '*.pacsave.[0-9]' > > + elif [[ -n $find ]]; then > > + find "${difffindpath}" $ -name \*.pacnew -o -name \*.pacorig -o -name \*.pacsave -o -name '*.pacsave.[0-9]' $ -print0 2> /dev/null > > .pacsave pattern change is out of scope. This isn't a logical change > given the patch subject. Better to split that into it's own patch > (before this patch so it can be merge independently). > again this was asked for in my previous submission. I will try and document it better. the original subject contained all three notes, so it was excessively long. > > + else > > + # parse local pacman db for backup files and look for pac* based on them > > + awk '/^%BACKUP%$/ { > > + while (getline) { > > + if (/^$/) { nextfile } > > + print $1 > > + } > > + }' "${pac_db}"/*/files | while read -r bkup; do > > + check_backup "/$bkup.pacnew" > > + check_backup "/$bkup.pacorig" > > + for f in "/$bkup.pacsave"{,.{0..9}}; do > > + check_backup "$f" > > + done > > + done > > + fi > > +} > > + > > # see http://mywiki.wooledge.org/BashFAQ/020 > > while IFS= read -u 3 -r -d '' pacfile; do > > file="${pacfile%.pac*}" > > @@ -82,16 +123,17 @@ while IFS= read -u 3 -r -d '' pacfile; do > > msg2 "Files are identical, removing..." > > rm -v "$pacfile" > > else > > - ask "(V)iew, (S)kip, (R)emove %s, (O)verwrite with %s: [v/s/r/o] " "$file_type" "$file_type" > > + ask "(V)iew, (S)kip, (R)emove %s, (O)verwrite with %s, (Q)uit: [v/s/r/o/q] " "$file_type" "$file_type" > > Out of the scope of this patch and IMHO unnecessary (^C is enough). > I agree that q is not necessary, but I find it annoying that it is not there. I have been thanked for adding it when sharing a previous version of this code. At least it shows that that is a reasonable time to quit. /shrug > > while read c; do > > case $c in > > + q|Q) exit 1;; > > The user asked us to quit, not an error. > will fix, thanks > > r|R) rm -v "$pacfile"; break ;; > > o|O) mv -v "$pacfile" "$file"; break ;; > > v|V) > > $diffprog "$pacfile" "$file" > > rm -iv "$pacfile"; break ;; > > s|S) break ;; > > - *) ask "Invalid answer. Try again: [v/s/r/o] "; continue ;; > > + *) ask "Invalid answer. Try again: [v/s/r/o/q] "; continue ;; > > esac > > done > > fi > > > > thanks for the comments. Do you think adding a prompt asking if pacdiff should run updatedb when called with -l is a good idea? Given the confusion brought by my misuse of git format-patch and combining commits into one. If it is preferred I can go back to pushing code to github and ask for further reviews when ready. Jonathan

3 2

[pacman-dev] [GIT] The official pacman repository branch, master, updated. v4.1.2-56-g897e2b1
by allan＠archlinux.org 05 Jul '13

05 Jul '13

This is an automated email from the git hooks/post-receive script. It was generated because a ref change was pushed to the repository containing the project "The official pacman repository". The branch, master has been updated via 897e2b121a107f6d250d38ff4d1d0a4960885c94 (commit) via eb19d41d5f85f169cee7570f783821cb705ad37a (commit) via 7b8f8753b15037bf4a88126ffde8195416432c72 (commit) from dcb3b9f85ff8761fa1fe7ef01df2d6b011b9cb73 (commit) Those revisions listed above that are new to this repository have not appeared on any other notification email; so we list those revisions in full, below. - Log ----------------------------------------------------------------- commit 897e2b121a107f6d250d38ff4d1d0a4960885c94 Author: Andrew Gregory <andrew.gregory.8(a)gmail.com> Date: Thu Jul 4 06:36:20 2013 -0400 use full path when checking for mountpoints Signed-off-by: Andrew Gregory <andrew.gregory.8(a)gmail.com> Signed-off-by: Allan McRae <allan(a)archlinux.org> commit eb19d41d5f85f169cee7570f783821cb705ad37a Author: Dave Reisner <dreisner(a)archlinux.org> Date: Wed Jul 3 20:33:19 2013 -0400 do not check error from close(2) On operating systems we support, the behavior is always such that the kernel will do the right thing as far as invalidating the file descriptor, regardless of the eventual return value. Therefore, potentially looping and calling close multiple times is wrong. At best, we call close again on an invalid FD and throw a spurious EBADF error. At worst, we might close an FD which doesn't belong to us when a multi-threaded application opens its own file descriptor between iterations of the loop. Signed-off-by: Dave Reisner <dreisner(a)archlinux.org> Signed-off-by: Allan McRae <allan(a)archlinux.org> commit 7b8f8753b15037bf4a88126ffde8195416432c72 Author: Simon Gomizelj <simongmzlj(a)gmail.com> Date: Wed Jul 3 22:34:14 2013 -0400 Consolidate and improve table implementations Implement both the VerbosePkgList and the summary message with the same table. Improve VerbosePkgList by caching attributes and cell's lengths instead of recaculating them. Right align every cell that containing a file size in both the VerbosePkgList and the summary. Simplify the printf statements and the alignment application. Signed-off-by: Simon Gomizelj <simongmzlj(a)gmail.com> Signed-off-by: Allan McRae <allan(a)archlinux.org> ----------------------------------------------------------------------- Summary of changes: lib/libalpm/add.c | 6 +- lib/libalpm/be_package.c | 10 +- lib/libalpm/be_sync.c | 2 +- lib/libalpm/dload.c | 2 +- lib/libalpm/remove.c | 15 ++- lib/libalpm/util.c | 28 +++--- lib/libalpm/util.h | 1 - src/pacman/conf.c | 5 +- src/pacman/util.c | 237 ++++++++++++++++++++++++----------------------- 9 files changed, 160 insertions(+), 146 deletions(-) hooks/post-receive -- The official pacman repository

1 0

[pacman-dev] [PATCH] do not check error from close(2)
by Dave Reisner 04 Jul '13

04 Jul '13

On operating systems we support, the behavior is always such that the kernel will do the right thing as far as invalidating the file descriptor, regardless of the eventual return value. Therefore, potentially looping and calling close multiple times is wrong. At best, we call close again on an invalid FD and throw a spurious EBADF error. At worst, we might close an FD which doesn't belong to us when a multi-threaded application opens its own file descriptor between iterations of the loop. Signed-off-by: Dave Reisner <dreisner(a)archlinux.org> --- This goes further than the current patch on the ML and removes the CLOSE macro defined in alpm as well. lib/libalpm/add.c | 6 +++--- lib/libalpm/be_package.c | 10 +++++----- lib/libalpm/be_sync.c | 2 +- lib/libalpm/dload.c | 2 +- lib/libalpm/util.c | 28 ++++++++++++++-------------- lib/libalpm/util.h | 1 - src/pacman/conf.c | 5 +---- 7 files changed, 25 insertions(+), 29 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index c20e7c6..d6dc748 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -550,7 +550,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, _alpm_log(handle, ALPM_LOG_ERROR, _("could not change directory to %s (%s)\n"), handle->root, strerror(errno)); _alpm_archive_read_free(archive); - CLOSE(fd); + close(fd); ret = -1; goto cleanup; } @@ -580,7 +580,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, errors += extract_single_file(handle, archive, entry, newpkg, oldpkg); } _alpm_archive_read_free(archive); - CLOSE(fd); + close(fd); /* restore the old cwd if we have it */ if(cwdfd >= 0) { @@ -588,7 +588,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, _alpm_log(handle, ALPM_LOG_ERROR, _("could not restore working directory (%s)\n"), strerror(errno)); } - CLOSE(cwdfd); + close(cwdfd); } if(errors) { diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index 5a70968..6262621 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -76,7 +76,7 @@ static void *_package_changelog_open(alpm_pkg_t *pkg) if(!changelog) { pkg->handle->pm_errno = ALPM_ERR_MEMORY; _alpm_archive_read_free(archive); - CLOSE(fd); + close(fd); return NULL; } changelog->archive = archive; @@ -86,7 +86,7 @@ static void *_package_changelog_open(alpm_pkg_t *pkg) } /* we didn't find a changelog */ _alpm_archive_read_free(archive); - CLOSE(fd); + close(fd); errno = ENOENT; return NULL; @@ -126,7 +126,7 @@ static int _package_changelog_close(const alpm_pkg_t UNUSED *pkg, void *fp) int ret; struct package_changelog *changelog = fp; ret = _alpm_archive_read_free(changelog->archive); - CLOSE(changelog->fd); + close(changelog->fd); free(changelog); return ret; } @@ -477,7 +477,7 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, } _alpm_archive_read_free(archive); - CLOSE(fd); + close(fd); /* internal fields for package struct */ newpkg->origin = ALPM_PKG_FROM_FILE; @@ -510,7 +510,7 @@ error: _alpm_pkg_free(newpkg); _alpm_archive_read_free(archive); if(fd >= 0) { - CLOSE(fd); + close(fd); } return NULL; diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index f9fd5d1..feda6f5 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -474,7 +474,7 @@ static int sync_db_populate(alpm_db_t *db) cleanup: _alpm_archive_read_free(archive); if(fd >= 0) { - CLOSE(fd); + close(fd); } return count; } diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 7513822..b759c78 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -366,7 +366,7 @@ static FILE *create_tempfile(struct dload_payload *payload, const char *localpat fchmod(fd, ~(_getumask()) & 0666) || !(fp = fdopen(fd, payload->tempfile_openmode))) { unlink(randpath); - CLOSE(fd); + close(fd); _alpm_log(payload->handle, ALPM_LOG_ERROR, _("failed to create temporary file for download\n")); free(randpath); diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index f84fb66..78cfc49 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -186,10 +186,10 @@ int _alpm_copyfile(const char *src, const char *dest) cleanup: free(buf); if(in >= 0) { - CLOSE(in); + close(in); } if(out >= 0) { - CLOSE(out); + close(out); } return ret; } @@ -275,7 +275,7 @@ error: _alpm_archive_read_free(*archive); *archive = NULL; if(fd >= 0) { - CLOSE(fd); + close(fd); } RET_ERR(handle, error, -1); } @@ -394,13 +394,13 @@ int _alpm_unpack(alpm_handle_t *handle, const char *path, const char *prefix, cleanup: umask(oldmask); _alpm_archive_read_free(archive); - CLOSE(fd); + close(fd); if(cwdfd >= 0) { if(fchdir(cwdfd) != 0) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not restore working directory (%s)\n"), strerror(errno)); } - CLOSE(cwdfd); + close(cwdfd); } return ret; @@ -537,12 +537,12 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[]) if(pid == 0) { /* this code runs for the child only (the actual chroot/exec) */ - CLOSE(1); - CLOSE(2); + close(1); + close(2); while(dup2(pipefd[1], 1) == -1 && errno == EINTR); while(dup2(pipefd[1], 2) == -1 && errno == EINTR); - CLOSE(pipefd[0]); - CLOSE(pipefd[1]); + close(pipefd[0]); + close(pipefd[1]); /* use fprintf instead of _alpm_log to send output through the parent */ if(chroot(handle->root) != 0) { @@ -564,10 +564,10 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[]) int status; FILE *pipe_file; - CLOSE(pipefd[1]); + close(pipefd[1]); pipe_file = fdopen(pipefd[0], "r"); if(pipe_file == NULL) { - CLOSE(pipefd[0]); + close(pipefd[0]); retval = 1; } else { while(!feof(pipe_file)) { @@ -609,7 +609,7 @@ cleanup: _alpm_log(handle, ALPM_LOG_ERROR, _("could not restore working directory (%s)\n"), strerror(errno)); } - CLOSE(cwdfd); + close(cwdfd); } return retval; @@ -784,7 +784,7 @@ static int md5_file(const char *path, unsigned char output[16]) MD5_Update(&ctx, buf, n); } - CLOSE(fd); + close(fd); free(buf); if(n < 0) { @@ -834,7 +834,7 @@ static int sha2_file(const char *path, unsigned char output[32], int is224) } } - CLOSE(fd); + close(fd); free(buf); if(n < 0) { diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h index 56031f3..8e577de 100644 --- a/lib/libalpm/util.h +++ b/lib/libalpm/util.h @@ -88,7 +88,6 @@ void _alpm_alloc_fail(size_t size); #endif #define OPEN(fd, path, flags) do { fd = open(path, flags | O_BINARY); } while(fd == -1 && errno == EINTR) -#define CLOSE(fd) do { int _ret; do { _ret = close(fd); } while(_ret == -1 && errno == EINTR); } while(0) /** * Used as a buffer/state holder for _alpm_archive_fgets(). diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 5adc96c..2985aba 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -257,14 +257,11 @@ static int download_with_xfercommand(const char *url, const char *localpath, cleanup: /* restore the old cwd if we have it */ if(cwdfd >= 0) { - int close_ret; if(fchdir(cwdfd) != 0) { pm_printf(ALPM_LOG_ERROR, _("could not restore working directory (%s)\n"), strerror(errno)); } - do { - close_ret = close(cwdfd); - } while(close_ret == -1 && errno == EINTR); + close(cwdfd); } if(ret == -1) { -- 1.8.3.2

2 1

[pacman-dev] [PATCH] use full path when checking for mountpoints
by Andrew Gregory 04 Jul '13

04 Jul '13

Signed-off-by: Andrew Gregory <andrew.gregory.8(a)gmail.com> --- lib/libalpm/remove.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index 0b4f80c..7cb86ff 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -267,6 +267,16 @@ int _alpm_remove_prepare(alpm_handle_t *handle, alpm_list_t **data) return 0; } +/** + * @brief Test if a directory is being used as a mountpoint. + * + * @param handle context handle + * @param directory path to test, must be absolute and include trailing '/' + * @param stbuf stat result for @a directory, may be NULL + * + * @return 0 if @a directory is not a mountpoint or on error, 1 if @a directory + * is a mountpoint + */ static int dir_is_mountpoint(alpm_handle_t *handle, const char *directory, const struct stat *stbuf) { @@ -317,13 +327,14 @@ static int can_remove_file(alpm_handle_t *handle, const alpm_file_t *file, return 1; } + snprintf(filepath, PATH_MAX, "%s%s", handle->root, file->name); + if(file->name[strlen(file->name) - 1] == '/' && - dir_is_mountpoint(handle, file->name, NULL)) { + dir_is_mountpoint(handle, filepath, NULL)) { /* we do not remove mountpoints */ return 1; } - snprintf(filepath, PATH_MAX, "%s%s", handle->root, file->name); /* If we fail write permissions due to a read-only filesystem, abort. * Assume all other possible failures are covered somewhere else */ if(_alpm_access(handle, NULL, filepath, W_OK) == -1) { -- 1.8.3.2

1 0

[pacman-dev] [PATCH] Consolidate and improve table implementations
by Simon Gomizelj 04 Jul '13

04 Jul '13

Implement both the VerbosePkgList and the summary message with the same table. Improve VerbosePkgList by caching attributes and cell's lengths instead of recaculating them. Right align every cell that containing a file size in both the VerbosePkgList and the summary. Simplify the printf statements and the alignment application. --- src/pacman/util.c | 237 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 123 insertions(+), 114 deletions(-) diff --git a/src/pacman/util.c b/src/pacman/util.c index bd09adc..acbf217 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -47,9 +47,17 @@ #include "callback.h" -struct table_row_t { - const char *label; - off_t size; +struct table_cell_t { + char *label; + size_t len; + int mode; +}; + +enum { + CELL_NORMAL = 0, + CELL_TITLE = 1, + CELL_RIGHT_ALIGN = 1 << 2, + CELL_FREE = 1 << 3 }; int trans_init(alpm_transflag_t flags, int check_valid) @@ -424,6 +432,60 @@ static size_t string_length(const char *s) return len; } +static void add_table_cell(alpm_list_t **row, char *label, int mode) +{ + struct table_cell_t *cell = malloc(sizeof(struct table_cell_t)); + + cell->label = label; + cell->mode = mode; + cell->len = string_length(label); + + *row = alpm_list_add(*row, cell); +} + +static void table_free_cell(void *ptr) +{ + struct table_cell_t *cell = ptr; + + if(cell) { + if((cell->mode & CELL_FREE) && cell->label) { + free(cell->label); + } + free(cell); + } +} + +static void table_free(alpm_list_t *headers, alpm_list_t *rows) +{ + alpm_list_t *i; + + alpm_list_free_inner(headers, table_free_cell); + + for(i = rows; i; i = alpm_list_next(i)) { + if(i->data) { + alpm_list_free_inner(i->data, table_free_cell); + alpm_list_free(i->data); + } + } + + alpm_list_free(headers); + alpm_list_free(rows); +} + +static void add_transaction_sizes_row(alpm_list_t **table, char *label, int size) +{ + alpm_list_t *row = NULL; + char *str; + const char *units; + double s = humanize_size(size, 'M', 2, &units); + pm_asprintf(&str, "%.2f %s", s, units); + + add_table_cell(&row, label, CELL_TITLE); + add_table_cell(&row, str, CELL_RIGHT_ALIGN | CELL_FREE); + + *table = alpm_list_add(*table, row); +} + void string_display(const char *title, const char *string, unsigned short cols) { if(title) { @@ -442,43 +504,30 @@ void string_display(const char *title, const char *string, unsigned short cols) static void table_print_line(const alpm_list_t *line, short col_padding, size_t colcount, size_t *widths, int *has_data) { - size_t i, lastcol = 0; + size_t i; int need_padding = 0; const alpm_list_t *curcell; - for(i = colcount; i > 0; i--) { - if(has_data[i - 1]) { - lastcol = i - 1; - break; - } - } - for(i = 0, curcell = line; curcell && i < colcount; i++, curcell = alpm_list_next(curcell)) { - const char *value; + const struct table_cell_t *cell = curcell->data; + const char *str = (cell->label ? cell->label : ""); int cell_padding; if(!has_data[i]) { continue; } - value = curcell->data; - if(!value) { - value = ""; - } - /* silly printf requires padding size to be an int */ - cell_padding = (int)widths[i] - (int)string_length(value); - if(cell_padding < 0) { - cell_padding = 0; - } + cell_padding = (cell->mode & CELL_RIGHT_ALIGN ? (int)widths[i] : -(int)widths[i]); + if(need_padding) { printf("%*s", col_padding, ""); } - /* left-align all but the last column */ - if(i != lastcol) { - printf("%s%*s", value, cell_padding, ""); + + if(cell->mode & CELL_TITLE) { + printf("%s%*s%s", config->colstr.title, cell_padding, str, config->colstr.nocolor); } else { - printf("%*s%s", cell_padding, "", value); + printf("%*s", cell_padding, str); } need_padding = 1; } @@ -487,7 +536,6 @@ static void table_print_line(const alpm_list_t *line, short col_padding, } - /** * Find the max string width of each column. Also determines whether values * exist in the column and sets the value in has_data accordingly. @@ -522,7 +570,8 @@ static size_t table_calc_widths(const alpm_list_t *header, } /* header determines column count and initial values of longest_strs */ for(i = header, curcol = 0; i; i = alpm_list_next(i), curcol++) { - colwidths[curcol] = string_length(i->data); + const struct table_cell_t *row = i->data; + colwidths[curcol] = row->len; /* note: header does not determine whether column has data */ } @@ -531,8 +580,8 @@ static size_t table_calc_widths(const alpm_list_t *header, /* grab first column of each row and iterate through columns */ const alpm_list_t *j = i->data; for(curcol = 0; j; j = alpm_list_next(j), curcol++) { - const char *str = j->data; - size_t str_len = string_length(str); + const struct table_cell_t *row = j->data; + size_t str_len = row ? row->len : 0; if(str_len > colwidths[curcol]) { colwidths[curcol] = str_len; @@ -571,20 +620,24 @@ static size_t table_calc_widths(const alpm_list_t *header, * @param cols the number of columns available in the terminal * @return -1 if not enough terminal cols available, else 0 */ -static int table_display(const char *title, const alpm_list_t *header, +static int table_display(const alpm_list_t *header, const alpm_list_t *rows, unsigned short cols) { const unsigned short padding = 2; - const alpm_list_t *i; + const alpm_list_t *i, *first; size_t *widths = NULL, totalcols, totalwidth; int *has_data = NULL; - if(rows == NULL || header == NULL) { + if(rows == NULL) { return 0; } - totalcols = alpm_list_count(header); - totalwidth = table_calc_widths(header, rows, padding, totalcols, + /* we want the first row. if no headers are provided, use the first + * entry of the rows array. */ + first = header ? header : rows->data; + + totalcols = alpm_list_count(first); + totalwidth = table_calc_widths(first, rows, padding, totalcols, &widths, &has_data); /* return -1 if terminal is not wide enough */ if(totalwidth > cols) { @@ -596,13 +649,11 @@ static int table_display(const char *title, const alpm_list_t *header, return -1; } - if(title != NULL) { - printf("%s\n\n", title); + if (header) { + table_print_line(header, padding, totalcols, widths, has_data); + printf("\n"); } - table_print_line(header, padding, totalcols, widths, has_data); - printf("\n"); - for(i = rows; i; i = alpm_list_next(i)) { table_print_line(i->data, padding, totalcols, widths, has_data); } @@ -759,23 +810,20 @@ void signature_display(const char *title, alpm_siglist_t *siglist, } /* creates a header row for use with table_display */ -static alpm_list_t *create_verbose_header(void) +static alpm_list_t *create_verbose_header(size_t count) { - alpm_list_t *res = NULL; - char *str; + alpm_list_t *ret = NULL; + + char *header; + pm_asprintf(&header, "%s (%zd)", _("Package"), count); + + add_table_cell(&ret, header, CELL_TITLE | CELL_FREE); + add_table_cell(&ret, _("Old Version"), CELL_TITLE); + add_table_cell(&ret, _("New Version"), CELL_TITLE); + add_table_cell(&ret, _("Net Change"), CELL_TITLE); + add_table_cell(&ret, _("Download Size"), CELL_TITLE); - str = _("Name"); - res = alpm_list_add(res, str); - str = _("Old Version"); - res = alpm_list_add(res, str); - str = _("New Version"); - res = alpm_list_add(res, str); - str = _("Net Change"); - res = alpm_list_add(res, str); - str = _("Download Size"); - res = alpm_list_add(res, str); - - return res; + return ret; } /* returns package info as list of strings */ @@ -798,23 +846,23 @@ static alpm_list_t *create_verbose_row(pm_target_t *target) } else { pm_asprintf(&str, "%s", alpm_pkg_get_name(target->remove)); } - ret = alpm_list_add(ret, str); + add_table_cell(&ret, str, CELL_NORMAL); /* old and new versions */ pm_asprintf(&str, "%s", target->remove != NULL ? alpm_pkg_get_version(target->remove) : ""); - ret = alpm_list_add(ret, str); + add_table_cell(&ret, str, CELL_NORMAL); pm_asprintf(&str, "%s", target->install != NULL ? alpm_pkg_get_version(target->install) : ""); - ret = alpm_list_add(ret, str); + add_table_cell(&ret, str, CELL_NORMAL); /* and size */ size -= target->remove ? alpm_pkg_get_isize(target->remove) : 0; size += target->install ? alpm_pkg_get_isize(target->install) : 0; human_size = humanize_size(size, 'M', 2, &label); pm_asprintf(&str, "%.2f %s", human_size, label); - ret = alpm_list_add(ret, str); + add_table_cell(&ret, str, CELL_RIGHT_ALIGN); size = target->install ? alpm_pkg_download_size(target->install) : 0; if(size != 0) { @@ -823,55 +871,18 @@ static alpm_list_t *create_verbose_row(pm_target_t *target) } else { str = NULL; } - ret = alpm_list_add(ret, str); + add_table_cell(&ret, str, CELL_RIGHT_ALIGN); return ret; } -static void add_transaction_sizes_row(alpm_list_t **table, const char *label, off_t size) -{ - struct table_row_t *row = malloc(sizeof(struct table_row_t)); - - row->label = label; - row->size = size; - - *table = alpm_list_add(*table, row); -} - -static void display_transaction_sizes(alpm_list_t *table) -{ - alpm_list_t *i; - int max_len = 0; - - for(i = table; i; i = alpm_list_next(i)) { - struct table_row_t *row = i->data; - int len = string_length(row->label); - - if(len > max_len) { - max_len = len; - } - } - - max_len += 2; - - for(i = table; i; i = alpm_list_next(i)) { - struct table_row_t *row = i->data; - const char *units; - const colstr_t *colstr = &config->colstr; - double s = humanize_size(row->size, 'M', 2, &units); - - printf("%s%-*s%s %.2f %s\n", colstr->title, max_len, row->label, - colstr->nocolor, s, units); - } -} - /* prepare a list of pkgs to display */ static void _display_targets(alpm_list_t *targets, int verbose) { char *str; off_t isize = 0, rsize = 0, dlsize = 0; unsigned short cols; - alpm_list_t *i, *rows = NULL, *names = NULL, *table = NULL; + alpm_list_t *i, *names = NULL, *header = NULL, *rows = NULL; if(!targets) { return; @@ -895,7 +906,10 @@ static void _display_targets(alpm_list_t *targets, int verbose) for(i = targets; i; i = alpm_list_next(i)) { pm_target_t *target = i->data; - rows = alpm_list_add(rows, create_verbose_row(target)); + if(verbose) { + rows = alpm_list_add(rows, create_verbose_row(target)); + } + if(target->install) { pm_asprintf(&str, "%s-%s", alpm_pkg_get_name(target->install), alpm_pkg_get_version(target->install)); @@ -910,48 +924,43 @@ static void _display_targets(alpm_list_t *targets, int verbose) } /* print to screen */ - pm_asprintf(&str, "%s (%zd):", _("Packages"), alpm_list_count(targets)); + pm_asprintf(&str, "%s (%zd)", _("Packages"), alpm_list_count(targets)); printf("\n"); cols = getcols(fileno(stdout)); if(verbose) { - alpm_list_t *header = create_verbose_header(); - if(table_display(str, header, rows, cols) != 0) { + header = create_verbose_header(alpm_list_count(targets)); + if(table_display(header, rows, cols) != 0) { /* fallback to list display if table wouldn't fit */ list_display(str, names, cols); } - alpm_list_free(header); } else { list_display(str, names, cols); } printf("\n"); - /* rows is a list of lists of strings, free inner lists here */ - for(i = rows; i; i = alpm_list_next(i)) { - alpm_list_t *lp = i->data; - FREELIST(lp); - } - alpm_list_free(rows); + table_free(header, rows); FREELIST(names); free(str); + rows = NULL; if(dlsize > 0 || config->op_s_downloadonly) { - add_transaction_sizes_row(&table, _("Total Download Size:"), dlsize); + add_transaction_sizes_row(&rows, _("Total Download Size:"), dlsize); } if(!config->op_s_downloadonly) { if(isize > 0) { - add_transaction_sizes_row(&table, _("Total Installed Size:"), isize); + add_transaction_sizes_row(&rows, _("Total Installed Size:"), isize); } if(rsize > 0 && isize == 0) { - add_transaction_sizes_row(&table, _("Total Removed Size:"), rsize); + add_transaction_sizes_row(&rows, _("Total Removed Size:"), rsize); } /* only show this net value if different from raw installed size */ if(isize > 0 && rsize > 0) { - add_transaction_sizes_row(&table, _("Net Upgrade Size:"), isize - rsize); + add_transaction_sizes_row(&rows, _("Net Upgrade Size:"), isize - rsize); } } - display_transaction_sizes(table); - FREELIST(table); + table_display(NULL, rows, cols); + table_free(NULL, rows); } static int target_cmp(const void *p1, const void *p2) -- 1.8.3

3 8

[pacman-dev] [PATCH] Looping for EINTR on close() is wrong on Linux
by Sergio Correia 04 Jul '13

04 Jul '13

From: Sergio Correia <sergio(a)correia.cc> Linux closes the descriptor unconditionally even if the close() call is interrupted. This patch changes the CLOSE macro in libalpm not to loop on EINTR any longer. Now it is simply a close() call. There was another site performing such a loop on EINTR after close, in pacman/conf.c, and it was also changed to a simple close() call. Links for reference: - http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html - http://utcc.utoronto.ca/~cks/space/blog/unix/CloseEINTR - https://sites.google.com/site/michaelsafyan/software-engineering/checkforei… - http://ewontfix.com/4/ - http://austingroupbugs.net/view.php?id=529 Signed-off-by: Sergio Correia <sergio(a)correia.cc> --- lib/libalpm/util.h | 10 +++++++++- src/pacman/conf.c | 7 +++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h index 24b7c22..09834af 100644 --- a/lib/libalpm/util.h +++ b/lib/libalpm/util.h @@ -88,7 +88,15 @@ void _alpm_alloc_fail(size_t size); #endif #define OPEN(fd, path, flags) do { fd = open(path, flags | O_BINARY); } while(fd == -1 && errno == EINTR) -#define CLOSE(fd) do { int _ret; do { _ret = close(fd); } while(_ret == -1 && errno == EINTR); } while(0) + +/* Linux closes the file descriptor unconditionally, even if it returns EINTR, and hence looping for such + * condition after calling close() is wrong and can be potentially dangerous, especially in multithreaded code. + * http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html + * http://utcc.utoronto.ca/~cks/space/blog/unix/CloseEINTR + * https://sites.google.com/site/michaelsafyan/software-engineering/checkforei… + * http://ewontfix.com/4/ + * http://austingroupbugs.net/view.php?id=529 */ +#define CLOSE(fd) do { close(fd); } while(0) /** * Used as a buffer/state holder for _alpm_archive_fgets(). diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 5adc96c..8a42306 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -257,14 +257,13 @@ static int download_with_xfercommand(const char *url, const char *localpath, cleanup: /* restore the old cwd if we have it */ if(cwdfd >= 0) { - int close_ret; if(fchdir(cwdfd) != 0) { pm_printf(ALPM_LOG_ERROR, _("could not restore working directory (%s)\n"), strerror(errno)); } - do { - close_ret = close(cwdfd); - } while(close_ret == -1 && errno == EINTR); + /* looping for EINTR on close() is wrong on Linux, as it closes the + * descriptor unconditionally even if the close() call is interrupted.*/ + close(cwdfd); } if(ret == -1) { -- 1.8.3.1

5 8

[pacman-dev] [PATCH] makepkg: Log commands to logfile
by Alain Kalker 03 Jul '13

03 Jul '13

For debugging PKGBUILDs, and for reporting build failures upstream, it is very important to know the exact commands that are executed leading up to a problem. By using `set -x` and an appropriate value for PS4, commands are logged before they are executed. Signed-off-by: Alain Kalker <a.c.kalker(a)gmail.com> --- scripts/makepkg.sh.in | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 2a39157..859159f 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1468,8 +1468,15 @@ run_function() { tee "$BUILDLOG" < "$logpipe" & local teepid=$! + local restoreps4=$PS4 + PS4='$ ' + set -x + $pkgfunc &>"$logpipe" + set +x + PS4=$restoreps4 + wait $teepid rm "$logpipe" else -- 1.8.3.2

3 9