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

[Petr Vorel <pvorel@suse.cz>]

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/

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp