Re: [Qemu-devel] [PATCH v2] linux-user: Original qemu-binfmt-conf.h is only able to write configuration into /proc/sys/fs/binfmt_misc, and the configuration is lost on reboot.

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3707 bytes --]

On 01/28/2016 03:08 PM, Laurent Vivier wrote:

Subject line is TOOO long.  I suggest:

linux-user: Fix qemu-binfmt-conf.h to store config across reboot

> This script can configure debian and systemd services to restore
> configuration on reboot. Moreover, it is able to manage binfmt
> credential and to configure the path of the interpreter.
> 

> diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
> old mode 100644
> new mode 100755
> index 289b1a3..56bc88e
> --- a/scripts/qemu-binfmt-conf.sh
> +++ b/scripts/qemu-binfmt-conf.sh
> @@ -1,72 +1,314 @@
>  #!/bin/sh
>  # enable automatic i386/ARM/M68K/MIPS/SPARC/PPC/s390 program execution by the kernel
>  

> +aarch64_magic='\x7fELF\x02\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\xb7\x00'
> +aarch64_mask='\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff'
> +aarch64_family=arm
> +
> +qemu_get_family() {

> +
> +usage() {
> +    cat <<!EOF

Use of '!EOF' is an unusual heredoc delimiter; it fails miserably if
history expansion is enabled.

> +Usage: qemu-binfmt-conf.sh [--qemu-path PATH][--debian][--systemd CPU]

> +       --credential: if yes, credential an security tokens are

s/an/and/

> +    echo -n "    "

'echo -n' is not portable.  Use 'printf' instead.

> +    for CPU in $qemu_target_list ;
> +    do
> +        echo -n "$CPU "

and again.

> +    done
> +    echo

This loop results in trailing whitespace (not fatal, but nice to avoid
where possible).  Also, using a shell loop is a waste of effort; when
you can just do:

printf "%s " $qemu_target_list

and get the same effect.

> +}
> +
> +qemu_check_access() {
> +    if [ ! -w "$1" ] ; then
> +        echo "ERROR: cannot write to $1" 1>&2
> +        exit 1

Checking whether a file is writable is often a TOCTTOU race; since you
have to handle failures to redirect to the file anyways (in case the
file changed between your check and the actual use), can you just skip
the check as redundant?


> +qemu_check_debian() {
> +    if [ ! -e /etc/debian_version ] ; then
> +        echo "WARNING: your system is not a Debian based distro" 1>&2
> +    elif ! installed_dpkg binfmt-support ; then
> +        echo "WARNING: package binfmt-support is needed !" 1>&2

Trailing '!' in error messages is shouting at the user; I tend to avoid
them.  But if you must use it, in English there is no space between the
final word and the punctuation: s/needed !/needed!/

> +    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

and again

> +qemu_generate_debian() {
> +    cat > "$EXPORTDIR/qemu-$cpu" <<!EOF
> +package qemu-$cpu

Again, !EOF is an unusual delimiter.

> +qemu_set_binfmts() {
> +    # probe cpu type
> +    host_family=$(qemu_get_family)
> +
> +    # register the interpreter for each cpu except for the native one
> +
> +    for cpu in ${qemu_target_list} ; do
> +        magic=$(eval echo \$${cpu}_magic)
> +        mask=$(eval echo \$${cpu}_mask)
> +        family=$(eval echo \$${cpu}_family)

Use of eval is risky; fortunately, it looks like $qemu_target_list is
under your control and can't be overridden by the user's environment to
do something malicious.

> +
> +        if [ "$magic" = "" -o "$mask" = "" -o "$family" = "" ] ; then

"[ ... -o ... ]" is not portable.  Use "[ ... ] || [ ... ]" instead.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]