[LTP] [PATCH 1/2] lapi/fs: Include lapi/fcntl.h + define _GNU_SOURCE

[Jan Stancek]

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/
>