Re: [Qemu-devel] [PATCH v2 1/3] linux-user: Add support for statx() syscall

[Laurent Vivier]

On 23/10/2018 14:07, Aleksandar Markovic wrote:
> From: Aleksandar Rikalo <address@hidden>
> 
> Implement support for translation of system call statx(). The
> implementation includes invoking other (more mature) syscalls
> (from the same 'stat' family) on the host side. This way,
> problems of availability of statx() on the host side are
> avoided.
> 
> Signed-off-by: Aleksandar Rikalo <address@hidden>
> Signed-off-by: Stefan Markovic <address@hidden>
> Signed-off-by: Aleksandar Markovic <address@hidden>
> ---
>  linux-user/syscall.c      | 129 
> +++++++++++++++++++++++++++++++++++++++++++++-
>  linux-user/syscall_defs.h |  38 ++++++++++++++
>  2 files changed, 166 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index d2cc971..8b01ab0 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -6710,7 +6710,8 @@ static abi_long do_syscall1(void *cpu_env, int num, 
> abi_long arg1,
>      abi_long ret;
>  #if defined(TARGET_NR_stat) || defined(TARGET_NR_stat64) \
>      || defined(TARGET_NR_lstat) || defined(TARGET_NR_lstat64) \
> -    || defined(TARGET_NR_fstat) || defined(TARGET_NR_fstat64)
> +    || defined(TARGET_NR_fstat) || defined(TARGET_NR_fstat64) \
> +    || defined(TARGET_NR_statx)
>      struct stat st;
>  #endif
>  #if defined(TARGET_NR_statfs) || defined(TARGET_NR_statfs64) \
> @@ -9635,6 +9636,132 @@ 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 = tswap32(arg1);
> +            int flags = tswap32(arg3);
> +

Normally arg1, arg3 are already in the host byte order (like arg2 below)

> +            p = lock_user_string(arg2);
> +            if (p == NULL) {
> +                return -TARGET_EFAULT;
> +            }
> +#if defined(__NR_statx)
> +            {
> +                /*
> +                 * It is assumed that struct statx is arhitecture independent
> +                 */
> +                struct target_statx host_stx;

if it is host_statx, the structure to use is statx, not target_statx.

> +                int mask = tswap32(arg4);

no tswap32() needed

> +
> +                ret = get_errno(syscall(__NR_statx, dirfd, p, flags, mask,
> +                                        &host_stx));
> +                if (!is_error(ret)) {
> +                    unlock_user(p, arg2, 0);
> +                    if (!lock_user_struct(VERIFY_WRITE, target_stx, arg5, 
> 0)) {
> +                        return -TARGET_EFAULT;
> +                    }
> +                    memset(target_stx, 0, sizeof(*target_stx));


It would be clearer to put the following conversion lines into a
separate function like host_to_target_statx().

> +                    __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);

perhaps you can also define host_target_statx_timestamp() function to
convert the timestamps.

> +                    __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);
> +                }
> +
> +                if (ret != TARGET_ENOSYS) {
> +                    break;

it should be "return ret;"

> +                }
> +            }
> +#endif

You use p here as it can be unlocked in the last 'if" body".

> +            if ((p == NULL) || (*((char *)p) == 0)) {
> +                /*
> +                 * By file descriptor
> +                 */

if AT_EMPY_PATH is not specified in the flags, you should return -ENOENT

> +                ret = get_errno(fstat(dirfd, &st));
> +                unlock_user(p, arg2, 0);
> +            } else if (*((char *)p) == '/') {
> +                /*
> +                 * By absolute pathname
> +                 */
> +                ret = get_errno(stat(path(p), &st));
> +                unlock_user(p, arg2, 0);
> +            } else {
> +                if (dirfd == AT_FDCWD) {
> +                    /*
> +                     * By pathname relative to the current working directory
> +                     */
> +                    ret = get_errno(stat(path(p), &st));
> +                    unlock_user(p, arg2, 0);
> +                } else {
> +                    /*
> +                     * By pathname 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);
> +                }
> +            }

I think you can put the unlock_user() only once here.

> +
> +            if (!is_error(ret)) {
> +                if (!lock_user_struct(VERIFY_WRITE, target_stx, arg5, 0)) {
> +                    return -TARGET_EFAULT;
> +                }
> +                memset(target_stx, 0, sizeof(*target_stx));
> +                __put_user(major(st.st_dev), &target_stx->stx_dev_major);
> +                __put_user(minor(st.st_dev), &target_stx->stx_dev_minor);
> +                __put_user(st.st_ino, &target_stx->stx_ino);
> +                __put_user(st.st_mode, &target_stx->stx_mode);
> +                __put_user(st.st_uid, &target_stx->stx_uid);
> +                __put_user(st.st_gid, &target_stx->stx_gid);
> +                __put_user(st.st_nlink, &target_stx->stx_nlink);
> +                __put_user(major(st.st_rdev), &target_stx->stx_rdev_major);
> +                __put_user(minor(st.st_rdev), &target_stx->stx_rdev_minor);
> +                __put_user(st.st_size, &target_stx->stx_size);
> +                __put_user(st.st_blksize, &target_stx->stx_blksize);
> +                __put_user(st.st_blocks, &target_stx->stx_blocks);
> +                __put_user(st.st_atime, &target_stx->stx_atime.tv_sec);
> +                __put_user(st.st_mtime, &target_stx->stx_mtime.tv_sec);
> +                __put_user(st.st_ctime, &target_stx->stx_ctime.tv_sec);
> +                unlock_user_struct(target_stx, arg5, 1);
> +            }
> +        }
> +        break;

should be "return ret;" now.

> +#endif
>  #ifdef TARGET_NR_lchown
>      case TARGET_NR_lchown:
>          if (!(p = lock_user_string(arg1)))
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 18d434d..b6c5c4f 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -2488,4 +2488,42 @@ struct target_user_cap_data {
>  /* Return size of the log buffer */
>  #define TARGET_SYSLOG_ACTION_SIZE_BUFFER   10
>  
> +struct target_statx_timestamp {
> +   int64_t tv_sec;
> +   uint32_t tv_nsec;
> +   int32_t __reserved;
> +};

You sould use abi_ullong, abi_uint and abi_int to have the good
alignment in the structure.

> +
> +struct target_statx {
> +   /* 0x00 */
> +   uint32_t stx_mask;       /* What results were written [uncond] */
> +   uint32_t stx_blksize;    /* Preferred general I/O size [uncond] */
> +   uint64_t stx_attributes; /* Flags conveying information about the file */
> +   /* 0x10 */
> +   uint32_t stx_nlink;      /* Number of hard links */
> +   uint32_t stx_uid;        /* User ID of owner */
> +   uint32_t stx_gid;        /* Group ID of owner */
> +   uint16_t stx_mode;       /* File mode */
> +   uint16_t __spare0[1];
> +   /* 0x20 */
> +   uint64_t stx_ino;        /* Inode number */
> +   uint64_t stx_size;       /* File size */
> +   uint64_t stx_blocks;     /* Number of 512-byte blocks allocated */
> +   uint64_t stx_attributes_mask; /* Mask to show what's supported in
> +                                    stx_attributes */
> +   /* 0x40 */
> +   struct target_statx_timestamp  stx_atime;  /* Last access time */
> +   struct target_statx_timestamp  stx_btime;  /* File creation time */
> +   struct target_statx_timestamp  stx_ctime;  /* Last attribute change time 
> */
> +   struct target_statx_timestamp  stx_mtime;  /* Last data modification time 
> */
> +   /* 0x80 */
> +   uint32_t stx_rdev_major;   /* Device ID of special file [if bdev/cdev] */
> +   uint32_t stx_rdev_minor;
> +   uint32_t stx_dev_major; /* ID of device containing file [uncond] */
> +   uint32_t stx_dev_minor;
> +   /* 0x90 */
> +   uint64_t __spare2[14];  /* Spare space for future expansion */
> +   /* 0x100 */
> +};
>

Same here use abi_XXX types.

>  #endif
> 

Thanks,
Laurent