| Message ID | 20240423132823.194179-2-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Build fixes | expand |
On Tue, Apr 23, 2024 at 3:28 PM Petr Vorel <pvorel@suse.cz> wrote: > > This fixes build error on musl (alpine): > > In file included from unlink09.c:18: > ../../../../include/lapi/fs.h:58:15: error: unknown type name 'loff_t' > 58 | static inline loff_t tst_max_lfs_filesize(void) > > loff_t is defined in <fcntl.h> (but guarded _GNU_SOURCE), but just for > safety include lapi/fcntl.h in case lapi/fs.h is included in test which > needs fallback definitions from lapi/fs.h. You probably meant lapi/fcntl.h here ^^ > > Because we require _GNU_SOURCE definition for code in lapi/fs.h, that's > why there is the definition in both unlink09.c (the actual fix) and > lapi/fs.h for visibility of the problem. > > Fixes: 2cf78f47a ("unlink: Add error tests for EPERM and EROFS") > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > #define _GNU_SOURCE pain again. Would you solve it differently? I'd likely go similar route, but I'd drop the hunk from unlink09.c. The test is not using loff_t directly, it includes a header, so it should be up to that header to work without pre-existing defines. > > include/lapi/fs.h | 5 ++++- > testcases/kernel/syscalls/unlink/unlink09.c | 2 ++ > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/lapi/fs.h b/include/lapi/fs.h > index c19ee821d..4680f0090 100644 > --- a/include/lapi/fs.h > +++ b/include/lapi/fs.h > @@ -9,15 +9,18 @@ > #ifndef LAPI_FS_H__ > #define LAPI_FS_H__ > > +#define _GNU_SOURCE /* loff_t in <fcntl.h> */ I'd also add to comment here that it's included via lapi/fcntl.h > + > #include "config.h" > + > #ifndef HAVE_MOUNT_SETATTR > # ifdef HAVE_LINUX_FS_H > # include <linux/fs.h> > # endif > #endif > > -#include <sys/user.h> > #include <limits.h> > +#include "lapi/fcntl.h" > #include "lapi/abisize.h" > > #ifndef FS_IOC_GETFLAGS > diff --git a/testcases/kernel/syscalls/unlink/unlink09.c b/testcases/kernel/syscalls/unlink/unlink09.c > index cc4b4a07e..7e3fffe5c 100644 > --- a/testcases/kernel/syscalls/unlink/unlink09.c > +++ b/testcases/kernel/syscalls/unlink/unlink09.c > @@ -13,6 +13,8 @@ > * - EROFS when target file is on a read-only filesystem. > */ > > +#define _GNU_SOURCE /* loff_t in <fcntl.h> */ > + > #include <sys/ioctl.h> > #include "tst_test.h" > #include "lapi/fs.h" > -- > 2.43.0 >
Hi Jan, > On Tue, Apr 23, 2024 at 3:28 PM Petr Vorel <pvorel@suse.cz> wrote: > > This fixes build error on musl (alpine): > > In file included from unlink09.c:18: > > ../../../../include/lapi/fs.h:58:15: error: unknown type name 'loff_t' > > 58 | static inline loff_t tst_max_lfs_filesize(void) > > loff_t is defined in <fcntl.h> (but guarded _GNU_SOURCE), but just for > > safety include lapi/fcntl.h in case lapi/fs.h is included in test which > > needs fallback definitions from lapi/fs.h. > You probably meant lapi/fcntl.h here ^^ +1 > > Because we require _GNU_SOURCE definition for code in lapi/fs.h, that's > > why there is the definition in both unlink09.c (the actual fix) and > > lapi/fs.h for visibility of the problem. > > Fixes: 2cf78f47a ("unlink: Add error tests for EPERM and EROFS") > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > --- > > #define _GNU_SOURCE pain again. Would you solve it differently? > I'd likely go similar route, but I'd drop the hunk from unlink09.c. > The test is not using loff_t directly, it includes a header, so it > should be up to First, thanks a lot for your review! OK, this will work, just lapi/fs.h must be loaded before tst_test.h, othewise it would fail on Alpine: In file included from unlink09.c:20: ../../../../include/lapi/fs.h:61:15: error: unknown type name 'loff_t' 61 | static inline loff_t tst_max_lfs_filesize(void) | ^~~~~~ ../../../../include/lapi/fs.h: In function 'tst_max_lfs_filesize': ../../../../include/lapi/fs.h:64:17: error: 'loff_t' undeclared (first use in this function); did you mean 'off_t'? 64 | return (loff_t)LLONG_MAX; | ^~~~~~ | off_t ../../../../include/lapi/fs.h:64:17: note: each undeclared identifier is reported only once for each function it appears in ../../../../include/lapi/fs.h:64:24: error: expected ';' before numeric constant 64 | return (loff_t)LLONG_MAX; | ^ | ; make: *** [../../../../include/mk/rules.mk:45: unlink09] Error 1 (glibc hides loff_t behind __USE_MISC, which I thought it it's in the end _GNU_SOURCE, but obviously not). And using include/lapi/fs.h and most of lapi headers it's ok to use them before tst_test.h (some of them are still used for the old API). But include/lapi/getrandom.h will break this assumption and it can cause the troubles if include/lapi/getrandom.h needs include/lapi/fcntl.h or <fcntl.h>). Also my not-yet-finished effort with safe_fallocate() [1] had this problem (requires <fcntl.h>), but I'll solve this with providing fallocate() declaration as you suggested. > that header to work without pre-existing defines. > > include/lapi/fs.h | 5 ++++- > > testcases/kernel/syscalls/unlink/unlink09.c | 2 ++ > > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/lapi/fs.h b/include/lapi/fs.h > > index c19ee821d..4680f0090 100644 > > --- a/include/lapi/fs.h > > +++ b/include/lapi/fs.h > > @@ -9,15 +9,18 @@ > > #ifndef LAPI_FS_H__ > > #define LAPI_FS_H__ > > +#define _GNU_SOURCE /* loff_t in <fcntl.h> */ > I'd also add to comment here that it's included via lapi/fcntl.h +1 Kind regards, Petr [1] https://lore.kernel.org/ltp/20240412114616.GB427746@pevik/
On Wed, Apr 24, 2024 at 2:17 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Jan, > > > On Tue, Apr 23, 2024 at 3:28 PM Petr Vorel <pvorel@suse.cz> wrote: > > > > This fixes build error on musl (alpine): > > > > In file included from unlink09.c:18: > > > ../../../../include/lapi/fs.h:58:15: error: unknown type name 'loff_t' > > > 58 | static inline loff_t tst_max_lfs_filesize(void) > > > > loff_t is defined in <fcntl.h> (but guarded _GNU_SOURCE), but just for > > > safety include lapi/fcntl.h in case lapi/fs.h is included in test which > > > needs fallback definitions from lapi/fs.h. > > > You probably meant lapi/fcntl.h here ^^ > > +1 > > > > Because we require _GNU_SOURCE definition for code in lapi/fs.h, that's > > > why there is the definition in both unlink09.c (the actual fix) and > > > lapi/fs.h for visibility of the problem. > > > > Fixes: 2cf78f47a ("unlink: Add error tests for EPERM and EROFS") > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > --- > > > #define _GNU_SOURCE pain again. Would you solve it differently? > > > I'd likely go similar route, but I'd drop the hunk from unlink09.c. > > The test is not using loff_t directly, it includes a header, so it > > should be up to > > First, thanks a lot for your review! > > OK, this will work, just lapi/fs.h must be loaded before tst_test.h, > othewise it would fail on Alpine: So this is essentially: ------------------------------------------ #include <fcntl.h> // from tst_test.h include chain #define _GNU_SOURCE // from lapi/fs.h #include <fcntl.h> int main(void) { loff_t asd; return 0; } ------------------------------------------ and it doesn't compile. And same applies if you include first any of these first: include/lapi/fcntl.h:#include <fcntl.h> include/lapi/io_uring.h:#include <fcntl.h> include/lapi/pidfd.h:#include <fcntl.h> include/safe_macros_fn.h:#include <fcntl.h> include/tst_safe_macros.h:#include <fcntl.h> Do we really need for tst_max_lfs_filesize() to return loff_t? If we changed it to "long long", we'd avoid lot of issues with includes and _GNU_SOURCE for just single user of this function. > > In file included from unlink09.c:20: > ../../../../include/lapi/fs.h:61:15: error: unknown type name 'loff_t' > 61 | static inline loff_t tst_max_lfs_filesize(void) > | ^~~~~~ > ../../../../include/lapi/fs.h: In function 'tst_max_lfs_filesize': > ../../../../include/lapi/fs.h:64:17: error: 'loff_t' undeclared (first use in this function); did you mean 'off_t'? > 64 | return (loff_t)LLONG_MAX; > | ^~~~~~ > | off_t > ../../../../include/lapi/fs.h:64:17: note: each undeclared identifier is reported only once for each function it appears in > ../../../../include/lapi/fs.h:64:24: error: expected ';' before numeric constant > 64 | return (loff_t)LLONG_MAX; > | ^ > | ; > make: *** [../../../../include/mk/rules.mk:45: unlink09] Error 1 > > (glibc hides loff_t behind __USE_MISC, which I thought it it's in the end > _GNU_SOURCE, but obviously not). > > And using include/lapi/fs.h and most of lapi headers it's ok to use them before > tst_test.h (some of them are still used for the old API). But > include/lapi/getrandom.h will break this assumption and it can cause the > troubles if include/lapi/getrandom.h needs include/lapi/fcntl.h or <fcntl.h>). > > Also my not-yet-finished effort with safe_fallocate() [1] had this problem > (requires <fcntl.h>), but I'll solve this with providing fallocate() declaration > as you suggested. > > > that header to work without pre-existing defines. > > > > > include/lapi/fs.h | 5 ++++- > > > testcases/kernel/syscalls/unlink/unlink09.c | 2 ++ > > > 2 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/include/lapi/fs.h b/include/lapi/fs.h > > > index c19ee821d..4680f0090 100644 > > > --- a/include/lapi/fs.h > > > +++ b/include/lapi/fs.h > > > @@ -9,15 +9,18 @@ > > > #ifndef LAPI_FS_H__ > > > #define LAPI_FS_H__ > > > > +#define _GNU_SOURCE /* loff_t in <fcntl.h> */ > > > I'd also add to comment here that it's included via lapi/fcntl.h > > +1 > > Kind regards, > Petr > > [1] https://lore.kernel.org/ltp/20240412114616.GB427746@pevik/ >
Hi Jan, all, ... > > OK, this will work, just lapi/fs.h must be loaded before tst_test.h, > > othewise it would fail on Alpine: > So this is essentially: > ------------------------------------------ > #include <fcntl.h> // from tst_test.h include chain > #define _GNU_SOURCE // from lapi/fs.h > #include <fcntl.h> > int main(void) > { > loff_t asd; > return 0; > } > ------------------------------------------ > and it doesn't compile. And same applies if you include first any of > these first: > include/lapi/fcntl.h:#include <fcntl.h> > include/lapi/io_uring.h:#include <fcntl.h> > include/lapi/pidfd.h:#include <fcntl.h> > include/safe_macros_fn.h:#include <fcntl.h> > include/tst_safe_macros.h:#include <fcntl.h> > Do we really need for tst_max_lfs_filesize() to return loff_t? If we > changed it to "long long", > we'd avoid lot of issues with includes and _GNU_SOURCE for just single > user of this function. +1. We might get extra warning when there is 32 bit, but it would make things much easier => I'll send another version. Kind regards, Petr > > In file included from unlink09.c:20: > > ../../../../include/lapi/fs.h:61:15: error: unknown type name 'loff_t' > > 61 | static inline loff_t tst_max_lfs_filesize(void) > > | ^~~~~~ > > ../../../../include/lapi/fs.h: In function 'tst_max_lfs_filesize': > > ../../../../include/lapi/fs.h:64:17: error: 'loff_t' undeclared (first use in this function); did you mean 'off_t'? > > 64 | return (loff_t)LLONG_MAX; > > | ^~~~~~ > > | off_t > > ../../../../include/lapi/fs.h:64:17: note: each undeclared identifier is reported only once for each function it appears in > > ../../../../include/lapi/fs.h:64:24: error: expected ';' before numeric constant > > 64 | return (loff_t)LLONG_MAX; > > | ^ > > | ; > > make: *** [../../../../include/mk/rules.mk:45: unlink09] Error 1 > > (glibc hides loff_t behind __USE_MISC, which I thought it it's in the end > > _GNU_SOURCE, but obviously not). ...
diff --git a/include/lapi/fs.h b/include/lapi/fs.h index c19ee821d..4680f0090 100644 --- a/include/lapi/fs.h +++ b/include/lapi/fs.h @@ -9,15 +9,18 @@ #ifndef LAPI_FS_H__ #define LAPI_FS_H__ +#define _GNU_SOURCE /* loff_t in <fcntl.h> */ + #include "config.h" + #ifndef HAVE_MOUNT_SETATTR # ifdef HAVE_LINUX_FS_H # include <linux/fs.h> # endif #endif -#include <sys/user.h> #include <limits.h> +#include "lapi/fcntl.h" #include "lapi/abisize.h" #ifndef FS_IOC_GETFLAGS diff --git a/testcases/kernel/syscalls/unlink/unlink09.c b/testcases/kernel/syscalls/unlink/unlink09.c index cc4b4a07e..7e3fffe5c 100644 --- a/testcases/kernel/syscalls/unlink/unlink09.c +++ b/testcases/kernel/syscalls/unlink/unlink09.c @@ -13,6 +13,8 @@ * - EROFS when target file is on a read-only filesystem. */ +#define _GNU_SOURCE /* loff_t in <fcntl.h> */ + #include <sys/ioctl.h> #include "tst_test.h" #include "lapi/fs.h"
This fixes build error on musl (alpine): In file included from unlink09.c:18: ../../../../include/lapi/fs.h:58:15: error: unknown type name 'loff_t' 58 | static inline loff_t tst_max_lfs_filesize(void) loff_t is defined in <fcntl.h> (but guarded _GNU_SOURCE), but just for safety include lapi/fcntl.h in case lapi/fs.h is included in test which needs fallback definitions from lapi/fs.h. Because we require _GNU_SOURCE definition for code in lapi/fs.h, that's why there is the definition in both unlink09.c (the actual fix) and lapi/fs.h for visibility of the problem. Fixes: 2cf78f47a ("unlink: Add error tests for EPERM and EROFS") Signed-off-by: Petr Vorel <pvorel@suse.cz> --- #define _GNU_SOURCE pain again. Would you solve it differently? include/lapi/fs.h | 5 ++++- testcases/kernel/syscalls/unlink/unlink09.c | 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-)