diff mbox series

[18/33] Implement stat related syscalls

Message ID 20230808060815.9001-19-kariem.taha2.7@gmail.com
State New
Headers show
Series Implement the stat system calls for FreeBSD. | expand

Commit Message

Karim Taha Aug. 8, 2023, 6:08 a.m. UTC 
From: Stacey Son <sson@FreeBSD.org>

Implement the following syscalls:
stat(2)
lstat(2)
fstat(2)
fstatat(2)
nstat
nfstat
nlstat

Signed-off-by: Stacey Son <sson@FreeBSD.org>
Signed-off-by: Karim Taha <kariem.taha2.7@gmail.com>
---
 bsd-user/freebsd/os-stat.h | 130 +++++++++++++++++++++++++++++++++++++
 1 file changed, 130 insertions(+)
 create mode 100644 bsd-user/freebsd/os-stat.h

Comments

Richard Henderson Aug. 8, 2023, 9:53 p.m. UTC | #1 
On 8/7/23 23:08, Karim Taha wrote:
> From: Stacey Son <sson@FreeBSD.org>
> 
> Implement the following syscalls:
> stat(2)
> lstat(2)
> fstat(2)
> fstatat(2)
> nstat
> nfstat
> nlstat
> 
> Signed-off-by: Stacey Son <sson@FreeBSD.org>
> Signed-off-by: Karim Taha <kariem.taha2.7@gmail.com>

Why are all of these in os-stat.h instead of os-stat.c?
Is this attempting to avoid clang's warning for unused static inline function in a c file?

> +/* stat(2) */
> +static inline abi_long do_freebsd11_stat(abi_long arg1, abi_long arg2)
> +{
> +    abi_long ret;
> +    void *p;
> +    struct freebsd11_stat st;
> +
> +    LOCK_PATH(p, arg1);
> +    ret = get_errno(freebsd11_stat(path(p), &st));
> +    UNLOCK_PATH(p, arg1);
> +    if (!is_error(ret)) {
> +        ret = h2t_freebsd11_stat(arg2, &st);
> +    }
> +    return ret;
> +}

The patch ordering is poor, because freebsd11_stat is used here but not introduced until 
patch 23, and do_freebsd11_stat itself is not used until patch 30.

And yet you delay compilation of os-stat.c until patch 29.  Patch 29 is either too early 
or too late, depending on the viewpoint.

If os-stat.c compilation was enabled earlier, it would require you to order all of the 
patches such that os-stat.c will always compile.

If os-stat.c compilation was enabled later (indeed last), you would not need to mark this 
function 'static inline' in order to avoid unused function warnings prior to their use in 
patches 30-33.

I prefer the ordering in which os-stat.c always compiles.  This probably requires patches 
23-27 be ordered first, and patches 30-33 be merged with patches 18-22.  There is no need 
for *any* of these functions to be marked inline -- leave that choice to the compiler.


r~
Warner Losh Aug. 9, 2023, 3:07 a.m. UTC | #2 
On Tue, Aug 8, 2023 at 3:53 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 8/7/23 23:08, Karim Taha wrote:
> > From: Stacey Son <sson@FreeBSD.org>
> >
> > Implement the following syscalls:
> > stat(2)
> > lstat(2)
> > fstat(2)
> > fstatat(2)
> > nstat
> > nfstat
> > nlstat
> >
> > Signed-off-by: Stacey Son <sson@FreeBSD.org>
> > Signed-off-by: Karim Taha <kariem.taha2.7@gmail.com>
>
> Why are all of these in os-stat.h instead of os-stat.c?
> Is this attempting to avoid clang's warning for unused static inline
> function in a c file?
>

I asked Stacey about this ages ago (why some things are like this) and he'd
indicated it was for better optimization (inlining) in
do_freebsd_syscall(). Other
inlines are in the .h files so that we can define a common set, and include
that
file and have netbsd/openbsd/freebsd pick and choose which ones to use
without
the warnings. The whole thing is a big mess right now, to be honest, but
I've tried
to avoid making structural changes in this area since I didn't want to
introduce bugs
and we have been chasing some weird corruption bug with threads for ages
(though
it might be related to the vm address space use bug that you fixed
recently, I've not
run things to see if that fixed it or not).

So, that's kinda why. I do agree with you that that would be a better
structure, but
can we use this structure for upstreaming and once we get the other issues
worked
out, we can do a restructure... We've moved things around a bit, and I'm
also waiting
for the NetBSD folks that contacted me to finish their efforts before I
pull the rug out
from them (or they timeout, which isn't quite yet).

Warner


> > +/* stat(2) */
> > +static inline abi_long do_freebsd11_stat(abi_long arg1, abi_long arg2)
> > +{
> > +    abi_long ret;
> > +    void *p;
> > +    struct freebsd11_stat st;
> > +
> > +    LOCK_PATH(p, arg1);
> > +    ret = get_errno(freebsd11_stat(path(p), &st));
> > +    UNLOCK_PATH(p, arg1);
> > +    if (!is_error(ret)) {
> > +        ret = h2t_freebsd11_stat(arg2, &st);
> > +    }
> > +    return ret;
> > +}
>
> The patch ordering is poor, because freebsd11_stat is used here but not
> introduced until
> patch 23, and do_freebsd11_stat itself is not used until patch 30.
>
> And yet you delay compilation of os-stat.c until patch 29.  Patch 29 is
> either too early
> or too late, depending on the viewpoint.
>
> If os-stat.c compilation was enabled earlier, it would require you to
> order all of the
> patches such that os-stat.c will always compile.
>
> If os-stat.c compilation was enabled later (indeed last), you would not
> need to mark this
> function 'static inline' in order to avoid unused function warnings prior
> to their use in
> patches 30-33.
>
> I prefer the ordering in which os-stat.c always compiles.  This probably
> requires patches
> 23-27 be ordered first, and patches 30-33 be merged with patches 18-22.
> There is no need
> for *any* of these functions to be marked inline -- leave that choice to
> the compiler.
>
>
> r~
>
Richard Henderson Aug. 9, 2023, 3:17 a.m. UTC | #3 
On 8/8/23 20:07, Warner Losh wrote:
> So, that's kinda why. I do agree with you that that would be a better structure, but
> can we use this structure for upstreaming and once we get the other issues worked
> out, we can do a restructure... We've moved things around a bit, and I'm also waiting
> for the NetBSD folks that contacted me to finish their efforts before I pull the rug out
> from them (or they timeout, which isn't quite yet).

Yes, that's fine.  I understand the pain.

In which case have a

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

for patches 18-33.  My only quibble with all of them related to this.


r~
diff mbox series

Patch

diff --git a/bsd-user/freebsd/os-stat.h b/bsd-user/freebsd/os-stat.h
new file mode 100644
index 0000000000..f8f99b4a72
--- /dev/null
+++ b/bsd-user/freebsd/os-stat.h
@@ -0,0 +1,130 @@ 
+/*
+ *  stat related system call shims and definitions
+ *
+ *  Copyright (c) 2013 Stacey D. Son
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef BSD_USER_FREEBSD_OS_STAT_H
+#define BSD_USER_FREEBSD_OS_STAT_H
+
+/* stat(2) */
+static inline abi_long do_freebsd11_stat(abi_long arg1, abi_long arg2)
+{
+    abi_long ret;
+    void *p;
+    struct freebsd11_stat st;
+
+    LOCK_PATH(p, arg1);
+    ret = get_errno(freebsd11_stat(path(p), &st));
+    UNLOCK_PATH(p, arg1);
+    if (!is_error(ret)) {
+        ret = h2t_freebsd11_stat(arg2, &st);
+    }
+    return ret;
+}
+
+/* lstat(2) */
+static inline abi_long do_freebsd11_lstat(abi_long arg1, abi_long arg2)
+{
+    abi_long ret;
+    void *p;
+    struct freebsd11_stat st;
+
+    LOCK_PATH(p, arg1);
+    ret = get_errno(freebsd11_lstat(path(p), &st));
+    UNLOCK_PATH(p, arg1);
+    if (!is_error(ret)) {
+        ret = h2t_freebsd11_stat(arg2, &st);
+    }
+    return ret;
+}
+
+/* fstat(2) */
+static inline abi_long do_freebsd_fstat(abi_long arg1, abi_long arg2)
+{
+    abi_long ret;
+    struct stat st;
+
+    ret = get_errno(fstat(arg1, &st));
+    if (!is_error(ret))  {
+        ret = h2t_freebsd_stat(arg2, &st);
+    }
+    return ret;
+}
+
+/* fstatat(2) */
+static inline abi_long do_freebsd_fstatat(abi_long arg1, abi_long arg2,
+        abi_long arg3, abi_long arg4)
+{
+    abi_long ret;
+    void *p;
+    struct stat st;
+
+    LOCK_PATH(p, arg2);
+    ret = get_errno(fstatat(arg1, p, &st, arg4));
+    UNLOCK_PATH(p, arg2);
+    if (!is_error(ret) && arg3) {
+        ret = h2t_freebsd_stat(arg3, &st);
+    }
+    return ret;
+}
+
+/* undocummented nstat(char *path, struct nstat *ub) syscall */
+static abi_long do_freebsd11_nstat(abi_long arg1, abi_long arg2)
+{
+    abi_long ret;
+    void *p;
+    struct freebsd11_stat st;
+
+    LOCK_PATH(p, arg1);
+    ret = get_errno(freebsd11_nstat(path(p), &st));
+    UNLOCK_PATH(p, arg1);
+    if (!is_error(ret)) {
+        ret = h2t_freebsd11_nstat(arg2, &st);
+    }
+    return ret;
+}
+
+/* undocummented nfstat(int fd, struct nstat *sb) syscall */
+static abi_long do_freebsd11_nfstat(abi_long arg1, abi_long arg2)
+{
+    abi_long ret;
+    struct freebsd11_stat st;
+
+    ret = get_errno(freebsd11_nfstat(arg1, &st));
+    if (!is_error(ret))  {
+        ret = h2t_freebsd11_nstat(arg2, &st);
+    }
+    return ret;
+}
+
+/* undocummented nlstat(char *path, struct nstat *ub) syscall */
+static abi_long do_freebsd11_nlstat(abi_long arg1, abi_long arg2)
+{
+    abi_long ret;
+    void *p;
+    struct freebsd11_stat st;
+
+    LOCK_PATH(p, arg1);
+    ret = get_errno(freebsd11_nlstat(path(p), &st));
+    UNLOCK_PATH(p, arg1);
+    if (!is_error(ret)) {
+        ret = h2t_freebsd11_nstat(arg2, &st);
+    }
+    return ret;
+}
+
+#endif /* BSD_USER_FREEBSD_OS_STAT_H */