Re: [Qemu-devel] [PATCH v12 3/5] linux-user: Add support for translation of statx() syscall
Le 27/06/2019 à 15:18, Aleksandar Markovic a écrit :
>> From: Laurent Vivier
>>
>>> @@ -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
Re: [Qemu-devel] [PATCH v12 3/5] linux-user: Add support for translation of statx() syscall
> From: Laurent Vivier
>
> > @@ -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.
Yours,
Aleksandar Markovic
> Thanks,
> Laurent
Re: [Qemu-devel] [PATCH v12 3/5] linux-user: Add support for translation of statx() syscall
Le 19/06/2019 à 16:17, Aleksandar Markovic a écrit :
> From: Aleksandar Rikalo
>
> Implement support for translation of system call statx().
>
> The implementation is based on "best effort" approach: if host is
> capable of executing statx(), host statx() is used. If not, the
> implementation includes invoking other (more mature) system calls
> (from the same 'stat' family) on the host side to achieve as close
> as possible functionality.
>
> Support for statx() in kernel and glibc was, however, introduced
> at different points of time (the difference is more than a year):
>
> - kernel: Linux 4.11 (30 April 2017)
> - glibc: glibc 2.28 (1 Aug 2018)
>
> In this patch, the availability of statx() support is established
> via __NR_statx (if it is defined, statx() is considered available).
> This coincedes with statx() introduction in kernel.
>
> However, the structure statx definition may not be available for hosts
> with glibc older than 2.28 (it is, by design, to be defined in one of
> glibc headers), even though the full statx() functionality may be
> supported in kernel, if the kernel is not older than 4.11. Hence,
> a structure "target_statx" is defined in this patch, to remove that
> dependency on glibc headers, and to use statx() functionality as soon
> as the host kernel is capable of supporting it. Such structure statx
> definition is used for both target and host structures statx (of
> course, this doesn't mean the endian arrangement is the same on
> target and host, and endian conversion is done in all necessary
> cases).
>
> Signed-off-by: Aleksandar Rikalo
> Signed-off-by: Aleksandar Markovic
> ---
> linux-user/syscall.c | 136
> +-
> linux-user/syscall_defs.h | 37 +
> 2 files changed, 172 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index d116287..e68a36c 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -43,6 +43,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -316,6 +317,14 @@ _syscall5(int, kcmp, pid_t, pid1, pid_t, pid2, int, type,
>unsigned long, idx1, unsigned long, idx2)
> #endif
>
> +/*
> + * It is assumed that struct statx is architecture independent.
> + */
> +#if defined(TARGET_NR_statx) && defined(__NR_statx)
> +_syscall5(int, statx, int, dirfd, const char *, pathname, int, flags,
> + unsigned int, mask, struct target_statx *, statxbuf)
> +#endif
> +
> static bitmask_transtbl fcntl_flags_tbl[] = {
>{ TARGET_O_ACCMODE, TARGET_O_WRONLY,O_ACCMODE, O_WRONLY,},
>{ TARGET_O_ACCMODE, TARGET_O_RDWR, O_ACCMODE, O_RDWR, },
> @@ -6517,6 +6526,48 @@ static inline abi_long host_to_target_stat64(void
> *cpu_env,
> }
> #endif
>
> +#if defined(TARGET_NR_statx) && defined(__NR_statx)
> +static inline abi_long host_to_target_statx(struct target_statx *host_stx,
> +abi_ulong target_addr)
> +{
> +struct target_statx *target_stx;
> +
> +if (!lock_user_struct(VERIFY_WRITE, target_stx, target_addr, 0)) {
> +return -TARGET_EFAULT;
> +}
> +memset(target_stx, 0, sizeof(*target_stx));
> +
> +__put_user(host_stx->stx_mask, &target_stx->stx_mask);
> +__put_user(host_stx->stx_blksize, &target_stx->stx_blksize);
> +__put_user(host_stx->stx_attributes, &target_stx->stx_attributes);
> +__put_user(host_stx->stx_nlink, &target_stx->stx_nlink);
> +__put_user(host_stx->stx_uid, &target_stx->stx_uid);
> +__put_user(host_stx->stx_gid, &target_stx->stx_gid);
> +__put_user(host_stx->stx_mode, &target_stx->stx_mode);
> +__put_user(host_stx->stx_ino, &target_stx->stx_ino);
> +__put_user(host_stx->stx_size, &target_stx->stx_size);
> +__put_user(host_stx->stx_blocks, &target_stx->stx_blocks);
> +__put_user(host_stx->stx_attributes_mask,
> &target_stx->stx_attributes_mask);
> +__put_user(host_stx->stx_atime.tv_sec, &target_stx->stx_atime.tv_sec);
> +__put_user(host_stx->stx_atime.tv_nsec, &target_stx->stx_atime.tv_nsec);
> +__put_user(host_stx->stx_btime.tv_sec, &target_stx->stx_atime.tv_sec);
> +__put_user(host_stx->stx_btime.tv_nsec, &target_stx->stx_atime.tv_nsec);
> +__put_user(host_stx->stx_ctime.tv_sec, &target_stx->stx_atime.tv_sec);
> +__put_user(host_stx->stx_ctime.tv_nsec, &target_stx->stx_atime.tv_nsec);
> +__put_user(host_stx->stx_mtime.tv_sec, &target_stx->stx_atime.tv_sec);
> +__put_user(host_stx->stx_mtime.tv_nsec, &target_stx->stx_atime.tv_nsec);
> +__put_user(host_stx->stx_rdev_major, &target_stx->stx_rdev_major);
> +__put_user(host_stx->stx_rdev_minor, &target_stx->stx_rdev_minor);
> +__put_user(host_stx->stx_dev_major, &target_stx->stx_dev_major);
> +__put_user(host_stx->stx_dev_minor, &target_stx->stx_dev_minor);
> +
> +unlock_user_struct(target_stx, target_addr, 1);
> +
> +return 0;
