Re: [Qemu-devel] [PATCH] linux-user: Add binfmt wrapper

[Alexander Graf <agraf@suse.de>]

On 14.07.14 17:59, Joakim Tjernlund wrote:
> Alexander Graf <agraf@suse.de> wrote on 2014/07/14 17:46:18:
>>
>> On 14.07.14 17:38, Joakim Tjernlund wrote:
>>> Alexander Graf <agraf@suse.de> wrote on 2014/07/14 17:21:33:
>>>
>>>> From: Alexander Graf <agraf@suse.de>
>>>> To: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>,
>>>> Cc: qemu-devel@nongnu.org
>>>> Date: 2014/07/14 17:21
>>>> Subject: Re: [PATCH] linux-user: Add binfmt wrapper
>>>>
>>>>
>>>> On 14.07.14 16:38, Joakim Tjernlund wrote:
>>>>> The popular binfmt-wrapper patch adds an additional
>>>>> executable which mangle argv suitable for binfmt flag P.
>>>>> In a chroot you need the both (statically linked) qemu-$arch
>>>>> and qemu-$arch-binfmt-wrapper. This is sub optimal and a
>>>>> better approach is to recognize the -binfmt-wrapper extension
>>>>> within linux-user(qemu-$arch) and mangle argv there.
>>>>> This just produces on executable which can be either copied to
>>>>> the chroot or bind mounted with the appropriate -binfmt-wrapper
>>>>> suffix.
>>>>>
>>>>> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
>>>> Please make sure to CC Riku on patches like this - he is the
> linux-user
>>>> maintainer.
>>> Doesn't he read the devel list? Anyhow CC:ed
>> He may or may not. Qemu-devel can be pretty high volume :).
>>
>>>>> ---
>>>>>     linux-user/main.c | 13 +++++++++++++
>>>>>     1 file changed, 13 insertions(+)
>>>>>
>>>>> diff --git a/linux-user/main.c b/linux-user/main.c
>>>>> index 71a33c7..212067a 100644
>>>>> --- a/linux-user/main.c
>>>>> +++ b/linux-user/main.c
>>>>> @@ -3828,6 +3828,19 @@ int main(int argc, char **argv, char **envp)
>>>>>         int i;
>>>>>         int ret;
>>>>>         int execfd;
>>>>> +    char *binfmt;
>>>>> +
>>>>> +    i = strlen( argv[0] ) - strlen ( "-binfmt-wrapper" );
>>>> The spaces are odd. Did this patch pass checkpatch.pl? Same comment
> goes
>>>> for almost all function invocations.
>>> ehh, didn't run it through checkpatch.pl. Easy to fix next time.
>>>
>>>>> +    binfmt = argv[0] + i;
>>>>> +    if (i > 0 && strcmp ( binfmt, "-binfmt-wrapper" ) == 0) {
>>>> This magic needs to be documented somewhere. In fact, I find it
> pretty
>>>> hard to use in real world scenarios. Imagine a distribution - should
> it
>>>> package every target binary twice? Should it create hardlinks all
> over?
>>> How does dists. handle your original binfmt-wrapper? This is not much
>>> different I think. Here you got a choice to create a hardlink or a
> copy.
>>> Any chroot will only have to bind mount binfmt-wrapper into the chroot
> or
>>> lxc container.
>> Yeah, and there are reasons my original approach isn't upstream :).
> What are those then? Hardly just packaging problem/choise.
>
>>>> I think we should try and find better magic :). Looking at the
>>>> binfmt_misc loading code, I think we can cheat a bit. If we pass the
> 'O'
>>>> flag (open target binary for handler), binfmt_misc will tell us the
>>>> binary fd in AT_EXECFD:
>>>>
>>>>                    NEW_AUX_ENT(AT_EXECFD, bprm->interp_data);
>>>>
>>>> We could then use this as a hint that we were spawned by binfmt_misc
>>>> rather than directly and interpret the first argv as target_argv[0].
>>>>
>>>> Then we can also add the P and O flags to scripts/qemu-binfmt-conf.sh
>>>> and have a solution that works well for everyone.
>>> What to do with P only then? Seems like most dists uses only P
>> If a distro uses the P flag it's not using upstream code, so they have
>> to deal with their own breakage :). Fortunately the binfmt install
>> scripts are usually part of a package too, so they can be updated
> easily.
>
> scripts/qemu-binfmt-conf.sh does not use any flag currently, I don't think
> that works either with current linux-user and choot/lxc
>
> You think everyone feel OK with new defaults like OP ?

Yes.


Alex