[PATCH 00/11] kas-contaner: shell scripting suggestions

[Konrad Schwarz]

This patch series contains small fixes and low-level code improvements to
`kas-container`.

Konrad Schwarz (11):
kas-container: error reporting logic
kas-container: clean/purge regex
kas-container: rootless isar docker control
kas-container: variable defaults
kas-container: pipeline/subshell simplifications
kas-container: avoid shell rescanning
kas-container: extracting KAS_BUILD_SYSTEM
kas-container: avoid backslash line continuation
kas-conatiner: default location of bash
kas-container: Usage message
kas-container: output usage to stderr on error

kas-container | 244 ++++++++++++++++++++++++++++----------------------
1 file changed, 138 insertions(+), 106 deletions(-)

--
2.47.3

[Konrad Schwarz]

Simplify the code by making `KAS_VERBOSE` hold the boolean values `true` or `false`.

Instead of expanding `$*` at the end of the debug string, pass arguments
as `"$@"` to `echo`. This achieves exactly the same result as before
without needing to create a large intermediate string.

Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
---
kas-container | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/kas-container b/kas-container
index 135fcf7..2a7b9dd 100755
--- a/kas-container
+++ b/kas-container
@@ -102,25 +102,24 @@ usage()

fatal_error()
{
- echo "${KAS_CONTAINER_SELF_NAME}: Error: $*" >&2
+ echo "${KAS_CONTAINER_SELF_NAME}: Error:" "$@" >&2
exit 1
}

warning()
{
- echo "${KAS_CONTAINER_SELF_NAME}: Warning: $*" >&2
+ echo "${KAS_CONTAINER_SELF_NAME}: Warning:" "$@" >&2
}

debug(){
- if [ -n "${KAS_VERBOSE}" ]; then
- echo "${KAS_CONTAINER_SELF_NAME}: Debug: $*" >&2
- fi
+ ${KAS_VERBOSE} &&
+ echo "${KAS_CONTAINER_SELF_NAME}: Debug:" "$@" >&2
}

trace()
{
- [ -n "${KAS_VERBOSE}" ] && echo "+ $*" >&2
- "$@"
+ ${KAS_VERBOSE} && echo "+" "$@" >&2
+ "$@"
}

prepare_sudo_cmd()
@@ -336,6 +335,7 @@ set_container_image_var()
# explicitly set via flag and implicitly via config.
BUILD_SYSTEM=""
KAS_OPTIONS_DIRECT=""
+KAS_VERBOSE=false
while [ $# -gt 0 ]; do
case "$1" in
--isar | --isar-privileged)
@@ -398,7 +398,7 @@ while [ $# -gt 0 ]; do
;;
-l | --log-level)
if [ "$2" = "debug" ]; then
- KAS_VERBOSE=1
+ KAS_VERBOSE=true
fi
KAS_OPTIONS_DIRECT="${KAS_OPTIONS_DIRECT} -l $2"
shift 2
--
2.47.3

[Konrad Schwarz]

Fix a precedence bug in the regex check for clean/purge:
anchors (`^`, `$`) bind more tightly than alternation (`|`).

Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
---

kas-container | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kas-container b/kas-container
index 2a7b9dd..56f3cd3 100755
--- a/kas-container
+++ b/kas-container
@@ -589,7 +589,7 @@ fi
# clean can be executed without config, hence manually forward the build system
case "${BUILD_SYSTEM}" in
isar*)
- if echo "${KAS_CMD}" | grep -qe "^clean\|purge"; then
+ if echo "${KAS_CMD}" | grep -qE -e '^clean|^purge'; then
KAS_OPTIONS="${KAS_OPTIONS} --${BUILD_SYSTEM}"
fi
;;
--
2.47.3

[Konrad Schwarz]

Use more compact shell syntax when setting default values,
eliminating duplication of variable names.

Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
---

kas-container | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/kas-container b/kas-container
index 1cd635f..68507f4 100755
--- a/kas-container
+++ b/kas-container
@@ -155,8 +155,9 @@ enable_isar_mode()
export PATH="${PATH}:/usr/sbin"
elif "${KAS_DOCKER_ROOTLESS}"; then
prepare_sudo_cmd
- DOCKER_HOST_DEFAULT="$(docker context inspect default --format '{{.Endpoints.docker.Host}}')"
- export DOCKER_HOST="${DOCKER_HOST:-$DOCKER_HOST_DEFAULT}"
+ export DOCKER_HOST="${DOCKER_HOST:-"$(
+ docker context inspect default --format '{{.Endpoints.docker.Host}}'
+ )"}"
debug "kas-isar does not support rootless docker. Using system docker in $DOCKER_HOST"
# force use of well-known system docker socket
KAS_CONTAINER_COMMAND="${_KAS_SUDO_CMD} ${KAS_CONTAINER_COMMAND}"
@@ -235,7 +236,7 @@ check_and_expand()
# shellcheck disable=2034
setup_kas_dirs()
{
- KAS_WORK_DIR="${KAS_WORK_DIR:-$(pwd)}"
+ : "${KAS_WORK_DIR:=$PWD}"
KAS_WORK_DIR="$(check_and_expand KAS_WORK_DIR required)"
KAS_BUILD_DIR="$(check_and_expand KAS_BUILD_DIR create)"
KAS_REPO_REF_DIR="$(check_and_expand KAS_REPO_REF_DIR required)"
@@ -323,10 +324,10 @@ set_container_image_var()
if [ -n "${KAS_CONTAINER_IMAGE}" ]; then
return
fi
- KAS_IMAGE_VERSION="${KAS_IMAGE_VERSION:-${KAS_IMAGE_VERSION_DEFAULT}}"
- KAS_CONTAINER_IMAGE_DISTRO="${KAS_CONTAINER_IMAGE_DISTRO:-${KAS_CONTAINER_IMAGE_DISTRO_DEFAULT}}"
- KAS_CONTAINER_IMAGE_NAME="${KAS_CONTAINER_IMAGE_NAME:-${KAS_CONTAINER_IMAGE_NAME_DEFAULT}}"
- KAS_CONTAINER_IMAGE_PATH="${KAS_CONTAINER_IMAGE_PATH:-${KAS_CONTAINER_IMAGE_PATH_DEFAULT}}"
+ : "${KAS_IMAGE_VERSION:=${KAS_IMAGE_VERSION_DEFAULT}}"
+ : "${KAS_CONTAINER_IMAGE_DISTRO:=${KAS_CONTAINER_IMAGE_DISTRO_DEFAULT}}"
+ : "${KAS_CONTAINER_IMAGE_NAME:=${KAS_CONTAINER_IMAGE_NAME_DEFAULT}}"
+ : "${KAS_CONTAINER_IMAGE_PATH:=${KAS_CONTAINER_IMAGE_PATH_DEFAULT}}"
KAS_CONTAINER_IMAGE="${KAS_CONTAINER_IMAGE_PATH}/${KAS_CONTAINER_IMAGE_NAME}:${KAS_IMAGE_VERSION}"
if [ -n "${KAS_CONTAINER_IMAGE_DISTRO}" ]; then
KAS_CONTAINER_IMAGE="${KAS_CONTAINER_IMAGE}-${KAS_CONTAINER_IMAGE_DISTRO}"
@@ -455,7 +456,7 @@ while [ $# -gt 0 ]; do
esac
done

-KAS_CONTAINER_ENGINE="${KAS_CONTAINER_ENGINE:-${KAS_DOCKER_ENGINE}}"
+: "${KAS_CONTAINER_ENGINE:=${KAS_DOCKER_ENGINE}}"
if [ -z "${KAS_CONTAINER_ENGINE}" ]; then
# Try to auto-detect a container engine
if command -v docker >/dev/null 2>&1 && docker -v 2>/dev/null | grep -q '^Docker'; then
@@ -602,7 +603,7 @@ set_container_image_var
if "${KAS_DOCKER_ROOTLESS}"; then
KAS_REPO_MOUNT_OPT_DEFAULT="ro"
fi
-KAS_REPO_MOUNT_OPT="${KAS_REPO_MOUNT_OPT:-${KAS_REPO_MOUNT_OPT_DEFAULT}}"
+: "${KAS_REPO_MOUNT_OPT:=${KAS_REPO_MOUNT_OPT_DEFAULT}}"

if [ "$(id -u)" -eq 0 ] && [ "${KAS_ALLOW_ROOT}" != "yes" ] ; then
fatal_error "Running as root - may break certain recipes." \
@@ -711,7 +712,7 @@ if [ -n "${KAS_GIT_CREDENTIAL_SOCKET}" ] ; then
set -- "$@" -v "$(realpath -e "${KAS_GIT_CREDENTIAL_SOCKET}")":/var/kas/userdata/.git-cache-socket
fi

-GIT_CREDENTIAL_HELPER="${GIT_CREDENTIAL_HELPER:-${KAS_GIT_CREDENTIAL_HELPER_DEFAULT}}"
+: "${GIT_CREDENTIAL_HELPER:=${KAS_GIT_CREDENTIAL_HELPER_DEFAULT}}"

if [ -n "${GIT_CREDENTIAL_HELPER}" ] ; then
set -- "$@" -e GIT_CREDENTIAL_HELPER="${GIT_CREDENTIAL_HELPER}"
--
2.47.3

[Konrad Schwarz]

Make `ISAR_DOCKER_ROOTLESS` a boolean: set it to either `true` or `false`.

Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
---

kas-container | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/kas-container b/kas-container
index 56f3cd3..1cd635f 100755
--- a/kas-container
+++ b/kas-container
@@ -153,14 +153,14 @@ enable_isar_mode()

KAS_CONTAINER_COMMAND="${_KAS_SUDO_CMD} ${KAS_CONTAINER_COMMAND}"

# preserved user PATH may lack sbin needed by privileged podman

export PATH="${PATH}:/usr/sbin"

- elif [ "${KAS_DOCKER_ROOTLESS}" = "1" ]; then
+ elif "${KAS_DOCKER_ROOTLESS}"; then
prepare_sudo_cmd

DOCKER_HOST_DEFAULT="$(docker context inspect default --format '{{.Endpoints.docker.Host}}')"

export DOCKER_HOST="${DOCKER_HOST:-$DOCKER_HOST_DEFAULT}"

debug "kas-isar does not support rootless docker. Using system docker in $DOCKER_HOST"
# force use of well-known system docker socket
KAS_CONTAINER_COMMAND="${_KAS_SUDO_CMD} ${KAS_CONTAINER_COMMAND}"

- KAS_DOCKER_ROOTLESS=0
+ KAS_DOCKER_ROOTLESS=false
fi
}

@@ -296,10 +296,12 @@ forward_dir()

check_docker_rootless()
{
- KAS_DOCKER_ROOTLESS=0
- if [ "$(docker context show)" = "rootless" ]; then
- KAS_DOCKER_ROOTLESS=1
- fi
+ case $(docker context show) in
+ rootless)
+ KAS_DOCKER_ROOTLESS=true;;
+ *)
+ KAS_DOCKER_ROOTLESS=false
+ esac
}

enable_docker_rootless()
@@ -597,7 +599,7 @@ esac

set_container_image_var

-if [ "${KAS_DOCKER_ROOTLESS}" = "1" ]; then
+if "${KAS_DOCKER_ROOTLESS}"; then
KAS_REPO_MOUNT_OPT_DEFAULT="ro"
fi
KAS_REPO_MOUNT_OPT="${KAS_REPO_MOUNT_OPT:-${KAS_REPO_MOUNT_OPT_DEFAULT}}"
@@ -608,7 +610,7 @@ if [ "$(id -u)" -eq 0 ] && [ "${KAS_ALLOW_ROOT}" != "yes" ] ; then
"KAS_ALLOW_ROOT=yes to override."
fi

-if [ "${KAS_DOCKER_ROOTLESS}" = "1" ]; then
+if "${KAS_DOCKER_ROOTLESS}"; then
enable_docker_rootless
fi

--
2.47.3

[Konrad Schwarz]

Simplify various pipelines/subshell invocations.

Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
---

kas-container | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/kas-container b/kas-container
index 68507f4..c070115 100755
--- a/kas-container
+++ b/kas-container
@@ -200,7 +200,9 @@ enable_oe_mode()

enable_unpriv_userns_docker()
{
- if [ -f /etc/os-release ] && grep -q 'NAME="Ubuntu"' /etc/os-release &&
+ # SC1091: FILE was not specified as input (/etc/os-release is assumed to be correct)
+ # shellcheck disable=1091
+ if [ -f /etc/os-release ] && (. /etc/os-release && test "$NAME" = Ubuntu) &&
[ -f /proc/sys/kernel/apparmor_restrict_unprivileged_userns ] &&
[ "$(cat /proc/sys/kernel/apparmor_restrict_unprivileged_userns)" = "1" ]; then
if [ -f /etc/apparmor.d/rootlesskit ]; then
@@ -250,10 +252,10 @@ setup_kas_dirs()
repo_path_of_file()
{
_DIR="$(dirname "$1")"
- _REPO_DIR=$(git -C "${_DIR}" rev-parse --show-toplevel 2>/dev/null) \
- || _REPO_DIR=$(hg --cwd "${_DIR}" root 2>/dev/null) \
- || _REPO_DIR=${_DIR}
- echo "$_REPO_DIR"
+ git -C "${_DIR}" rev-parse --show-toplevel 2>/dev/null ||
+ hg --cwd "${_DIR}" root 2>/dev/null ||
+ printf '%s\n' "${_DIR}"
+ unset _DIR
}

# Params: ARG
@@ -441,7 +443,7 @@ while [ $# -gt 0 ]; do
break
;;
dump)
- if printf '%s\0' "$@" | grep -xqz -- '--inplace\|-i'; then
+ if printf '%s\n' "$@" | grep -qE -e '^--inplace$|^-i$'; then
KAS_REPO_MOUNT_OPT_DEFAULT="rw"
else
KAS_REPO_MOUNT_OPT_DEFAULT="ro"
@@ -537,12 +539,11 @@ done
if [ -n "${KAS_FIRST_FILES}" ]; then
KAS_REPO_DIR=$(echo "${KAS_REPO_DIRS}" | awk '{print $1}')
else
- KAS_REPO_DIR=$(pwd)
+ KAS_REPO_DIR=$PWD
fi

SOURCE_DIR_HOST=$(
- grep -e "^_source_dir_host: " "${KAS_WORK_DIR}/.config.yaml" 2>/dev/null | \
- sed 's/_source_dir_host:[ ]\+//')
+ sed -n 's/^_source_dir_host: *//p' "${KAS_WORK_DIR}/.config.yaml")
if [ -n "${SOURCE_DIR_HOST}" ]; then
KAS_REPO_DIR="${SOURCE_DIR_HOST}"
fi
@@ -563,7 +564,9 @@ if [ "${KAS_CMD}" = "menu" ]; then
set -- "$@" -e _KAS_REPO_DIR_HOST="${KAS_REPO_DIR}"
fi

- if [ "$(echo "${KAS_FIRST_FILES}" | wc -w)" -ne "1" ]; then
+ # SC2086: Double quote to prevent globbing and word splitting.
+ # shellcheck disable=2086
+ if [ "$(set -- ${KAS_FIRST_FILES} && echo $#)" -ne "1" ]; then
fatal_error "menu plugin only supports a single Kconfig file"
fi
BUILD_SYSTEM=${BUILD_SYSTEM:-$(tr '\n' '\f' 2>/dev/null < "${KAS_FIRST_FILES}" |
@@ -576,9 +579,10 @@ else
fi

# We only get the first build system and let kas check if mixed
- _KAS_FIRST_FILE=$(echo "${KAS_FIRST_FILES}" | awk '{print $1}')
- BUILD_SYSTEM=${BUILD_SYSTEM:-$(grep -e "^build_system: " "${_KAS_FIRST_FILE}" 2>/dev/null |
- sed 's/build_system:[ ]\+//')}
+ # SC2086: Double quote to prevent globbing and word splitting.
+ # shellcheck disable=2086
+ _KAS_FIRST_FILE=$(set -- ${KAS_FIRST_FILES} && printf '%s\n' "$1")
+ : ${BUILD_SYSTEM:=$(sed -n '/^build_system: */s///p' "${_KAS_FIRST_FILE}")}
fi

if [ "${BUILD_SYSTEM}" = "isar" ] || [ "${BUILD_SYSTEM}" = "isar-privileged" ]; then
@@ -616,7 +620,9 @@ if "${KAS_DOCKER_ROOTLESS}"; then
fi

if [ "${KAS_CMD}" = "diff" ]; then
- if [ "$(echo "${KAS_FILES}" | wc -w)" -eq "2" ]; then
+ # SC2086: Double quote to prevent globbing and word splitting.
+ # shellcheck disable=2086
+ if [ "$(set -- ${KAS_FILES} && echo $#)" -eq "2" ]; then
_KAS_REPO_DIR1="$(echo "${KAS_REPO_DIRS}" | awk '{print $1}')"
_KAS_REPO_DIR2="$(echo "${KAS_REPO_DIRS}" | awk '{print $2}')"
_KAS_FILES1="$(echo "${KAS_FILES}" | awk '{print $1}' | sed 's|'"${_KAS_REPO_DIR1}"'/|/repo/|g')"
--
2.47.3

[Konrad Schwarz]

Fix a situation where arguments were being rescanned/subjected
to word splitting.

Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
---

kas-container | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kas-container b/kas-container
index c070115..0b40d3e 100755
--- a/kas-container
+++ b/kas-container
@@ -264,9 +264,14 @@ process_file_arg()
_KAS_FILES=
_KAS_FIRST_FILE=
_KAS_REPO_DIR=
+ _KAS_IFS=$IFS
+ IFS=:

# SC2086: Double quote to prevent globbing and word splitting.

# shellcheck disable=2086
- for FILE in $(IFS=':'; echo $ARG); do
+ set -- $ARG
+ IFS=$_KAS_IFS
+ unset _KAS_IFS
+ for FILE; do
if ! KAS_REAL_FILE="$(realpath -qe "$FILE")"; then
fatal_error "configuration file '${FILE}' not found"
fi
--
2.47.3

[Konrad Schwarz]

Posix sh and bash will automatically continue a line
ending in one of the operators `&&`, `||`, and `|`,
eliminating the need for a trailing backslash.

Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
---

kas-container | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kas-container b/kas-container
index 4d384a8..0ea3edf 100755
--- a/kas-container
+++ b/kas-container
@@ -232,6 +232,7 @@ check_and_expand()
;;
esac
realpath -e "$_varval"
+ unset _varval
}

# SC2034: DIR appears unused (ignore, as they are used inside eval)
@@ -660,8 +661,8 @@ forward_dir KAS_REPO_REF_DIR "/repo-ref" "rw"
forward_dir SSTATE_DIR "/sstate" "rw"
forward_dir KAS_BUILDTOOLS_DIR "/buildtools" "rw"

-if git_com_dir=$(git -C "${KAS_REPO_DIR}" rev-parse --git-common-dir 2>/dev/null) \
- && [ "$git_com_dir" != "$(git -C "${KAS_REPO_DIR}" rev-parse --git-dir)" ]; then
+if git_com_dir=$(git -C "${KAS_REPO_DIR}" rev-parse --git-common-dir 2>/dev/null) &&
+ [ "$git_com_dir" != "$(git -C "${KAS_REPO_DIR}" rev-parse --git-dir)" ]; then

KAS_GIT_OVERLAY_FILE=""
kas_container_cleanup()
--
2.47.3

[Konrad Schwarz]

The `sed` script uses GNU extensions (control character `\f`,
regex alternation) not in POSIX. The script also outputs
all lines containing `default` after `config KAS_BUILD_SYSTEM`,
which may be a problem if `KConfig` is ever extended.

Extract the `KAS_BUILD_SYSTEM` default from `Kconfig` without
converting the entire file into a single line, using
only POSIX `sed` features.

Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
---

kas-container | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/kas-container b/kas-container
index 0b40d3e..4d384a8 100755
--- a/kas-container
+++ b/kas-container
@@ -574,10 +574,20 @@ if [ "${KAS_CMD}" = "menu" ]; then

if [ "$(set -- ${KAS_FIRST_FILES} && echo $#)" -ne "1" ]; then
fatal_error "menu plugin only supports a single Kconfig file"
fi

- BUILD_SYSTEM=${BUILD_SYSTEM:-$(tr '\n' '\f' 2>/dev/null < "${KAS_FIRST_FILES}" |
- sed -e 's/\(.*\fconfig KAS_BUILD_SYSTEM\f\(.*\)\|.*\)/\2/' \
- -e 's/\f\([[:alpha:]].*\|$\)//' \
- -e 's/.*default \"\(.*\)\".*/\1/')}
+ BUILD_SYSTEM=${BUILD_SYSTEM:-$(sed -n '
+ /^config KAS_BUILD_SYSTEM$/b found_kas_build_system
+ d ;# delete & start the next cycle
+
+ : found_kas_build_system
+ N ;# append next line
+ s/.*\n// ;# delete previous line
+ /^[[:alpha:]]/b found_kas_build_system
+ /.*default "\([^"]*\)".*/ {
+ s//\1/p ;# output default value
+ q ;# and quit
+ }
+ b found_kas_build_system
+ ' "${KAS_FIRST_FILES}")}
else
if [ -z "${KAS_FIRST_FILES}" ]; then
KAS_FIRST_FILES="${KAS_WORK_DIR}/.config.yaml"
--
2.47.3

[Konrad Schwarz]

According to hier(7), `/bin` should contain programs
used in single user mode/for repair, while `/usr/bin`
is the primary directory for executable programs.

Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
---

kas-container | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kas-container b/kas-container
index 0ea3edf..416ef2d 100755
--- a/kas-container
+++ b/kas-container
@@ -788,11 +788,11 @@ done

# propagate only supported SHELL settings
case "$SHELL" in
-/bin/sh | /bin/bash | /bin/dash)
+/bin/sh | */bash | */dash)
set -- "$@" -e "SHELL=$SHELL"
;;
*)
- set -- "$@" -e "SHELL=/bin/bash"
+ set -- "$@" -e "SHELL=/usr/bin/bash"
;;
esac

--
2.47.3

[Konrad Schwarz]

If the usage message is output because of an error,
redirect the help message to standard error.

Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
---

kas-container | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/kas-container b/kas-container
index 508bc8f..392e021 100755
--- a/kas-container
+++ b/kas-container
@@ -40,6 +40,10 @@ usage()
EXIT_CODE="$1"
SELF="${KAS_CONTAINER_SELF_NAME}"

+ if [ "$EXIT_CODE" != 0 ]; then
+ exec >&2
+ fi
+
printf -- 'Usage: %s [OPTIONS] { build | shell } [KASOPTIONS] [KASFILE]\n' "${SELF}"
printf -- ' %s [OPTIONS] { checkout | dump | lock } [KASOPTIONS] [KASFILE]\n' "${SELF}"
printf -- ' %s [OPTIONS] { diff } [KASOPTIONS] config1 config2\n' "${SELF}"
--
2.47.3

[Konrad Schwarz]

Use the `--` argument delimiter to avoid spurious warnings about
arguments starting with `-`.

Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
---

kas-container | 115 ++++++++++++++++++++++++++------------------------
1 file changed, 59 insertions(+), 56 deletions(-)

diff --git a/kas-container b/kas-container
index 416ef2d..508bc8f 100755
--- a/kas-container
+++ b/kas-container
@@ -40,62 +40,65 @@ usage()

EXIT_CODE="$1"
SELF="${KAS_CONTAINER_SELF_NAME}"

- printf "%b" "Usage: ${SELF} [OPTIONS] { build | shell } [KASOPTIONS] [KASFILE]\n"
- printf "%b" " ${SELF} [OPTIONS] { checkout | dump | lock } [KASOPTIONS] [KASFILE]\n"
- printf "%b" " ${SELF} [OPTIONS] { diff } [KASOPTIONS] config1 config2\n"
- printf "%b" " ${SELF} [OPTIONS] for-all-repos [KASOPTIONS] [KASFILE] COMMAND\n"
- printf "%b" " ${SELF} [OPTIONS] { clean | cleansstate | cleanall | purge} [KASFILE]\n"
- printf "%b" " ${SELF} [OPTIONS] menu [KCONFIG]\n"
- printf "%b" "\nPositional arguments:\n"
- printf "%b" "build\t\t\tCheck out repositories and build target.\n"
- printf "%b" "checkout\t\tCheck out repositories but do not build.\n"
- printf "%b" "diff\t\t\tCompare two kas configurations.\n"
- printf "%b" "dump\t\t\tCheck out repositories and write flat version\n"
- printf "%b" " \t\t\tof config to stdout.\n"
- printf "%b" "lock\t\t\tCreate and update kas project lockfiles.\n"
- printf "%b" "shell\t\t\tRun a shell in the build environment.\n"
- printf "%b" "for-all-repos\t\tRun specified command in each repository.\n"
- printf "%b" "clean\t\t\tClean build artifacts, keep sstate cache and " \
- "downloads.\n"
- printf "%b" "cleansstate\t\tClean build artifacts and sstate cache, " \
- "keep downloads.\n"
- printf "%b" "cleanall\t\tClean build artifacts, sstate cache and " \
- "downloads.\n"
- printf "%b" "purge\t\t\tRemove all data managed by kas. Run with '--dry-run'\n"
- printf "%b" " \t\t\tto check what would be removed.\n"
- printf "%b" "menu\t\t\tProvide configuration menu and trigger " \
- "configured build.\n"
- printf "%b" "\nOptional arguments:\n"
- printf "%b" "--isar-privileged\tRun an Isar build in privileged mode. " \
- "To force the use\n"
- printf "%b" "\t\t\tof run0 over sudo, set KAS_SUDO_CMD=run0.\n"
- printf "%b" "--isar-rootless\t\tRun an Isar build in rootless mode.\n"
- printf "%b" "--runtime-args\t\tAdditional arguments to pass to the " \
- "container runtime.\n"
- printf "%b" "\t\t\tfor running the build.\n"
- printf "%b" "-l, --log-level\t\tSet log level (default=info).\n"
- printf "%b" "--version\t\tPrint program version.\n"
- printf "%b" "--ssh-dir\t\tDirectory containing SSH configurations.\n"
- printf "%b" "\t\t\tAvoid \$HOME/.ssh unless you fully trust the " \
- "container.\n"
- printf "%b" "--ssh-agent\t\tForward ssh-agent socket to the container.\n"
- printf "%b" "--aws-dir\t\tDirectory containing AWScli configuration.\n"
- printf "%b" "\t\t\tAvoid \$HOME/.aws unless you fully trust the " \
- "container.\n"
- printf "%b" "--git-credential-store\tFile path to the git credential " \
- "store.\n"
- printf "%b" "--git-credential-socket\tPath to the git credential cache " \
- "socket.\n"
- printf "%b" "--no-proxy-from-env\tDo not inherit proxy settings from " \
- "environment.\n"
- printf "%b" "--repo-ro\t\tMount current repository read-only\n" \
- "\t\t\t(default for build command).\n"
- printf "%b" "--repo-rw\t\tMount current repository writable\n" \
- "\t\t\t(default for shell command).\n"
- printf "%b" "-h, --help\t\tShow this help message and exit.\n"
- printf "%b" "\n"
- printf "%b" "You can force the use of podman over docker using " \
- "KAS_CONTAINER_ENGINE=podman.\n"

+ printf -- 'Usage: %s [OPTIONS] { build | shell } [KASOPTIONS] [KASFILE]\n' "${SELF}"

+ printf -- ' %s [OPTIONS] { checkout | dump | lock } [KASOPTIONS] [KASFILE]\n' "${SELF}"
+ printf -- ' %s [OPTIONS] { diff } [KASOPTIONS] config1 config2\n' "${SELF}"
+ printf -- ' %s [OPTIONS] for-all-repos [KASOPTIONS] [KASFILE] COMMAND\n' "${SELF}"
+ printf -- ' %s [OPTIONS] { clean | cleansstate | cleanall | purge} [KASFILE]\n' "${SELF}"
+ printf -- ' %s [OPTIONS] menu [KCONFIG]\n' "${SELF}"
+ printf -- '\nPositional arguments:\n'
+ printf -- 'build\t\t\tCheck out repositories and build target.\n'
+ printf -- 'checkout\t\tCheck out repositories but do not build.\n'
+ printf -- 'diff\t\t\tCompare two kas configurations.\n'
+ printf -- 'dump\t\t\tCheck out repositories and write flat version\n'
+ printf -- ' \t\t\tof config to stdout.\n'
+ printf -- 'lock\t\t\tCreate and update kas project lockfiles.\n'
+ printf -- 'shell\t\t\tRun a shell in the build environment.\n'
+ printf -- 'for-all-repos\t\tRun specified command in each repository.\n'
+ printf -- 'clean\t\t\tClean build artifacts, keep sstate cache and %b' \
+ 'downloads.\n'
+ printf -- 'cleansstate\t\tClean build artifacts and sstate cache, %b' \
+ 'keep downloads.\n'
+ printf -- 'cleanall\t\tClean build artifacts, sstate cache and %b' \
+ 'downloads.\n'
+ printf -- 'purge\t\t\tRemove all data managed by kas. Run with '--dry-run'\n'
+ printf -- ' \t\t\tto check what would be removed.\n'
+ printf -- 'menu\t\t\tProvide configuration menu and trigger %b' \
+ 'configured build.\n'
+ printf -- '\nOptional arguments:\n'
+ printf -- '--isar-privileged\tRun an Isar build in privileged mode. %b' \
+ 'To force the use\n'
+ printf -- '\t\t\tof run0 over sudo, set KAS_SUDO_CMD=run0.\n'
+ printf -- '--isar-rootless\t\tRun an Isar build in rootless mode.\n'
+ printf -- '--runtime-args\t\tAdditional arguments to pass to the %b' \
+ 'container runtime.\n'
+ printf -- '\t\t\tfor running the build.\n'
+ printf -- '-l, --log-level\t\tSet log level (default=info).\n'
+ printf -- '--version\t\tPrint program version.\n'
+ printf -- '--ssh-dir\t\tDirectory containing SSH configurations.\n'
+ # SC2016: Expressions don't expand in single quotes, use double quotes for that.
+ # shellcheck disable=2016
+ printf -- '\t\t\tAvoid $HOME/.ssh unless you fully trust the %b' \
+ 'container.\n'
+ printf -- '--ssh-agent\t\tForward ssh-agent socket to the container.\n'
+ printf -- '--aws-dir\t\tDirectory containing AWScli configuration.\n'
+ # shellcheck disable=2016
+ printf -- '\t\t\tAvoid $HOME/.aws unless you fully trust the %b' \
+ 'container.\n'
+ printf -- '--git-credential-store\tFile path to the git credential %b' \
+ 'store.\n'
+ printf -- '--git-credential-socket\tPath to the git credential cache %b' \
+ 'socket.\n'
+ printf -- '--no-proxy-from-env\tDo not inherit proxy settings from %b' \
+ 'environment.\n'
+ printf -- '--repo-ro\t\tMount current repository read-only\n%b' \
+ '\t\t\t(default for build command).\n'
+ printf -- '--repo-rw\t\tMount current repository writable\n%b' \
+ '\t\t\t(default for shell command).\n'
+ printf -- '-h, --help\t\tShow this help message and exit.\n'
+ printf -- '\n'
+ printf -- 'You can force the use of podman over docker using %b' \
+ 'KAS_CONTAINER_ENGINE=podman.\n'

exit "${EXIT_CODE:-1}"
}
--
2.47.3

[Jan Kiszka]

On 10.06.26 16:48, 'Konrad Schwarz' via kas-devel wrote:
> Simplify the code by making `KAS_VERBOSE` hold the boolean values `true` or `false`.
>

Yeah, we could do that.

> Instead of expanding `$*` at the end of the debug string, pass arguments
> as `"$@"` to `echo`. This achieves exactly the same result as before
> without needing to create a large intermediate string.

But that is not directly related to the change above, is it? If not,
split it up.

So, $* is much slower than $@, or what is the benefit practically?

Jan

Siemens AG, Foundational Technologies
Linux Expert Center

[Jan Kiszka]

On 10.06.26 16:48, 'Konrad Schwarz' via kas-devel wrote:

> Fix a precedence bug in the regex check for clean/purge:
> anchors (`^`, `$`) bind more tightly than alternation (`|`).
>

Thanks, good catch.

Now for the impact of the current pattern: Is it correct that it was not
biting us so far in practice because none of values KAS_CMD can take was
running into a wrong match? Would be good to elaborate on this here as well.

> Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
> ---
> kas-container | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kas-container b/kas-container
> index 2a7b9dd..56f3cd3 100755
> --- a/kas-container
> +++ b/kas-container
> @@ -589,7 +589,7 @@ fi
> # clean can be executed without config, hence manually forward the build system
> case "${BUILD_SYSTEM}" in
> isar*)
> - if echo "${KAS_CMD}" | grep -qe "^clean\|purge"; then
> + if echo "${KAS_CMD}" | grep -qE -e '^clean|^purge'; then
> KAS_OPTIONS="${KAS_OPTIONS} --${BUILD_SYSTEM}"
> fi
> ;;

Jan

[Jan Kiszka]

On 10.06.26 16:48, 'Konrad Schwarz' via kas-devel wrote:

> Make `ISAR_DOCKER_ROOTLESS` a boolean: set it to either `true` or `false`.

...to simplify the code, right?

Any advantage of using case/esac here over plain if / else or the
existing pattern?

> + KAS_DOCKER_ROOTLESS=true;;
> + *)
> + KAS_DOCKER_ROOTLESS=false
> + esac
> }
>
> enable_docker_rootless()
> @@ -597,7 +599,7 @@ esac
>
> set_container_image_var
>
> -if [ "${KAS_DOCKER_ROOTLESS}" = "1" ]; then
> +if "${KAS_DOCKER_ROOTLESS}"; then
> KAS_REPO_MOUNT_OPT_DEFAULT="ro"
> fi
> KAS_REPO_MOUNT_OPT="${KAS_REPO_MOUNT_OPT:-${KAS_REPO_MOUNT_OPT_DEFAULT}}"
> @@ -608,7 +610,7 @@ if [ "$(id -u)" -eq 0 ] && [ "${KAS_ALLOW_ROOT}" != "yes" ] ; then
> "KAS_ALLOW_ROOT=yes to override."
> fi
>
> -if [ "${KAS_DOCKER_ROOTLESS}" = "1" ]; then
> +if "${KAS_DOCKER_ROOTLESS}"; then
> enable_docker_rootless
> fi
>

[Jan Kiszka]

On 10.06.26 16:48, 'Konrad Schwarz' via kas-devel wrote:

> Use more compact shell syntax when setting default values,
> eliminating duplication of variable names.
>
> Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
> ---
> kas-container | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/kas-container b/kas-container
> index 1cd635f..68507f4 100755
> --- a/kas-container
> +++ b/kas-container
> @@ -155,8 +155,9 @@ enable_isar_mode()
> export PATH="${PATH}:/usr/sbin"
> elif "${KAS_DOCKER_ROOTLESS}"; then
> prepare_sudo_cmd
> - DOCKER_HOST_DEFAULT="$(docker context inspect default --format '{{.Endpoints.docker.Host}}')"
> - export DOCKER_HOST="${DOCKER_HOST:-$DOCKER_HOST_DEFAULT}"
> + export DOCKER_HOST="${DOCKER_HOST:-"$(
> + docker context inspect default --format '{{.Endpoints.docker.Host}}'
> + )"}"

Hmm, I somehow prefer the existing, sightly more compact form.

But doesn't your trick from below work here as well? Maybe like this:

: "${DOCKER_HOST:-"$(docker context inspect default --format '{{.Endpoints.docker.Host}}'}"
export DOCKER_HOST

> debug "kas-isar does not support rootless docker. Using system docker in $DOCKER_HOST"
> # force use of well-known system docker socket
> KAS_CONTAINER_COMMAND="${_KAS_SUDO_CMD} ${KAS_CONTAINER_COMMAND}"
> @@ -235,7 +236,7 @@ check_and_expand()
> # shellcheck disable=2034
> setup_kas_dirs()
> {
> - KAS_WORK_DIR="${KAS_WORK_DIR:-$(pwd)}"
> + : "${KAS_WORK_DIR:=$PWD}"

Ok, haven't seen that pattern before in common scripts, but I admit it's more compact, specifically below.

[Jan Kiszka]

On 10.06.26 16:48, 'Konrad Schwarz' via kas-devel wrote:

> Simplify various pipelines/subshell invocations.
>
> Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
> ---
> kas-container | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/kas-container b/kas-container
> index 68507f4..c070115 100755
> --- a/kas-container
> +++ b/kas-container
> @@ -200,7 +200,9 @@ enable_oe_mode()
>
> enable_unpriv_userns_docker()
> {
> - if [ -f /etc/os-release ] && grep -q 'NAME="Ubuntu"' /etc/os-release &&
> + # SC1091: FILE was not specified as input (/etc/os-release is assumed to be correct)
> + # shellcheck disable=1091
> + if [ -f /etc/os-release ] && (. /etc/os-release && test "$NAME" = Ubuntu) &&

Given that the alternative needs a shellcheck exception, what is the
advantage then?

> [ -f /proc/sys/kernel/apparmor_restrict_unprivileged_userns ] &&
> [ "$(cat /proc/sys/kernel/apparmor_restrict_unprivileged_userns)" = "1" ]; then
> if [ -f /etc/apparmor.d/rootlesskit ]; then
> @@ -250,10 +252,10 @@ setup_kas_dirs()
> repo_path_of_file()
> {
> _DIR="$(dirname "$1")"
> - _REPO_DIR=$(git -C "${_DIR}" rev-parse --show-toplevel 2>/dev/null) \
> - || _REPO_DIR=$(hg --cwd "${_DIR}" root 2>/dev/null) \
> - || _REPO_DIR=${_DIR}
> - echo "$_REPO_DIR"
> + git -C "${_DIR}" rev-parse --show-toplevel 2>/dev/null ||
> + hg --cwd "${_DIR}" root 2>/dev/null ||
> + printf '%s\n' "${_DIR}"

So, the advantage is avoiding a subshell? One fork less?

But please indent the concatenated line

git ... ||
hg ... ||
printf ...

> + unset _DIR
> }
>
> # Params: ARG
> @@ -441,7 +443,7 @@ while [ $# -gt 0 ]; do
> break
> ;;
> dump)
> - if printf '%s\0' "$@" | grep -xqz -- '--inplace\|-i'; then
> + if printf '%s\n' "$@" | grep -qE -e '^--inplace$|^-i$'; then

Looks unrelated to the subject.

> KAS_REPO_MOUNT_OPT_DEFAULT="rw"
> else
> KAS_REPO_MOUNT_OPT_DEFAULT="ro"
> @@ -537,12 +539,11 @@ done
> if [ -n "${KAS_FIRST_FILES}" ]; then
> KAS_REPO_DIR=$(echo "${KAS_REPO_DIRS}" | awk '{print $1}')
> else
> - KAS_REPO_DIR=$(pwd)
> + KAS_REPO_DIR=$PWD
> fi
>
> SOURCE_DIR_HOST=$(
> - grep -e "^_source_dir_host: " "${KAS_WORK_DIR}/.config.yaml" 2>/dev/null | \
> - sed 's/_source_dir_host:[ ]\+//')
> + sed -n 's/^_source_dir_host: *//p' "${KAS_WORK_DIR}/.config.yaml")

^^
Two spaces here, that's subtle to spot. Why not " \+"? My original [ ]
should not be needed, should they?

Otherwise, nice simplification. Need to remember "sed -n".

> if [ -n "${SOURCE_DIR_HOST}" ]; then
> KAS_REPO_DIR="${SOURCE_DIR_HOST}"
> fi
> @@ -563,7 +564,9 @@ if [ "${KAS_CMD}" = "menu" ]; then
> set -- "$@" -e _KAS_REPO_DIR_HOST="${KAS_REPO_DIR}"
> fi
>
> - if [ "$(echo "${KAS_FIRST_FILES}" | wc -w)" -ne "1" ]; then
> + # SC2086: Double quote to prevent globbing and word splitting.
> + # shellcheck disable=2086
> + if [ "$(set -- ${KAS_FIRST_FILES} && echo $#)" -ne "1" ]; then

And that is faster? But it needs an exception, hmm...

> fatal_error "menu plugin only supports a single Kconfig file"
> fi
> BUILD_SYSTEM=${BUILD_SYSTEM:-$(tr '\n' '\f' 2>/dev/null < "${KAS_FIRST_FILES}" |
> @@ -576,9 +579,10 @@ else
> fi
>
> # We only get the first build system and let kas check if mixed
> - _KAS_FIRST_FILE=$(echo "${KAS_FIRST_FILES}" | awk '{print $1}')
> - BUILD_SYSTEM=${BUILD_SYSTEM:-$(grep -e "^build_system: " "${_KAS_FIRST_FILE}" 2>/dev/null |
> - sed 's/build_system:[ ]\+//')}
> + # SC2086: Double quote to prevent globbing and word splitting.
> + # shellcheck disable=2086
> + _KAS_FIRST_FILE=$(set -- ${KAS_FIRST_FILES} && printf '%s\n' "$1")
> + : ${BUILD_SYSTEM:=$(sed -n '/^build_system: */s///p' "${_KAS_FIRST_FILE}")}

" \+" would be preferred, as above.

And why not 's/build_system: \+//p'?

> fi
>
> if [ "${BUILD_SYSTEM}" = "isar" ] || [ "${BUILD_SYSTEM}" = "isar-privileged" ]; then
> @@ -616,7 +620,9 @@ if "${KAS_DOCKER_ROOTLESS}"; then
> fi
>
> if [ "${KAS_CMD}" = "diff" ]; then
> - if [ "$(echo "${KAS_FILES}" | wc -w)" -eq "2" ]; then
> + # SC2086: Double quote to prevent globbing and word splitting.
> + # shellcheck disable=2086
> + if [ "$(set -- ${KAS_FILES} && echo $#)" -eq "2" ]; then
> _KAS_REPO_DIR1="$(echo "${KAS_REPO_DIRS}" | awk '{print $1}')"
> _KAS_REPO_DIR2="$(echo "${KAS_REPO_DIRS}" | awk '{print $2}')"
> _KAS_FILES1="$(echo "${KAS_FILES}" | awk '{print $1}' | sed 's|'"${_KAS_REPO_DIR1}"'/|/repo/|g')"

Patterns need to sink in, but I think they can and help to make things a
bit more compact or even slightly faster.

[Jan Kiszka]

On 10.06.26 16:48, 'Konrad Schwarz' via kas-devel wrote:

> Fix a situation where arguments were being rescanned/subjected
> to word splitting.
>

Is this a bug fix? Then please explain when it broke.

Or is this an optimization (avoiding subshell spawning)?

> Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
> ---
> kas-container | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/kas-container b/kas-container
> index c070115..0b40d3e 100755
> --- a/kas-container
> +++ b/kas-container
> @@ -264,9 +264,14 @@ process_file_arg()
> _KAS_FILES=
> _KAS_FIRST_FILE=
> _KAS_REPO_DIR=
> + _KAS_IFS=$IFS
> + IFS=:
> # SC2086: Double quote to prevent globbing and word splitting.
> # shellcheck disable=2086
> - for FILE in $(IFS=':'; echo $ARG); do
> + set -- $ARG
> + IFS=$_KAS_IFS
> + unset _KAS_IFS
> + for FILE; do
> if ! KAS_REAL_FILE="$(realpath -qe "$FILE")"; then
> fatal_error "configuration file '${FILE}' not found"
> fi

It's rather complex code, much harder to read code. So would need to fix
a real problem. As optimization, it would have to bring some slightly
measurable speed gain.

[Jan Kiszka]

On 10.06.26 16:48, 'Konrad Schwarz' via kas-devel wrote:

> The `sed` script uses GNU extensions (control character `\f`,
> regex alternation) not in POSIX. The script also outputs
> all lines containing `default` after `config KAS_BUILD_SYSTEM`,
> which may be a problem if `KConfig` is ever extended.
>

"default" is already a Kconfig keyword today. According to your
description, it should already be broken. Can you please construct a
Kconfig file that demonstrate that? That's for the impact assessment,
not to justify the change.

We could use that chance an make the match on default more precise:

^[[:blank:]]\+default...

> + s//\1/p ;# output default value
> + q ;# and quit
> + }
> + b found_kas_build_system
> + ' "${KAS_FIRST_FILES}")}

It has comments, that helped. I wouldn't have been able to come up with
that myself, and AI didn't exist back then. :)

> else
> if [ -z "${KAS_FIRST_FILES}" ]; then
> KAS_FIRST_FILES="${KAS_WORK_DIR}/.config.yaml"

[Jan Kiszka]

On 10.06.26 16:48, 'Konrad Schwarz' via kas-devel wrote:

> Posix sh and bash will automatically continue a line
> ending in one of the operators `&&`, `||`, and `|`,
> eliminating the need for a trailing backslash.
>
> Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
> ---
> kas-container | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kas-container b/kas-container
> index 4d384a8..0ea3edf 100755
> --- a/kas-container
> +++ b/kas-container
> @@ -232,6 +232,7 @@ check_and_expand()
> ;;
> esac
> realpath -e "$_varval"
> + unset _varval

Unrelated?

> }
>
> # SC2034: DIR appears unused (ignore, as they are used inside eval)
> @@ -660,8 +661,8 @@ forward_dir KAS_REPO_REF_DIR "/repo-ref" "rw"
> forward_dir SSTATE_DIR "/sstate" "rw"
> forward_dir KAS_BUILDTOOLS_DIR "/buildtools" "rw"
>
> -if git_com_dir=$(git -C "${KAS_REPO_DIR}" rev-parse --git-common-dir 2>/dev/null) \
> - && [ "$git_com_dir" != "$(git -C "${KAS_REPO_DIR}" rev-parse --git-dir)" ]; then
> +if git_com_dir=$(git -C "${KAS_REPO_DIR}" rev-parse --git-common-dir 2>/dev/null) &&
> + [ "$git_com_dir" != "$(git -C "${KAS_REPO_DIR}" rev-parse --git-dir)" ]; then
>
> KAS_GIT_OVERLAY_FILE=""
> kas_container_cleanup()

Looks good otherwise.

[Jan Kiszka]

On 10.06.26 16:48, 'Konrad Schwarz' via kas-devel wrote:

> According to hier(7), `/bin` should contain programs
> used in single user mode/for repair, while `/usr/bin`
> is the primary directory for executable programs.
>
> Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
> ---
> kas-container | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kas-container b/kas-container
> index 0ea3edf..416ef2d 100755
> --- a/kas-container
> +++ b/kas-container
> @@ -788,11 +788,11 @@ done
>
> # propagate only supported SHELL settings
> case "$SHELL" in
> -/bin/sh | /bin/bash | /bin/dash)
> +/bin/sh | */bash | */dash)

This indeed makes us more flexible.

> set -- "$@" -e "SHELL=$SHELL"
> ;;
> *)
> - set -- "$@" -e "SHELL=/bin/bash"
> + set -- "$@" -e "SHELL=/usr/bin/bash"

Still unusual, but as we know our target rootfs (a Debian container), we
could already do that.

> ;;
> esac

[Jan Kiszka]

On 10.06.26 16:48, 'Konrad Schwarz' via kas-devel wrote:

> Use the `--` argument delimiter to avoid spurious warnings about
> arguments starting with `-`.
>

Warnings where? How to reproduce?

I think we can keep the original alignment of continued lines, e.g.:

printf -- 'You can force the use of podman over docker using %b' \

'KAS_CONTAINER_ENGINE=podman.\n'

Any particular reason to use '' instead of "" for the strings? Just out
of curiosity.

[Jan Kiszka]

On 10.06.26 16:48, 'Konrad Schwarz' via kas-devel wrote:

> If the usage message is output because of an error,
> redirect the help message to standard error.
>

Not against it per se, but do other tools something like that as well?

> Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
> ---
> kas-container | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/kas-container b/kas-container
> index 508bc8f..392e021 100755
> --- a/kas-container
> +++ b/kas-container
> @@ -40,6 +40,10 @@ usage()
> EXIT_CODE="$1"
> SELF="${KAS_CONTAINER_SELF_NAME}"
>
> + if [ "$EXIT_CODE" != 0 ]; then
> + exec >&2

That likely deserved a comment as it is not a commonly used pattern

> + fi
> +
> printf -- 'Usage: %s [OPTIONS] { build | shell } [KASOPTIONS] [KASFILE]\n' "${SELF}"
> printf -- ' %s [OPTIONS] { checkout | dump | lock } [KASOPTIONS] [KASFILE]\n' "${SELF}"
> printf -- ' %s [OPTIONS] { diff } [KASOPTIONS] config1 config2\n' "${SELF}"

[Jan Kiszka]

On 10.06.26 16:48, 'Konrad Schwarz' via kas-devel wrote:

Thanks for the changes. I still have "some" comments, but this looks
generally worthwhile - or is even fixing corner cases.

[Schwarz, Konrad]

> -----Original Message-----
> From: Kiszka, Jan (FT RPD CED) <jan.k...@siemens.com>
> Sent: Wednesday, June 10, 2026 17:12
> To: Schwarz, Konrad (FT RPD CED OES-DE) <konrad....@siemens.com>;
> kas-...@googlegroups.com
> Subject: Re: [PATCH 01/11] kas-container: error reporting logic

> > - echo "${KAS_CONTAINER_SELF_NAME}: Error: $*" >&2
> > + echo "${KAS_CONTAINER_SELF_NAME}: Error:" "$@" >&2>

> So, $* is much slower than $@, or what is the benefit practically?

The current solution makes a big string by interpolating all arguments `$*` into the first argument,
then calls `echo` with this string, which then must be deallocated.

The proposed solution `echo prefix "$@"` extends the argument list of `echo` with
additional arguments, i.e., only pointer (and not string) copying is necessary.
`echo` then outputs its arguments separated by blanks.

The result is the same.

There will not be a measurable performance difference
(especially if echo is not built in),
unless you are outputting extremely long strings.

It makes more sense from the embedded/kernel viewpoint: "prefer a list of buffers
over creating one big buffer from smaller ones". And it is a very small change.

--
Konrad Schwarz
Siemens AG

[Schwarz, Konrad]

> -----Original Message-----
> From: Kiszka, Jan (FT RPD CED) <jan.k...@siemens.com>
> Sent: Wednesday, June 10, 2026 17:12
> To: Schwarz, Konrad (FT RPD CED OES-DE) <konrad....@siemens.com>;
> kas-...@googlegroups.com

> Subject: Re: [PATCH 02/11] kas-container: clean/purge regex
>
> > Fix a precedence bug in the regex check for clean/purge:
> > anchors (`^`, `$`) bind more tightly than alternation (`|`).

> > - if echo "${KAS_CMD}" | grep -qe "^clean\|purge"; then
> > + if echo "${KAS_CMD}" | grep -qE -e '^clean|^purge'; then

> Now for the impact of the current pattern: Is it correct that it was not biting us so
> far in practice because none of values KAS_CMD can take was running into a
> wrong match? Would be good to elaborate on this here as well.

So, looking at this in the wider context: all legal values of `KAS_CMD` are
checked in the main argument processing loop, so the anchor here
does not strictly seem necessary: neither `clean` nor `purge` occurs
anywhere other than the start of a command.

OTOH, if the author of the original code thought it would be a good idea
to anchor `clean`, why should it be a bad idea to anchor `purge`?
Should a hypothetical command `xxx-do-not-clean` be excluded here,
but `xxx-do-not-purge` accepted?

[Schwarz, Konrad]

> -----Original Message-----
> From: Kiszka, Jan (FT RPD CED) <jan.k...@siemens.com>
> Sent: Wednesday, June 10, 2026 17:14
> To: Schwarz, Konrad (FT RPD CED OES-DE) <konrad....@siemens.com>;
> kas-...@googlegroups.com

> Subject: Re: [PATCH 03/11] kas-container: rootless isar docker control
>
> On 10.06.26 16:48, 'Konrad Schwarz' via kas-devel wrote:
> > Make `ISAR_DOCKER_ROOTLESS` a boolean: set it to either `true` or `false`.
>
> ...to simplify the code, right?

Exactly.

[Jan Kiszka]

Ok, then just split this change off from the boolean change and state
that it is a micro-optimization. Also revisit your subject line at this
chance.

Thanks,
Jan

[Jan Kiszka]

Fixes: ddb6f050b770 ("fix(kas-container): pass isar argument on clean into container")

Felix' commit. You could at that before your signed-off. Let's keep the
anchor, just clarify in the commit that the imprecise regex is currently
not causing practical problem due to the given value space of KAS_CMD.

[Jan Kiszka]

Then write this into your commit message. :)

[Jörg Sommer]

'Konrad Schwarz' via kas-devel schrieb am Mi 10. Jun, 16:48 (+0200):

> Fix a precedence bug in the regex check for clean/purge:
> anchors (`^`, `$`) bind more tightly than alternation (`|`).
>

> Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
> ---

> kas-container | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>

> diff --git a/kas-container b/kas-container
> index 2a7b9dd..56f3cd3 100755
> --- a/kas-container
> +++ b/kas-container

> @@ -589,7 +589,7 @@ fi
> # clean can be executed without config, hence manually forward the build system
> case "${BUILD_SYSTEM}" in
> isar*)

> - if echo "${KAS_CMD}" | grep -qe "^clean\|purge"; then
> + if echo "${KAS_CMD}" | grep -qE -e '^clean|^purge'; then

> KAS_OPTIONS="${KAS_OPTIONS} --${BUILD_SYSTEM}"
> fi

This could also be done without a subprocess:

case "$KAS_CMD" in
clean*|purge*) KAS_OPTIONS="${KAS_OPTIONS} --${BUILD_SYSTEM}";;
esac

--
Navimatix GmbH T: 03641 - 327 99 0
Tatzendpromenade 2 F: 03641 - 526 306
07745 Jena www.navimatix.de

Geschäftsführer: Steffen Späthe, Jan Rommeley
Registergericht: Amtsgericht Jena, HRB 501480

[Jörg Sommer]

'Jan Kiszka' via kas-devel schrieb am Mi 10. Jun, 17:14 (+0200):

> On 10.06.26 16:48, 'Konrad Schwarz' via kas-devel wrote:
> > @@ -296,10 +296,12 @@ forward_dir()
> >
> > check_docker_rootless()
> > {
> > - KAS_DOCKER_ROOTLESS=0
> > - if [ "$(docker context show)" = "rootless" ]; then
> > - KAS_DOCKER_ROOTLESS=1
> > - fi
> > + case $(docker context show) in
> > + rootless)
>
> Any advantage of using case/esac here over plain if / else or the
> existing pattern?

if calls a subprocess, often `test` aka `[`, to decide. case is built-in.
The disadvantage is that you do not see the evaluation in with `set -x` (but
zsh does).

[Jörg Sommer]

'Konrad Schwarz' via kas-devel schrieb am Mi 10. Jun, 16:48 (+0200):
> Simplify various pipelines/subshell invocations.

>
> Signed-off-by: Konrad Schwarz <konrad....@siemens.com>
> ---

> kas-container | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>

> diff --git a/kas-container b/kas-container
> index 68507f4..c070115 100755
> --- a/kas-container
> +++ b/kas-container

> @@ -200,7 +200,9 @@ enable_oe_mode()
>
> enable_unpriv_userns_docker()
> {
> - if [ -f /etc/os-release ] && grep -q 'NAME="Ubuntu"' /etc/os-release &&
> + # SC1091: FILE was not specified as input (/etc/os-release is assumed to be correct)
> + # shellcheck disable=1091
> + if [ -f /etc/os-release ] && (. /etc/os-release && test "$NAME" = Ubuntu) &&

Isn't this too dangerous? If someone puts an `rm -rf /` in os-release, it
gets ugly.

> [ -f /proc/sys/kernel/apparmor_restrict_unprivileged_userns ] &&
> [ "$(cat /proc/sys/kernel/apparmor_restrict_unprivileged_userns)" = "1" ]; then
> if [ -f /etc/apparmor.d/rootlesskit ]; then
> @@ -250,10 +252,10 @@ setup_kas_dirs()
> repo_path_of_file()
> {
> _DIR="$(dirname "$1")"
> - _REPO_DIR=$(git -C "${_DIR}" rev-parse --show-toplevel 2>/dev/null) \
> - || _REPO_DIR=$(hg --cwd "${_DIR}" root 2>/dev/null) \
> - || _REPO_DIR=${_DIR}
> - echo "$_REPO_DIR"
> + git -C "${_DIR}" rev-parse --show-toplevel 2>/dev/null ||
> + hg --cwd "${_DIR}" root 2>/dev/null ||
> + printf '%s\n' "${_DIR}"

This relies on that the commands do not print something else in the error
case. But this sounds sensible.

> dump)
> - if printf '%s\0' "$@" | grep -xqz -- '--inplace\|-i'; then
> + if printf '%s\n' "$@" | grep -qE -e '^--inplace$|^-i$'; then

> KAS_REPO_MOUNT_OPT_DEFAULT="rw"
> else
> KAS_REPO_MOUNT_OPT_DEFAULT="ro"

Maybe this shown better what the intention of the code is.

KAS_REPO_MOUNT_OPT_DEFAULT="ro"
for arg; do
case "$arg" in
-i|--inplace) KAS_REPO_MOUNT_OPT_DEFAULT="rw";;
esac
done

> @@ -537,12 +539,11 @@ done
> if [ -n "${KAS_FIRST_FILES}" ]; then
> KAS_REPO_DIR=$(echo "${KAS_REPO_DIRS}" | awk '{print $1}')
> else
> - KAS_REPO_DIR=$(pwd)
> + KAS_REPO_DIR=$PWD

Good one!

> - if [ "$(echo "${KAS_FIRST_FILES}" | wc -w)" -ne "1" ]; then
> + # SC2086: Double quote to prevent globbing and word splitting.
> + # shellcheck disable=2086

> + if [ "$(set -- ${KAS_FIRST_FILES} && echo $#)" -ne "1" ]; then

Isn't globbing still possible here? Set `set -f` before:

if (set -f && set -- ${KAS_FIRST_FILES} && test $# -ne 1); then

If it is a matter of word splitting, I think this would also do:

case "$KAS_FIRST_FILES" in
*"$IFS"*) fatal_error …

or

*" "*) fatal_error …

> fatal_error "menu plugin only supports a single Kconfig file"
> fi

> BUILD_SYSTEM=${BUILD_SYSTEM:-$(tr '\n' '\f' 2>/dev/null < "${KAS_FIRST_FILES}" |

> @@ -576,9 +579,10 @@ else
> fi
>
> # We only get the first build system and let kas check if mixed
> - _KAS_FIRST_FILE=$(echo "${KAS_FIRST_FILES}" | awk '{print $1}')
> - BUILD_SYSTEM=${BUILD_SYSTEM:-$(grep -e "^build_system: " "${_KAS_FIRST_FILE}" 2>/dev/null |
> - sed 's/build_system:[ ]\+//')}

> + # SC2086: Double quote to prevent globbing and word splitting.
> + # shellcheck disable=2086

An idea: In our projects we add the explanation for this exception:

# shellcheck disable=2086 # we want word splitting of KAS_FIRST_FILES

> + _KAS_FIRST_FILE=$(set -- ${KAS_FIRST_FILES} && printf '%s\n' "$1")

Add an `set -f` to prevent globbing

> + : ${BUILD_SYSTEM:=$(sed -n '/^build_system: */s///p' "${_KAS_FIRST_FILE}")}

Why not merge this like you did above?

sed -n 's/^build_system: *//p' "${_KAS_FIRST_FILE}"

[Jörg Sommer]

'Konrad Schwarz' via kas-devel schrieb am Mi 10. Jun, 16:48 (+0200):

> Fix a situation where arguments were being rescanned/subjected
> to word splitting.

Out of curiosity: Did you found any statement in official docs when/how
often this evaluation happens? I know no statement that helps to clarify,
esp. the case

```
for i
do
set -- xy
done
```

But for example:

```
% dash -xc 'for i in $(echo a b); do true; done'
+ echo a b
+ true
+ true
% bash --norc -xc 'for i in $(echo a b); do true; done'
++ echo a b
+ for i in $(echo a b)
+ true
+ for i in $(echo a b)
+ true
% zsh -f -xc 'for i in $(echo a b c); do true; done'
+/etc/zsh/zshenv:15> [[ -z /bin:/usr/bin:/sbin:/usr/sbin:/usr/local/bin:/usr/local/sbin || /bin:/usr/bin:/sbin:/usr/sbin:/usr/local/bin:/usr/local/sbin == /bin:/usr/bin ]]
+zsh:1> echo a b
+zsh:1> i=a
+zsh:1> true
+zsh:1> i=b
+zsh:1> true
```

It is not:

```
+ echo a b
+ true
+ echo a b
+ true
```

[Schwarz, Konrad]

> -----Original Message-----
> From: Jörg Sommer <joerg....@navimatix.de>

> Out of curiosity: Did you found any statement in official docs when/how
> often this evaluation happens?

My mental model is the following:
in pure POSIX `sh`, the only safe storage for an array of words is `"$@"`.

So, with some caveats, `sh` can only deal with one array of words at a time.
If you need more than one (which `kas-container` theoretically does, but probably never
in actual practice, i.e., if arguments should never contain `IFS` characters),
you need `ksh`/`bash` array variables.

The suggested patch is a very local improvement: `for I in $(echo $some_var); do` to
will always fail if a word in `$some_var` contains IFS characters, whereas
`for I in "$@"; do", or, equivalently, `for i; do` (or `for i in "$some_var[@]"; do`) will not.

But to be capable of dealing with multiple lists of (arbitrary) words
requires a better shell than POSIX `/bin/sh`.

Should `kas-container` ever switch to such a more capable shell, this would be one place
less that must be corrected.

[Schwarz, Konrad]

> -----Original Message-----
> > enable_unpriv_userns_docker()
> > {
> > - if [ -f /etc/os-release ] && grep -q 'NAME="Ubuntu"' /etc/os-release &&
> > + # SC1091: FILE was not specified as input (/etc/os-release is assumed to
> be correct)
> > + # shellcheck disable=1091
> > + if [ -f /etc/os-release ] && (. /etc/os-release && test "$NAME" = Ubuntu) &&
>
> Isn't this too dangerous? If someone puts an `rm -rf /` in os-release, it
> gets ugly.

Someone who can modify `/etc/os-release` already has root capabilities,
so this does not open a new avenue of attack, IMO.

Note: https://www.freedesktop.org/software/systemd/man/latest/os-release.html
states it must be compatible to `sh` variable assignments.

> > @@ -250,10 +252,10 @@ setup_kas_dirs()
> > repo_path_of_file()
> > {
> > _DIR="$(dirname "$1")"
> > - _REPO_DIR=$(git -C "${_DIR}" rev-parse --show-toplevel 2>/dev/null) \
> > - || _REPO_DIR=$(hg --cwd "${_DIR}" root 2>/dev/null) \
> > - || _REPO_DIR=${_DIR}
> > - echo "$_REPO_DIR"
> > + git -C "${_DIR}" rev-parse --show-toplevel 2>/dev/null ||
> > + hg --cwd "${_DIR}" root 2>/dev/null ||
> > + printf '%s\n' "${_DIR}"
>
> This relies on that the commands do not print something else in the error
> case. But this sounds sensible.

The end result would be the same in both cases.

You are correct.

> If it is a matter of word splitting, I think this would also do:
>
> case "$KAS_FIRST_FILES" in
> *"$IFS"*) fatal_error …
>
> or
>
> *" "*) fatal_error …
>
> > fatal_error "menu plugin only supports a single Kconfig file"
> > fi
> > BUILD_SYSTEM=${BUILD_SYSTEM:-$(tr '\n' '\f' 2>/dev/null <
> "${KAS_FIRST_FILES}" |
> > @@ -576,9 +579,10 @@ else
> > fi
> >
> > # We only get the first build system and let kas check if mixed
> > - _KAS_FIRST_FILE=$(echo "${KAS_FIRST_FILES}" | awk '{print $1}')
> > - BUILD_SYSTEM=${BUILD_SYSTEM:-$(grep -e "^build_system: "
> "${_KAS_FIRST_FILE}" 2>/dev/null |
> > - sed 's/build_system:[ ]\+//')}
> > + # SC2086: Double quote to prevent globbing and word splitting.
> > + # shellcheck disable=2086
>
> An idea: In our projects we add the explanation for this exception:
>
> # shellcheck disable=2086 # we want word splitting of KAS_FIRST_FILES

I followed the existing convention of this script, but I think this is a good idea.

> > + _KAS_FIRST_FILE=$(set -- ${KAS_FIRST_FILES} && printf '%s\n' "$1")
>
> Add an `set -f` to prevent globbing

Yes

> > + : ${BUILD_SYSTEM:=$(sed -n '/^build_system: */s///p'
> "${_KAS_FIRST_FILE}")}
>
> Why not merge this like you did above?
>
> sed -n 's/^build_system: *//p' "${_KAS_FIRST_FILE}"

I was focusing on eliminating the `grep` (IIRC). But
your code is shorter.