[pacman-dev] [PATCH 1/2] makepkg: fix regression in split package function checking
Commit 13748ca0529 inversed the nature of one test wherein the if clause would throw a fatal error if a legitimate package function was defined in PKGBUILD. Signed-off-by: Andres P <aepd87@gmail.com> --- scripts/makepkg.sh.in | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index dbc4047..f2ec78e 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1255,7 +1255,7 @@ check_sanity() { if (( ${#pkgname[@]} > 1 )); then for pkg in ${pkgname[@]}; do - if declare -f package_${pkg} >/dev/null; then + if ! declare -f package_${pkg} >/dev/null; then error "$(gettext "missing package function for split package '%s'")" "$pkg" return 1 fi -- 1.7.1
Instead of making the lock file with touch, use mkdir since it's the only portable atomic transaction available to shell scripts. Signed-off-by: Andres P <aepd87@gmail.com> --- scripts/pacman-optimize.sh.in | 12 +++++------- 1 files changed, 5 insertions(+), 7 deletions(-) diff --git a/scripts/pacman-optimize.sh.in b/scripts/pacman-optimize.sh.in index 78b2345..f95f827 100644 --- a/scripts/pacman-optimize.sh.in +++ b/scripts/pacman-optimize.sh.in @@ -67,7 +67,7 @@ die() { } die_r() { - rm -f "$lockfile" + rm -rf "$lock" die "$@" } @@ -109,15 +109,13 @@ fi # strip any trailing slash from our dbroot dbroot="${dbroot%/}" -# form the path to our lockfile location -lockfile="${dbroot}/db.lck" +# form the path to our db lock +lock="${dbroot}/db.lck" # make sure pacman isn't running -if [[ -f $lockfile ]]; then +if ! mkdir "$lock"; then die "$(gettext "Pacman lock file was found. Cannot run while pacman is running.")" fi -# do not let pacman run while we do this -touch "$lockfile" workdir=$(mktemp -d /tmp/pacman-optimize.XXXXXXXXXX) || die_r "$(gettext "ERROR: Can not create temp directory for database building.")\n" >&2 @@ -174,7 +172,7 @@ fi rm -rf "$dbroot.old" # remove the lock file and our working directory with sums and tarfile -rm -f "$lockfile" +rm -rf "$lock" rm -rf "$workdir" echo -- 1.7.1
On Mon, Jun 21, 2010 at 1:55 PM, Andres P <aepd87@gmail.com> wrote:
Instead of making the lock file with touch, use mkdir since it's the only portable atomic transaction available to shell scripts.
In fact, there is another way to do an atomic operation. Look at repo-add script, in function check_repo_db (I've seen this advice in some article some time ago too..). The first command tries to create a file with set -o noclobber. So, if the file already exists, it will fail and will not alter the lock file. If it doesn't exist, the lock file will be created and the process will proceed. if (set -o noclobber; echo "$$" > "$LOCKFILE") 2>/dev/null; then # We acquired the lock else # The lock already exists and we can't proceed. fi This would avoid changing the db.lock to a directory. Maybe this could break some kind of script or utility out there... -- A: Because it obfuscates the reading. Q: Why is top posting so bad? ------------------------------------------- Denis A. Altoe Falqueto -------------------------------------------
On Mon, Jun 21, 2010 at 12:37 PM, Denis A. Altoé Falqueto <denisfalqueto@gmail.com> wrote:
On Mon, Jun 21, 2010 at 1:55 PM, Andres P <aepd87@gmail.com> wrote:
Instead of making the lock file with touch, use mkdir since it's the only portable atomic transaction available to shell scripts.
In fact, there is another way to do an atomic operation. Look at repo-add script, in function check_repo_db (I've seen this advice in some article some time ago too..). The first command tries to create a file with set -o noclobber. So, if the file already exists, it will fail and will not alter the lock file. If it doesn't exist, the lock file will be created and the process will proceed.
if (set -o noclobber; echo "$$" > "$LOCKFILE") 2>/dev/null; then # We acquired the lock else # The lock already exists and we can't proceed. fi
Redirections are not atomic. First of all, echo isn't atomic so the ret val is old. Second, even when using this $ true > lock or simply $ > lock is still not atomic. I realize that this doesn't mean squat until I present my sources, but I will eventually. But the important factor is portability, as I'll explain below.
This would avoid changing the db.lock to a directory. Maybe this could break some kind of script or utility out there...
That would be a bug with that script in particular, because pacman does not care if it's a directory or not, as it should. The lock file in acpid, to name one of the many programs that smartly implement directories as lockfiles, is done this way for a reason. No clobber isn't available in all shells, so you are assuming that only bash/ksh will be interacting with the lockfile. makepkg and similar bash scripts in the source distribution aren't the only ways to interact with pacman. As proof, install scripts are supposed to be sh compatible. So we should keep compat with sh when it comes to something as low level and crucial as locking the db. Andres P
On Mon, Jun 21, 2010 at 2:22 PM, Andres P <aepd87@gmail.com> wrote:
On Mon, Jun 21, 2010 at 12:37 PM, Denis A. Altoé Falqueto <denisfalqueto@gmail.com> wrote:
On Mon, Jun 21, 2010 at 1:55 PM, Andres P <aepd87@gmail.com> wrote:
Instead of making the lock file with touch, use mkdir since it's the only portable atomic transaction available to shell scripts.
In fact, there is another way to do an atomic operation. Look at repo-add script, in function check_repo_db (I've seen this advice in some article some time ago too..). The first command tries to create a file with set -o noclobber. So, if the file already exists, it will fail and will not alter the lock file. If it doesn't exist, the lock file will be created and the process will proceed.
if (set -o noclobber; echo "$$" > "$LOCKFILE") 2>/dev/null; then # We acquired the lock else # The lock already exists and we can't proceed. fi
Redirections are not atomic.
First of all, echo isn't atomic so the ret val is old.
Second, even when using this $ true > lock or simply $ > lock is still not atomic.
Sure, there're not atomic operations in bash, but the important thing there is the setting -o noclobber. The code path that can lead to race condition is only the redirection, instead of the check-and-act normal way of doing locking. In fact, if it were not for the filesystem implementing the real creation of the directory, mkdir would not be atomic too.
This would avoid changing the db.lock to a directory. Maybe this could break some kind of script or utility out there...
That would be a bug with that script in particular, because pacman does not care if it's a directory or not, as it should.
The lock file in acpid, to name one of the many programs that smartly implement directories as lockfiles, is done this way for a reason.
No clobber isn't available in all shells, so you are assuming that only bash/ksh will be interacting with the lockfile.
Yes, I like portability too, but pacman-optimize starts with a #!/bin/bash, so we are actually using bash anyway, even if the user has a different default shell.
makepkg and similar bash scripts in the source distribution aren't the only ways to interact with pacman. As proof, install scripts are supposed to be sh compatible.
So we should keep compat with sh when it comes to something as low level and crucial as locking the db.
I don't want to argue or anything, just wanted to shed some light on a different implementation that would not create a new behavior (directory instead of file). I don't have any counter examples for your code, as it really works as expected :) -- A: Because it obfuscates the reading. Q: Why is top posting so bad? ------------------------------------------- Denis A. Altoe Falqueto -------------------------------------------
participants (2)
-
Andres P
-
Denis A. Altoé Falqueto