scripts/qemu-binfmt-conf.sh | 89 +++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 49 deletions(-)
Currently qemu-binfmt-conf.sh does a number of strange things.
1. --systemd requires an argument - the CPU type to register,
while --debian (which is actually --binfmt-support) does not
accept such an argument, so it is not possible to specify which
CPU(s) to register for debian.
2. Why this "ALL" at all?
3. it just ignores extra command-line arguments. It would be
logical to specify which CPUs to register (multiple!) as the
additional arguments.
4. Even if a CPU is explicitly requested, it does not register
anything if this CPU is of the same family as host one. But
this is wrong, since quite often it *is* desirable to do this
registration, - like, when running in i386 when the system is
not capable of running x86-64 binaries, and countless other
examples
5. It ignores errors
6. It ignores wrong command line arguments
Fix this, and simplify things a bit.
1. Stop accepting an argument for --systemd. With getopt_long,
this argument, if given, will be returned as a non-optional
parameter so compatibility with current version is preserved.
2. Accept optional arguments and generate registration for the
given CPUs only. In case no extra arguments are given, register
for all supportd CPUs except of the same family as host.
3. Recognze "ALL" "CPU" to keep compatibility with current version
(but do not document it).
4. Warn but perform registration anyway if a cpu of the same family
has been requested.
5. In help text, use --debian and --systemd as alternatives to each
other, to make it clear the two can not be used at the same time.
6. Tiny optimization of eval expression.
7. Fold the list of supported CPUs to fit in 80 columns.
8. Exit with non-zero code in case registration fails or the command
line is wrong.
9. Remove explicit checking for writability of various things.
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
scripts/qemu-binfmt-conf.sh | 89 +++++++++++++++++--------------------
1 file changed, 40 insertions(+), 49 deletions(-)
diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index 6ef9f118d9..27d506000d 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -182,10 +182,10 @@ qemu_get_family() {
usage() {
cat <<EOF
-Usage: qemu-binfmt-conf.sh [--qemu-path PATH][--debian][--systemd CPU]
+Usage: qemu-binfmt-conf.sh [--qemu-path PATH][--debian|--systemd]
[--help][--credential yes|no][--exportdir PATH]
[--persistent yes|no][--qemu-suffix SUFFIX]
- [--preserve-argv0 yes|no]
+ [--preserve-argv0 yes|no] [CPU...]
Configure binfmt_misc to use qemu interpreter
@@ -217,21 +217,17 @@ Usage: qemu-binfmt-conf.sh [--qemu-path PATH][--debian][--systemd CPU]
With systemd, binfmt files are loaded by systemd-binfmt.service
+ If CPU(s) are specified in the command line, configure binfmt_misc for
+ this set of CPUs only. If no types are specified, make configuration
+ for all supported CPU types except the ones from the host CPU family.
+
The environment variable HOST_ARCH allows to override 'uname' to generate
configuration files for a different architecture than the current one.
- where CPU is one of:
-
- $qemu_target_list
+ Supported CPUs are:
EOF
-}
-
-qemu_check_access() {
- if [ ! -w "$1" ] ; then
- echo "ERROR: cannot write to $1" 1>&2
- exit 1
- fi
+echo $qemu_target_list | fold -w68 -s | sed 's/^/ /'
}
qemu_check_bintfmt_misc() {
@@ -246,8 +242,6 @@ qemu_check_bintfmt_misc() {
exit 1
fi
fi
-
- qemu_check_access /proc/sys/fs/binfmt_misc/register
}
installed_dpkg() {
@@ -260,14 +254,12 @@ qemu_check_debian() {
elif ! installed_dpkg binfmt-support ; then
echo "WARNING: package binfmt-support is needed" 1>&2
fi
- qemu_check_access "$EXPORTDIR"
}
qemu_check_systemd() {
if ! systemctl -q is-enabled systemd-binfmt.service ; then
echo "WARNING: systemd-binfmt.service is missing or disabled" 1>&2
fi
- qemu_check_access "$EXPORTDIR"
}
qemu_generate_register() {
@@ -287,16 +279,16 @@ qemu_generate_register() {
qemu_register_interpreter() {
echo "Setting $qemu as binfmt interpreter for $cpu"
- qemu_generate_register > /proc/sys/fs/binfmt_misc/register
+ qemu_generate_register > /proc/sys/fs/binfmt_misc/register || exit 1
}
qemu_generate_systemd() {
echo "Setting $qemu as binfmt interpreter for $cpu for systemd-binfmt.service"
- qemu_generate_register > "$EXPORTDIR/qemu-$cpu.conf"
+ qemu_generate_register > "$EXPORTDIR/qemu-$cpu.conf" || exit 1
}
qemu_generate_debian() {
- cat > "$EXPORTDIR/qemu-$cpu" <<EOF
+ cat > "$EXPORTDIR/qemu-$cpu" <<EOF || exit 1
package qemu-$cpu
interpreter $qemu
magic $magic
@@ -311,16 +303,22 @@ qemu_set_binfmts() {
# probe cpu type
host_family=$(qemu_get_family)
- # register the interpreter for each cpu except for the native one
+ if [ $# -ne 0 ]; then # explicitly requested CPU
+ explicit=yes
+ else # all supported CPUs except of the same family as host CPU
+ explicit=
+ set -- $qemu_target_list
+ fi
- for cpu in ${qemu_target_list} ; do
- magic=$(eval echo \$${cpu}_magic)
- mask=$(eval echo \$${cpu}_mask)
- family=$(eval echo \$${cpu}_family)
+ for cpu in "$@" ; do
+ eval \
+ magic=\"\$${cpu}_magic\" \
+ mask=\"\$${cpu}_mask\" \
+ family=\"\$${cpu}_family\"
if [ "$magic" = "" ] || [ "$mask" = "" ] || [ "$family" = "" ] ; then
- echo "INTERNAL ERROR: unknown cpu $cpu" 1>&2
- continue
+ echo "ERROR: unknown cpu $cpu" >&2
+ exit 1
fi
qemu="$QEMU_PATH/qemu-$cpu"
@@ -329,9 +327,12 @@ qemu_set_binfmts() {
fi
qemu="$qemu$QEMU_SUFFIX"
- if [ "$host_family" != "$family" ] ; then
- $BINFMT_SET
+ if [ "$host_family" = "$family" ] ; then
+ [ -n "$explicit" ] || continue
+ echo "WARNING: requested CPU $cpu is of the same family as host CPU" >&2
+ echo "WARNING: this might break the system" >&2
fi
+ $BINFMT_SET
done
}
@@ -347,9 +348,9 @@ PERSISTENT=no
PRESERVE_ARG0=no
QEMU_SUFFIX=""
-_longopts="debian,systemd:,qemu-path:,qemu-suffix:,exportdir:,help,credential:,\
+_longopts="debian,systemd,qemu-path:,qemu-suffix:,exportdir:,help,credential:,\
persistent:,preserve-argv0:"
-options=$(getopt -o ds:Q:S:e:hc:p:g:F: -l ${_longopts} -- "$@")
+options=$(getopt -o dsQ:S:e:hc:p:g:F: -l ${_longopts} -- "$@") || exit 1
eval set -- "$options"
while true ; do
@@ -363,23 +364,6 @@ while true ; do
CHECK=qemu_check_systemd
BINFMT_SET=qemu_generate_systemd
EXPORTDIR=${EXPORTDIR:-$SYSTEMDDIR}
- shift
- # check given cpu is in the supported CPU list
- if [ "$1" != "ALL" ] ; then
- for cpu in ${qemu_target_list} ; do
- if [ "$cpu" = "$1" ] ; then
- break
- fi
- done
-
- if [ "$cpu" = "$1" ] ; then
- qemu_target_list="$1"
- else
- echo "ERROR: unknown CPU \"$1\"" 1>&2
- usage
- exit 1
- fi
- fi
;;
-Q|--qemu-path)
shift
@@ -409,12 +393,19 @@ while true ; do
shift
PRESERVE_ARG0="$1"
;;
- *)
+ --)
+ shift
break
;;
+ *)
+ exit 1
+ ;;
esac
shift
done
$CHECK
-qemu_set_binfmts
+if [ ALL = "$1" ]; then
+ set --
+fi
+qemu_set_binfmts "$@"
--
2.39.2A friendly ping? Thanks, /mjt 21.07.2023 14:50, Michael Tokarev wrote: > Currently qemu-binfmt-conf.sh does a number of strange things. > > 1. --systemd requires an argument - the CPU type to register, > while --debian (which is actually --binfmt-support) does not > accept such an argument, so it is not possible to specify which > CPU(s) to register for debian. > > 2. Why this "ALL" at all? > > 3. it just ignores extra command-line arguments. It would be > logical to specify which CPUs to register (multiple!) as the > additional arguments. > > 4. Even if a CPU is explicitly requested, it does not register > anything if this CPU is of the same family as host one. But > this is wrong, since quite often it *is* desirable to do this > registration, - like, when running in i386 when the system is > not capable of running x86-64 binaries, and countless other > examples > > 5. It ignores errors > > 6. It ignores wrong command line arguments > > Fix this, and simplify things a bit. > > 1. Stop accepting an argument for --systemd. With getopt_long, > this argument, if given, will be returned as a non-optional > parameter so compatibility with current version is preserved. > > 2. Accept optional arguments and generate registration for the > given CPUs only. In case no extra arguments are given, register > for all supportd CPUs except of the same family as host. > > 3. Recognze "ALL" "CPU" to keep compatibility with current version > (but do not document it). > > 4. Warn but perform registration anyway if a cpu of the same family > has been requested. > > 5. In help text, use --debian and --systemd as alternatives to each > other, to make it clear the two can not be used at the same time. > > 6. Tiny optimization of eval expression. > > 7. Fold the list of supported CPUs to fit in 80 columns. > > 8. Exit with non-zero code in case registration fails or the command > line is wrong. > > 9. Remove explicit checking for writability of various things. > > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > --- > scripts/qemu-binfmt-conf.sh | 89 +++++++++++++++++-------------------- > 1 file changed, 40 insertions(+), 49 deletions(-)
09.09.2023 16:23, Michael Tokarev : > A friendly ping? A friendly ping #2? Thanks, /mjt > 21.07.2023 14:50, Michael Tokarev wrote: >> Currently qemu-binfmt-conf.sh does a number of strange things. >> >> 1. --systemd requires an argument - the CPU type to register, >> while --debian (which is actually --binfmt-support) does not >> accept such an argument, so it is not possible to specify which >> CPU(s) to register for debian. >> >> 2. Why this "ALL" at all? >> >> 3. it just ignores extra command-line arguments. It would be >> logical to specify which CPUs to register (multiple!) as the >> additional arguments. >> >> 4. Even if a CPU is explicitly requested, it does not register >> anything if this CPU is of the same family as host one. But >> this is wrong, since quite often it *is* desirable to do this >> registration, - like, when running in i386 when the system is >> not capable of running x86-64 binaries, and countless other >> examples >> >> 5. It ignores errors >> >> 6. It ignores wrong command line arguments >> >> Fix this, and simplify things a bit. >> >> 1. Stop accepting an argument for --systemd. With getopt_long, >> this argument, if given, will be returned as a non-optional >> parameter so compatibility with current version is preserved. >> >> 2. Accept optional arguments and generate registration for the >> given CPUs only. In case no extra arguments are given, register >> for all supportd CPUs except of the same family as host. >> >> 3. Recognze "ALL" "CPU" to keep compatibility with current version >> (but do not document it). >> >> 4. Warn but perform registration anyway if a cpu of the same family >> has been requested. >> >> 5. In help text, use --debian and --systemd as alternatives to each >> other, to make it clear the two can not be used at the same time. >> >> 6. Tiny optimization of eval expression. >> >> 7. Fold the list of supported CPUs to fit in 80 columns. >> >> 8. Exit with non-zero code in case registration fails or the command >> line is wrong. >> >> 9. Remove explicit checking for writability of various things. >> >> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> >> --- >> scripts/qemu-binfmt-conf.sh | 89 +++++++++++++++++-------------------- >> 1 file changed, 40 insertions(+), 49 deletions(-) > >
On Sun, 18 Feb 2024 at 09:32, Michael Tokarev <mjt@tls.msk.ru> wrote: > > 09.09.2023 16:23, Michael Tokarev : > > A friendly ping? > > A friendly ping #2? Looking at the patch, the commit message lists 9 separate things it does. That suggests it ought to be a 9-patch patchset, not a single patch. I bet most of those 9 would be much easier to review than this, too... thanks -- PMM
23.02.2024 18:55, Peter Maydell: > On Sun, 18 Feb 2024 at 09:32, Michael Tokarev <mjt@tls.msk.ru> wrote: >> >> 09.09.2023 16:23, Michael Tokarev : >>> A friendly ping? >> >> A friendly ping #2? > > Looking at the patch, the commit message lists 9 > separate things it does. That suggests it ought to be > a 9-patch patchset, not a single patch. I bet most of > those 9 would be much easier to review than this, too... When I was doing that, I tried to split things up. But it's.. difficult. Having in mind the amount of issues this script has. Trying to make small changes would be a huge work. All the mentioned points (well, most) are the effect of an almost rewrite. I tried to switch to this script in debian (from the debian-specific thing which was used as a prototype for qemu-binfmt-conf.sh) but failed, that was the result of a minimal cleanup.. And yes, I know it's difficult to review. There's a trade-off - either make it good, or make it easier to review ;) Dunno what to do with it now :) Thanks, /mjt
© 2016 - 2024 Red Hat, Inc.