[LTP] [PATCH 0/2] fanotify musl fixes

[LTP] [PATCH 0/2] fanotify musl fixes

[Petr Vorel]

Hi,

2 fixes fanotify builds for musl. What I don't like on this solution is
mixing our lapi implementation with kernel and glibc sources.

Also, understand, if you don't want to put these workarounds just for
musl. Alternative would be to detect presence of
fanotify_event_info_fid.fsid.val with autotools and just TCONF with
TST_TEST_TCONF() if missing (+ revert b8aebc835).

https://travis-ci.org/pevik/ltp/builds/607410576

Kind regards,
Petr

Petr Vorel (2):
  fanotify: Move __kernel_fsid_t definition to correct place
  fanotify: Rename fanotify_event_info_fid struct

 testcases/kernel/syscalls/fanotify/fanotify.h | 22 +++++++++----------
 .../kernel/syscalls/fanotify/fanotify13.c     |  4 ++--
 .../kernel/syscalls/fanotify/fanotify15.c     |  4 ++--
 3 files changed, 15 insertions(+), 15 deletions(-)

-- 
2.24.0.rc2

[LTP] [PATCH 1/2] fanotify: Move __kernel_fsid_t definition to correct place

[Petr Vorel]

This fixes errors:

fanotify.h:149:55: error: unknown type name ?__kernel_fsid_t?
 static inline void fanotify_get_fid(const char *path, __kernel_fsid_t *fsid,
                                                       ^~~~~~~~~~~~~~~
fanotify13.c:47:2: error: unknown type name ?__kernel_fsid_t?
  __kernel_fsid_t fsid;

Fixes: b8aebc835 ("fanotify: Fix missing __kernel_fsid_t definition")

Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
---
 testcases/kernel/syscalls/fanotify/fanotify.h | 20 ++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify.h b/testcases/kernel/syscalls/fanotify/fanotify.h
index 435f100d8..563a4eb5b 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify.h
+++ b/testcases/kernel/syscalls/fanotify/fanotify.h
@@ -116,6 +116,15 @@ struct fanotify_mark_type {
 	const char * name;
 };
 
+#ifdef HAVE_NAME_TO_HANDLE_AT
+#ifndef __kernel_fsid_t
+typedef struct {
+	int	val[2];
+} lapi_fsid_t;
+#define __kernel_fsid_t lapi_fsid_t
+#endif
+#endif /* HAVE_NAME_TO_HANDLE_AT */
+
 #ifndef FAN_REPORT_FID
 #define FAN_REPORT_FID		0x00000200
 
@@ -126,20 +135,13 @@ struct fanotify_event_info_header {
 };
 
 #ifdef HAVE_NAME_TO_HANDLE_AT
-#ifndef __kernel_fsid_t
-typedef struct {
-	int	val[2];
-} lapi_fsid_t;
-#define __kernel_fsid_t lapi_fsid_t
-#endif
-
 struct fanotify_event_info_fid {
 	struct fanotify_event_info_header hdr;
 	__kernel_fsid_t fsid;
 	unsigned char handle[0];
 };
-#endif
-#endif
+#endif /* HAVE_NAME_TO_HANDLE_AT */
+#endif /* ! FAN_REPORT_FID */
 
 #ifdef HAVE_NAME_TO_HANDLE_AT
 /*
-- 
2.24.0.rc2

[LTP] [PATCH 2/2] fanotify: Rename fanotify_event_info_fid struct

[Petr Vorel]

To fix build on musl, which also defines fanotify_event_info_fid,
but uses fsid_t type for fsid instead of __kernel_fsid_t.

This fixes errors:

fanotify13.c: In function ?do_test?:
fanotify13.c:278:20: error: ?fsid_t? {aka ?struct __fsid_t?} has no member named ?val?; did you mean ?__val??
    event_fid->fsid.val[0],
                    ^~~
../../../../include/tst_test.h:49:53: note: in definition of macro ?tst_res?
  tst_res_(__FILE__, __LINE__, (ttype), (arg_fmt), ##__VA_ARGS__)
                                                     ^~~~~~~~~~~
fanotify13.c:279:20: error: ?fsid_t? {aka ?struct __fsid_t?} has no member named ?val?; did you mean ?__val??
    event_fid->fsid.val[1],

musl (unlike glibc and uclibc-ng) defines fanotify_event_info_fid in
fanotify.h and uses fsid_t as type for fanotify_event_info_fid.fsid
member, which defines __val[2] (unlike val[2] in __kernel_fsid_t).

/* musl (include/sys/fanotify.h) */
struct fanotify_event_info_fid {
	struct fanotify_event_info_header hdr;
	fsid_t fsid;
	unsigned char handle[];
};
/* musl (include/sys/statfs.h) */
typedef struct __fsid_t {
	int __val[2];
} fsid_t;

/* kernel (include/uapi/linux/fanotify.h) */
struct fanotify_event_info_fid {
	struct fanotify_event_info_header hdr;
	__kernel_fsid_t fsid;
	/*
	 * Following is an opaque struct file_handle that can be passed as
	 * an argument to open_by_handle_at(2).
	 */
	unsigned char handle[0];
};
/* kernel include/uapi/asm-generic/posix_types.h */
typedef struct {
	int	val[2];
} __kernel_fsid_t;

Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
---
 testcases/kernel/syscalls/fanotify/fanotify.h   | 6 ++----
 testcases/kernel/syscalls/fanotify/fanotify13.c | 4 ++--
 testcases/kernel/syscalls/fanotify/fanotify15.c | 4 ++--
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify.h b/testcases/kernel/syscalls/fanotify/fanotify.h
index 563a4eb5b..faac178cf 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify.h
+++ b/testcases/kernel/syscalls/fanotify/fanotify.h
@@ -133,17 +133,15 @@ struct fanotify_event_info_header {
 	uint8_t pad;
 	uint16_t len;
 };
+#endif /* ! FAN_REPORT_FID */
 
 #ifdef HAVE_NAME_TO_HANDLE_AT
-struct fanotify_event_info_fid {
+struct lapi_fanotify_event_info_fid {
 	struct fanotify_event_info_header hdr;
 	__kernel_fsid_t fsid;
 	unsigned char handle[0];
 };
-#endif /* HAVE_NAME_TO_HANDLE_AT */
-#endif /* ! FAN_REPORT_FID */
 
-#ifdef HAVE_NAME_TO_HANDLE_AT
 /*
  * Helper function used to obtain fsid and file_handle for a given path.
  * Used by test files correlated to FAN_REPORT_FID functionality.
diff --git a/testcases/kernel/syscalls/fanotify/fanotify13.c b/testcases/kernel/syscalls/fanotify/fanotify13.c
index 030734285..e0ce887e7 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify13.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify13.c
@@ -159,7 +159,7 @@ static void do_test(unsigned int number)
 
 	struct file_handle *event_file_handle;
 	struct fanotify_event_metadata *metadata;
-	struct fanotify_event_info_fid *event_fid;
+	struct lapi_fanotify_event_info_fid *event_fid;
 	struct test_case_t *tc = &test_cases[number];
 	struct fanotify_mark_type *mark = &tc->mark;
 
@@ -207,7 +207,7 @@ static void do_test(unsigned int number)
 	for (i = 0, metadata = (struct fanotify_event_metadata *) events_buf;
 		FAN_EVENT_OK(metadata, len);
 		metadata = FAN_EVENT_NEXT(metadata, len), i++) {
-		event_fid = (struct fanotify_event_info_fid *) (metadata + 1);
+		event_fid = (struct lapi_fanotify_event_info_fid *) (metadata + 1);
 		event_file_handle = (struct file_handle *) event_fid->handle;
 
 		/* File descriptor is redundant with FAN_REPORT_FID */
diff --git a/testcases/kernel/syscalls/fanotify/fanotify15.c b/testcases/kernel/syscalls/fanotify/fanotify15.c
index e9e926078..e9f1ff70d 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify15.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify15.c
@@ -53,7 +53,7 @@ static void do_test(void)
 
 	struct file_handle *event_file_handle;
 	struct fanotify_event_metadata *metadata;
-	struct fanotify_event_info_fid *event_fid;
+	struct lapi_fanotify_event_info_fid *event_fid;
 
 	if (fanotify_mark(fanotify_fd, FAN_MARK_ADD | FAN_MARK_FILESYSTEM,
 				FAN_CREATE | FAN_DELETE | FAN_ATTRIB |
@@ -125,7 +125,7 @@ static void do_test(void)
 	for (i = 0, metadata = (struct fanotify_event_metadata *) events_buf;
 		FAN_EVENT_OK(metadata, len);
 		metadata = FAN_EVENT_NEXT(metadata,len), i++) {
-		event_fid = (struct fanotify_event_info_fid *) (metadata + 1);
+		event_fid = (struct lapi_fanotify_event_info_fid *) (metadata + 1);
 		event_file_handle = (struct file_handle *) event_fid->handle;
 
 		if (i >= count) {
-- 
2.24.0.rc2

[LTP] [PATCH 1/2] fanotify: Move __kernel_fsid_t definition to correct place

[Cyril Hrubis]

Hi!
> Fixes: b8aebc835 ("fanotify: Fix missing __kernel_fsid_t definition")

Looks good, one minor comment below.

> Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
> ---
>  testcases/kernel/syscalls/fanotify/fanotify.h | 20 ++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify.h b/testcases/kernel/syscalls/fanotify/fanotify.h
> index 435f100d8..563a4eb5b 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify.h
> +++ b/testcases/kernel/syscalls/fanotify/fanotify.h
> @@ -116,6 +116,15 @@ struct fanotify_mark_type {
>  	const char * name;
>  };
>  
> +#ifdef HAVE_NAME_TO_HANDLE_AT
> +#ifndef __kernel_fsid_t
> +typedef struct {
> +	int	val[2];
> +} lapi_fsid_t;
> +#define __kernel_fsid_t lapi_fsid_t
> +#endif
> +#endif /* HAVE_NAME_TO_HANDLE_AT */

Can we please indent the inner ifdef/endif/define statements?

It's done by spaces after the hash, so it should look like:

#ifndef FOO
# define bar
# ifdef BAR
code comes here
and here
# endif
#endif

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] fanotify: Rename fanotify_event_info_fid struct

[Cyril Hrubis]

Hi!
> To fix build on musl, which also defines fanotify_event_info_fid,
> but uses fsid_t type for fsid instead of __kernel_fsid_t.
> 
> This fixes errors:
> 
> fanotify13.c: In function ???do_test???:
> fanotify13.c:278:20: error: ???fsid_t??? {aka ???struct __fsid_t???} has no member named ???val???; did you mean ???__val????
>     event_fid->fsid.val[0],
>                     ^~~
> ../../../../include/tst_test.h:49:53: note: in definition of macro ???tst_res???
>   tst_res_(__FILE__, __LINE__, (ttype), (arg_fmt), ##__VA_ARGS__)
>                                                      ^~~~~~~~~~~
> fanotify13.c:279:20: error: ???fsid_t??? {aka ???struct __fsid_t???} has no member named ???val???; did you mean ???__val????
>     event_fid->fsid.val[1],
> 
> musl (unlike glibc and uclibc-ng) defines fanotify_event_info_fid in
> fanotify.h and uses fsid_t as type for fanotify_event_info_fid.fsid
> member, which defines __val[2] (unlike val[2] in __kernel_fsid_t).

I don't know, this really sounds like a bug in musl.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] fanotify: Rename fanotify_event_info_fid struct

[Jan Stancek]

----- Original Message -----
> --- a/testcases/kernel/syscalls/fanotify/fanotify.h
> +++ b/testcases/kernel/syscalls/fanotify/fanotify.h
> @@ -133,17 +133,15 @@ struct fanotify_event_info_header {
>  	uint8_t pad;
>  	uint16_t len;
>  };
> +#endif /* ! FAN_REPORT_FID */
>  
>  #ifdef HAVE_NAME_TO_HANDLE_AT
> -struct fanotify_event_info_fid {
> +struct lapi_fanotify_event_info_fid {
>  	struct fanotify_event_info_header hdr;
>  	__kernel_fsid_t fsid;
>  	unsigned char handle[0];
>  };

I think I see what you mean by "mixing glibc/lapi/kernel types".
This structure could be combination of various types and now it's
used even if sys/fanotify.h provides one.

As alternative idea, we could add some accessor macro for that 'val' field.
On musl macro would return '__val', and elsewhere 'val'.

[LTP] [PATCH 1/2] fanotify: Move __kernel_fsid_t definition to correct place

[Petr Vorel]

Hi,

> Hi!
> > Fixes: b8aebc835 ("fanotify: Fix missing __kernel_fsid_t definition")

> Looks good, one minor comment below.

> > Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
> > ---
> >  testcases/kernel/syscalls/fanotify/fanotify.h | 20 ++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)

> > diff --git a/testcases/kernel/syscalls/fanotify/fanotify.h b/testcases/kernel/syscalls/fanotify/fanotify.h
> > index 435f100d8..563a4eb5b 100644
> > --- a/testcases/kernel/syscalls/fanotify/fanotify.h
> > +++ b/testcases/kernel/syscalls/fanotify/fanotify.h
> > @@ -116,6 +116,15 @@ struct fanotify_mark_type {
> >  	const char * name;
> >  };

> > +#ifdef HAVE_NAME_TO_HANDLE_AT
> > +#ifndef __kernel_fsid_t
> > +typedef struct {
> > +	int	val[2];
> > +} lapi_fsid_t;
> > +#define __kernel_fsid_t lapi_fsid_t
> > +#endif
> > +#endif /* HAVE_NAME_TO_HANDLE_AT */

> Can we please indent the inner ifdef/endif/define statements?

> It's done by spaces after the hash, so it should look like:

> #ifndef FOO
> # define bar
> # ifdef BAR
> code comes here
> and here
> # endif
> #endif
Sure, sorry for not remembering it.
I'll probably push this one once we agree on second patch.

Kind regards,
Petr

[LTP] [PATCH 2/2] fanotify: Rename fanotify_event_info_fid struct

[Petr Vorel]

> Hi!
> > To fix build on musl, which also defines fanotify_event_info_fid,
> > but uses fsid_t type for fsid instead of __kernel_fsid_t.

> > This fixes errors:

> > fanotify13.c: In function ???do_test???:
> > fanotify13.c:278:20: error: ???fsid_t??? {aka ???struct __fsid_t???} has no member named ???val???; did you mean ???__val????
> >     event_fid->fsid.val[0],
> >                     ^~~
> > ../../../../include/tst_test.h:49:53: note: in definition of macro ???tst_res???
> >   tst_res_(__FILE__, __LINE__, (ttype), (arg_fmt), ##__VA_ARGS__)
> >                                                      ^~~~~~~~~~~
> > fanotify13.c:279:20: error: ???fsid_t??? {aka ???struct __fsid_t???} has no member named ???val???; did you mean ???__val????
> >     event_fid->fsid.val[1],

> > musl (unlike glibc and uclibc-ng) defines fanotify_event_info_fid in
> > fanotify.h and uses fsid_t as type for fanotify_event_info_fid.fsid
> > member, which defines __val[2] (unlike val[2] in __kernel_fsid_t).

> I don't know, this really sounds like a bug in musl.
What would you propose to fix? Change fsid_t member to val[2]?
I'm a bit confused, which one is correct.
Or removing fanotify_event_info_fid struct?

In musl: 32b82cf5 ("fix the fsid_t structure to match name of __val")
changed it from val[2] to __val[2] in 2011 with comment:

    this is a case of poorly written man pages not matching the actual
    implementation, and why i hate implementing nonstandard interfaces
    with no actual documentation of how they're intended to work.

Kernel defines __kernel_fsid_t with val[2]
/* kernel include/uapi/asm-generic/posix_types.h */
#ifndef __kernel_fsid_t
typedef struct {
	int	val[2];
} __kernel_fsid_t;
#endif

And it was using val[2] at least in v2.6.31-rc1 - no sing to be __val[2].

glibc has __val[2] member. That's what triggered musl change I guess.
It looks to me it was here 2.3.1, before it used __u_quad_t
(typedef unsigned long long int __u_quad_t), but still with __val[2].

/* glibc posix/bits/types.h */
__STD_TYPE __FSID_T_TYPE __fsid_t;	/* Type of file system IDs.  */
/* glibc bits/typesizes.h */
#define __FSID_T_TYPE		struct { int __val[2]; }
/* glibc bits/statvfs.h */
struct statvfs
  {
...
    __fsid_t f_fsid;

Uclibc defines __kernel_fsid_t, sometimes defines it as val[2] and sometimes
have a condition __USE_ALL to have it val[2] (not sure if __USE_ALL is a
default, I haven't found any doc about it (is it uclibc specific or what?)

/* uclibc include/sys/types.h */
typedef __fsid_t fsid_t;

/* uclibc libc/sysdeps/linux/x86_64/bits/kernel_types.h */
typedef struct {
#ifdef __USE_ALL
	int val[2];
#else
	int __val[2];
#endif
} __kernel_fsid_t;

#endif
/* uclibc libc/sysdeps/linux/aarch64/bits/kernel_types.h */
typedef struct {
	int	val[2];
} __kernel_fsid_t;

But for __fsid_t uses the same code as glibc, with __val[2].

=> libc types use __val[2], kernel types __val.
The problem is really with mixing kernel and libc struct.
That's why I'd be to ask musl to remove it.

Kind regards,
Petr

[LTP] [PATCH 2/2] fanotify: Rename fanotify_event_info_fid struct

[Petr Vorel]

Hi Jan,

> ----- Original Message -----
> > --- a/testcases/kernel/syscalls/fanotify/fanotify.h
> > +++ b/testcases/kernel/syscalls/fanotify/fanotify.h
> > @@ -133,17 +133,15 @@ struct fanotify_event_info_header {
> >  	uint8_t pad;
> >  	uint16_t len;
> >  };
> > +#endif /* ! FAN_REPORT_FID */

> >  #ifdef HAVE_NAME_TO_HANDLE_AT
> > -struct fanotify_event_info_fid {
> > +struct lapi_fanotify_event_info_fid {
> >  	struct fanotify_event_info_header hdr;
> >  	__kernel_fsid_t fsid;
> >  	unsigned char handle[0];
> >  };

> I think I see what you mean by "mixing glibc/lapi/kernel types".
> This structure could be combination of various types and now it's
> used even if sys/fanotify.h provides one.

> As alternative idea, we could add some accessor macro for that 'val' field.
> On musl macro would return '__val', and elsewhere 'val'.

Sure, I can do the detection, probably with autotools detection
(although that macro could be defined without it, but current state can change).

Kind regards,
Petr