Re: [PATCH] linux-user: Fix inotify on aarch64

[Laurent Vivier <laurent@vivier.eu>]

Le 26/01/2022 à 21:26, Paul Brook a écrit :
> The inotify implementation originally called the raw host syscalls.
> Commit 3b3f24add0 changed this to use the glibc wrappers. However ifdefs
> in syscall.c still test for presence of the raw syscalls.
> 
> This causes a problem on e.g. aarch64 hosts which never had the
> inotify_init syscall - it had been obsoleted by inotify_init1 before
> aarch64 was invented! However it does have a perfectly good glibc
> implementation of inotify_wait.
> 
> Fix this by removing all the raw __NR_inotify_* tests, and instead check
> CONFIG_INOTIFY, which already tests for the glibc functionality we use.
> 
> Also remove the now-pointless sys_inotify* wrappers.
> 
> Tested using x86-64 inotifywatch on aarch64 host, and vice-versa
> 
> Signed-off-by: Paul Brook <paul@nowt.org>
> ---
>   linux-user/fd-trans.c |  5 ++---
>   linux-user/syscall.c  | 50 +++++++++----------------------------------
>   2 files changed, 12 insertions(+), 43 deletions(-)
> 
> diff --git a/linux-user/fd-trans.c b/linux-user/fd-trans.c
> index 6941089959..30e7b49112 100644
> --- a/linux-user/fd-trans.c
> +++ b/linux-user/fd-trans.c
> @@ -1460,9 +1460,8 @@ TargetFdTrans target_eventfd_trans = {
>       .target_to_host_data = swap_data_eventfd,
>   };
>   
> -#if (defined(TARGET_NR_inotify_init) && defined(__NR_inotify_init)) || \
> -    (defined(CONFIG_INOTIFY1) && defined(TARGET_NR_inotify_init1) && \
> -     defined(__NR_inotify_init1))
> +#if defined(CONFIG_INOTIFY) && (defined(TARGET_NR_inotify_init) || \
> +        defined(TARGET_NR_inotify_init1))
>   static abi_long host_to_target_data_inotify(void *buf, size_t len)
>   {
>       struct inotify_event *ev;
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 56a3e17183..17cc38fe34 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -272,9 +272,6 @@ static type name (type1 arg1,type2 arg2,type3 arg3,type4 arg4,type5 arg5,	\
>   #if defined(__NR_futex_time64)
>   # define __NR_sys_futex_time64 __NR_futex_time64
>   #endif
> -#define __NR_sys_inotify_init __NR_inotify_init
> -#define __NR_sys_inotify_add_watch __NR_inotify_add_watch
> -#define __NR_sys_inotify_rm_watch __NR_inotify_rm_watch
>   #define __NR_sys_statx __NR_statx
>   
>   #if defined(__alpha__) || defined(__x86_64__) || defined(__s390x__)
> @@ -447,33 +444,6 @@ static int sys_renameat2(int oldfd, const char *old,
>   
>   #ifdef CONFIG_INOTIFY
>   #include <sys/inotify.h>
> -
> -#if defined(TARGET_NR_inotify_init) && defined(__NR_inotify_init)
> -static int sys_inotify_init(void)
> -{
> -  return (inotify_init());
> -}
> -#endif
> -#if defined(TARGET_NR_inotify_add_watch) && defined(__NR_inotify_add_watch)
> -static int sys_inotify_add_watch(int fd,const char *pathname, int32_t mask)
> -{
> -  return (inotify_add_watch(fd, pathname, mask));
> -}
> -#endif
> -#if defined(TARGET_NR_inotify_rm_watch) && defined(__NR_inotify_rm_watch)
> -static int sys_inotify_rm_watch(int fd, int32_t wd)
> -{
> -  return (inotify_rm_watch(fd, wd));
> -}
> -#endif
> -#ifdef CONFIG_INOTIFY1
> -#if defined(TARGET_NR_inotify_init1) && defined(__NR_inotify_init1)
> -static int sys_inotify_init1(int flags)
> -{
> -  return (inotify_init1(flags));
> -}
> -#endif
> -#endif
>   #else
>   /* Userspace can usually survive runtime without inotify */
>   #undef TARGET_NR_inotify_init
> @@ -12263,35 +12233,35 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>       case TARGET_NR_futex_time64:
>           return do_futex_time64(cpu, arg1, arg2, arg3, arg4, arg5, arg6);
>   #endif
> -#if defined(TARGET_NR_inotify_init) && defined(__NR_inotify_init)
> +#ifdef CONFIG_INOTIFY
> +#if defined(TARGET_NR_inotify_init)
>       case TARGET_NR_inotify_init:
> -        ret = get_errno(sys_inotify_init());
> +        ret = get_errno(inotify_init());
>           if (ret >= 0) {
>               fd_trans_register(ret, &target_inotify_trans);
>           }
>           return ret;
>   #endif
> -#ifdef CONFIG_INOTIFY1
> -#if defined(TARGET_NR_inotify_init1) && defined(__NR_inotify_init1)
> +#if defined(TARGET_NR_inotify_init1) && defined(CONFIG_INOTIFY1)
>       case TARGET_NR_inotify_init1:
> -        ret = get_errno(sys_inotify_init1(target_to_host_bitmask(arg1,
> +        ret = get_errno(inotify_init1(target_to_host_bitmask(arg1,
>                                             fcntl_flags_tbl)));
>           if (ret >= 0) {
>               fd_trans_register(ret, &target_inotify_trans);
>           }
>           return ret;
>   #endif
> -#endif
> -#if defined(TARGET_NR_inotify_add_watch) && defined(__NR_inotify_add_watch)
> +#if defined(TARGET_NR_inotify_add_watch)
>       case TARGET_NR_inotify_add_watch:
>           p = lock_user_string(arg2);
> -        ret = get_errno(sys_inotify_add_watch(arg1, path(p), arg3));
> +        ret = get_errno(inotify_add_watch(arg1, path(p), arg3));
>           unlock_user(p, arg2, 0);
>           return ret;
>   #endif
> -#if defined(TARGET_NR_inotify_rm_watch) && defined(__NR_inotify_rm_watch)
> +#if defined(TARGET_NR_inotify_rm_watch)
>       case TARGET_NR_inotify_rm_watch:
> -        return get_errno(sys_inotify_rm_watch(arg1, arg2));
> +        return get_errno(inotify_rm_watch(arg1, arg2));
> +#endif
>   #endif
>   
>   #if defined(TARGET_NR_mq_open) && defined(__NR_mq_open)

Applied to my linux-user-for-7.0 branch.

Thanks,
Laurent