Re: [PATCH 2/5] linux-user: Add strace support for printing argument of syscalls used for extend attributes

[Laurent Vivier]

Le 02/06/2020 à 13:53, Filip Bozuta a écrit :
> From: Filip Bozuta <Filip.Bozuta@syrmia.com>
> 
> This patch implements strace argument printing functionality for following 
> syscalls:
> 
>     *getxattr, lgetxattr, fgetxattr - retrieve an extended attribute value
> 
>         ssize_t getxattr(const char *path, const char *name, void *value, 
> size_t size)
>         ssize_t lgetxattr(const char *path, const char *name, void *value, 
> size_t size)
>         ssize_t fgetxattr(int fd, const char *name, void *value, size_t size)
>         man page: https://www.man7.org/linux/man-pages/man2/getxattr.2.html
> 
>     *listxattr, llistxattr, flistxattr - list extended attribute names
> 
>         ssize_t listxattr(const char *path, char *list, size_t size)
>         ssize_t llistxattr(const char *path, char *list, size_t size)
>         ssize_t flistxattr(int fd, char *list, size_t size)
>         man page: https://www.man7.org/linux/man-pages/man2/listxattr.2.html
> 
> Implementation notes:
> 
>     All of the syscalls have strings as argument types and thus a separate
>     printing function was stated in file "strace.list" for every one of them.
>     All of these printing functions were defined in "strace.c" using existing
>     printing functions for appropriate argument types:
>        "print_strig()" - for (const char*) type
>        "print_pointer()" - for (char*) and (void *) type
>        "print_raw_param()" for (int) and (size_t) type
>     Syscalls "getxattr()" and "lgetxattr()" have the same number and type of
>     arguments and thus their print functions ("print_getxattr", 
> "print_lgetxattr")
>     share a same definition. The same statement applies to syscalls 
> "listxattr()"
>     and "llistxattr()".
> 
> Signed-off-by: Filip Bozuta <Filip.Bozuta@syrmia.com>
> ---
>  linux-user/strace.c    | 60 ++++++++++++++++++++++++++++++++++++++++++
>  linux-user/strace.list | 12 ++++-----
>  2 files changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index c578876d22..5cf419989c 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -1629,6 +1629,66 @@ print_fcntl(const struct syscallname *name,
>  #define print_fcntl64   print_fcntl
>  #endif
>  
> +#ifdef TARGET_NR_fgetxattr
> +static void
> +print_fgetxattr(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_string(arg1, 0);
> +    print_pointer(arg2, 0);
> +    print_raw_param("%u", arg3, 1);

size_t is generally "unsigned long", so you should use TARGET_FMT_lu

> +    print_syscall_epilogue(name);
> +}
> +#endif
> +
> +#ifdef TARGET_NR_flistxattr
> +static void
> +print_flistxattr(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(arg1, 0);
> +    print_raw_param("%u", arg2, 1);

TARGET_FMT_lu

> +    print_syscall_epilogue(name);
> +}
> +#endif
> +
> +#if defined(TARGET_NR_getxattr) || defined(TARGET_NR_lgetxattr)
> +static void
> +print_getxattr(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_string(arg0, 0);
> +    print_string(arg1, 0);
> +    print_pointer(arg2, 0);
> +    print_raw_param("%u", arg3, 1);

TARGET_FMT_lu

> +    print_syscall_epilogue(name);
> +}
> +#define print_lgetxattr     print_getxattr
> +#endif
> +
> +#if defined(TARGET_NR_listxattr) || defined(TARGET_NR_llistxattr)
> +static void
> +print_listxattr(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_string(arg0, 0);
> +    print_pointer(arg1, 0);
> +    print_raw_param("%u", arg2, 1);

TARGET_FMT_lu

> +    print_syscall_epilogue(name);
> +}
> +#define print_llistxattr     print_listxattr
> +#endif
> +
>  #ifdef TARGET_NR_futimesat
>  static void
>  print_futimesat(const struct syscallname *name,
> diff --git a/linux-user/strace.list b/linux-user/strace.list
> index fb9799e7e6..8d51c54bca 100644
> --- a/linux-user/strace.list
> +++ b/linux-user/strace.list
> @@ -218,13 +218,13 @@
>  { TARGET_NR_fdatasync, "fdatasync" , "%s(%d)", NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_fgetxattr
> -{ TARGET_NR_fgetxattr, "fgetxattr" , NULL, NULL, NULL },
> +{ TARGET_NR_fgetxattr, "fgetxattr" , NULL, print_fgetxattr, NULL },
>  #endif
>  #ifdef TARGET_NR_finit_module
>  { TARGET_NR_finit_module, "finit_module" , NULL, NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_flistxattr
> -{ TARGET_NR_flistxattr, "flistxattr" , NULL, NULL, NULL },
> +{ TARGET_NR_flistxattr, "flistxattr" , NULL, print_flistxattr, NULL },
>  #endif
>  #ifdef TARGET_NR_flock
>  { TARGET_NR_flock, "flock" , NULL, NULL, NULL },
> @@ -396,7 +396,7 @@
>  { TARGET_NR_getuid32, "getuid32" , NULL, NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_getxattr
> -{ TARGET_NR_getxattr, "getxattr" , NULL, NULL, NULL },
> +{ TARGET_NR_getxattr, "getxattr" , NULL, print_getxattr, NULL },
>  #endif
>  #ifdef TARGET_NR_getxgid
>  { TARGET_NR_getxgid, "getxgid" , NULL, NULL, NULL },
> @@ -480,7 +480,7 @@
>  { TARGET_NR_lchown32, "lchown32" , NULL, NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_lgetxattr
> -{ TARGET_NR_lgetxattr, "lgetxattr" , NULL, NULL, NULL },
> +{ TARGET_NR_lgetxattr, "lgetxattr" , NULL, print_lgetxattr, NULL },
>  #endif
>  #ifdef TARGET_NR_link
>  { TARGET_NR_link, "link" , NULL, print_link, NULL },
> @@ -495,10 +495,10 @@
>  { TARGET_NR_listen, "listen" , "%s(%d,%d)", NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_listxattr
> -{ TARGET_NR_listxattr, "listxattr" , NULL, NULL, NULL },
> +{ TARGET_NR_listxattr, "listxattr" , NULL, print_listxattr, NULL },
>  #endif
>  #ifdef TARGET_NR_llistxattr
> -{ TARGET_NR_llistxattr, "llistxattr" , NULL, NULL, NULL },
> +{ TARGET_NR_llistxattr, "llistxattr" , NULL, print_llistxattr, NULL },
>  #endif
>  #ifdef TARGET_NR__llseek
>  { TARGET_NR__llseek, "_llseek" , NULL, print__llseek, NULL },
> 

For the listxattr functions perhaps you can add a
print_syscall_ret_listxattr function to dump the returned list of
attributes.

It would be interesting to have also a print_syscall_ret_XXX function
for the getxattr functions to dump the returned values but it is not as
easy as for the list as value can be textual or binary data.

Thanks,
Laurent