Re: [PATCH 03/13] linux-user: Add faccessat2() syscall

[Helge Deller]

On 8/26/22 17:17, Richard Henderson wrote:
> On 8/26/22 07:18, Helge Deller wrote:
>> Add implementation and strace output for faccessat2().
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> ---
>>   linux-user/strace.c    |  2 +-
>>   linux-user/strace.list |  3 +++
>>   linux-user/syscall.c   | 12 ++++++++++++
>>   3 files changed, 16 insertions(+), 1 deletion(-)
>
> I've done this one, and cleaned up the code a bit too:
>
> https://lore.kernel.org/qemu-devel/20220729201414.29869-1-richard.henderson@linaro.org/

Btw, your patch is missing the strace parts.

Furthermore, IMHO I think it's a bad idea to replace userspace-requested
syscalls with other (emulated) syscalls - UNLESS(!!) we have to.

I'm debugging frequenty failures with qemu-user which happen building
debian packages. For that I often compare native strace output with qemu
QEMU_STRACE=1 output. If you replace syscalls with others it makes comparing
differences harder.

And, with your patch, if the target has TARGET_NR_faccessat2, but the host
hasn't (and as such emulate it via the host libc), a configure check in the 
guest
would return success. I think it's better to let the guest then fall back, not
the host.

I think we should really just emulate the syscalls the host kernel provides.

Anyway, I'm still ok to drop my patch if you want.

Helge

>
>
> r~
>
>>
>> diff --git a/linux-user/strace.c b/linux-user/strace.c
>> index 27309f1106..e8c63aa4c2 100644
>> --- a/linux-user/strace.c
>> +++ b/linux-user/strace.c
>> @@ -1962,7 +1962,7 @@ print_execv(CPUArchState *cpu_env, const struct 
>> syscallname *name,
>>   }
>>   #endif
>>
>> -#ifdef TARGET_NR_faccessat
>> +#if defined(TARGET_NR_faccessat) || defined(TARGET_NR_faccessat2)
>>   static void
>>   print_faccessat(CPUArchState *cpu_env, const struct syscallname *name,
>>                   abi_long arg0, abi_long arg1, abi_long arg2,
>> diff --git a/linux-user/strace.list b/linux-user/strace.list
>> index a78cdf3cdf..6e88da7fad 100644
>> --- a/linux-user/strace.list
>> +++ b/linux-user/strace.list
>> @@ -177,6 +177,9 @@
>>   #ifdef TARGET_NR_faccessat
>>   { TARGET_NR_faccessat, "faccessat" , NULL, print_faccessat, NULL },
>>   #endif
>> +#ifdef TARGET_NR_faccessat2
>> +{ TARGET_NR_faccessat2, "faccessat2" , NULL, print_faccessat, NULL },
>> +#endif
>>   #ifdef TARGET_NR_fadvise64
>>   { TARGET_NR_fadvise64, "fadvise64" , NULL, NULL, NULL },
>>   #endif
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index f409121202..f51c4fbabd 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -399,6 +399,9 @@ _syscall3(int, ioprio_set, int, which, int, who, int, 
>> ioprio)
>>   #if defined(TARGET_NR_getrandom) && defined(__NR_getrandom)
>>   _syscall3(int, getrandom, void *, buf, size_t, buflen, unsigned int, flags)
>>   #endif
>> +#if defined(TARGET_NR_faccessat2) && defined(__NR_faccessat2)
>> +_syscall4(int, faccessat2, int, dirfd, char *, pathname, int, mode, int, 
>> flags)
>> +#endif
>>
>>   #if defined(TARGET_NR_kcmp) && defined(__NR_kcmp)
>>   _syscall5(int, kcmp, pid_t, pid1, pid_t, pid2, int, type,
>> @@ -9098,6 +9101,15 @@ static abi_long do_syscall1(CPUArchState *cpu_env, 
>> int num, abi_long arg1,
>>           unlock_user(p, arg2, 0);
>>           return ret;
>>   #endif
>> +#if defined(TARGET_NR_faccessat2) && defined(__NR_faccessat2)
>> +    case TARGET_NR_faccessat2:
>> +        if (!(p = lock_user_string(arg2))) {
>> +            return -TARGET_EFAULT;
>> +        }
>> +        ret = get_errno(faccessat2(arg1, p, arg3, arg4));
>> +        unlock_user(p, arg2, 0);
>> +        return ret;
>> +#endif
>>   #ifdef TARGET_NR_nice /* not on alpha */
>>       case TARGET_NR_nice:
>>           return get_errno(nice(arg1));
>> --
>> 2.37.1
>>
>