| Message ID | 20220613013051.1741434-1-guoren@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | uapi: Fixup strace compile error | expand |
Am Montag, 13. Juni 2022, 03:30:51 CEST schrieb guoren@kernel.org: > From: Guo Ren <guoren@linux.alibaba.com> > > There is no CONFIG_64BIT in userspace, we shouldn't limit it with > __BITS_PER_LONG == 32 to break the compatibility. Just export F_*64 > definitions to userspace permanently. > > gcc-11 -DHAVE_CONFIG_H -I./linux/x86_64 -I../../../src/linux/x86_64 > -I./linux/generic -I../../../src/linux/generic -I. -I../../../src > -DIN_STRACE=1 -isystem /opt/kernel/include -Wall -Wextra > -Wno-missing-field-initializers -Wno-unused-parameter -Wdate-time > -Wformat-security -Wimplicit-fallthrough=5 -Winit-self -Wlogical-op > -Wmissing-prototypes -Wnested-externs -Wold-style-definition > -Wtrampolines -Wundef -Wwrite-strings -Werror -g -O2 -c -o > libstrace_a-fetch_bpf_fprog.o `test -f 'fetch_bpf_fprog.c' || echo > '../../../src/'`fetch_bpf_fprog.c > In file included from ../../../src/defs.h:404, > from ../../../src/fcntl.c:12: > ../../../src/xlat/fcntlcmds.h:54:7: error: ‘F_GETLK64’ undeclared here > (not in a function); did you mean ‘F_GETLK’? > 54 | XLAT(F_GETLK64), > | ^~~~~~~~~ > ../../../src/xlat.h:64:54: note: in definition of macro ‘XLAT’ > 64 | # define XLAT(val) { (unsigned)(val), #val > } > | ^~~ > ../../../src/xlat/fcntlcmds.h:57:7: error: ‘F_SETLK64’ undeclared here > (not in a function); did you mean ‘F_SETLK’? > 57 | XLAT(F_SETLK64), > | ^~~~~~~~~ > ../../../src/xlat.h:64:54: note: in definition of macro ‘XLAT’ > 64 | # define XLAT(val) { (unsigned)(val), #val > } > | ^~~ > ../../../src/xlat/fcntlcmds.h:60:7: error: ‘F_SETLKW64’ undeclared here > (not in a function); did you mean ‘F_SETLKW’? > 60 | XLAT(F_SETLKW64), > | ^~~~~~~~~~ > ../../../src/xlat.h:64:54: note: in definition of macro ‘XLAT’ > 64 | # define XLAT(val) { (unsigned)(val), #val > } > | ^~~ > make[4]: *** [Makefile:5017: libstrace_a-fcntl.o] Error 1 > > comment by Eugene: > Actually, it's quite the opposite: "ifndef" usage made it vailable at all > times to the userspace, and this change has actually broken building strace > with the latest kernel headers[1][2]. There could be some debate whether > having these F_*64 definitions exposed to the user space 64-bit > applications, but it seems that were no harm (as they were exposed already > for quite some time), and they are useful at least for strace for compat > application tracing purposes. > > Fixes: 306f7cc1e9061 "uapi: always define F_GETLK64/F_SETLK64/F_SETLKW64 in fcntl.h" > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > Reported-by: Eugene Syromiatnikov <esyr@redhat.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: Palmer Dabbelt <palmer@rivosinc.com> > --- > include/uapi/asm-generic/fcntl.h | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h > index f13d37b60775..cd6bd65ec25d 100644 > --- a/include/uapi/asm-generic/fcntl.h > +++ b/include/uapi/asm-generic/fcntl.h > @@ -116,13 +116,11 @@ > #define F_GETSIG 11 /* for sockets. */ > #endif > > -#if __BITS_PER_LONG == 32 || defined(__KERNEL__) > #ifndef F_GETLK64 > #define F_GETLK64 12 /* using 'struct flock64' */ > #define F_SETLK64 13 > #define F_SETLKW64 14 > #endif > -#endif /* __BITS_PER_LONG == 32 || defined(__KERNEL__) */ Looks like prviously there were a number of ways these constants were ifdef'd - or not. A number of platforms already had no ifdef of any sort around them before, so this looks like the sane way to do it. Though in the original patch the special-mips-variant also gained the #if __BITS_PER_LONG == 32 ... conditional in arch/mips/include/uapi/asm/fcntl.h . So, is it also affected by this issue? Heiko
The change looks fine, but the commit log could use some work, please move the notes from the back to the front and make them a standalone commit log and just drop the rest.
On Mon, 13 Jun 2022 07:29:47 PDT (-0700), Christoph Hellwig wrote: > The change looks fine, but the commit log could use some work, please > move the notes from the back to the front and make them a standalone > commit log and just drop the rest. I'm digging through old stuff and don't see a v2, not sure if I just missed it. Either way I don't think it's really a RISC-V thing so Acked-by: Palmer Dabbelt <palmer@rivosinc.com> in case that helps. Thanks!
On Mon, Jun 13, 2022 at 10:29 PM Christoph Hellwig <hch@lst.de> wrote: > > The change looks fine, but the commit log could use some work, please > move the notes from the back to the front and make them a standalone > commit log and just drop the rest. Okay.
On Mon, Jun 13, 2022 at 9:58 PM Heiko Stübner <heiko@sntech.de> wrote: > > Am Montag, 13. Juni 2022, 03:30:51 CEST schrieb guoren@kernel.org: > > From: Guo Ren <guoren@linux.alibaba.com> > > > > There is no CONFIG_64BIT in userspace, we shouldn't limit it with > > __BITS_PER_LONG == 32 to break the compatibility. Just export F_*64 > > definitions to userspace permanently. > > > > gcc-11 -DHAVE_CONFIG_H -I./linux/x86_64 -I../../../src/linux/x86_64 > > -I./linux/generic -I../../../src/linux/generic -I. -I../../../src > > -DIN_STRACE=1 -isystem /opt/kernel/include -Wall -Wextra > > -Wno-missing-field-initializers -Wno-unused-parameter -Wdate-time > > -Wformat-security -Wimplicit-fallthrough=5 -Winit-self -Wlogical-op > > -Wmissing-prototypes -Wnested-externs -Wold-style-definition > > -Wtrampolines -Wundef -Wwrite-strings -Werror -g -O2 -c -o > > libstrace_a-fetch_bpf_fprog.o `test -f 'fetch_bpf_fprog.c' || echo > > '../../../src/'`fetch_bpf_fprog.c > > In file included from ../../../src/defs.h:404, > > from ../../../src/fcntl.c:12: > > ../../../src/xlat/fcntlcmds.h:54:7: error: ‘F_GETLK64’ undeclared here > > (not in a function); did you mean ‘F_GETLK’? > > 54 | XLAT(F_GETLK64), > > | ^~~~~~~~~ > > ../../../src/xlat.h:64:54: note: in definition of macro ‘XLAT’ > > 64 | # define XLAT(val) { (unsigned)(val), #val > > } > > | ^~~ > > ../../../src/xlat/fcntlcmds.h:57:7: error: ‘F_SETLK64’ undeclared here > > (not in a function); did you mean ‘F_SETLK’? > > 57 | XLAT(F_SETLK64), > > | ^~~~~~~~~ > > ../../../src/xlat.h:64:54: note: in definition of macro ‘XLAT’ > > 64 | # define XLAT(val) { (unsigned)(val), #val > > } > > | ^~~ > > ../../../src/xlat/fcntlcmds.h:60:7: error: ‘F_SETLKW64’ undeclared here > > (not in a function); did you mean ‘F_SETLKW’? > > 60 | XLAT(F_SETLKW64), > > | ^~~~~~~~~~ > > ../../../src/xlat.h:64:54: note: in definition of macro ‘XLAT’ > > 64 | # define XLAT(val) { (unsigned)(val), #val > > } > > | ^~~ > > make[4]: *** [Makefile:5017: libstrace_a-fcntl.o] Error 1 > > > > comment by Eugene: > > Actually, it's quite the opposite: "ifndef" usage made it vailable at all > > times to the userspace, and this change has actually broken building strace > > with the latest kernel headers[1][2]. There could be some debate whether > > having these F_*64 definitions exposed to the user space 64-bit > > applications, but it seems that were no harm (as they were exposed already > > for quite some time), and they are useful at least for strace for compat > > application tracing purposes. > > > > Fixes: 306f7cc1e9061 "uapi: always define F_GETLK64/F_SETLK64/F_SETLKW64 in fcntl.h" > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > Reported-by: Eugene Syromiatnikov <esyr@redhat.com> > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Heiko Stuebner <heiko@sntech.de> > > Cc: Palmer Dabbelt <palmer@rivosinc.com> > > --- > > include/uapi/asm-generic/fcntl.h | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h > > index f13d37b60775..cd6bd65ec25d 100644 > > --- a/include/uapi/asm-generic/fcntl.h > > +++ b/include/uapi/asm-generic/fcntl.h > > @@ -116,13 +116,11 @@ > > #define F_GETSIG 11 /* for sockets. */ > > #endif > > > > -#if __BITS_PER_LONG == 32 || defined(__KERNEL__) > > #ifndef F_GETLK64 > > #define F_GETLK64 12 /* using 'struct flock64' */ > > #define F_SETLK64 13 > > #define F_SETLKW64 14 > > #endif > > -#endif /* __BITS_PER_LONG == 32 || defined(__KERNEL__) */ > > Looks like prviously there were a number of ways these constants > were ifdef'd - or not. A number of platforms already had no ifdef of > any sort around them before, so this looks like the sane way to do it. > > Though in the original patch the special-mips-variant also gained the > #if __BITS_PER_LONG == 32 ... > conditional in arch/mips/include/uapi/asm/fcntl.h . > So, is it also affected by this issue? Not sure about mips. > > > Heiko > >
Thx for the reply, I even forgot that patch. I would update V2 with your acked, and Christoph advice. On Thu, Aug 4, 2022 at 8:40 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Mon, 13 Jun 2022 07:29:47 PDT (-0700), Christoph Hellwig wrote: > > The change looks fine, but the commit log could use some work, please > > move the notes from the back to the front and make them a standalone > > commit log and just drop the rest. > > I'm digging through old stuff and don't see a v2, not sure if I just > missed it. Either way I don't think it's really a RISC-V thing so > > Acked-by: Palmer Dabbelt <palmer@rivosinc.com> > > in case that helps. > > Thanks! -- Best Regards Guo Ren
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h index f13d37b60775..cd6bd65ec25d 100644 --- a/include/uapi/asm-generic/fcntl.h +++ b/include/uapi/asm-generic/fcntl.h @@ -116,13 +116,11 @@ #define F_GETSIG 11 /* for sockets. */ #endif -#if __BITS_PER_LONG == 32 || defined(__KERNEL__) #ifndef F_GETLK64 #define F_GETLK64 12 /* using 'struct flock64' */ #define F_SETLK64 13 #define F_SETLKW64 14 #endif -#endif /* __BITS_PER_LONG == 32 || defined(__KERNEL__) */ #ifndef F_SETOWN_EX #define F_SETOWN_EX 15