[PATCH RFC 3/3] linux-user/syscall.c: consolidate statfs calls further

Michael Tokarev posted 3 patches 1 month ago
Maintainers: Laurent Vivier <laurent@vivier.eu>
[PATCH RFC 3/3] linux-user/syscall.c: consolidate statfs calls further
Posted by Michael Tokarev 1 month ago
Since statfs & statfs64 implementations are exactly the same,
differs only in "64" suffix, merge them into one using a common
macro.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 linux-user/syscall.c | 110 ++++++++++++++++---------------------------
 1 file changed, 40 insertions(+), 70 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1f84c296d5..1b888bccfc 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -10972,80 +10972,50 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
         return ret;
     case TARGET_NR_setpriority:
         return get_errno(setpriority(arg1, arg2, arg3));
-#ifdef TARGET_NR_statfs
-    case TARGET_NR_statfs:
-    case TARGET_NR_fstatfs:
-        {
-            struct statf stfs;
-            struct target_statfs *target_stfs;
-            if (num == TARGET_NR_statfs) {
-                if (!(p = lock_user_string(arg1))) {
-                    return -TARGET_EFAULT;
-                }
-                ret = get_errno(statfs(path(p), &stfs));
-                unlock_user(p, arg1, 0);
-            } else /* if (num == TARGET_NR_fstatfs) */ {
-                ret = get_errno(fstatfs(arg1, &stfs));
-            }
-            if (is_error(ret)) {
-                return ret;
-            }
-            if (!lock_user_struct(VERIFY_WRITE, target_stfs, arg2, 0))
-                return -TARGET_EFAULT;
-            __put_user(stfs.f_type, &target_stfs->f_type);
-            __put_user(stfs.f_bsize, &target_stfs->f_bsize);
-            __put_user(stfs.f_blocks, &target_stfs->f_blocks);
-            __put_user(stfs.f_bfree, &target_stfs->f_bfree);
-            __put_user(stfs.f_bavail, &target_stfs->f_bavail);
-            __put_user(stfs.f_files, &target_stfs->f_files);
-            __put_user(stfs.f_ffree, &target_stfs->f_ffree);
-            __put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
-            __put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
-            __put_user(stfs.f_namelen, &target_stfs->f_namelen);
-            __put_user(stfs.f_frsize, &target_stfs->f_frsize);
-            __put_user(stfs.f_flags, &target_stfs->f_flags);
-            memset(target_stfs->f_spare, 0, sizeof(target_stfs->f_spare));
-            unlock_user_struct(target_stfs, arg2, 1);
-            return ret;
+
+#define statfs_fstatfs_impl(variant) /* variant is statfs or statfs64 */ \
+    case TARGET_NR_##variant: \
+    case TARGET_NR_f##variant: \
+        { \
+            struct statfs stfs; \
+            struct target_##variant *target_stfs; \
+            if (num == TARGET_NR_##variant) { \
+                if (!(p = lock_user_string(arg1))) { \
+                    return -TARGET_EFAULT; \
+                } \
+                ret = get_errno(statfs(path(p), &stfs)); \
+                unlock_user(p, arg1, 0); \
+            } else /* if (num == TARGET_NR_f##variant) */ { \
+                ret = get_errno(fstatfs(arg1, &stfs)); \
+            } \
+            if (is_error(ret)) { \
+                return ret; \
+            } \
+            if (!lock_user_struct(VERIFY_WRITE, target_stfs, arg2, 0)) \
+                return -TARGET_EFAULT; \
+            __put_user(stfs.f_type, &target_stfs->f_type); \
+            __put_user(stfs.f_bsize, &target_stfs->f_bsize); \
+            __put_user(stfs.f_blocks, &target_stfs->f_blocks); \
+            __put_user(stfs.f_bfree, &target_stfs->f_bfree); \
+            __put_user(stfs.f_bavail, &target_stfs->f_bavail); \
+            __put_user(stfs.f_files, &target_stfs->f_files); \
+            __put_user(stfs.f_ffree, &target_stfs->f_ffree); \
+            __put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]); \
+            __put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]); \
+            __put_user(stfs.f_namelen, &target_stfs->f_namelen); \
+            __put_user(stfs.f_frsize, &target_stfs->f_frsize); \
+            __put_user(stfs.f_flags, &target_stfs->f_flags); \
+            memset(target_stfs->f_spare, 0, sizeof(target_stfs->f_spare)); \
+            unlock_user_struct(target_stfs, arg2, 1); \
+            return ret; \
         }
+#ifdef TARGET_NR_statfs
+    statfs_fstatfs_impl(statfs);
 #endif
 #ifdef TARGET_NR_statfs64
-    case TARGET_NR_statfs64:
-    case TARGET_NR_fstatfs64:
-        {
-            struct statfs stfs;
-            struct target_statfs64 *target_stfs;
-            if (num == TARGET_NR_statfs64) {
-                if (!(p = lock_user_string(arg1))) {
-                    return -TARGET_EFAULT;
-                }
-                ret = get_errno(statfs(path(p), &stfs));
-                unlock_user(p, arg1, 0);
-                } else /* if (num == TARGET_NR_fstatfs64) */ {
-                ret = get_errno(fstatfs(arg1, &stfs));
-            }
-            if (is_error(ret)) {
-                return ret;
-            }
-            if (!lock_user_struct(VERIFY_WRITE, target_stfs, arg3, 0))
-                return -TARGET_EFAULT;
-            __put_user(stfs.f_type, &target_stfs->f_type);
-            __put_user(stfs.f_bsize, &target_stfs->f_bsize);
-            __put_user(stfs.f_blocks, &target_stfs->f_blocks);
-            __put_user(stfs.f_bfree, &target_stfs->f_bfree);
-            __put_user(stfs.f_bavail, &target_stfs->f_bavail);
-            __put_user(stfs.f_files, &target_stfs->f_files);
-            __put_user(stfs.f_ffree, &target_stfs->f_ffree);
-            __put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
-            __put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
-            __put_user(stfs.f_namelen, &target_stfs->f_namelen);
-            __put_user(stfs.f_frsize, &target_stfs->f_frsize);
-            __put_user(stfs.f_flags, &target_stfs->f_flags);
-            memset(target_stfs->f_spare, 0, sizeof(target_stfs->f_spare));
-            unlock_user_struct(target_stfs, arg3, 1);
-            return ret;
-        }
+    statfs_fstatfs_impl(statfs64);
 #endif
+
 #ifdef TARGET_NR_socketcall
     case TARGET_NR_socketcall:
         return do_socketcall(arg1, arg2);
-- 
2.47.3
Re: [PATCH RFC 3/3] linux-user/syscall.c: consolidate statfs calls further
Posted by Richard Henderson 4 weeks, 1 day ago
On 1/10/26 08:41, Michael Tokarev wrote:
> Since statfs & statfs64 implementations are exactly the same,
> differs only in "64" suffix, merge them into one using a common
> macro.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

Most of the common code should be split to a host_to_target_statfs() and(?) 
host_to_target_statfs64().  Once that's done, there's not much use in a macro.


r~
Re: [PATCH RFC 3/3] linux-user/syscall.c: consolidate statfs calls further
Posted by Michael Tokarev 4 weeks, 1 day ago
On 1/11/26 03:03, Richard Henderson wrote:
> On 1/10/26 08:41, Michael Tokarev wrote:
>> Since statfs & statfs64 implementations are exactly the same,
>> differs only in "64" suffix, merge them into one using a common
>> macro.
>>
>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> 
> Most of the common code should be split to a host_to_target_statfs() 
> and(?) host_to_target_statfs64().  Once that's done, there's not much 
> use in a macro.

It will be exactly the same, - be it in its own function(s) or directly
in syscall1(): there will be 2 copies of exactly the same text, dealing
with 2 different types of target_statfs structs.  This is where a macro
like in this patch/rfc comes into play.  An alternative is C++ template
mechanism (which doesn't work in C, obviously), or a repeat of the same
code - just like we have now.

The same situation is with struct stat, but it is much worse, because
there are more possible permutations - there's eabi_stat64 on ARM and
struct target_stat vs target_stat64, and several combinations of
TARGET_STAT64_HAS_BROKEN_ST_INO and HAVE_STRUCT_STAT_ST_ATIM.  It's
kind of insane in this context.  While it's trivial to use a macro
approach for statfs, it's not as easy for struct stat.

Personally, I see no big advantage of splitting some syscalls into
separate functions out of huge syscall1().  Yes, this function is large,
but it wont be much better if some parts of it are split out, but there
will be slightly more code (function definitions).  Maybe a bit more..
style, so to say, but splitting it wont give us much..

Thanks,

/mjt