Re: [Qemu-devel] [PATCH v12 3/5] linux-user: Add support for translation of statx() syscall

[Laurent Vivier]

Le 27/06/2019 à 15:18, Aleksandar Markovic a écrit :
>> From: Laurent Vivier <address@hidden>
>>
>>> @@ -10173,6 +10225,88 @@ static abi_long do_syscall1(void *cpu_env, int 
>>> num, abi_long > arg1,
>>>              ret = host_to_target_stat64(cpu_env, arg3, &st);
>>>          return ret;
>>>  #endif
>>> +#if defined(TARGET_NR_statx)
>>> +    case TARGET_NR_statx:
>>> +        {
>>> +            struct target_statx *target_stx;
>>> +            int dirfd = arg1;
>>> +            int flags = arg3;
>>> +
>>> +            p = lock_user_string(arg2);
>>> +            if (p == NULL) {
>>> +                return -TARGET_EFAULT;
>>> +            }
>>> +#if defined(__NR_statx)
>>> +            {
>>> +                /*
>>> +                 * It is assumed that struct statx is architecture 
>>> independent.
>>> +                 */
>>> +                struct target_statx host_stx;
>>> +                int mask = arg4;
>>> +
>>> +                ret = get_errno(statx(dirfd, p, flags, mask, &host_stx));
>>> +                if (!is_error(ret)) {
>>> +                    if (host_to_target_statx(&host_stx, arg5) != 0) {
>>> +                        unlock_user(p, arg2, 0);
>>> +                        return -TARGET_EFAULT;
>>> +                    }
>>> +                }
>>> +
>>> +                if (ret != -TARGET_ENOSYS) {
>>> +                    unlock_user(p, arg2, 0);
>>> +                    return ret;
>>> +                }
>>> +            }
>>> +#endif
>>> +            if (*((char *)p) == 0) {
>>> +                /*
>>> +                 * By file descriptor
>>> +                 */
>>> +                if (flags & AT_EMPTY_PATH) {
>>> +                    unlock_user(p, arg2, 0);
>>> +                    return -TARGET_ENOENT;
>>> +                }
>>> +                ret = get_errno(fstat(dirfd, &st));
>>> +            } else if (*((char *)p) == '/') {
>>> +                /*
>>> +                 * By absolute pathname
>>> +                 */
>>> +                ret = get_errno(stat(path(p), &st));
>>> +            } else {
>>> +                /*
>>> +                 * By pathname relative to the current working directory
>>> +                 * (if 'dirfd' is AT_FDCWD) or relative to the directory
>>> +                 * referred to by the file descriptor 'dirfd'.
>>> +                 */
>>> +                 ret = get_errno(fstatat(dirfd, path(p), &st, flags));
>>> +            }
>>> +            unlock_user(p, arg2, 0);
>>
>> Could you explain why we can't use fstatat() for the two previous cases
>> "(*((char *)p) == 0)" and "(*((char *)p) == '/')"?
>>
> 
> Man page on fstatat (http://man7.org/linux/man-pages/man2/stat.2.html)
> says:
> 
>    AT_EMPTY_PATH (since Linux 2.6.39)
>           If pathname is an empty string, operate on the file referred
>           to by dirfd (which may have been obtained using the open(2)
>           O_PATH flag).  In this case, dirfd can refer to any type of
>           file, not just a directory, and the behavior of fstatat() is
>           similar to that of fstat().  If dirfd is AT_FDCWD, the call
>           operates on the current working directory.  This flag is
>           Linux-specific; define _GNU_SOURCE to obtain its definition.
> 
> So it looks the branch "if (*((char *)p) == 0)" can be handled by
> fstatat().
> 
> Also, the man page says:
> 
>    If pathname is absolute, then dirfd is ignored.
> 
> So, it looks the case "else if (*((char *)p) == '/')" can also be
> handled by fstatat().
> 
> Very similar descriptions of the cases above can be found in
> the man page for statx
> (http://man7.org/linux/man-pages/man2/statx.2.html).
> 
> The whole string of if statements after "#endif" above should be now,
> in my opinion:
> 
>         ret = get_errno(fstatat(dirfd, path(p), &st, flags));
>         unlock_user(p, arg2, 0);
> 
> ... and I will submit the patch with such code, if noone objects.
> 

I agree.

Thanks,
Laurent