[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] linux-user: Original qemu-binfmt-conf.h is o
From: |
Laurent Vivier |
Subject: |
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. |
Date: |
Thu, 28 Jan 2016 23:51:44 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 |
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