Message ID | 20230808060815.9001-19-kariem.taha2.7@gmail.com |
---|---|
State | New |
Headers | show |
Series | Implement the stat system calls for FreeBSD. | expand |
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~
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~ >
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 --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 */