Re: [Qemu-devel] [PATCH 1/4] linux-user: Add support for sysfs() syscall

[Aleksandar Markovic]

Hello, Peter

I truly appreciate your review. The reason for this patch is LTP only, as far 
as I know. I will address all your other concerns in the next version of the 
patch.

Thanks,
Aleksandar

________________________________________
From: Peter Maydell address@hidden
Sent: Tuesday, October 04, 2016 1:35 PM
To: Aleksandar Markovic
Cc: QEMU Developers; Riku Voipio; Laurent Vivier; Aurelien Jarno; Yongbok Kim; 
Leon Alrae; Petar Jovanovic; Miodrag Dinic; Aleksandar Rikalo; Aleksandar 
Markovic
Subject: Re: [PATCH 1/4] linux-user: Add support for sysfs() syscall

On 4 October 2016 at 11:05, Aleksandar Markovic
<address@hidden> wrote:
> 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.

Are you implementing this because you have an actual need for
it, or just for LTP coverage purposes? (The manpage says
it's obsolete. Either answer is fine, I'm just curious.)

> Its specific is that its declaration is threefold:
>
>    int sysfs(int option, const char *fsname);
>    int sysfs(int option, unsigned int fs_index, char *buf);
>    int sysfs(int option);
>
> Its implementation in Linux kernel is at fs/filesystems.c, line 184.
>
> The implementation in Qemu user mode is based on invocation of host's
> sysfs(), and its key part is in the correspondent case segment of the

"corresponding"

> 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.
>
> Buffer needed for the second cases has size defined with preprocessor
> constant FST_NAME_MAX, that is set to 32. .This is good enough for

Stray '.'

> virtually all practical purposes. In that bufer, a name of the type

"buffer"

> of the filesystem is stored. It is the same string as the second
> column of the content of /proc/filesyste, and it is rarely longer

"filesystems"

> than 10 bytes.
>
> Relevant support for "-strace" option is also included (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    | 27 +++++++++++++++++++++++++++
>  linux-user/strace.list |  2 +-
>  linux-user/syscall.c   | 40 +++++++++++++++++++++++++++++++++++++---
>  3 files changed, 65 insertions(+), 4 deletions(-)
>
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index 1e51360..d9d5858 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -2220,6 +2220,33 @@ print_kill(const struct syscallname *name,
>  }
>  #endif
>
> +#ifdef 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);
> +    /* arg0 is normally 1, 2, or 3 */
> +    switch (arg0) {
> +    case 1:
> +        print_raw_param("%d", arg0, 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;
> +    /* if arg0 is 3, desired output is the same as default */
> +    default:
> +        print_raw_param("%d", arg0, 1);
> +        break;
> +    }
> +    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 608f7e0..93bc9d0 100644
> --- a/linux-user/strace.list
> +++ b/linux-user/strace.list
> @@ -1476,7 +1476,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 0815f30..fe8042c 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7379,7 +7379,7 @@ static int do_openat(void *cpu_env, int dirfd, const 
> char *pathname, int flags,
>
>      if (fake_open->filename) {
>          const char *tmpdir;
> -        char filename[PATH_MAX];
> +        char filename[128];
>          int fd, r;
>
>          /* create temporary file to map stat to */

This looks like an accidental change?

> @@ -9560,9 +9560,43 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>      case TARGET_NR_bdflush:
>          goto unimplemented;
>  #endif
> -#ifdef TARGET_NR_sysfs
> +#if defined(TARGET_NR_sysfs)
> +#define FST_NAME_MAX 32
>      case TARGET_NR_sysfs:
> -        goto unimplemented;
> +        switch (arg1) {
> +        case 1:
> +            {
> +                p = lock_user_string(arg2);
> +                if (!p) {
> +                    goto efault;
> +                }
> +                ret = get_errno(syscall(__NR_sysfs, arg1, p));
> +                unlock_user(p, arg2, 0);
> +            }
> +            break;
> +        case 2:
> +            {
> +                char buf[FST_NAME_MAX];
> +                memset(buf, 0, FST_NAME_MAX);

It would be better to just get the host sysfs() to copy directly
into the guest memory: it saves allocating a temp buffer and it
means that if the name is unexpectedly long it overruns the
guest's buffer rather than something on the QEMU stack.

> +                ret = get_errno(syscall(__NR_sysfs, arg1, arg2, buf));
> +                if (!is_error(ret)) {
> +                    int len = FST_NAME_MAX - 1;
> +                    if (len > strlen(buf)) {
> +                        len = strlen(buf);
> +                    }
> +                    if (copy_to_user(arg3, buf, len + 1) != 0) {
> +                        goto efault;
> +                    }
> +                }
> +            }
> +            break;
> +        case 3:
> +            ret = get_errno(syscall(__NR_sysfs, arg1));
> +            break;
> +        default:
> +            ret = -EINVAL;

This should be a target errno, not a host one.

> +        }
> +        break;
>  #endif
>      case TARGET_NR_personality:
>          ret = get_errno(personality(arg1));
> --
> 2.9.3
>

thanks
-- PMM