Re: [Qemu-devel] [PATCH v5 3/8] linux-user: Add support for sysfs() syscall

[Laurent Vivier]

Le 14/09/2016 à 22:19, Aleksandar Markovic a écrit :
> From: Aleksandar Markovic <address@hidden>
> 
> This patch implements Qemu user mode sysfs() syscall support.
> 
> Syscall sysfs() involves returning information about the filesystem types
> currently present in the kernel, and can operate in three distinct flavors,
> depending on its first argument.
> 
> The implementation is based on invocation of host's sysfs(), and
> its key part is in the correspondent case segment of the main switch
> statement of the function do_syscall(), in file linux-user/syscalls.c.
> All necessary conversions of data structures from target to host and from
> host to target are covered. Based on the value of the first argument, three
> cases are distinguished, and such conversions are implemented separately
> for each case. Relevant support for "-strace" option is included in files
> linux-user/strace.c and linux-user/strace.list.
> 
> This patch also fixes failures of LTP tests sysfs01, sysfs02, sysfs03,
> sysfs04, sysfs05, and sysfs06, if executed in Qemu user mode.
> 
> Signed-off-by: Aleksandar Markovic <address@hidden>
> ---
>  linux-user/strace.c    | 25 +++++++++++++++++++++++++
>  linux-user/strace.list |  2 +-
>  linux-user/syscall.c   | 42 +++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 67 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index 4524c70..61911e7 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -2151,6 +2151,31 @@ print_kill(const struct syscallname *name,
>  }
>  #endif
>  
> +#if defined(TARGET_NR_sysfs)
> +static void
> +print_sysfs(const struct syscallname *name,
> +    abi_long arg0, abi_long arg1, abi_long arg2,
> +    abi_long arg3, abi_long arg4, abi_long arg5)
> +{
> +    print_syscall_prologue(name);

perhaps you can move here:

        print_raw_param("%d", arg0, 0);

> +    switch (arg0) {
> +    case 1:
> +        print_raw_param("%d", arg0, 1);

should be ", 0);"

> +        print_string(arg1, 1);
> +        break;
> +    case 2:
> +        print_raw_param("%d", arg0, 0);
> +        print_raw_param("%u", arg1, 0);
> +        print_pointer(arg2, 1);
> +        break;
> +    default:
> +        print_raw_param("%d", arg0, 1);
> +        break;

in syscall.c, you use 3 for this, and default is EINVAL.

> +    }
> +    print_syscall_epilogue(name);
> +}
> +#endif
> +
>  /*
>   * An array of all of the syscalls we know about
>   */
> diff --git a/linux-user/strace.list b/linux-user/strace.list
> index 00b2e9b..0bf1bea 100644
> --- a/linux-user/strace.list
> +++ b/linux-user/strace.list
> @@ -1371,7 +1371,7 @@
>  { TARGET_NR_sys_epoll_wait, "sys_epoll_wait" , NULL, NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_sysfs
> -{ TARGET_NR_sysfs, "sysfs" , NULL, NULL, NULL },
> +{ TARGET_NR_sysfs, "sysfs" , NULL, print_sysfs, NULL },
>  #endif
>  #ifdef TARGET_NR_sysinfo
>  { TARGET_NR_sysinfo, "sysinfo" , NULL, NULL, NULL },
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index eab9207..3436ee6 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -9549,7 +9549,47 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>  #endif
>  #ifdef TARGET_NR_sysfs
>      case TARGET_NR_sysfs:
> -        goto unimplemented;
> +        switch (arg1) {
> +        case 1:
> +            {
> +                if (arg2 != 0) {

"arg2 == NULL" will be managed by lock_user_string()

> +                    p = lock_user_string(arg2);
> +                    if (!p) {
> +                        goto efault;
> +                    }
> +                    ret = get_errno(syscall(__NR_sysfs, arg1, p));
> +                    unlock_user(p, arg2, 0);
> +                } else {
> +                    ret = get_errno(syscall(__NR_sysfs, arg1, NULL));

why?

> +                }
> +            }
> +            break;
> +        case 2:
> +            {
> +                if (arg3 != 0) {
> +                    char buf[PATH_MAX];

PATH_MAX (4096) should be a little bit excessive for a filesystem name.
they have rarely more than 16 characters.

> +                    int len;
> +                    memset(buf, 0, PATH_MAX);
> +                    ret = get_errno(syscall(__NR_sysfs, arg1, arg2, buf));
> +                    len = PATH_MAX;
> +                    if (len > strlen(buf)) {
> +                        len = strlen(buf);
> +                    }
> +                    if (copy_to_user(arg3, buf, len) != 0) {
> +                        goto efault;
> +                    }
> +                } else {
> +                    ret = get_errno(syscall(__NR_sysfs, arg1, arg2, NULL));

Why?
just let the copy_to_user() trigger the "goto efault" in the case of
arg3 == 0.

> +                }
> +            }
> +            break;
> +        case 3:
> +            ret = get_errno(syscall(__NR_sysfs, arg1));
> +            break;
> +        default:
> +            ret = -EINVAL;
> +        }
> +        break;
>  #endif
>      case TARGET_NR_personality:
>          ret = get_errno(personality(arg1));
>