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.

[Laurent Vivier]

Le 28/01/2016 23:29, Eric Blake a écrit :
> 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

OK. I was waiting this comment ;)

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

Well, I've learned that on HP-UX 10.20 (in '90s), so I'm not surprised
it could have some troubles now. I will change that.

>> +Usage: qemu-binfmt-conf.sh [--qemu-path
>> PATH][--debian][--systemd CPU]
> 
>> +       --credential: if yes, credential an security tokens are
> 
> s/an/and/

OK

> 
>> +    echo -n "    "
> 
> 'echo -n' is not portable.  Use 'printf' instead.

OK

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

OK

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

OK

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

Checking right access allows to know if the system supports
binfmt_misc, debian packages or systemd, and if we can write here (are
we root ?, see Alex comment), so this check is really needed here. No
need to care of TOCTTOU.

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

Yes, I often forget French and English differ in the use of
punctuation. :)
I will remove the '!'.

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

OK

>> +qemu_generate_debian() { +    cat > "$EXPORTDIR/qemu-$cpu"
>> <<!EOF +package qemu-$cpu
> 
> Again, !EOF is an unusual delimiter.

OK

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

OK

Thank you!

Laurent