From 97e48be39e80ffe50946606b912ca28d3612cc61 Mon Sep 17 00:00:00 2001 From: Dennis Camera Date: Sat, 20 Jun 2020 21:11:28 +0200 Subject: [PATCH 1/6] [type/__package_opkg] Fix explorer running in parallel --- .../type/__package_opkg/explorer/pkg_status | 57 ++++++++++++++----- 1 file changed, 44 insertions(+), 13 deletions(-) diff --git a/cdist/conf/type/__package_opkg/explorer/pkg_status b/cdist/conf/type/__package_opkg/explorer/pkg_status index 5da4f742..1ba88e81 100755 --- a/cdist/conf/type/__package_opkg/explorer/pkg_status +++ b/cdist/conf/type/__package_opkg/explorer/pkg_status @@ -1,7 +1,8 @@ -#!/bin/sh +#!/bin/sh -e # # 2011 Nico Schottelius (nico-cdist at schottelius.org) # 2012 Giel van Schijndel (giel plus cdist at mortis dot eu) +# 2020 Dennis Camera (dennis.camera at ssrq-sds-fds.ch) # # This file is part of cdist. # @@ -19,21 +20,51 @@ # along with cdist. If not, see . # # -# Retrieve the status of a package - parsed opkg output +# Retrieve the status of a package - parses opkg output # -if [ -f "$__object/parameter/name" ]; then - name="$(cat "$__object/parameter/name")" +LOCKFILE="${__type_explorer}/cdist_opkg.lock" +_lock() ( + set -o noclobber + until echo $$>"${LOCKFILE}" + do + while test -f "${LOCKFILE}"; do sleep 1; done + done + +) 2>/dev/null +_unlock() { + if test -s "${LOCKFILE}" && test "$(cat "${LOCKFILE}")" = $$ + then + rm "${LOCKFILE}" + fi +} + + +if test -f "${__object}/parameter/name" +then + pkg_name=$(cat "${__object}/parameter/name") else - name="$__object_id" + pkg_name=$__object_id fi -# Except dpkg failing, if package is not known / installed -if opkg status "$name" 2>/dev/null | grep -q "^Status: install user installed$"; then - echo "present" - exit 0 -elif [ "$(opkg info "$name" 2> /dev/null | wc -l)" -eq 0 ]; then - echo "absent notpresent" - exit 0 + +# NOTE: We need to lock parallel execution of this explorer because opkg will +# try to acquire the OPKG lock (usually /var/lock/opkg.lock) using lockf(2) for +# every operation. It will not wait for the lock but terminate with an error. +# This leads to incorrect 'absent notpresent' statuses when parallel execution +# is enabled. +trap _unlock EXIT +_lock + + +# Except opkg failing, if package is not known / installed +if opkg status "${pkg_name}" 2>/dev/null \ + | grep -q -e '^Status: [^ ][^ ]* [^ ][^ ]* installed$' +then + echo 'present' +elif opkg info "${pkg_name}" 2>/dev/null | grep -q . +then + echo 'absent notpresent' +else + echo 'absent' fi -echo "absent" From e79b26a61f790108b90c3625d554f6df4086d616 Mon Sep 17 00:00:00 2001 From: Dennis Camera Date: Sun, 21 Jun 2020 13:15:38 +0200 Subject: [PATCH 2/6] [type/__package_opkg] Also lock execution of code-remote --- .../type/__package_opkg/explorer/pkg_status | 11 ++- cdist/conf/type/__package_opkg/gencode-remote | 94 +++++++++++++------ 2 files changed, 73 insertions(+), 32 deletions(-) diff --git a/cdist/conf/type/__package_opkg/explorer/pkg_status b/cdist/conf/type/__package_opkg/explorer/pkg_status index 1ba88e81..f5a6f098 100755 --- a/cdist/conf/type/__package_opkg/explorer/pkg_status +++ b/cdist/conf/type/__package_opkg/explorer/pkg_status @@ -23,7 +23,9 @@ # Retrieve the status of a package - parses opkg output # -LOCKFILE="${__type_explorer}/cdist_opkg.lock" +__type_path=${__object%%${__object_id}*} + +LOCKFILE="${__type_path}/cdist_opkg.lock" _lock() ( set -o noclobber until echo $$>"${LOCKFILE}" @@ -48,9 +50,10 @@ else fi -# NOTE: We need to lock parallel execution of this explorer because opkg will -# try to acquire the OPKG lock (usually /var/lock/opkg.lock) using lockf(2) for -# every operation. It will not wait for the lock but terminate with an error. +# NOTE: We need to lock parallel execution of type explorers and code-remote +# because opkg will try to acquire the OPKG lock (usually /var/lock/opkg.lock) +# using lockf(2) for every operation. +# It will not wait for the lock but terminate with an error. # This leads to incorrect 'absent notpresent' statuses when parallel execution # is enabled. trap _unlock EXIT diff --git a/cdist/conf/type/__package_opkg/gencode-remote b/cdist/conf/type/__package_opkg/gencode-remote index 269d5f49..ad90dc24 100755 --- a/cdist/conf/type/__package_opkg/gencode-remote +++ b/cdist/conf/type/__package_opkg/gencode-remote @@ -2,6 +2,7 @@ # # 2011,2013 Nico Schottelius (nico-cdist at schottelius.org) # 2012 Giel van Schijndel (giel plus cdist at mortis dot eu) +# 2020 Dennis Camera (dennis.camera at ssrq-sds-fds.ch) # # This file is part of cdist. # @@ -19,41 +20,78 @@ # along with cdist. If not, see . # # -# Manage packages on OpenWRT and co. +# Manage packages on OpenWrt, optware, and co. # -if [ -f "$__object/parameter/name" ]; then - name="$(cat "$__object/parameter/name")" +if test -f "${__object}/parameter/name" +then + name=$(cat "${__object}/parameter/name") else - name="$__object_id" + name=$__object_id fi -state_should="$(cat "$__object/parameter/state")" +state_should=$(cat "${__object}/parameter/state") +state_is=$(cat "${__object}/explorer/pkg_status") -state_is="$(cat "$__object/explorer/pkg_status")" -case "$state_is" in - absent*) - present="$(echo "$state_is" | cut -d ' ' -f 2)" - state_is="absent" - ;; +case $state_is +in + (absent*) + presence=$(echo "${state_is}" | cut -d ' ' -f 2) + state_is='absent' + ;; esac -[ "$state_is" = "$state_should" ] && exit 0 +if test "${state_is}" = "${state_should}" +then + exit 0 +fi -case "$state_should" in - present) - if [ "$present" = "notpresent" ]; then - echo "opkg --verbosity=0 update" - fi - echo "opkg --verbosity=0 install '$name'" - echo "installed" >> "$__messages_out" - ;; - absent) - echo "opkg --verbosity=0 remove '$name'" - echo "removed" >> "$__messages_out" - ;; - *) - echo "Unknown state: ${state_should}" >&2 - exit 1 - ;; +cat <<'EOF' +__type_path=${__object%%${__object_id}*} + +LOCKFILE="${__type_path}/cdist_opkg.lock" +_lock() ( + set -o noclobber + until echo $$>"${LOCKFILE}" + do + while test -f "${LOCKFILE}"; do sleep 1; done + done + +) 2>/dev/null +_unlock() { + if test -s "${LOCKFILE}" && test "$(cat "${LOCKFILE}")" = $$ + then + rm "${LOCKFILE}" + fi +} +EOF + +# NOTE: We need to lock parallel execution of code-remote to ensure that it is +# not executed concurrently with a type explorer. +# opkg will try to acquire the OPKG lock (usually /var/lock/opkg.lock) using +# lockf(2) for every operation. +# It will not wait for the lock but terminate with an error leading to an +# incorrect outcome. +echo 'trap _unlock EXIT' +echo '_lock' + +case $state_should +in + (present) + if test "${presence}" = 'notpresent' + then + echo 'opkg --verbosity=0 update' + fi + + printf "opkg --verbosity=0 install '%s'\n" "${name}" + echo 'installed' >>"${__messages_out}" + ;; + (absent) + printf "opkg --verbosity=0 remove '%s'" "${name}" + echo 'removed' >>"${__messages_out}" + ;; + (*) + printf 'Unknown state: %s\n' "${state_should}" >&2 + exit 1 + ;; esac From 6aae58dea77002dea0b57d20cbb01b5e2a1066b5 Mon Sep 17 00:00:00 2001 From: Dennis Camera Date: Sun, 21 Jun 2020 17:35:28 +0200 Subject: [PATCH 3/6] [type/__package_opkg] Mark lock variables readonly --- cdist/conf/type/__package_opkg/explorer/pkg_status | 4 ++-- cdist/conf/type/__package_opkg/gencode-remote | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cdist/conf/type/__package_opkg/explorer/pkg_status b/cdist/conf/type/__package_opkg/explorer/pkg_status index f5a6f098..9d993055 100755 --- a/cdist/conf/type/__package_opkg/explorer/pkg_status +++ b/cdist/conf/type/__package_opkg/explorer/pkg_status @@ -23,9 +23,9 @@ # Retrieve the status of a package - parses opkg output # -__type_path=${__object%%${__object_id}*} +readonly __type_path=${__object%%${__object_id}*} +readonly LOCKFILE="${__type_path}/cdist_opkg.lock" -LOCKFILE="${__type_path}/cdist_opkg.lock" _lock() ( set -o noclobber until echo $$>"${LOCKFILE}" diff --git a/cdist/conf/type/__package_opkg/gencode-remote b/cdist/conf/type/__package_opkg/gencode-remote index ad90dc24..c7f21751 100755 --- a/cdist/conf/type/__package_opkg/gencode-remote +++ b/cdist/conf/type/__package_opkg/gencode-remote @@ -47,9 +47,9 @@ then fi cat <<'EOF' -__type_path=${__object%%${__object_id}*} +readonly __type_path=${__object%%${__object_id}*} +readonly LOCKFILE="${__type_path}/cdist_opkg.lock" -LOCKFILE="${__type_path}/cdist_opkg.lock" _lock() ( set -o noclobber until echo $$>"${LOCKFILE}" From 3649555f3522ea80f1999ca2cb7f8ddd112cbb33 Mon Sep 17 00:00:00 2001 From: Dennis Camera Date: Mon, 22 Jun 2020 09:31:59 +0200 Subject: [PATCH 4/6] [type/__package_opkg] Do not lock execution of code-remote (revert) Instead, rely on `nonparallel`. In any case cdist should never run explorer and code concurrently even if the dependency graph would allow to do so as it would result in many more synchronization issues than this one. --- cdist/conf/type/__package_opkg/gencode-remote | 32 ++----------------- 1 file changed, 2 insertions(+), 30 deletions(-) diff --git a/cdist/conf/type/__package_opkg/gencode-remote b/cdist/conf/type/__package_opkg/gencode-remote index c7f21751..28caff71 100755 --- a/cdist/conf/type/__package_opkg/gencode-remote +++ b/cdist/conf/type/__package_opkg/gencode-remote @@ -25,9 +25,9 @@ if test -f "${__object}/parameter/name" then - name=$(cat "${__object}/parameter/name") + name=$(cat "${__object}/parameter/name") else - name=$__object_id + name=$__object_id fi state_should=$(cat "${__object}/parameter/state") @@ -46,34 +46,6 @@ then exit 0 fi -cat <<'EOF' -readonly __type_path=${__object%%${__object_id}*} -readonly LOCKFILE="${__type_path}/cdist_opkg.lock" - -_lock() ( - set -o noclobber - until echo $$>"${LOCKFILE}" - do - while test -f "${LOCKFILE}"; do sleep 1; done - done - -) 2>/dev/null -_unlock() { - if test -s "${LOCKFILE}" && test "$(cat "${LOCKFILE}")" = $$ - then - rm "${LOCKFILE}" - fi -} -EOF - -# NOTE: We need to lock parallel execution of code-remote to ensure that it is -# not executed concurrently with a type explorer. -# opkg will try to acquire the OPKG lock (usually /var/lock/opkg.lock) using -# lockf(2) for every operation. -# It will not wait for the lock but terminate with an error leading to an -# incorrect outcome. -echo 'trap _unlock EXIT' -echo '_lock' case $state_should in From a9778965be7154c0f42cf0170142ee2ec571d7ec Mon Sep 17 00:00:00 2001 From: Dennis Camera Date: Wed, 24 Jun 2020 08:47:22 +0200 Subject: [PATCH 5/6] [type/__package_opkg] Use mkdir(1) to lock instead of noclobber noclobber is potentially unsafe, because it relies on the underlying shell to implement noclobber in a safe way that avoids race conditions between multiple processes. mkdir is safer because it is mandated by POSIX to "fail" if the target already exists. --- .../type/__package_opkg/explorer/pkg_status | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/cdist/conf/type/__package_opkg/explorer/pkg_status b/cdist/conf/type/__package_opkg/explorer/pkg_status index 9d993055..09c550e5 100755 --- a/cdist/conf/type/__package_opkg/explorer/pkg_status +++ b/cdist/conf/type/__package_opkg/explorer/pkg_status @@ -24,21 +24,28 @@ # readonly __type_path=${__object%%${__object_id}*} -readonly LOCKFILE="${__type_path}/cdist_opkg.lock" +test -d "${__type_path}" || { echo 'Cannot determine __type_path' >&2; exit 1; } +readonly LOCKDIR="${__type_path:?}/.cdist_opkg.lock.dir" -_lock() ( - set -o noclobber - until echo $$>"${LOCKFILE}" +_lock() { + until mkdir "${LOCKDIR:?}" 2>/dev/null do - while test -f "${LOCKFILE}"; do sleep 1; done + while test -d "${LOCKDIR}" + do + # DEBUG: printf 'Locked by PID: %u\n' "$(cat "${LOCKDIR}/pid")" + sleep 1 + done done - -) 2>/dev/null + echo $$ >"${LOCKDIR:?}/pid" +} _unlock() { - if test -s "${LOCKFILE}" && test "$(cat "${LOCKFILE}")" = $$ + test -d "${LOCKDIR}" || return 0 + if test -s "${LOCKDIR}/pid" then - rm "${LOCKFILE}" + test "$(cat "${LOCKDIR}/pid")" = $$ || return 1 + rm "${LOCKDIR:?}/pid" fi + rmdir "${LOCKDIR:?}" } From 5364d3bc9037969c415140523c5cac2f8891b967 Mon Sep 17 00:00:00 2001 From: Dennis Camera Date: Wed, 24 Jun 2020 20:49:48 +0200 Subject: [PATCH 6/6] [type/__package_opkg] Implement flock locking if available --- .../type/__package_opkg/explorer/pkg_status | 55 ++++++++++++------- 1 file changed, 36 insertions(+), 19 deletions(-) diff --git a/cdist/conf/type/__package_opkg/explorer/pkg_status b/cdist/conf/type/__package_opkg/explorer/pkg_status index 09c550e5..de7b896b 100755 --- a/cdist/conf/type/__package_opkg/explorer/pkg_status +++ b/cdist/conf/type/__package_opkg/explorer/pkg_status @@ -25,28 +25,45 @@ readonly __type_path=${__object%%${__object_id}*} test -d "${__type_path}" || { echo 'Cannot determine __type_path' >&2; exit 1; } -readonly LOCKDIR="${__type_path:?}/.cdist_opkg.lock.dir" +readonly LOCKFILE="${__type_path:?}/.cdist_opkg.lock" -_lock() { - until mkdir "${LOCKDIR:?}" 2>/dev/null - do - while test -d "${LOCKDIR}" +if command -v flock >/dev/null 2>&1 +then + # use flock (if available) on FD 9 + _lock() { + exec 9<>"${LOCKFILE:?}" + flock -x 9 + echo $$>&9 + } + _unlock() { + :>"${LOCKFILE:?}" + flock -u 9 + exec 9<&- + } +else + # fallback to mkdir if flock is missing + _lock() { + until mkdir "${LOCKFILE:?}.dir" 2>/dev/null do - # DEBUG: printf 'Locked by PID: %u\n' "$(cat "${LOCKDIR}/pid")" - sleep 1 + while test -d "${LOCKFILE}.dir" + do + # DEBUG: + # printf 'Locked by PID: %u\n' "$(cat "${LOCKFILE}.dir/pid")" + sleep 1 + done done - done - echo $$ >"${LOCKDIR:?}/pid" -} -_unlock() { - test -d "${LOCKDIR}" || return 0 - if test -s "${LOCKDIR}/pid" - then - test "$(cat "${LOCKDIR}/pid")" = $$ || return 1 - rm "${LOCKDIR:?}/pid" - fi - rmdir "${LOCKDIR:?}" -} + echo $$ >"${LOCKFILE:?}.dir/pid" + } + _unlock() { + test -d "${LOCKFILE}.dir" || return 0 + if test -s "${LOCKFILE}.dir/pid" + then + test "$(cat "${LOCKFILE}.dir/pid")" = $$ || return 1 + rm "${LOCKFILE:?}.dir/pid" + fi + rmdir "${LOCKFILE:?}.dir" + } +fi if test -f "${__object}/parameter/name"