linux-user/syscall.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)
There's a slight difference in fcntl locking handling on arm and others:
arm has 2 variants of struct flock argument handling, with and without eabi.
For this reason, we currently take address of the conversion function into
a variable, and for arm without eabi case, assign a different conversion
function to this pointer. All functions are declared as inline, - and for
an inline function, it is not clear how one can have an address of it at
all.
Instead of using a function pointer, use a macro, defined differently for
arm (with arithmetic if based on eabi presence) and for all other targets
(using regular argument conversion function directly).
While at it, replace tabs with spaces in nearby code.
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
linux-user/syscall.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index d418e2864a..786623d395 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6880,9 +6880,6 @@ static inline abi_long copy_to_user_flock(abi_ulong target_flock_addr,
return 0;
}
-typedef abi_long from_flock64_fn(struct flock *fl, abi_ulong target_addr);
-typedef abi_long to_flock64_fn(abi_ulong target_addr, const struct flock *fl);
-
#if defined(TARGET_ARM) && TARGET_ABI_BITS == 32
struct target_oabi_flock64 {
abi_short l_type;
@@ -12402,14 +12399,19 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
{
int cmd;
struct flock fl;
- from_flock64_fn *copyfrom = copy_from_user_flock64;
- to_flock64_fn *copyto = copy_to_user_flock64;
#ifdef TARGET_ARM
- if (!cpu_env->eabi) {
- copyfrom = copy_from_user_oabi_flock64;
- copyto = copy_to_user_oabi_flock64;
- }
+# define copyfrom(fl, arg) \
+ (cpu_env->eabi ? \
+ copy_from_user_flock64(fl, arg) : \
+ copy_from_user_oabi_flock64(fl, arg))
+# define copyto(arg, fl) \
+ (cpu_env->eabi ? \
+ copy_to_user_flock64(arg, fl) : \
+ copy_to_user_oabi_flock64(arg, fl))
+#else
+# define copyfrom copy_from_user_flock64
+# define copyto copy_to_user_flock64
#endif
cmd = target_to_host_fcntl_cmd(arg2);
@@ -12427,7 +12429,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
if (ret == 0) {
ret = copyto(arg3, &fl);
}
- break;
+ break;
case TARGET_F_SETLK64:
case TARGET_F_SETLKW64:
@@ -12436,10 +12438,12 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
break;
}
ret = get_errno(safe_fcntl(arg1, cmd, &fl));
- break;
+ break;
default:
ret = do_fcntl(arg1, arg2, arg3);
break;
+#undef copyfrom
+#undef copyto
}
return ret;
}
--
2.39.2On Sat, 7 Sept 2024 at 08:00, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> There's a slight difference in fcntl locking handling on arm and others:
> arm has 2 variants of struct flock argument handling, with and without eabi.
> For this reason, we currently take address of the conversion function into
> a variable, and for arm without eabi case, assign a different conversion
> function to this pointer. All functions are declared as inline, - and for
> an inline function, it is not clear how one can have an address of it at
> all.
There's no problem with this. "inline" is just a hint to the compiler
to say "this is probably better inlined where you can". For the case
where we take the address of a function, that's a case where it can't
be inlined. But other places where the function is called directly can be
inlined without problems. For functions where are only ever used via
function pointers, the "inline" annotation is a bit pointless and we could
remove it; arguably for other cases we could also remove it and let
the compiler make the call about inline vs not. As it happens the kernel
is not inclined to inline these functions even if we ask for it, probably
because they're too big for that to be a good idea.
> Instead of using a function pointer, use a macro, defined differently for
> arm (with arithmetic if based on eabi presence) and for all other targets
> (using regular argument conversion function directly).
>
> While at it, replace tabs with spaces in nearby code.
>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
> linux-user/syscall.c | 26 +++++++++++++++-----------
> 1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index d418e2864a..786623d395 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -6880,9 +6880,6 @@ static inline abi_long copy_to_user_flock(abi_ulong target_flock_addr,
> return 0;
> }
>
> -typedef abi_long from_flock64_fn(struct flock *fl, abi_ulong target_addr);
> -typedef abi_long to_flock64_fn(abi_ulong target_addr, const struct flock *fl);
> -
> #if defined(TARGET_ARM) && TARGET_ABI_BITS == 32
> struct target_oabi_flock64 {
> abi_short l_type;
> @@ -12402,14 +12399,19 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
> {
> int cmd;
> struct flock fl;
> - from_flock64_fn *copyfrom = copy_from_user_flock64;
> - to_flock64_fn *copyto = copy_to_user_flock64;
>
> #ifdef TARGET_ARM
> - if (!cpu_env->eabi) {
> - copyfrom = copy_from_user_oabi_flock64;
> - copyto = copy_to_user_oabi_flock64;
> - }
> +# define copyfrom(fl, arg) \
> + (cpu_env->eabi ? \
> + copy_from_user_flock64(fl, arg) : \
> + copy_from_user_oabi_flock64(fl, arg))
> +# define copyto(arg, fl) \
> + (cpu_env->eabi ? \
> + copy_to_user_flock64(arg, fl) : \
> + copy_to_user_oabi_flock64(arg, fl))
> +#else
> +# define copyfrom copy_from_user_flock64
> +# define copyto copy_to_user_flock64
> #endif
This seems to me to be less readable than the current code.
Unless there's a specific performance issue with what we have
(which seems unlikely for flock), I would prefer not to
macroify things like this.
Looking at the generated code, for the non-arm cases the
compiler is smart enough to turn the call into a direct
call to copy_from_user_flock64 already.
> cmd = target_to_host_fcntl_cmd(arg2);
> @@ -12427,7 +12429,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
> if (ret == 0) {
> ret = copyto(arg3, &fl);
> }
> - break;
> + break;
>
> case TARGET_F_SETLK64:
> case TARGET_F_SETLKW64:
> @@ -12436,10 +12438,12 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
> break;
> }
> ret = get_errno(safe_fcntl(arg1, cmd, &fl));
> - break;
> + break;
> default:
> ret = do_fcntl(arg1, arg2, arg3);
> break;
> +#undef copyfrom
> +#undef copyto
> }
> return ret;
> }
thanks
-- PMM
© 2016 - 2024 Red Hat, Inc.