[PATCH 0/4] Minor improvements to scripts/run-qemu.sh
None of these are major changes. See individual commit messages for details. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- Demi Marie Obenour (4): scripts/run-qemu.sh: better error checks & formatting scripts/run-qemu.sh: decrease indentation scripts/run-qemu.sh: Preserve ,, in QEMU arguments scripts/run-qemu.sh: Unset variables from the environment scripts/run-qemu.sh | 123 +++++++++++++++++++++++++++------------------------- 1 file changed, 65 insertions(+), 58 deletions(-) --- base-commit: 22e216712322cdfb85094bbd27ff34c4366fad41 change-id: 20251107-simple-sh-quote-fix-cf956d5422c3 -- Sincerely, Demi Marie Obenour (she/her/hers)
No other functional change intended. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- scripts/run-qemu.sh | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/scripts/run-qemu.sh b/scripts/run-qemu.sh index 64fd29259ab108bc547cb7c74623ae9dc288b3b7..9c6c8193bbeba5916038c82d8f76992051719c19 100755 --- a/scripts/run-qemu.sh +++ b/scripts/run-qemu.sh @@ -1,11 +1,15 @@ -#!/bin/sh -ue +#!/bin/sh -- # SPDX-FileCopyrightText: 2023-2025 Alyssa Ross <hi@alyssa.is> # SPDX-License-Identifier: EUPL-1.2+ # This script wraps around QEMU to paper over platform differences, # which can't be handled portably in Make language. +set -uef +if [ -n ${ARCH+test} ]; then + ARCH=$(uname -m) +fi -case "${ARCH:="$(uname -m)"}" in +case $ARCH in aarch64) machine=virt,accel=kvm:tcg,gic-version=3,iommu=smmuv3 ;; @@ -17,13 +21,13 @@ case "${ARCH:="$(uname -m)"}" in esac i=0 -while [ $i -lt $# ]; do - arg="$1" +while [ "$i" -lt "$#" ]; do + arg=$1 shift - case "$arg" in + case $arg in -append) - set -- "$@" -append "${append:+$append }$1" + set -- "$@" -append ${append:+"$append "}"$1" i=$((i + 2)) shift continue @@ -31,10 +35,10 @@ while [ $i -lt $# ]; do -device) IFS=, for opt in $1; do - case "$opt" in - *-iommu) - unset iommu - ;; + case $opt in + *-iommu) + unset iommu + ;; esac break done -- 2.51.2
Demi Marie Obenour <demiobenour@gmail.com> writes:
No other functional change intended.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- scripts/run-qemu.sh | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
If we're doing formatting changes like this it's definitely time to adopt a shell script formatter, so I'm going to apply my previous series for that[1]. If you can find a shell formatter that does matched parentheses for case statements, we can switch. :) [1]: https://spectrum-os.org/lists/archives/spectrum-devel/20251001102838.7086-1-...
diff --git a/scripts/run-qemu.sh b/scripts/run-qemu.sh index 64fd29259ab108bc547cb7c74623ae9dc288b3b7..9c6c8193bbeba5916038c82d8f76992051719c19 100755 --- a/scripts/run-qemu.sh +++ b/scripts/run-qemu.sh @@ -1,11 +1,15 @@ -#!/bin/sh -ue +#!/bin/sh -- # SPDX-FileCopyrightText: 2023-2025 Alyssa Ross <hi@alyssa.is> # SPDX-License-Identifier: EUPL-1.2+
# This script wraps around QEMU to paper over platform differences, # which can't be handled portably in Make language. +set -uef +if [ -n ${ARCH+test} ]; then + ARCH=$(uname -m) +fi
-case "${ARCH:="$(uname -m)"}" in +case $ARCH in aarch64) machine=virt,accel=kvm:tcg,gic-version=3,iommu=smmuv3 ;; @@ -17,13 +21,13 @@ case "${ARCH:="$(uname -m)"}" in esac
i=0 -while [ $i -lt $# ]; do - arg="$1" +while [ "$i" -lt "$#" ]; do + arg=$1 shift
- case "$arg" in + case $arg in -append) - set -- "$@" -append "${append:+$append }$1" + set -- "$@" -append ${append:+"$append "}"$1" i=$((i + 2)) shift continue @@ -31,10 +35,10 @@ while [ $i -lt $# ]; do -device) IFS=, for opt in $1; do - case "$opt" in - *-iommu) - unset iommu - ;; + case $opt in + *-iommu) + unset iommu + ;; esac break done
-- 2.51.2
Demi Marie Obenour <demiobenour@gmail.com> writes:
No other functional change intended.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- scripts/run-qemu.sh | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
This is a lot of changes all in one, without individual explanation. I'd appreciate it if they could be broken up and explained, because I'm going to have to ask what the purpose of each change is.
diff --git a/scripts/run-qemu.sh b/scripts/run-qemu.sh index 64fd29259ab108bc547cb7c74623ae9dc288b3b7..9c6c8193bbeba5916038c82d8f76992051719c19 100755 --- a/scripts/run-qemu.sh +++ b/scripts/run-qemu.sh @@ -1,11 +1,15 @@ -#!/bin/sh -ue +#!/bin/sh --
The idea here is to be robust against the script being invoked with argv[0] set to "-c" or something?
# SPDX-FileCopyrightText: 2023-2025 Alyssa Ross <hi@alyssa.is> # SPDX-License-Identifier: EUPL-1.2+
# This script wraps around QEMU to paper over platform differences, # which can't be handled portably in Make language. +set -uef
Adding -f makes sense.
+if [ -n ${ARCH+test} ]; then + ARCH=$(uname -m) +fi
-case "${ARCH:="$(uname -m)"}" in +case $ARCH in aarch64) machine=virt,accel=kvm:tcg,gic-version=3,iommu=smmuv3 ;;
Why is this better?
@@ -17,13 +21,13 @@ case "${ARCH:="$(uname -m)"}" in esac
i=0 -while [ $i -lt $# ]; do - arg="$1" +while [ "$i" -lt "$#" ]; do + arg=$1 shift
- case "$arg" in + case $arg in -append)
Makes sense as long as we're consistent about it. I wonder if we could get the formatter to do this.
- set -- "$@" -append "${append:+$append }$1" + set -- "$@" -append ${append:+"$append "}"$1" i=$((i + 2)) shift continue
Don't understand this one. We've gone from one set of quotes to two sequential ones.
On 11/13/25 07:23, Alyssa Ross wrote:
Demi Marie Obenour <demiobenour@gmail.com> writes:
No other functional change intended.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- scripts/run-qemu.sh | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
This is a lot of changes all in one, without individual explanation. I'd appreciate it if they could be broken up and explained, because I'm going to have to ask what the purpose of each change is.
diff --git a/scripts/run-qemu.sh b/scripts/run-qemu.sh index 64fd29259ab108bc547cb7c74623ae9dc288b3b7..9c6c8193bbeba5916038c82d8f76992051719c19 100755 --- a/scripts/run-qemu.sh +++ b/scripts/run-qemu.sh @@ -1,11 +1,15 @@ -#!/bin/sh -ue +#!/bin/sh --
The idea here is to be robust against the script being invoked with argv[0] set to "-c" or something?
Correct.
# SPDX-FileCopyrightText: 2023-2025 Alyssa Ross <hi@alyssa.is> # SPDX-License-Identifier: EUPL-1.2+
# This script wraps around QEMU to paper over platform differences, # which can't be handled portably in Make language. +set -uef
Adding -f makes sense.
+if [ -n ${ARCH+test} ]; then + ARCH=$(uname -m) +fi
-case "${ARCH:="$(uname -m)"}" in +case $ARCH in aarch64) machine=virt,accel=kvm:tcg,gic-version=3,iommu=smmuv3 ;;
Why is this better?
If uname exits with a non-zero status, the script will exit rather than continuing.
@@ -17,13 +21,13 @@ case "${ARCH:="$(uname -m)"}" in esac
i=0 -while [ $i -lt $# ]; do - arg="$1" +while [ "$i" -lt "$#" ]; do + arg=$1 shift
- case "$arg" in + case $arg in -append)
Makes sense as long as we're consistent about it. I wonder if we could get the formatter to do this.
+1
- set -- "$@" -append "${append:+$append }$1" + set -- "$@" -append ${append:+"$append "}"$1" i=$((i + 2)) shift continue
Don't understand this one. We've gone from one set of quotes to two sequential ones.
I remember reading that the first version might not conform to POSIX. I'm not sure if it matters though. -- Sincerely, Demi Marie Obenour (she/her/hers)
Demi Marie Obenour <demiobenour@gmail.com> writes:
On 11/13/25 07:23, Alyssa Ross wrote:
Demi Marie Obenour <demiobenour@gmail.com> writes:
No other functional change intended.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- scripts/run-qemu.sh | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
This is a lot of changes all in one, without individual explanation. I'd appreciate it if they could be broken up and explained, because I'm going to have to ask what the purpose of each change is.
diff --git a/scripts/run-qemu.sh b/scripts/run-qemu.sh index 64fd29259ab108bc547cb7c74623ae9dc288b3b7..9c6c8193bbeba5916038c82d8f76992051719c19 100755 --- a/scripts/run-qemu.sh +++ b/scripts/run-qemu.sh @@ -1,11 +1,15 @@ -#!/bin/sh -ue +#!/bin/sh --
The idea here is to be robust against the script being invoked with argv[0] set to "-c" or something?
Correct.
Seems a little paranoid but I suppose it doesn't hurt. I'd take a treewide change that did this for every /bin/sh shebang.
+if [ -n ${ARCH+test} ]; then + ARCH=$(uname -m) +fi
-case "${ARCH:="$(uname -m)"}" in +case $ARCH in aarch64) machine=virt,accel=kvm:tcg,gic-version=3,iommu=smmuv3 ;;
Why is this better?
If uname exits with a non-zero status, the script will exit rather than continuing.
I see — makes sense. And why is the check not just the following? if [ -z "${ARCH-}" ]; then
- set -- "$@" -append "${append:+$append }$1" + set -- "$@" -append ${append:+"$append "}"$1" i=$((i + 2)) shift continue
Don't understand this one. We've gone from one set of quotes to two sequential ones.
I remember reading that the first version might not conform to POSIX. I'm not sure if it matters though.
POSIX says[1]:
If a parameter expansion occurs inside double-quotes:
* Pathname expansion shall not be performed on the results of the expansion.
* Field splitting shall not be performed on the results of the expansion.
Seems desirable to me. [1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#ta...
On 11/13/25 11:56, Alyssa Ross wrote:
Demi Marie Obenour <demiobenour@gmail.com> writes:
On 11/13/25 07:23, Alyssa Ross wrote:
Demi Marie Obenour <demiobenour@gmail.com> writes:
No other functional change intended.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- scripts/run-qemu.sh | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
This is a lot of changes all in one, without individual explanation. I'd appreciate it if they could be broken up and explained, because I'm going to have to ask what the purpose of each change is.
diff --git a/scripts/run-qemu.sh b/scripts/run-qemu.sh index 64fd29259ab108bc547cb7c74623ae9dc288b3b7..9c6c8193bbeba5916038c82d8f76992051719c19 100755 --- a/scripts/run-qemu.sh +++ b/scripts/run-qemu.sh @@ -1,11 +1,15 @@ -#!/bin/sh -ue +#!/bin/sh --
The idea here is to be robust against the script being invoked with argv[0] set to "-c" or something?
Correct.
Seems a little paranoid but I suppose it doesn't hurt. I'd take a treewide change that did this for every /bin/sh shebang.
Will do later.
+if [ -n ${ARCH+test} ]; then + ARCH=$(uname -m) +fi
-case "${ARCH:="$(uname -m)"}" in +case $ARCH in aarch64) machine=virt,accel=kvm:tcg,gic-version=3,iommu=smmuv3 ;;
Why is this better?
If uname exits with a non-zero status, the script will exit rather than continuing.
I see — makes sense. And why is the check not just the following?
if [ -z "${ARCH-}" ]; then
That works just as well.
- set -- "$@" -append "${append:+$append }$1" + set -- "$@" -append ${append:+"$append "}"$1" i=$((i + 2)) shift continue
Don't understand this one. We've gone from one set of quotes to two sequential ones.
I remember reading that the first version might not conform to POSIX. I'm not sure if it matters though.
POSIX says[1]:
If a parameter expansion occurs inside double-quotes:
* Pathname expansion shall not be performed on the results of the expansion.
* Field splitting shall not be performed on the results of the expansion.
Seems desirable to me.
[1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#ta...
I recall reading about shells not properly handling ${append:+"$append"} properly when the whole expansion is itself quoted The double quotes around "$append" prevent it from being expanded. -- Sincerely, Demi Marie Obenour (she/her/hers)
The rightward drift made the code harder to read. No functional change. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- scripts/run-qemu.sh | 106 ++++++++++++++++++++++++++-------------------------- 1 file changed, 53 insertions(+), 53 deletions(-) diff --git a/scripts/run-qemu.sh b/scripts/run-qemu.sh index 9c6c8193bbeba5916038c82d8f76992051719c19..ab324d8fda0810516cb2180d1372bbeb546f52bc 100755 --- a/scripts/run-qemu.sh +++ b/scripts/run-qemu.sh @@ -10,14 +10,14 @@ if [ -n ${ARCH+test} ]; then fi case $ARCH in - aarch64) - machine=virt,accel=kvm:tcg,gic-version=3,iommu=smmuv3 - ;; - x86_64) - append="console=ttyS0${append:+ $append}" - iommu=intel-iommu,intremap=on - machine=q35,accel=kvm:tcg,kernel-irqchip=split - ;; +aarch64) + machine=virt,accel=kvm:tcg,gic-version=3,iommu=smmuv3 + ;; +x86_64) + append="console=ttyS0${append:+ $append}" + iommu=intel-iommu,intremap=on + machine=q35,accel=kvm:tcg,kernel-irqchip=split + ;; esac i=0 @@ -26,49 +26,49 @@ while [ "$i" -lt "$#" ]; do shift case $arg in - -append) - set -- "$@" -append ${append:+"$append "}"$1" - i=$((i + 2)) - shift - continue - ;; - -device) - IFS=, - for opt in $1; do - case $opt in - *-iommu) - unset iommu - ;; - esac - break - done - unset IFS - ;; - -machine) - set -- "$@" "$arg" - i=$((i + 1)) - arg= + -append) + set -- "$@" -append ${append:+"$append "}"$1" + i=$((i + 2)) + shift + continue + ;; + -device) + IFS=, + for opt in $1; do + case $opt in + *-iommu) + unset iommu + ;; + esac + break + done + unset IFS + ;; + -machine) + set -- "$@" "$arg" + i=$((i + 1)) + arg= - IFS=, - for opt in $1; do - case "$opt" in - virtualization=on) - case "$ARCH" in - aarch64) - opt="$opt,accel=tcg" - ;; - *) - continue - ;; - esac - ;; + IFS=, + for opt in $1; do + case "$opt" in + virtualization=on) + case "$ARCH" in + aarch64) + opt="$opt,accel=tcg" + ;; + *) + continue + ;; esac + ;; + esac - arg="$arg${arg:+,}$opt" - done - unset IFS + arg="$arg${arg:+,}$opt" + done + unset IFS - shift + shift esac set -- "$@" "$arg" @@ -78,12 +78,12 @@ done for arg; do case "$arg" in - -append) - append= - ;; - -kernel) - kernel=1 - ;; + -append) + append= + ;; + -kernel) + kernel=1 + ;; esac done -- 2.51.2
QEMU allows commas in option values to be escaped by doubling them. Therefore, doubled commas should be preserved by scripts/run-qemu.sh. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- scripts/run-qemu.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/run-qemu.sh b/scripts/run-qemu.sh index ab324d8fda0810516cb2180d1372bbeb546f52bc..14c58729dfceb3a0d2566fcfe076d6c10433419f 100755 --- a/scripts/run-qemu.sh +++ b/scripts/run-qemu.sh @@ -64,7 +64,7 @@ while [ "$i" -lt "$#" ]; do ;; esac - arg="$arg${arg:+,}$opt" + arg="$arg,$opt" done unset IFS -- 2.51.2
Demi Marie Obenour <demiobenour@gmail.com> writes:
QEMU allows commas in option values to be escaped by doubling them. Therefore, doubled commas should be preserved by scripts/run-qemu.sh.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- scripts/run-qemu.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
This makes all the tests fail: qemu-system-aarch64: -machine ,virtualization=on,accel=tcg: Invalid parameter ''
diff --git a/scripts/run-qemu.sh b/scripts/run-qemu.sh index ab324d8fda0810516cb2180d1372bbeb546f52bc..14c58729dfceb3a0d2566fcfe076d6c10433419f 100755 --- a/scripts/run-qemu.sh +++ b/scripts/run-qemu.sh @@ -64,7 +64,7 @@ while [ "$i" -lt "$#" ]; do ;; esac
- arg="$arg${arg:+,}$opt" + arg="$arg,$opt" done unset IFS
-- 2.51.2
On 11/13/25 07:28, Alyssa Ross wrote:
Demi Marie Obenour <demiobenour@gmail.com> writes:
QEMU allows commas in option values to be escaped by doubling them. Therefore, doubled commas should be preserved by scripts/run-qemu.sh.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- scripts/run-qemu.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
This makes all the tests fail:
qemu-system-aarch64: -machine ,virtualization=on,accel=tcg: Invalid parameter ''
Whoops, just drop this.
diff --git a/scripts/run-qemu.sh b/scripts/run-qemu.sh index ab324d8fda0810516cb2180d1372bbeb546f52bc..14c58729dfceb3a0d2566fcfe076d6c10433419f 100755 --- a/scripts/run-qemu.sh +++ b/scripts/run-qemu.sh @@ -64,7 +64,7 @@ while [ "$i" -lt "$#" ]; do ;; esac
- arg="$arg${arg:+,}$opt" + arg="$arg,$opt" done unset IFS
-- 2.51.2 -- Sincerely, Demi Marie Obenour (she/her/hers)
On 11/13/25 07:28, Alyssa Ross wrote:
Demi Marie Obenour <demiobenour@gmail.com> writes:
QEMU allows commas in option values to be escaped by doubling them. Therefore, doubled commas should be preserved by scripts/run-qemu.sh.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- scripts/run-qemu.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
This makes all the tests fail:
qemu-system-aarch64: -machine ,virtualization=on,accel=tcg: Invalid parameter ''
diff --git a/scripts/run-qemu.sh b/scripts/run-qemu.sh index ab324d8fda0810516cb2180d1372bbeb546f52bc..14c58729dfceb3a0d2566fcfe076d6c10433419f 100755 --- a/scripts/run-qemu.sh +++ b/scripts/run-qemu.sh @@ -64,7 +64,7 @@ while [ "$i" -lt "$#" ]; do ;; esac
- arg="$arg${arg:+,}$opt" + arg="$arg,$opt" done unset IFS
-- 2.51.2
This was just wrong. I'll drop it. -- Sincerely, Demi Marie Obenour (she/her/hers)
These could affect the script. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- scripts/run-qemu.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/scripts/run-qemu.sh b/scripts/run-qemu.sh index 14c58729dfceb3a0d2566fcfe076d6c10433419f..ba32467956a509b1744ade461f2fe5f20ae7a5b9 100755 --- a/scripts/run-qemu.sh +++ b/scripts/run-qemu.sh @@ -9,12 +9,15 @@ if [ -n ${ARCH+test} ]; then ARCH=$(uname -m) fi +unset kernel + case $ARCH in aarch64) machine=virt,accel=kvm:tcg,gic-version=3,iommu=smmuv3 + unset iommu append ;; x86_64) - append="console=ttyS0${append:+ $append}" + append=console=ttyS0 iommu=intel-iommu,intremap=on machine=q35,accel=kvm:tcg,kernel-irqchip=split ;; @@ -79,7 +82,7 @@ done for arg; do case "$arg" in -append) - append= + unset append ;; -kernel) kernel=1 -- 2.51.2
Demi Marie Obenour <demiobenour@gmail.com> writes:
These could affect the script.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- scripts/run-qemu.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Makes sense.
diff --git a/scripts/run-qemu.sh b/scripts/run-qemu.sh index 14c58729dfceb3a0d2566fcfe076d6c10433419f..ba32467956a509b1744ade461f2fe5f20ae7a5b9 100755 --- a/scripts/run-qemu.sh +++ b/scripts/run-qemu.sh @@ -9,12 +9,15 @@ if [ -n ${ARCH+test} ]; then ARCH=$(uname -m) fi
+unset kernel +
Would be clearer if this was attached to the for loop that actually sets kernel, I think. Then as I reader I don't need to keep it in my head for as long. (I can make this change and commit if you agree.)
On 11/13/25 07:27, Alyssa Ross wrote:
Demi Marie Obenour <demiobenour@gmail.com> writes:
These could affect the script.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- scripts/run-qemu.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Makes sense.
diff --git a/scripts/run-qemu.sh b/scripts/run-qemu.sh index 14c58729dfceb3a0d2566fcfe076d6c10433419f..ba32467956a509b1744ade461f2fe5f20ae7a5b9 100755 --- a/scripts/run-qemu.sh +++ b/scripts/run-qemu.sh @@ -9,12 +9,15 @@ if [ -n ${ARCH+test} ]; then ARCH=$(uname -m) fi
+unset kernel +
Would be clearer if this was attached to the for loop that actually sets kernel, I think. Then as I reader I don't need to keep it in my head for as long. (I can make this change and commit if you agree.)
Feel free to fix up on commit. -- Sincerely, Demi Marie Obenour (she/her/hers)
This patch has been committed as 26c6622abe98797d5d3417300b4473fd119d6d25, which can be viewed online at https://spectrum-os.org/git/spectrum/commit/?id=26c6622abe98797d5d3417300b44.... This is an automated message. Send comments/questions/requests to: Alyssa Ross <hi@alyssa.is>
participants (3)
-
Alyssa Ross -
Alyssa Ross -
Demi Marie Obenour