Re: [Qemu-devel] [PATCH v2 12/16] linux-user: support {name_to, open_by}_handle_at syscalls

[Riku Voipio]

Hi,

Comments inline:

On Sat, Jun 21, 2014 at 11:53:07PM +0100, Paul Burton wrote:
> Implement support for the name_to_handle_at and open_by_handle_at
> syscalls, allowing their use by the target program.
> 
> Signed-off-by: Paul Burton <address@hidden>
> ---
> Changes in v2:
>   - None.
> ---
>  linux-user/strace.c    | 30 ++++++++++++++++++++++++++++++
>  linux-user/strace.list |  6 ++++++
>  linux-user/syscall.c   | 50 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 86 insertions(+)
> 
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index ea6c1d2..c20ddf1 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -1552,6 +1552,36 @@ print_kill(const struct syscallname *name,
>  }
>  #endif
>  
> +#ifdef TARGET_NR_name_to_handle_at
> +static void
> +print_name_to_handle_at(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);
> +    print_at_dirfd(arg0, 0);
> +    print_string(arg1, 0);
> +    print_pointer(arg2, 0);
> +    print_pointer(arg3, 0);
> +    print_raw_param("0x%x", arg4, 1);
> +    print_syscall_epilogue(name);
> +}
> +#endif
> +
> +#ifdef TARGET_NR_open_by_handle_at
> +static void
> +print_open_by_handle_at(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);
> +    print_raw_param("%d", arg0, 0);
> +    print_pointer(arg2, 0);
> +    print_open_flags(arg3, 1);
> +    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 8de972a..147f579 100644
> --- a/linux-user/strace.list
> +++ b/linux-user/strace.list
> @@ -582,6 +582,9 @@
>  #ifdef TARGET_NR_munmap
>  { TARGET_NR_munmap, "munmap" , NULL, print_munmap, NULL },
>  #endif
> +#ifdef TARGET_NR_name_to_handle_at
> +{ TARGET_NR_name_to_handle_at, "name_to_handle_at" , NULL, 
> print_name_to_handle_at, NULL },
> +#endif
>  #ifdef TARGET_NR_nanosleep
>  { TARGET_NR_nanosleep, "nanosleep" , NULL, NULL, NULL },
>  #endif
> @@ -624,6 +627,9 @@
>  #ifdef TARGET_NR_openat
>  { TARGET_NR_openat, "openat" , NULL, print_openat, NULL },
>  #endif
> +#ifdef TARGET_NR_open_by_handle_at
> +{ TARGET_NR_open_by_handle_at, "open_by_handle_at" , NULL, 
> print_open_by_handle_at, NULL },
> +#endif
>  #ifdef TARGET_NR_osf_adjtime
>  { TARGET_NR_osf_adjtime, "osf_adjtime" , NULL, NULL, NULL },
>  #endif
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index fb36f46..0495781 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -5347,6 +5347,56 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>          unlock_user(p, arg2, 0);
>          break;
>  #endif
> +#ifdef TARGET_NR_name_to_handle_at

+#if defined(TARGET_NR_name_to_handle_at) && defined(__NR_name_to_handle_at)

This is something that all other syscall definitions need - else qemu won't 
compile
on old distributions anymore.

> +    case TARGET_NR_name_to_handle_at:
> +        {
> +            struct file_handle *fh;
> +            uint32_t sz;
> +            int mount_id;
> +
> +            if (!(p = lock_user_string(arg2)))
> +                goto efault;
> +
> +            if (get_user_u32(sz, arg3)) {
> +                unlock_user(p, arg2, 0);
> +                goto efault;
> +            }
> +
> +            if (!(fh = lock_user(VERIFY_WRITE, arg3, sizeof(*fh) + sz, 1))) {
> +                unlock_user(p, arg2, 0);
> +                goto efault;
> +            }
> +
> +            ret = get_errno(name_to_handle_at(arg1, path(p), fh,
> +                                              &mount_id, arg5));
> +
> +            unlock_user(p, arg2, 0);
> +            unlock_user(p, arg3, sizeof(*fh) + sz);

I think:
+            unlock_user(fh, arg3, sizeof(*fh) + sz);

> +
> +            if (put_user_s32(mount_id, arg4))
> +                goto efault;
> +        }
> +        break;
> +#endif
> +#ifdef TARGET_NR_open_by_handle_at
> +    case TARGET_NR_open_by_handle_at:
> +        {
> +            struct file_handle *fh;
> +            uint32_t sz;
> +
> +            if (get_user_u32(sz, arg2))
> +                goto efault;
> +
> +            if (!(fh = lock_user(VERIFY_WRITE, arg2, sizeof(*fh) + sz, 1)))
> +                goto efault;
> +
> +            ret = get_errno(open_by_handle_at(arg1, fh,
> +                    target_to_host_bitmask(arg3, fcntl_flags_tbl)));
> +
> +            unlock_user(p, arg2, sizeof(*fh) + sz);

And here too:

+            unlock_user(fh, arg2, sizeof(*fh) + sz);

> +        }
> +        break;
> +#endif
>      case TARGET_NR_close:
>          ret = get_errno(close(arg1));
>          break;
> -- 
> 2.0.0