Signed-off-by: Unai Martinez-Corral <unai.martinezcorral@ehu.eus>
---
 scripts/qemu-binfmt-conf.sh | 190 +++++++++++++++++++++++-------------
 1 file changed, 121 insertions(+), 69 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index b5a16742a1..b1aa4a312a 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -6,6 +6,35 @@ mips mipsel mipsn32 mipsn32el mips64 mips64el \
 sh4 sh4eb s390x aarch64 aarch64_be hppa riscv32 riscv64 xtensa xtensaeb \
 microblaze microblazeel or1k x86_64"

+# check if given target CPUS is/are in the supported target list
+qemu_check_target_list() {
+    all="$qemu_target_list"
+    if [ "x$1" = "xALL" ]; then
+      checked_target_list="$all"
+      return
+    fi
+    list=""
+    bIFS="$IFS"
+    IFS=" ,"
+    for target in $@; do
+        unknown_target="true"
+        for cpu in $all ; do
+            if [ "x$cpu" = "x$target" ] ; then
+                list="$list $target"
+                unknown_target="false"
+                break
+            fi
+        done
+        if [ "$unknown_target" = "true" ] ; then
+            echo "ERROR: unknown CPU \"$target\"" 1>&2
+            usage
+            exit 1
+        fi
+    done
+    IFS="$bIFS"
+    checked_target_list="$list"
+}
+
 i386_magic='\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x03\x00'
 i386_mask='\xff\xff\xff\xff\xff\xfe\xfe\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff'
 i386_family=i386
@@ -167,45 +196,48 @@ qemu_get_family() {

 usage() {
     cat <<EOF
-Usage: qemu-binfmt-conf.sh [--qemu-path PATH][--debian][--systemd CPU]
-                           [--help][--credential yes|no][--exportdir PATH]
-                           [--persistent yes|no][--qemu-suffix SUFFIX]
-
-       Configure binfmt_misc to use qemu interpreter
-
-       --help:        display this usage
-       --qemu-path:   set path to qemu interpreter ($QEMU_PATH)
-       --qemu-suffix: add a suffix to the default interpreter name
-       --debian:      don't write into /proc,
-                      instead generate update-binfmts templates
-       --systemd:     don't write into /proc,
-                      instead generate file for systemd-binfmt.service
-                      for the given CPU. If CPU is "ALL", generate a
-                      file for all known cpus
-       --exportdir:   define where to write configuration files
-                      (default: $SYSTEMDDIR or $DEBIANDIR)
-       --credential:  if yes, credential and security tokens are
-                      calculated according to the binary to interpret
-       --persistent:  if yes, the interpreter is loaded when binfmt is
-                      configured and remains in memory. All future uses
-                      are cloned from the open file.
-
-    To import templates with update-binfmts, use :
-
-        sudo update-binfmts --importdir ${EXPORTDIR:-$DEBIANDIR}
--import qemu-CPU
-
-    To remove interpreter, use :
-
-        sudo update-binfmts --package qemu-CPU --remove qemu-CPU $QEMU_PATH
-
-    With systemd, binfmt files are loaded by systemd-binfmt.service
-
-    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
+Usage: qemu-binfmt-conf.sh [--help][--qemu-path PATH][--qemu-suffix SUFFIX]
+                           [--persistent][--credential][--exportdir PATH]
+                           [--reset ARCHS][--systemd][--debian][CPUS]
+
+    Configure binfmt_misc to use qemu interpreter for the given target CPUS.
+    Supported formats for CPUS are: single arch or comma/space separated list.
+    If CPUS is empty, configure all known cpus. See QEMU target list below.
+
+    --help:        display this usage.
+    --qemu-path:   set path to qemu interpreter ($QEMU_PATH).
+    --qemu-suffix: add a suffix to the default interpreter name.
+    --persistent:  if present, the interpreter is loaded when binfmt is
+                   configured and remains in memory. All future uses
+                   are cloned from the open file.
+    --credential:  if present, credential and security tokens are
+                   calculated according to the binary to interpret.
+    --exportdir:   define where to write configuration files
+                   (default: $SYSTEMDDIR or $DEBIANDIR).
+    --reset:       remove registered interpreter for target ARCHS (comma
+                   separated list). If ARCHS is 'ALL', remove all registered
+                   'qemu-*' interpreters.
+    --systemd:     don't write into /proc,
+                   instead generate file(s) for systemd-binfmt.service;
+                   environment variable HOST_ARCH allows to override 'uname'
+                   to generate configuration files for a different
+                   architecture than the current one.
+    --debian:      don't write into /proc,
+                   instead generate update-binfmts templates.
+
+      To import templates with update-binfmts, use :
+
+        sudo update-binfmts \\
+             --importdir ${EXPORTDIR:-$DEBIANDIR} \\
+             --import qemu-CPU
+
+      To remove interpreter, use :
+
+        sudo update-binfmts \\
+             --package qemu-CPU \\
+             --remove qemu-CPU $QEMU_PATH
+
+    QEMU target list: $qemu_target_list

 EOF
 }
@@ -255,10 +287,10 @@ qemu_check_systemd() {

 qemu_generate_register() {
     flags=""
-    if [ "$CREDENTIAL" = "yes" ] ; then
+    if [ "x$CREDENTIAL" = "xyes" ] ; then
         flags="OC"
     fi
-    if [ "$PERSISTENT" = "yes" ] ; then
+    if [ "x$PERSISTENT" = "xyes" ] ; then
         flags="${flags}F"
     fi

@@ -286,35 +318,65 @@ EOF
 }

 qemu_set_binfmts() {
+    if [ "x$1" = "xNONE" ]; then
+      return
+    fi
+
     # probe cpu type
     host_family=$(qemu_get_family)

-    # register the interpreter for each cpu except for the native one
+    # reduce the list of target interpreters to those given in the CLI
+    targets="$@"
+    if [ $# -eq 0 ]; then
+      targets="ALL"
+    fi
+    qemu_check_target_list $targets

-    for cpu in ${qemu_target_list} ; do
+    # register the interpreter for each target except for the native one
+    for cpu in $checked_target_list; do
         magic=$(eval echo \$${cpu}_magic)
         mask=$(eval echo \$${cpu}_mask)
         family=$(eval echo \$${cpu}_family)

-        if [ "$magic" = "" ] || [ "$mask" = "" ] || [ "$family" = "" ] ; then
+        if [ "x$magic" = "x" ] || [ "x$mask" = "x" ] || [ "x$family"
= "x" ] ; then
             echo "INTERNAL ERROR: unknown cpu $cpu" 1>&2
             continue
         fi

         qemu="$QEMU_PATH/qemu-$cpu"
-        if [ "$cpu" = "i486" ] ; then
+        if [ "x$cpu" = "xi486" ] ; then
             qemu="$QEMU_PATH/qemu-i386"
         fi

         qemu="$qemu$QEMU_SUFFIX"
-        if [ "$host_family" != "$family" ] ; then
+        if [ "x$host_family" != "x$family" ] ; then
             $BINFMT_SET
         fi
     done
 }

+qemu_remove_notimplemented() {
+    echo "ERROR: option reset not implemented for this mode yet" 1>&2
+    usage
+    exit 1
+}
+
+qemu_remove_interpreter() {
+    names='qemu-*'
+    if [ "x$1" != "xALL" ]; then
+        qemu_check_target_list $1
+        unset names pre
+        for t in $checked_target_list; do
+            names="${names}${pre}qemu-$t"
+            pre=' -o -name '
+        done
+    fi
+    find /proc/sys/fs/binfmt_misc/ -type f -name $names -exec sh -c
'printf %s -1 > {}' \;
+}
+
 CHECK=qemu_check_bintfmt_misc
 BINFMT_SET=qemu_register_interpreter
+BINFMT_REMOVE=qemu_remove_interpreter

 SYSTEMDDIR="/etc/binfmt.d"
 DEBIANDIR="/usr/share/binfmts"
@@ -324,37 +386,27 @@ CREDENTIAL=no
 PERSISTENT=no
 QEMU_SUFFIX=""

-options=$(getopt -o ds:Q:S:e:hc:p: -l
debian,systemd:,qemu-path:,qemu-suffix:,exportdir:,help,credential:,persistent:
-- "$@")
+options=$(getopt -o r:dsQ:S:e:hcp -l
reset:,debian,systemd,qemu-path:,qemu-suffix:,exportdir:,help,credential,persistent
-- "$@")
 eval set -- "$options"

 while true ; do
     case "$1" in
+    -r|--reset)
+        shift
+        $CHECK
+        qemu_remove_interpreter $1
+        ;;
     -d|--debian)
         CHECK=qemu_check_debian
         BINFMT_SET=qemu_generate_debian
+        BINFMT_REMOVE=qemu_remove_notimplemented
         EXPORTDIR=${EXPORTDIR:-$DEBIANDIR}
         ;;
     -s|--systemd)
         CHECK=qemu_check_systemd
         BINFMT_SET=qemu_generate_systemd
+        BINFMT_REMOVE=qemu_remove_notimplemented
         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
@@ -373,12 +425,10 @@ while true ; do
         exit 1
         ;;
     -c|--credential)
-        shift
-        CREDENTIAL="$1"
+        CREDENTIAL=yes
         ;;
     -p|--persistent)
-        shift
-        PERSISTENT="$1"
+        PERSISTENT=yes
         ;;
     *)
         break
@@ -387,5 +437,7 @@ while true ; do
     shift
 done

+shift
+
 $CHECK
-qemu_set_binfmts
+qemu_set_binfmts $@
-- 
2.20.1

On 3/5/19 1:32 PM, Unai Martinez Corral wrote:
> Signed-off-by: Unai Martinez-Corral <unai.martinezcorral@ehu.eus>

Welcome to the qemu community, and I hope you find your experience with 
your first patch pleasant.

Long subject line, I'd trim ' (no arg)'. The commit body itself should 
go into the "why" the patch is useful (what behaviors are you fixing, 
what risk of backwards-compatibility breaks do we have to contend with 
for older clients of the script, etc).  Also, a v2 patch is best sent as 
a new top-level thread, rather than in-reply to the v1 (as some of our 
automated CI tooling misses it otherwise).  More submission hints can be 
found at: https://wiki.qemu.org/Contribute/SubmitAPatch

There may be enough changes here to be worth splitting this into 
multiple patches, for easier review (focus on one thing per patch: 
adding --reset sounds different enough from changing -p/-c, which in 
turn sounds different from fixing pre-existing shell usage bugs 
regarding prepending x when using []).

> ---
>   scripts/qemu-binfmt-conf.sh | 190 +++++++++++++++++++++++-------------
>   1 file changed, 121 insertions(+), 69 deletions(-)
> 
> diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
> index b5a16742a1..b1aa4a312a 100755
> --- a/scripts/qemu-binfmt-conf.sh
> +++ b/scripts/qemu-binfmt-conf.sh
> @@ -6,6 +6,35 @@ mips mipsel mipsn32 mipsn32el mips64 mips64el \
>   sh4 sh4eb s390x aarch64 aarch64_be hppa riscv32 riscv64 xtensa xtensaeb \
>   microblaze microblazeel or1k x86_64"
> 
> +# check if given target CPUS is/are in the supported target list
> +qemu_check_target_list() {
> +    all="$qemu_target_list"
> +    if [ "x$1" = "xALL" ]; then
> +      checked_target_list="$all"
> +      return
> +    fi
> +    list=""
> +    bIFS="$IFS"
> +    IFS=" ,"

If you are allowing space, why not tab and newline?

> +    for target in $@; do

The ' in $@' is redundant, but doesn't hurt.

> +        unknown_target="true"
> +        for cpu in $all ; do

Why the space before ';' here, but not two lines earlier? The space 
doesn't hurt, but being consistent is nice.

> +            if [ "x$cpu" = "x$target" ] ; then

Similar questions about why only some of your additions have space 
before ; after ]

> +                list="$list $target"
> +                unknown_target="false"
> +                break
> +            fi
> +        done
> +        if [ "$unknown_target" = "true" ] ; then
> +            echo "ERROR: unknown CPU \"$target\"" 1>&2
> +            usage
> +            exit 1
> +        fi
> +    done
> +    IFS="$bIFS"
> +    checked_target_list="$list"
> +}
> +
>   i386_magic='\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x03\x00'
>   i386_mask='\xff\xff\xff\xff\xff\xfe\xfe\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff'
>   i386_family=i386
> @@ -167,45 +196,48 @@ qemu_get_family() {
> 
>   usage() {
>       cat <<EOF
> -Usage: qemu-binfmt-conf.sh [--qemu-path PATH][--debian][--systemd CPU]
> -                           [--help][--credential yes|no][--exportdir PATH]
> -                           [--persistent yes|no][--qemu-suffix SUFFIX]
> -

> +Usage: qemu-binfmt-conf.sh [--help][--qemu-path PATH][--qemu-suffix SUFFIX]
> +                           [--persistent][--credential][--exportdir PATH]
> +                           [--reset ARCHS][--systemd][--debian][CPUS]
> +

You are making a backwards-incompatible change with -p/-c - why is that 
okay? (It might be, but the commit message needs to give it more attention).

> +    Configure binfmt_misc to use qemu interpreter for the given target CPUS.
> +    Supported formats for CPUS are: single arch or comma/space separated list.
> +    If CPUS is empty, configure all known cpus. See QEMU target list below.

The wholesale reindentation of the --help text makes it harder to see 
what is actually new content. A separate patch to reflow/reindent the 
text before making additions will better call the reviewer's attention 
to the important changes.


>   $CHECK
> -qemu_set_binfmts
> +qemu_set_binfmts $@

This should be "$@", not $@.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

2019/3/5 17:57, Eric Blake:
> I don't have a strong preference between the two forms - so this is your
> chance to assert your creative liberty. But I don't know enough about
> binfmt_misc to know if the newline matters. I'm just reviewing on the
> basis of shell portability, and hope that other reviewers more familiar
> with binfmt will review on content.

According to https://www.kernel.org/doc/Documentation/admin-guide/binfmt-misc.rst
it seems that only the integer matters. However, I'd also like to hear
any other thoughts.

> you may want to split this into a series of
> patches for v3, although you could also wait a day or so to see if
> anyone else reviews and minimize the list churn.

I will do so. It will take some time to reorganize the content and
apply the latest fixes.

> Welcome to the qemu community, and I hope you find your experience with
> your first patch pleasant.

Thanks! I'm really struggling to find a proper mail client, since I'm
on Windows and none of the web clients work properly for me.
But I hope I'll find some workaround.

> Long subject line, I'd trim ' (no arg)'. The commit body itself should
> go into the "why" the patch is useful (what behaviors are you fixing,
> what risk of backwards-compatibility breaks do we have to contend with
> for older clients of the script, etc).

Will do.

>  Also, a v2 patch is best sent as
> a new top-level thread, rather than in-reply to the v1 (as some of our
> automated CI tooling misses it otherwise).

I'm sorry I missed this explanation.

Thanks again for your time and suggestions.

Unai