Re: [Qemu-devel] [PATCH v3 6/10] qemu-binfmt-conf.sh: generalize <CPU> to positional <CPUS>

[Laurent Vivier]

On 06/03/2019 05:50, Unai Martinez-Corral wrote:
> This breaks brackward compatibility.
> 
> Option '--systemd CPU' allows to register binfmt interpreters for a
> single target architecture or for 'ALL' (of them). This patch
> generalizes the approach to support it in any mode (default, '--debian'
> or '--systemd'). To do so, option 'systemd' is changed to be boolean
> (no args). Then, all the positional arguments are considered to be a
> list of target architectures.
> 
> The list can be separated by spaces, tabs, newlines or commas. If no

I think it would be simpler to not manage commas, and to use the default
field separator.

> positional argument is provided, or when it is 'ALL', all of the
> architectures in qemu_target_list are registered.
> 
> Conversely, argument value 'NONE' allows to make a 'dry run' of the
> script. I.e., checks are executed according to the mode, but no
> interpreter is registered.
> 
> Signed-off-by: Unai Martinez-Corral <address@hidden>
> ---
>  scripts/qemu-binfmt-conf.sh | 92 +++++++++++++++++++++++--------------
>  1 file changed, 57 insertions(+), 35 deletions(-)
> 
> diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
> index c113ff131e..2751363089 100755
> --- a/scripts/qemu-binfmt-conf.sh
> +++ b/scripts/qemu-binfmt-conf.sh
> @@ -6,6 +6,36 @@ 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"

Why do you need to introduce a new variable "all", you can use directly
"qemu_target_list".

> +    if [ "x$1" = "xALL" ] ; then
> +      checked_target_list="$all"
> +      return
> +    fi
> +    list=""

No need to introduce a new variable, you can use directly
checked_target_list

> +    bIFS="$IFS"
> +    IFS=$"$IFS",

Keep it simple, don't manage comma.

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

Use directly "qemu_target_list"

> +            if [ "x$cpu" = "x$target" ] ; then
> +                list="$list $target"
> +                unknown_target="false"
> +                break
> +            fi
> +        done
> +        if [ "$unknown_target" = "true" ] ; then

You don't need a new variable, you can check "x$cpu" = "x$target" to
know if you have found the target.

> +            IFS="$bIFS"
> +            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,11 +197,14 @@ qemu_get_family() {
> 
>  usage() {
>      cat <<EOF
> -Usage: qemu-binfmt-conf.sh [--path PATH][--debian][--systemd CPU]
> +Usage: qemu-binfmt-conf.sh [--path PATH][--debian][--systemd]
>                             [--help][--credential][--exportdir PATH]
> -                           [--persistent][--suffix SUFFIX]
> +                           [--persistent][--suffix SUFFIX][CPUS]
> 
> -       Configure binfmt_misc to use qemu interpreter
> +       Configure binfmt_misc to use qemu interpreter for the given CPUS.
> +       Supported formats for CPUS are: single arch or comma/space separated 
> list.
> +       See QEMU target list below. If CPUS is 'ALL' or empty, configure all 
> known
> +       cpus. If CPUS is 'NONE', no interpreter is configured.
> 
>         --help:        display this usage
>         --path:        set path to qemu interpreter ($QEMU_PATH)
> @@ -180,9 +213,10 @@ Usage: qemu-binfmt-conf.sh [--path 
> PATH][--debian][--systemd CPU]
>         --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
> +                      instead generate file for systemd-binfmt.service;
> +                      environment variable HOST_ARCH allows to override 
> 'uname'
> +                      to generate configuration files for a different
> +                      architecture than the current one.
>         --exportdir:   define where to write configuration files
>                        (default: $SYSTEMDDIR or $DEBIANDIR)
>         --credential:  if present, credential and security tokens are
> @@ -201,14 +235,7 @@ Usage: qemu-binfmt-conf.sh [--path 
> PATH][--debian][--systemd CPU]
> 
>          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
> +    QEMU target list: $qemu_target_list
> 
>  EOF
>  }
> @@ -289,12 +316,22 @@ EOF
>  }
> 
>  qemu_set_binfmts() {
> +    if [ "x$1" = "xNONE" ] ; then
> +      return
> +    fi
> +
>      # probe cpu type
>      host_family=$(qemu_get_family)
> 
> +    # 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
> +
>      # register the interpreter for each cpu except for the native one
> -
> -    for cpu in ${qemu_target_list} ; do
> +    for cpu in $checked_target_list ; do
>          magic=$(eval echo \$${cpu}_magic)
>          mask=$(eval echo \$${cpu}_mask)
>          family=$(eval echo \$${cpu}_family)
> @@ -327,7 +364,7 @@ QEMU_SUFFIX="${QEMU_SUFFIX:-}"
>  QEMU_CREDENTIAL="${QEMU_CREDENTIAL:-no}"
>  QEMU_PERSISTENT="${QEMU_PERSISTENT:-no}"
> 
> -options=$(getopt -o ds:Q:S:e:hcp -l 
> debian,systemd:,path:,suffix:,exportdir:,help,credential,persistent -- "$@")
> +options=$(getopt -o :dsQ:S:e:hcp -l 
> debian,systemd,path:,suffix:,exportdir:,help,credential,persistent -- "$@")

I think you don't need to add the ":" at the beginning of the list.

Thanks,
Laurent