Re: [PATCH 2/3] linux-user: Add strace support for printing arguments of syscalls used to lock and unlock memory

[Laurent Vivier]

Le 26/06/2020 à 23:39, Filip Bozuta a écrit :
> This patch implements strace argument printing functionality for following 
> syscalls:
> 
>     * mlock, munlock, mlockall, munlockall - lock and unlock memory
> 
>        int mlock(const void *addr, size_t len)
>        int munlock(const void *addr, size_t len)
>        int mlockall(int flags)
>        int munlockall(void)
>        man page: https://man7.org/linux/man-pages/man2/mlock.2.html
> 
> Implementation notes:
> 
>     Syscall mlockall() takes an argument that is composed of predefined values
>     which represent flags that determine the type of locking operation that is
>     to be performed. For that reason, a printing function "print_mlockall" was
>     stated in file "strace.list". This printing function uses an already 
> existing
>     function "print_flags()" to print the "flags" argument.  These flags are 
> stated
>     inside an array "mlockall_flags" that contains values of type "struct 
> flags".
>     These values are instantiated using an existing macro "FLAG_GENERIC()".
>     The other syscalls have only primitive argument types, so the
>     rest of the implementation was handled by stating an appropriate
>     printing format in file "strace.list". Syscall mlock2() is not 
> implemented in
>     "syscall.c" and thus it's argument printing is not implemented in this 
> patch.
> 
> Signed-off-by: Filip Bozuta <Filip.Bozuta@syrmia.com>
> ---
>  linux-user/strace.c    | 21 +++++++++++++++++++++
>  linux-user/strace.list |  8 ++++----
>  2 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index dccfbc46e9..1fc4404310 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -1161,6 +1161,15 @@ UNUSED static struct flags falloc_flags[] = {
>  #endif
>  };
>  
> +UNUSED static struct flags mlockall_flags[] = {
> +    FLAG_GENERIC(MCL_CURRENT),

I think you should use FLAG_TARGET() rather than FLAG_GENERIC().
perhaps we should also rename them from TARGET_MLOCKALL_MCL_XXX to
TARGET_MCL_XXX?

> +    FLAG_GENERIC(MCL_FUTURE),
> +#ifdef MCL_ONFAULT
> +    FLAG_GENERIC(MCL_ONFAULT),
> +#endif

perhaps it is worth to update syscall.c target_to_host_mlockall_arg()
with this new flag?

> +    FLAG_END,
> +};
> +
>  /*
>   * print_xxx utility functions.  These are used to print syscall
>   * parameters in certain format.  All of these have parameter
> @@ -1939,6 +1948,18 @@ print_truncate(const struct syscallname *name,
>  #define print_truncate64     print_truncate
>  #endif
>  
> +#ifdef TARGET_NR_mlockall
> +static void
> +print_mlockall(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_flags(mlockall_flags, arg0, 1);
> +    print_syscall_epilogue(name);
> +}
> +#endif
> +
>  #if defined(TARGET_NR_socket)
>  static void
>  print_socket(const struct syscallname *name,
> diff --git a/linux-user/strace.list b/linux-user/strace.list
> index 3b77b22daf..822b6be49c 100644
> --- a/linux-user/strace.list
> +++ b/linux-user/strace.list
> @@ -567,13 +567,13 @@
>  { TARGET_NR_mknodat, "mknodat" , NULL, print_mknodat, NULL },
>  #endif
>  #ifdef TARGET_NR_mlock
> -{ TARGET_NR_mlock, "mlock" , NULL, NULL, NULL },
> +{ TARGET_NR_mlock, "mlock" , "%s(%p," TARGET_FMT_lu ")", NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_mlock2
>  { TARGET_NR_mlock2, "mlock2" , NULL, NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_mlockall
> -{ TARGET_NR_mlockall, "mlockall" , NULL, NULL, NULL },
> +{ TARGET_NR_mlockall, "mlockall" , NULL, print_mlockall, NULL },
>  #endif
>  #ifdef TARGET_NR_mmap
>  { TARGET_NR_mmap, "mmap" , NULL, print_mmap, print_syscall_ret_addr },
> @@ -636,10 +636,10 @@
>  { TARGET_NR_multiplexer, "multiplexer" , NULL, NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_munlock
> -{ TARGET_NR_munlock, "munlock" , NULL, NULL, NULL },
> +{ TARGET_NR_munlock, "munlock" , "%s(%p," TARGET_FMT_lu ")", NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_munlockall
> -{ TARGET_NR_munlockall, "munlockall" , NULL, NULL, NULL },
> +{ TARGET_NR_munlockall, "munlockall" , "%s()", NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_munmap
>  { TARGET_NR_munmap, "munmap" , NULL, print_munmap, NULL },
> 

Thanks,
Laurent