<sys/stat.h>: Use Linux kernel UAPI header if available and useful

Message ID 87zhmwt76d.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series
  • <sys/stat.h>: Use Linux kernel UAPI header if available and useful
Related show

Commit Message

Florian Weimer June 5, 2019, 2:53 p.m.
This will automatically import new STATX_* constants.  It also avoids
a conflict between <sys/stat.h> and <linux/stat.h>.

The 256 constant in the new static assert has been validated using
build-many-glibcs.py on all supported architectures.

2019-06-05  Florian Weimer  <fweimer@redhat.com>

	* io/bits/statx.h: Fix typo in error message.  Include
	<linux/stat.h> is available.  Define statx-related constants only
	if STATX_ALL is not defined by <linux/stat.h> (or the header is
	missing altogether).
	* sysdeps/unix/sysv/linux/statx.c: Add static assert for the size
	of struct statx.

Comments

Andreas Schwab June 5, 2019, 3:12 p.m. | #1
On Jun 05 2019, Florian Weimer <fweimer@redhat.com> wrote:

> 	* io/bits/statx.h: Fix typo in error message.  Include

> 	<linux/stat.h> is available.  Define statx-related constants only


s/is/if/

> diff --git a/io/bits/statx.h b/io/bits/statx.h

> index cff14b2543..d0dcc353ba 100644

> --- a/io/bits/statx.h

> +++ b/io/bits/statx.h

> @@ -19,9 +19,14 @@

>  /* This interface is based on <linux/stat.h> in Linux.  */

>  

>  #ifndef _SYS_STAT_H

> -# error Never include <bits/stat.x.h> directly, include <sys/stat.h> instead.

> +# error Never include <bits/statx.h> directly, include <sys/stat.h> instead.

>  #endif

>  

> +#if __glibc_has_include (<linux/stat.h>)

> +# include <linux/stat.h>

> +#endif


Wouldn't it be better to add sysdeps/unix/sysv/linux/bits/statx.h where
<linux/stat.h> is included unconditionally?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer June 5, 2019, 3:16 p.m. | #2
* Andreas Schwab:

> On Jun 05 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> 	* io/bits/statx.h: Fix typo in error message.  Include

>> 	<linux/stat.h> is available.  Define statx-related constants only

>

> s/is/if/

>

>> diff --git a/io/bits/statx.h b/io/bits/statx.h

>> index cff14b2543..d0dcc353ba 100644

>> --- a/io/bits/statx.h

>> +++ b/io/bits/statx.h

>> @@ -19,9 +19,14 @@

>>  /* This interface is based on <linux/stat.h> in Linux.  */

>>  

>>  #ifndef _SYS_STAT_H

>> -# error Never include <bits/stat.x.h> directly, include <sys/stat.h> instead.

>> +# error Never include <bits/statx.h> directly, include <sys/stat.h> instead.

>>  #endif

>>  

>> +#if __glibc_has_include (<linux/stat.h>)

>> +# include <linux/stat.h>

>> +#endif

>

> Wouldn't it be better to add sysdeps/unix/sysv/linux/bits/statx.h where

> <linux/stat.h> is included unconditionally?


I don't think <linux/stat.h> exists as an UAPI header in Linux 3.2,
which is currently the minimum kernel version.  I think it was only
added in Linux 3.7.

Thanks,
Florian
Florian Weimer June 6, 2019, 10:52 a.m. | #3
* Florian Weimer:

> * Andreas Schwab:

>

>> On Jun 05 2019, Florian Weimer <fweimer@redhat.com> wrote:

>>

>>> 	* io/bits/statx.h: Fix typo in error message.  Include

>>> 	<linux/stat.h> is available.  Define statx-related constants only

>>

>> s/is/if/

>>

>>> diff --git a/io/bits/statx.h b/io/bits/statx.h

>>> index cff14b2543..d0dcc353ba 100644

>>> --- a/io/bits/statx.h

>>> +++ b/io/bits/statx.h

>>> @@ -19,9 +19,14 @@

>>>  /* This interface is based on <linux/stat.h> in Linux.  */

>>>  

>>>  #ifndef _SYS_STAT_H

>>> -# error Never include <bits/stat.x.h> directly, include <sys/stat.h> instead.

>>> +# error Never include <bits/statx.h> directly, include <sys/stat.h> instead.

>>>  #endif

>>>  

>>> +#if __glibc_has_include (<linux/stat.h>)

>>> +# include <linux/stat.h>

>>> +#endif

>>

>> Wouldn't it be better to add sysdeps/unix/sysv/linux/bits/statx.h where

>> <linux/stat.h> is included unconditionally?

>

> I don't think <linux/stat.h> exists as an UAPI header in Linux 3.2,

> which is currently the minimum kernel version.  I think it was only

> added in Linux 3.7.


I'm not happy about the optics of referencing to <linux/stat.h> outside
the Linux sysdeps tree, but avoiding that is a bit involved.  See the
patch below.

Thanks,
Florian

<sys/stat.h>: Use Linux kernel UAPI header if available and useful

This will automatically import new STATX_* constants.  It also avoids
a conflict between <sys/stat.h> and <linux/stat.h>.

The 256 constant in the new static assert has been validated using
build-many-glibcs.py on all supported architectures.

2019-06-05  Florian Weimer  <fweimer@redhat.com>

	Linux: Use kernel headers for statx definitions if available.
	* include/bits/statx-generic.h: New file.
	* io/Makefile (headers): Add bits/statx-generic.h.
	* io/bits/statx-generic.h: New file.  Mostly copied from
	io/bits/statx.h.
	* io/bits/statx.h: Rewrite to include <bits/statx-generic.h>.
	* sysdeps/unix/sysv/linux/bits/statx.h: New file.
	* sysdeps/unix/sysv/linux/statx.c: Add static assert for the size
	of struct statx.

diff --git a/include/bits/statx-generic.h b/include/bits/statx-generic.h
new file mode 100644
index 0000000000..21674721b6
--- /dev/null
+++ b/include/bits/statx-generic.h
@@ -0,0 +1 @@
+#include <io/bits/statx-generic.h>
diff --git a/io/Makefile b/io/Makefile
index f2404db041..7f0a861c56 100644
--- a/io/Makefile
+++ b/io/Makefile
@@ -25,7 +25,7 @@ include ../Makeconfig
 headers := sys/stat.h bits/stat.h sys/statfs.h bits/statfs.h sys/vfs.h \
 	   sys/statvfs.h bits/statvfs.h fcntl.h sys/fcntl.h bits/fcntl.h \
 	   poll.h sys/poll.h bits/poll.h bits/fcntl2.h bits/poll2.h \
-	   bits/statx.h utime.h ftw.h fts.h sys/sendfile.h
+	   bits/statx.h bits/statx-generic.h utime.h ftw.h fts.h sys/sendfile.h
 
 routines :=								\
 	utime								\
diff --git a/io/bits/statx-generic.h b/io/bits/statx-generic.h
new file mode 100644
index 0000000000..f183eaff49
--- /dev/null
+++ b/io/bits/statx-generic.h
@@ -0,0 +1,93 @@
+/* Generic statx-related definitions and declarations.
+   Copyright (C) 2018-2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* This interface is based on <linux/stat.h> in Linux.  */
+
+#ifndef _SYS_STAT_H
+# error Never include <bits/statx-generic.h> directly, include <sys/stat.h> instead.
+#endif
+
+#ifndef STATX_TYPE
+struct statx_timestamp
+{
+  __int64_t tv_sec;
+  __uint32_t tv_nsec;
+  __int32_t __statx_timestamp_pad1[1];
+};
+
+/* Warning: The kernel may add additional fields to this struct in the
+   future.  Only use this struct for calling the statx function, not
+   for storing data.  (Expansion will be controlled by the mask
+   argument of the statx function.)  */
+struct statx
+{
+  __uint32_t stx_mask;
+  __uint32_t stx_blksize;
+  __uint64_t stx_attributes;
+  __uint32_t stx_nlink;
+  __uint32_t stx_uid;
+  __uint32_t stx_gid;
+  __uint16_t stx_mode;
+  __uint16_t __statx_pad1[1];
+  __uint64_t stx_ino;
+  __uint64_t stx_size;
+  __uint64_t stx_blocks;
+  __uint64_t stx_attributes_mask;
+  struct statx_timestamp stx_atime;
+  struct statx_timestamp stx_btime;
+  struct statx_timestamp stx_ctime;
+  struct statx_timestamp stx_mtime;
+  __uint32_t stx_rdev_major;
+  __uint32_t stx_rdev_minor;
+  __uint32_t stx_dev_major;
+  __uint32_t stx_dev_minor;
+  __uint64_t __statx_pad2[14];
+};
+
+# define STATX_TYPE 0x0001U
+# define STATX_MODE 0x0002U
+# define STATX_NLINK 0x0004U
+# define STATX_UID 0x0008U
+# define STATX_GID 0x0010U
+# define STATX_ATIME 0x0020U
+# define STATX_MTIME 0x0040U
+# define STATX_CTIME 0x0080U
+# define STATX_INO 0x0100U
+# define STATX_SIZE 0x0200U
+# define STATX_BLOCKS 0x0400U
+# define STATX_BASIC_STATS 0x07ffU
+# define STATX_ALL 0x0fffU
+# define STATX_BTIME 0x0800U
+# define STATX__RESERVED 0x80000000U
+
+# define STATX_ATTR_COMPRESSED 0x0004
+# define STATX_ATTR_IMMUTABLE 0x0010
+# define STATX_ATTR_APPEND 0x0020
+# define STATX_ATTR_NODUMP 0x0040
+# define STATX_ATTR_ENCRYPTED 0x0800
+# define STATX_ATTR_AUTOMOUNT 0x1000
+#endif /* !STATX_TYPE */
+
+__BEGIN_DECLS
+
+/* Fill *BUF with information about PATH in DIRFD.  */
+int statx (int __dirfd, const char *__restrict __path, int __flags,
+           unsigned int __mask, struct statx *__restrict __buf)
+  __THROW __nonnull ((2, 5));
+
+__END_DECLS
diff --git a/io/bits/statx.h b/io/bits/statx.h
index cff14b2543..b3147bfa8a 100644
--- a/io/bits/statx.h
+++ b/io/bits/statx.h
@@ -1,4 +1,4 @@
-/* statx-related definitions and declarations.
+/* statx-related definitions and declarations.  Generic version.
    Copyright (C) 2018-2019 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -19,73 +19,8 @@
 /* This interface is based on <linux/stat.h> in Linux.  */
 
 #ifndef _SYS_STAT_H
-# error Never include <bits/stat.x.h> directly, include <sys/stat.h> instead.
+# error Never include <bits/statx.h> directly, include <sys/stat.h> instead.
 #endif
 
-struct statx_timestamp
-{
-  __int64_t tv_sec;
-  __uint32_t tv_nsec;
-  __int32_t __statx_timestamp_pad1[1];
-};
-
-/* Warning: The kernel may add additional fields to this struct in the
-   future.  Only use this struct for calling the statx function, not
-   for storing data.  (Expansion will be controlled by the mask
-   argument of the statx function.)  */
-struct statx
-{
-  __uint32_t stx_mask;
-  __uint32_t stx_blksize;
-  __uint64_t stx_attributes;
-  __uint32_t stx_nlink;
-  __uint32_t stx_uid;
-  __uint32_t stx_gid;
-  __uint16_t stx_mode;
-  __uint16_t __statx_pad1[1];
-  __uint64_t stx_ino;
-  __uint64_t stx_size;
-  __uint64_t stx_blocks;
-  __uint64_t stx_attributes_mask;
-  struct statx_timestamp stx_atime;
-  struct statx_timestamp stx_btime;
-  struct statx_timestamp stx_ctime;
-  struct statx_timestamp stx_mtime;
-  __uint32_t stx_rdev_major;
-  __uint32_t stx_rdev_minor;
-  __uint32_t stx_dev_major;
-  __uint32_t stx_dev_minor;
-  __uint64_t __statx_pad2[14];
-};
-
-#define STATX_TYPE 0x0001U
-#define STATX_MODE 0x0002U
-#define STATX_NLINK 0x0004U
-#define STATX_UID 0x0008U
-#define STATX_GID 0x0010U
-#define STATX_ATIME 0x0020U
-#define STATX_MTIME 0x0040U
-#define STATX_CTIME 0x0080U
-#define STATX_INO 0x0100U
-#define STATX_SIZE 0x0200U
-#define STATX_BLOCKS 0x0400U
-#define STATX_BASIC_STATS 0x07ffU
-#define STATX_ALL 0x0fffU
-#define STATX_BTIME 0x0800U
-#define STATX__RESERVED 0x80000000U
-
-#define STATX_ATTR_COMPRESSED 0x0004
-#define STATX_ATTR_IMMUTABLE 0x0010
-#define STATX_ATTR_APPEND 0x0020
-#define STATX_ATTR_NODUMP 0x0040
-#define STATX_ATTR_ENCRYPTED 0x0800
-#define STATX_ATTR_AUTOMOUNT 0x1000
-
-__BEGIN_DECLS
-
-/* Fill *BUF with information about PATH in DIRFD.  */
-int statx (int __dirfd, const char *__restrict __path, int __flags,
-           unsigned int __mask, struct statx *__restrict __buf)
-  __THROW __nonnull ((2, 5));
-
-__END_DECLS
+/* Use the generic definitions.  */
+#include <bits/statx-generic.h>
diff --git a/sysdeps/unix/sysv/linux/bits/statx.h b/sysdeps/unix/sysv/linux/bits/statx.h
new file mode 100644
index 0000000000..df782a571d
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/bits/statx.h
@@ -0,0 +1,30 @@
+/* statx-related definitions and declarations.  Linux version.
+   Copyright (C) 2018-2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* This interface is based on <linux/stat.h> in Linux.  */
+
+#ifndef _SYS_STAT_H
+# error Never include <bits/statx.h> directly, include <sys/stat.h> instead.
+#endif
+
+/* Use the Linux kernel header if available.  */
+#if __glibc_has_include (<linux/stat.h>)
+# include <linux/stat.h>
+#endif
+
+#include <bits/statx-generic.h>
diff --git a/sysdeps/unix/sysv/linux/statx.c b/sysdeps/unix/sysv/linux/statx.c
index b99e30dc3e..5d634960a6 100644
--- a/sysdeps/unix/sysv/linux/statx.c
+++ b/sysdeps/unix/sysv/linux/statx.c
@@ -21,6 +21,11 @@
 
 #include "statx_generic.c"
 
+/* Ensure that the kernel headers have not changed the struct size.
+   If this ever happens, it may be necessary to introduce a new symbol
+   version.  */
+_Static_assert (sizeof (struct statx) == 256, "struct statx size changed");
+
 int
 statx (int fd, const char *path, int flags,
        unsigned int mask, struct statx *buf)
Andreas Schwab June 6, 2019, 4:05 p.m. | #4
On Jun 06 2019, Florian Weimer <fweimer@redhat.com> wrote:

> diff --git a/sysdeps/unix/sysv/linux/statx.c b/sysdeps/unix/sysv/linux/statx.c

> index b99e30dc3e..5d634960a6 100644

> --- a/sysdeps/unix/sysv/linux/statx.c

> +++ b/sysdeps/unix/sysv/linux/statx.c

> @@ -21,6 +21,11 @@

>  

>  #include "statx_generic.c"

>  

> +/* Ensure that the kernel headers have not changed the struct size.

> +   If this ever happens, it may be necessary to introduce a new symbol

> +   version.  */

> +_Static_assert (sizeof (struct statx) == 256, "struct statx size changed");


Why is that needed?  Isn't the point of statx that only the known parts
are filled?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer June 6, 2019, 4:15 p.m. | #5
* Andreas Schwab:

> On Jun 06 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> diff --git a/sysdeps/unix/sysv/linux/statx.c b/sysdeps/unix/sysv/linux/statx.c

>> index b99e30dc3e..5d634960a6 100644

>> --- a/sysdeps/unix/sysv/linux/statx.c

>> +++ b/sysdeps/unix/sysv/linux/statx.c

>> @@ -21,6 +21,11 @@

>>  

>>  #include "statx_generic.c"

>>  

>> +/* Ensure that the kernel headers have not changed the struct size.

>> +   If this ever happens, it may be necessary to introduce a new symbol

>> +   version.  */

>> +_Static_assert (sizeof (struct statx) == 256, "struct statx size changed");

>

> Why is that needed?  Isn't the point of statx that only the known parts

> are filled?


There have been some discussions that future kernel versions might add
additional fields to the struct, outside the padding.  Userspace would
request that by setting an additional flag.

I'm not sure if this will ever happen, but I thin it's reasonable to
check using this flag.

Thanks,
Florian
Andreas Schwab June 11, 2019, 8:37 a.m. | #6
On Jun 06 2019, Florian Weimer <fweimer@redhat.com> wrote:

> * Andreas Schwab:

>

>> On Jun 06 2019, Florian Weimer <fweimer@redhat.com> wrote:

>>

>>> diff --git a/sysdeps/unix/sysv/linux/statx.c b/sysdeps/unix/sysv/linux/statx.c

>>> index b99e30dc3e..5d634960a6 100644

>>> --- a/sysdeps/unix/sysv/linux/statx.c

>>> +++ b/sysdeps/unix/sysv/linux/statx.c

>>> @@ -21,6 +21,11 @@

>>>  

>>>  #include "statx_generic.c"

>>>  

>>> +/* Ensure that the kernel headers have not changed the struct size.

>>> +   If this ever happens, it may be necessary to introduce a new symbol

>>> +   version.  */

>>> +_Static_assert (sizeof (struct statx) == 256, "struct statx size changed");

>>

>> Why is that needed?  Isn't the point of statx that only the known parts

>> are filled?

>

> There have been some discussions that future kernel versions might add

> additional fields to the struct, outside the padding.  Userspace would

> request that by setting an additional flag.


And why is that a problem?  Either the kernel knows about the flag and
happily fills the structure, or it will return an error.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer June 11, 2019, 8:53 a.m. | #7
* Andreas Schwab:

> On Jun 06 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> * Andreas Schwab:

>>

>>> On Jun 06 2019, Florian Weimer <fweimer@redhat.com> wrote:

>>>

>>>> diff --git a/sysdeps/unix/sysv/linux/statx.c b/sysdeps/unix/sysv/linux/statx.c

>>>> index b99e30dc3e..5d634960a6 100644

>>>> --- a/sysdeps/unix/sysv/linux/statx.c

>>>> +++ b/sysdeps/unix/sysv/linux/statx.c

>>>> @@ -21,6 +21,11 @@

>>>>  

>>>>  #include "statx_generic.c"

>>>>  

>>>> +/* Ensure that the kernel headers have not changed the struct size.

>>>> +   If this ever happens, it may be necessary to introduce a new symbol

>>>> +   version.  */

>>>> +_Static_assert (sizeof (struct statx) == 256, "struct statx size changed");

>>>

>>> Why is that needed?  Isn't the point of statx that only the known parts

>>> are filled?

>>

>> There have been some discussions that future kernel versions might add

>> additional fields to the struct, outside the padding.  Userspace would

>> request that by setting an additional flag.

>

> And why is that a problem?  Either the kernel knows about the flag and

> happily fills the structure, or it will return an error.


The emulation code in io/statx_generic.c does not know about the new
structure and will initialize the full struct statx object, even if the
new flag is not specified.

Thanks,
Florian
Andreas Schwab June 11, 2019, 9:05 a.m. | #8
On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

> The emulation code in io/statx_generic.c does not know about the new

> structure and will initialize the full struct statx object, even if the

> new flag is not specified.


That looks like a bug, it should only touch the parts related to
STATX_BASIC_STATS.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer June 11, 2019, 9:13 a.m. | #9
* Andreas Schwab:

> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> The emulation code in io/statx_generic.c does not know about the new

>> structure and will initialize the full struct statx object, even if the

>> new flag is not specified.

>

> That looks like a bug, it should only touch the parts related to

> STATX_BASIC_STATS.


This is not how the interface is defined.  It is expected to clear the
entire struct (all 256 bytes).

We can either duplicate the struct definition (like we did before this
change), or add the assert, so that we are told once the duplication
becomes necessary.  I slightly prefer the second approach.

Maybe we could rewrite the code in io/statx_generic.c to avoid the
compound literal, so that we only initialize the first 256 bytes of the
struct, regardless of its definition.  I do not think we have another
dependency on the struct size in the glibc sources that becomes
problematic if the kernel enlarges its definition.

Thanks,
Florian
Andreas Schwab June 11, 2019, 9:23 a.m. | #10
On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

> We can either duplicate the struct definition (like we did before this

> change), or add the assert, so that we are told once the duplication

> becomes necessary.  I slightly prefer the second approach.


Duplication looks like the cleanest approach, since the fallback never
wants to use a different one.  It will use the original definition
forever.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer June 11, 2019, 9:41 a.m. | #11
* Andreas Schwab:

> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> We can either duplicate the struct definition (like we did before this

>> change), or add the assert, so that we are told once the duplication

>> becomes necessary.  I slightly prefer the second approach.

>

> Duplication looks like the cleanest approach, since the fallback never

> wants to use a different one.  It will use the original definition

> forever.


Ideally, we would still write a test that the duplicate and the kernel
version match, and that is kind of hard.

I will come up with a patch with the duplication and remove the asssert.
However, I do not really like it.  At this point, we don't know if we
will ever actually need the duplicate.

Thanks,
Florian
Andreas Schwab June 11, 2019, 9:51 a.m. | #12
On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

> Ideally, we would still write a test that the duplicate and the kernel

> version match, and that is kind of hard.


If that's ever an issue, it's a kernel issue, not a glibc issue.  The
point of the statx interface is that it stays compatible without any
need for versioning.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Andreas Schwab June 11, 2019, 10:01 a.m. | #13
On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

> I will come up with a patch with the duplication and remove the asssert.

> However, I do not really like it.  At this point, we don't know if we

> will ever actually need the duplicate.


We already have that duplicate anyway, as we need it for pre-statx
kernels.  But for those kernels, struct statx never changes.  If the
kernel handles statx, we don't care about the size.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer June 11, 2019, 10:04 a.m. | #14
* Andreas Schwab:

> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> I will come up with a patch with the duplication and remove the asssert.

>> However, I do not really like it.  At this point, we don't know if we

>> will ever actually need the duplicate.

>

> We already have that duplicate anyway, as we need it for pre-statx

> kernels.  But for those kernels, struct statx never changes.  If the

> kernel handles statx, we don't care about the size.


So should we write statx_generic in such a way that it does not depend
on the size of struct stax?  That is, get rid of the compund literal?

Thanks,
Florian
Andreas Schwab June 11, 2019, 10:36 a.m. | #15
On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

> So should we write statx_generic in such a way that it does not depend

> on the size of struct stax?


The fallback uses the original definition, and must never write beyond
that.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer June 11, 2019, 10:46 a.m. | #16
* Andreas Schwab:

> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> So should we write statx_generic in such a way that it does not depend

>> on the size of struct stax?

>

> The fallback uses the original definition, and must never write beyond

> that.


The question is whether we achieve this with a separate struct type, or
by changing it so that it only writes to all the fields in the original
definition (i.e., remove the struct assigned and write to all members
explicitly, including those that need to be zeroed).

Thanks,
Florian
Andreas Schwab June 11, 2019, 11:54 a.m. | #17
On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

> * Andreas Schwab:

>

>> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>>

>>> So should we write statx_generic in such a way that it does not depend

>>> on the size of struct stax?

>>

>> The fallback uses the original definition, and must never write beyond

>> that.

>

> The question is whether we achieve this with a separate struct type, or

> by changing it so that it only writes to all the fields in the original

> definition (i.e., remove the struct assigned and write to all members

> explicitly, including those that need to be zeroed).


Since we already have that separate type, using that would be the
cleanest.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer June 11, 2019, 11:57 a.m. | #18
* Andreas Schwab:

> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> * Andreas Schwab:

>>

>>> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>>>

>>>> So should we write statx_generic in such a way that it does not depend

>>>> on the size of struct stax?

>>>

>>> The fallback uses the original definition, and must never write beyond

>>> that.

>>

>> The question is whether we achieve this with a separate struct type, or

>> by changing it so that it only writes to all the fields in the original

>> definition (i.e., remove the struct assigned and write to all members

>> explicitly, including those that need to be zeroed).

>

> Since we already have that separate type, using that would be the

> cleanest.


This will need preprocessor hacks.  I don't think there is another way
to rename a struct tag.  It also doesn't help that the tag is the same
as a function name.

Do we really want to move in this direction?

Thanks,
Florian
Andreas Schwab June 11, 2019, 12:20 p.m. | #19
On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

> This will need preprocessor hacks.


Does it?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer June 11, 2019, 12:30 p.m. | #20
* Andreas Schwab:

> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> This will need preprocessor hacks.

>

> Does it?


How else can we implement this?  We need to include both <linux/stat.h>
and <io/bits/statx-generic.h>, and as written, the two conflict.  And
outside of glibc, <io/bits/statx-generic.h> must define struct statx.

So we would have to add a macro that allows us to change the tag name of
the definition in <io/bits/statx-generic.h>, I think.

Alternative, we could compile statx_generic separately and directly
include <io/bits/statx-generic.h>, bypassing the sysdeps search path.
But that means we cannot include <sys/stat.h>, but we need it for the
AT_* constants.

In the meantime, I've looked at restricting the fields we write to in
statx_generic.  We need to write to what is padding today, due to the
way future extensions have been defined.  But if the kernel turns
padding into actual struct members, the code will fail to compile
because the member names are no longer correct.  If we want to avoid
that, we would have to zero-initialize *buf with a 256-byte memset, and
write only to the non-zero members with actual data.

I think we should duplicate the current definition in statx_generic, or
go with original approach (with the _Static_assert).

Thanks,
Florian
Andreas Schwab June 11, 2019, 12:47 p.m. | #21
On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

> Alternative, we could compile statx_generic separately and directly

> include <io/bits/statx-generic.h>, bypassing the sysdeps search path.

> But that means we cannot include <sys/stat.h>, but we need it for the

> AT_* constants.


If struct statx is put in a separate file it can be included twice, with
arrangment for defining, say, struct statx_internal the second time.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer June 11, 2019, 12:51 p.m. | #22
* Andreas Schwab:

> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> Alternative, we could compile statx_generic separately and directly

>> include <io/bits/statx-generic.h>, bypassing the sysdeps search path.

>> But that means we cannot include <sys/stat.h>, but we need it for the

>> AT_* constants.

>

> If struct statx is put in a separate file it can be included twice, with

> arrangment for defining, say, struct statx_internal the second time.


You mean like this?

#include <sys/stat.h>
/* Change the tag of the struct definition.  */
#define statx statx_original
#include <bits/types/struct_statx.h>
#undef statx

I still consider this a preprocessor hack.  I would prefer the static
assert to this.

Thanks,
Florian
Andreas Schwab June 11, 2019, 1:08 p.m. | #23
On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

> * Andreas Schwab:

>

>> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>>

>>> Alternative, we could compile statx_generic separately and directly

>>> include <io/bits/statx-generic.h>, bypassing the sysdeps search path.

>>> But that means we cannot include <sys/stat.h>, but we need it for the

>>> AT_* constants.

>>

>> If struct statx is put in a separate file it can be included twice, with

>> arrangment for defining, say, struct statx_internal the second time.

>

> You mean like this?

>

> #include <sys/stat.h>

> /* Change the tag of the struct definition.  */

> #define statx statx_original

> #include <bits/types/struct_statx.h>

> #undef statx

>

> I still consider this a preprocessor hack.


It is only needed as long as the fallback is needed.

> I would prefer the static assert to this.


In any case, the assertion is in the wrong place, since only the
fallback needs it.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer June 11, 2019, 1:16 p.m. | #24
* Andreas Schwab:

> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> * Andreas Schwab:

>>

>>> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>>>

>>>> Alternative, we could compile statx_generic separately and directly

>>>> include <io/bits/statx-generic.h>, bypassing the sysdeps search path.

>>>> But that means we cannot include <sys/stat.h>, but we need it for the

>>>> AT_* constants.

>>>

>>> If struct statx is put in a separate file it can be included twice, with

>>> arrangment for defining, say, struct statx_internal the second time.

>>

>> You mean like this?

>>

>> #include <sys/stat.h>

>> /* Change the tag of the struct definition.  */

>> #define statx statx_original

>> #include <bits/types/struct_statx.h>

>> #undef statx

>>

>> I still consider this a preprocessor hack.

>

> It is only needed as long as the fallback is needed.


Sure, except that the fallback is also used on Hurd, so I assume that
code is here to stay.

>> I would prefer the static assert to this.

>

> In any case, the assertion is in the wrong place, since only the

> fallback needs it.


Only on Linux, we potentially compile against an out-of-tree definition
of struct statx, so this is why I added the assert there.

I can see arguments for adding it to either file.

If you want, I can repost the patch with the assert, this time moved to
io/statx_generic.c.

Thanks,
Florian
Andreas Schwab June 11, 2019, 1:40 p.m. | #25
On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

> * Andreas Schwab:

>

>> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>>

>>> * Andreas Schwab:

>>>

>>>> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>>>>

>>>>> Alternative, we could compile statx_generic separately and directly

>>>>> include <io/bits/statx-generic.h>, bypassing the sysdeps search path.

>>>>> But that means we cannot include <sys/stat.h>, but we need it for the

>>>>> AT_* constants.

>>>>

>>>> If struct statx is put in a separate file it can be included twice, with

>>>> arrangment for defining, say, struct statx_internal the second time.

>>>

>>> You mean like this?

>>>

>>> #include <sys/stat.h>

>>> /* Change the tag of the struct definition.  */

>>> #define statx statx_original

>>> #include <bits/types/struct_statx.h>

>>> #undef statx

>>>

>>> I still consider this a preprocessor hack.

>>

>> It is only needed as long as the fallback is needed.

>

> Sure, except that the fallback is also used on Hurd, so I assume that

> code is here to stay.


When it is no longer needed on Linux it can be simplified anyway.

>>> I would prefer the static assert to this.

>>

>> In any case, the assertion is in the wrong place, since only the

>> fallback needs it.

>

> Only on Linux, we potentially compile against an out-of-tree definition

> of struct statx, so this is why I added the assert there.


But it's the Linux version that doesn't really care about the size.

statx_generic is really operating on its own, non-changing definition of
struct statx, and I think that should be made explicit.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer June 11, 2019, 1:43 p.m. | #26
* Andreas Schwab:

>>>> I would prefer the static assert to this.

>>>

>>> In any case, the assertion is in the wrong place, since only the

>>> fallback needs it.

>>

>> Only on Linux, we potentially compile against an out-of-tree definition

>> of struct statx, so this is why I added the assert there.

>

> But it's the Linux version that doesn't really care about the size.

>

> statx_generic is really operating on its own, non-changing definition of

> struct statx, and I think that should be made explicit.


So should I move the _Static_assert to statx_generic?  It's not clear
what you want me to do.

I can implement the preprocessor hack as well if you want me to.

Thanks,
Florian
Andreas Schwab June 11, 2019, 1:58 p.m. | #27
On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

> I can implement the preprocessor hack as well if you want me to.


Yes, that's what I meant with non-changing.  statx_generic is using a
fixed subset of statx and thus should only use the fallback definition
of struct statx.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer June 11, 2019, 5:36 p.m. | #28
* Andreas Schwab:

> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> I can implement the preprocessor hack as well if you want me to.

>

> Yes, that's what I meant with non-changing.  statx_generic is using a

> fixed subset of statx and thus should only use the fallback definition

> of struct statx.


The patch below seems to work.  I'm still re-testing it with old kernel
headers.

Thanks,
Florian

<sys/stat.h>: Use Linux kernel UAPI header if available and useful

This will automatically import new STATX_* constants.  It also avoids
a conflict between <sys/stat.h> and <linux/stat.h>.

2019-06-11  Florian Weimer  <fweimer@redhat.com>

	Linux: Use kernel headers for statx definitions if available.
	* include/bits/statx-generic.h: New file.
	* include/bits/types/struct_statx.h: Likewise.
	* include/bits/types/struct_statx_timestamp.h: Likewise.
	* io/Makefile (headers): Add bits/statx-generic.h.
	* io/bits/statx-generic.h: New file.  Partly copied from
	io/bits/statx.h.
	* io/statx_generic.c: Include <bits/types/struct_statx.h> to
	define original_statx.
	* io/bits/types/struct_statx.h: Likewise.
	* io/bits/types/struct_statx_timestamp.h: Likewise.
	(statx_generic): Use original_statx.
	* io/bits/statx.h: Rewrite to include <bits/statx-generic.h>.
	* sysdeps/unix/sysv/linux/bits/statx.h: New file.

diff --git a/include/bits/statx-generic.h b/include/bits/statx-generic.h
new file mode 100644
index 0000000000..21674721b6
--- /dev/null
+++ b/include/bits/statx-generic.h
@@ -0,0 +1 @@
+#include <io/bits/statx-generic.h>
diff --git a/include/bits/types/struct_statx.h b/include/bits/types/struct_statx.h
new file mode 100644
index 0000000000..82add6484f
--- /dev/null
+++ b/include/bits/types/struct_statx.h
@@ -0,0 +1 @@
+#include <io/bits/types/struct_statx.h>
diff --git a/include/bits/types/struct_statx_timestamp.h b/include/bits/types/struct_statx_timestamp.h
new file mode 100644
index 0000000000..9fbedd5749
--- /dev/null
+++ b/include/bits/types/struct_statx_timestamp.h
@@ -0,0 +1 @@
+#include <io/bits/types/struct_statx_timestamp.h>
diff --git a/io/Makefile b/io/Makefile
index f2404db041..088e86da77 100644
--- a/io/Makefile
+++ b/io/Makefile
@@ -25,7 +25,9 @@ include ../Makeconfig
 headers := sys/stat.h bits/stat.h sys/statfs.h bits/statfs.h sys/vfs.h \
 	   sys/statvfs.h bits/statvfs.h fcntl.h sys/fcntl.h bits/fcntl.h \
 	   poll.h sys/poll.h bits/poll.h bits/fcntl2.h bits/poll2.h \
-	   bits/statx.h utime.h ftw.h fts.h sys/sendfile.h
+	   bits/statx.h bits/statx-generic.h bits/types/struct_statx.h \
+	   bits/types/struct_statx_timestamp.h \
+	   utime.h ftw.h fts.h sys/sendfile.h
 
 routines :=								\
 	utime								\
diff --git a/io/bits/statx-generic.h b/io/bits/statx-generic.h
new file mode 100644
index 0000000000..1f5abbf148
--- /dev/null
+++ b/io/bits/statx-generic.h
@@ -0,0 +1,60 @@
+/* Generic statx-related definitions and declarations.
+   Copyright (C) 2018-2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* This interface is based on <linux/stat.h> in Linux.  */
+
+#ifndef _SYS_STAT_H
+# error Never include <bits/statx-generic.h> directly, include <sys/stat.h> instead.
+#endif
+
+#include <bits/types/struct_statx_timestamp.h>
+#include <bits/types/struct_statx.h>
+
+#ifndef STATX_TYPE
+# define STATX_TYPE 0x0001U
+# define STATX_MODE 0x0002U
+# define STATX_NLINK 0x0004U
+# define STATX_UID 0x0008U
+# define STATX_GID 0x0010U
+# define STATX_ATIME 0x0020U
+# define STATX_MTIME 0x0040U
+# define STATX_CTIME 0x0080U
+# define STATX_INO 0x0100U
+# define STATX_SIZE 0x0200U
+# define STATX_BLOCKS 0x0400U
+# define STATX_BASIC_STATS 0x07ffU
+# define STATX_ALL 0x0fffU
+# define STATX_BTIME 0x0800U
+# define STATX__RESERVED 0x80000000U
+
+# define STATX_ATTR_COMPRESSED 0x0004
+# define STATX_ATTR_IMMUTABLE 0x0010
+# define STATX_ATTR_APPEND 0x0020
+# define STATX_ATTR_NODUMP 0x0040
+# define STATX_ATTR_ENCRYPTED 0x0800
+# define STATX_ATTR_AUTOMOUNT 0x1000
+#endif /* !STATX_TYPE */
+
+__BEGIN_DECLS
+
+/* Fill *BUF with information about PATH in DIRFD.  */
+int statx (int __dirfd, const char *__restrict __path, int __flags,
+           unsigned int __mask, struct statx *__restrict __buf)
+  __THROW __nonnull ((2, 5));
+
+__END_DECLS
diff --git a/io/bits/statx.h b/io/bits/statx.h
index cff14b2543..b3147bfa8a 100644
--- a/io/bits/statx.h
+++ b/io/bits/statx.h
@@ -1,4 +1,4 @@
-/* statx-related definitions and declarations.
+/* statx-related definitions and declarations.  Generic version.
    Copyright (C) 2018-2019 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -19,73 +19,8 @@
 /* This interface is based on <linux/stat.h> in Linux.  */
 
 #ifndef _SYS_STAT_H
-# error Never include <bits/stat.x.h> directly, include <sys/stat.h> instead.
+# error Never include <bits/statx.h> directly, include <sys/stat.h> instead.
 #endif
 
-struct statx_timestamp
-{
-  __int64_t tv_sec;
-  __uint32_t tv_nsec;
-  __int32_t __statx_timestamp_pad1[1];
-};
-
-/* Warning: The kernel may add additional fields to this struct in the
-   future.  Only use this struct for calling the statx function, not
-   for storing data.  (Expansion will be controlled by the mask
-   argument of the statx function.)  */
-struct statx
-{
-  __uint32_t stx_mask;
-  __uint32_t stx_blksize;
-  __uint64_t stx_attributes;
-  __uint32_t stx_nlink;
-  __uint32_t stx_uid;
-  __uint32_t stx_gid;
-  __uint16_t stx_mode;
-  __uint16_t __statx_pad1[1];
-  __uint64_t stx_ino;
-  __uint64_t stx_size;
-  __uint64_t stx_blocks;
-  __uint64_t stx_attributes_mask;
-  struct statx_timestamp stx_atime;
-  struct statx_timestamp stx_btime;
-  struct statx_timestamp stx_ctime;
-  struct statx_timestamp stx_mtime;
-  __uint32_t stx_rdev_major;
-  __uint32_t stx_rdev_minor;
-  __uint32_t stx_dev_major;
-  __uint32_t stx_dev_minor;
-  __uint64_t __statx_pad2[14];
-};
-
-#define STATX_TYPE 0x0001U
-#define STATX_MODE 0x0002U
-#define STATX_NLINK 0x0004U
-#define STATX_UID 0x0008U
-#define STATX_GID 0x0010U
-#define STATX_ATIME 0x0020U
-#define STATX_MTIME 0x0040U
-#define STATX_CTIME 0x0080U
-#define STATX_INO 0x0100U
-#define STATX_SIZE 0x0200U
-#define STATX_BLOCKS 0x0400U
-#define STATX_BASIC_STATS 0x07ffU
-#define STATX_ALL 0x0fffU
-#define STATX_BTIME 0x0800U
-#define STATX__RESERVED 0x80000000U
-
-#define STATX_ATTR_COMPRESSED 0x0004
-#define STATX_ATTR_IMMUTABLE 0x0010
-#define STATX_ATTR_APPEND 0x0020
-#define STATX_ATTR_NODUMP 0x0040
-#define STATX_ATTR_ENCRYPTED 0x0800
-#define STATX_ATTR_AUTOMOUNT 0x1000
-
-__BEGIN_DECLS
-
-/* Fill *BUF with information about PATH in DIRFD.  */
-int statx (int __dirfd, const char *__restrict __path, int __flags,
-           unsigned int __mask, struct statx *__restrict __buf)
-  __THROW __nonnull ((2, 5));
-
-__END_DECLS
+/* Use the generic definitions.  */
+#include <bits/statx-generic.h>
diff --git a/io/bits/types/struct_statx.h b/io/bits/types/struct_statx.h
new file mode 100644
index 0000000000..4f3ae3ece6
--- /dev/null
+++ b/io/bits/types/struct_statx.h
@@ -0,0 +1,55 @@
+/* Definition of the generic version of struct statx.
+   Copyright (C) 2018-2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _SYS_STAT_H
+# error Never include <bits/types/struct_statx.h> directly, include <sys/stat.h> instead.
+#endif
+
+#ifndef __statx_defined
+#define __statx_defined 1
+
+/* Warning: The kernel may add additional fields to this struct in the
+   future.  Only use this struct for calling the statx function, not
+   for storing data.  (Expansion will be controlled by the mask
+   argument of the statx function.)  */
+struct statx
+{
+  __uint32_t stx_mask;
+  __uint32_t stx_blksize;
+  __uint64_t stx_attributes;
+  __uint32_t stx_nlink;
+  __uint32_t stx_uid;
+  __uint32_t stx_gid;
+  __uint16_t stx_mode;
+  __uint16_t __statx_pad1[1];
+  __uint64_t stx_ino;
+  __uint64_t stx_size;
+  __uint64_t stx_blocks;
+  __uint64_t stx_attributes_mask;
+  struct statx_timestamp stx_atime;
+  struct statx_timestamp stx_btime;
+  struct statx_timestamp stx_ctime;
+  struct statx_timestamp stx_mtime;
+  __uint32_t stx_rdev_major;
+  __uint32_t stx_rdev_minor;
+  __uint32_t stx_dev_major;
+  __uint32_t stx_dev_minor;
+  __uint64_t __statx_pad2[14];
+};
+
+#endif /* __statx_defined */
diff --git a/io/bits/types/struct_statx_timestamp.h b/io/bits/types/struct_statx_timestamp.h
new file mode 100644
index 0000000000..0f104ef84e
--- /dev/null
+++ b/io/bits/types/struct_statx_timestamp.h
@@ -0,0 +1,33 @@
+/* Definition of the generic version of struct statx_timestamp.
+   Copyright (C) 2018-2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _SYS_STAT_H
+# error Never include <bits/types/struct_statx_timestamp.h> directly, include <sys/stat.h> instead.
+#endif
+
+#ifndef __statx_timestamp_defined
+#define __statx_timestamp_defined 1
+
+struct statx_timestamp
+{
+  __int64_t tv_sec;
+  __uint32_t tv_nsec;
+  __int32_t __statx_timestamp_pad1[1];
+};
+
+#endif /* __statx_timestamp_defined */
diff --git a/io/statx_generic.c b/io/statx_generic.c
index 10225ef5cb..ddc4097249 100644
--- a/io/statx_generic.c
+++ b/io/statx_generic.c
@@ -18,9 +18,16 @@
 
 #include <errno.h>
 #include <fcntl.h>
+#include <string.h>
 #include <sys/stat.h>
 #include <sys/sysmacros.h>
 
+/* Obtain the original definition of struct statx.  */
+#undef __statx_defined
+#define statx original_statx
+#include <bits/types/struct_statx.h>
+#undef statx
+
 static inline struct statx_timestamp
 statx_convert_timestamp (struct timespec tv)
 {
@@ -57,7 +64,7 @@ statx_generic (int fd, const char *path, int flags,
   /* The interface is defined in such a way that unused (padding)
      fields have to be cleared.  STATX_BASIC_STATS corresponds to the
      data which is available via fstatat64.  */
-  *buf = (struct statx)
+  struct original_statx obuf =
     {
       .stx_mask = STATX_BASIC_STATS,
       .stx_blksize = st.st_blksize,
@@ -76,6 +83,8 @@ statx_generic (int fd, const char *path, int flags,
       .stx_dev_major = major (st.st_dev),
       .stx_dev_minor = minor (st.st_dev),
     };
+  _Static_assert (sizeof (*buf) >= sizeof (obuf), "struct statx size");
+  memcpy (buf, &obuf, sizeof (obuf));
 
   return 0;
 }
diff --git a/sysdeps/unix/sysv/linux/bits/statx.h b/sysdeps/unix/sysv/linux/bits/statx.h
new file mode 100644
index 0000000000..d36f44efc6
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/bits/statx.h
@@ -0,0 +1,34 @@
+/* statx-related definitions and declarations.  Linux version.
+   Copyright (C) 2018-2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* This interface is based on <linux/stat.h> in Linux.  */
+
+#ifndef _SYS_STAT_H
+# error Never include <bits/statx.h> directly, include <sys/stat.h> instead.
+#endif
+
+/* Use the Linux kernel header if available.  */
+#if __glibc_has_include (<linux/stat.h>)
+# include <linux/stat.h>
+# ifdef STATX_TYPE
+#  define __statx_timestamp_defined 1
+#  define __statx_defined 1
+# endif
+#endif
+
+#include <bits/statx-generic.h>
Carlos O'Donell June 11, 2019, 9:01 p.m. | #29
On 6/11/19 1:36 PM, Florian Weimer wrote:
> * Andreas Schwab:

> 

>> On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

>>

>>> I can implement the preprocessor hack as well if you want me to.

>>

>> Yes, that's what I meant with non-changing.  statx_generic is using a

>> fixed subset of statx and thus should only use the fallback definition

>> of struct statx.

> 

> The patch below seems to work.  I'm still re-testing it with old kernel

> headers.

> 

> Thanks,

> Florian

> 

> <sys/stat.h>: Use Linux kernel UAPI header if available and useful

> 

> This will automatically import new STATX_* constants.  It also avoids

> a conflict between <sys/stat.h> and <linux/stat.h>.

> 

> 2019-06-11  Florian Weimer  <fweimer@redhat.com>

> 

> 	Linux: Use kernel headers for statx definitions if available.

> 	* include/bits/statx-generic.h: New file.

> 	* include/bits/types/struct_statx.h: Likewise.

> 	* include/bits/types/struct_statx_timestamp.h: Likewise.

> 	* io/Makefile (headers): Add bits/statx-generic.h.

> 	* io/bits/statx-generic.h: New file.  Partly copied from

> 	io/bits/statx.h.

> 	* io/statx_generic.c: Include <bits/types/struct_statx.h> to

> 	define original_statx.

> 	* io/bits/types/struct_statx.h: Likewise.

> 	* io/bits/types/struct_statx_timestamp.h: Likewise.

> 	(statx_generic): Use original_statx.

> 	* io/bits/statx.h: Rewrite to include <bits/statx-generic.h>.

> 	* sysdeps/unix/sysv/linux/bits/statx.h: New file.


I'm OK with this for master. Andreas should also comment.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>


> diff --git a/include/bits/statx-generic.h b/include/bits/statx-generic.h

> new file mode 100644

> index 0000000000..21674721b6

> --- /dev/null

> +++ b/include/bits/statx-generic.h

> @@ -0,0 +1 @@

> +#include <io/bits/statx-generic.h>


OK.

> diff --git a/include/bits/types/struct_statx.h b/include/bits/types/struct_statx.h

> new file mode 100644

> index 0000000000..82add6484f

> --- /dev/null

> +++ b/include/bits/types/struct_statx.h

> @@ -0,0 +1 @@

> +#include <io/bits/types/struct_statx.h>


OK.

> diff --git a/include/bits/types/struct_statx_timestamp.h b/include/bits/types/struct_statx_timestamp.h

> new file mode 100644

> index 0000000000..9fbedd5749

> --- /dev/null

> +++ b/include/bits/types/struct_statx_timestamp.h

> @@ -0,0 +1 @@

> +#include <io/bits/types/struct_statx_timestamp.h>


OK.

> diff --git a/io/Makefile b/io/Makefile

> index f2404db041..088e86da77 100644

> --- a/io/Makefile

> +++ b/io/Makefile

> @@ -25,7 +25,9 @@ include ../Makeconfig

>  headers := sys/stat.h bits/stat.h sys/statfs.h bits/statfs.h sys/vfs.h \

>  	   sys/statvfs.h bits/statvfs.h fcntl.h sys/fcntl.h bits/fcntl.h \

>  	   poll.h sys/poll.h bits/poll.h bits/fcntl2.h bits/poll2.h \

> -	   bits/statx.h utime.h ftw.h fts.h sys/sendfile.h

> +	   bits/statx.h bits/statx-generic.h bits/types/struct_statx.h \

> +	   bits/types/struct_statx_timestamp.h \

> +	   utime.h ftw.h fts.h sys/sendfile.h


OK.

>  

>  routines :=								\

>  	utime								\

> diff --git a/io/bits/statx-generic.h b/io/bits/statx-generic.h

> new file mode 100644

> index 0000000000..1f5abbf148

> --- /dev/null

> +++ b/io/bits/statx-generic.h

> @@ -0,0 +1,60 @@

> +/* Generic statx-related definitions and declarations.

> +   Copyright (C) 2018-2019 Free Software Foundation, Inc.


s/2018-2019/2019/g

This looks like a new-from-scratch file even if it's based on the 
other stat.h header.

> +   This file is part of the GNU C Library.

> +

> +   The GNU C Library is free software; you can redistribute it and/or

> +   modify it under the terms of the GNU Lesser General Public

> +   License as published by the Free Software Foundation; either

> +   version 2.1 of the License, or (at your option) any later version.

> +

> +   The GNU C Library is distributed in the hope that it will be useful,

> +   but WITHOUT ANY WARRANTY; without even the implied warranty of

> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

> +   Lesser General Public License for more details.

> +

> +   You should have received a copy of the GNU Lesser General Public

> +   License along with the GNU C Library; if not, see

> +   <http://www.gnu.org/licenses/>.  */

> +

> +/* This interface is based on <linux/stat.h> in Linux.  */

> +

> +#ifndef _SYS_STAT_H

> +# error Never include <bits/statx-generic.h> directly, include <sys/stat.h> instead.

> +#endif

> +

> +#include <bits/types/struct_statx_timestamp.h>

> +#include <bits/types/struct_statx.h>


OK, include types.

> +

> +#ifndef STATX_TYPE

> +# define STATX_TYPE 0x0001U

> +# define STATX_MODE 0x0002U

> +# define STATX_NLINK 0x0004U

> +# define STATX_UID 0x0008U

> +# define STATX_GID 0x0010U

> +# define STATX_ATIME 0x0020U

> +# define STATX_MTIME 0x0040U

> +# define STATX_CTIME 0x0080U

> +# define STATX_INO 0x0100U

> +# define STATX_SIZE 0x0200U

> +# define STATX_BLOCKS 0x0400U

> +# define STATX_BASIC_STATS 0x07ffU

> +# define STATX_ALL 0x0fffU

> +# define STATX_BTIME 0x0800U

> +# define STATX__RESERVED 0x80000000U

> +

> +# define STATX_ATTR_COMPRESSED 0x0004

> +# define STATX_ATTR_IMMUTABLE 0x0010

> +# define STATX_ATTR_APPEND 0x0020

> +# define STATX_ATTR_NODUMP 0x0040

> +# define STATX_ATTR_ENCRYPTED 0x0800

> +# define STATX_ATTR_AUTOMOUNT 0x1000

> +#endif /* !STATX_TYPE */

> +

> +__BEGIN_DECLS

> +

> +/* Fill *BUF with information about PATH in DIRFD.  */

> +int statx (int __dirfd, const char *__restrict __path, int __flags,

> +           unsigned int __mask, struct statx *__restrict __buf)

> +  __THROW __nonnull ((2, 5));

> +

> +__END_DECLS


OK.

> diff --git a/io/bits/statx.h b/io/bits/statx.h

> index cff14b2543..b3147bfa8a 100644

> --- a/io/bits/statx.h

> +++ b/io/bits/statx.h

> @@ -1,4 +1,4 @@

> -/* statx-related definitions and declarations.

> +/* statx-related definitions and declarations.  Generic version.


OK.

>     Copyright (C) 2018-2019 Free Software Foundation, Inc.

>     This file is part of the GNU C Library.

>  

> @@ -19,73 +19,8 @@

>  /* This interface is based on <linux/stat.h> in Linux.  */

>  

>  #ifndef _SYS_STAT_H

> -# error Never include <bits/stat.x.h> directly, include <sys/stat.h> instead.

> +# error Never include <bits/statx.h> directly, include <sys/stat.h> instead.


OK.

>  #endif

>  

> -struct statx_timestamp

> -{

> -  __int64_t tv_sec;

> -  __uint32_t tv_nsec;

> -  __int32_t __statx_timestamp_pad1[1];

> -};

> -

> -/* Warning: The kernel may add additional fields to this struct in the

> -   future.  Only use this struct for calling the statx function, not

> -   for storing data.  (Expansion will be controlled by the mask

> -   argument of the statx function.)  */

> -struct statx

> -{

> -  __uint32_t stx_mask;

> -  __uint32_t stx_blksize;

> -  __uint64_t stx_attributes;

> -  __uint32_t stx_nlink;

> -  __uint32_t stx_uid;

> -  __uint32_t stx_gid;

> -  __uint16_t stx_mode;

> -  __uint16_t __statx_pad1[1];

> -  __uint64_t stx_ino;

> -  __uint64_t stx_size;

> -  __uint64_t stx_blocks;

> -  __uint64_t stx_attributes_mask;

> -  struct statx_timestamp stx_atime;

> -  struct statx_timestamp stx_btime;

> -  struct statx_timestamp stx_ctime;

> -  struct statx_timestamp stx_mtime;

> -  __uint32_t stx_rdev_major;

> -  __uint32_t stx_rdev_minor;

> -  __uint32_t stx_dev_major;

> -  __uint32_t stx_dev_minor;

> -  __uint64_t __statx_pad2[14];

> -};

> -

> -#define STATX_TYPE 0x0001U

> -#define STATX_MODE 0x0002U

> -#define STATX_NLINK 0x0004U

> -#define STATX_UID 0x0008U

> -#define STATX_GID 0x0010U

> -#define STATX_ATIME 0x0020U

> -#define STATX_MTIME 0x0040U

> -#define STATX_CTIME 0x0080U

> -#define STATX_INO 0x0100U

> -#define STATX_SIZE 0x0200U

> -#define STATX_BLOCKS 0x0400U

> -#define STATX_BASIC_STATS 0x07ffU

> -#define STATX_ALL 0x0fffU

> -#define STATX_BTIME 0x0800U

> -#define STATX__RESERVED 0x80000000U

> -

> -#define STATX_ATTR_COMPRESSED 0x0004

> -#define STATX_ATTR_IMMUTABLE 0x0010

> -#define STATX_ATTR_APPEND 0x0020

> -#define STATX_ATTR_NODUMP 0x0040

> -#define STATX_ATTR_ENCRYPTED 0x0800

> -#define STATX_ATTR_AUTOMOUNT 0x1000

> -

> -__BEGIN_DECLS

> -

> -/* Fill *BUF with information about PATH in DIRFD.  */

> -int statx (int __dirfd, const char *__restrict __path, int __flags,

> -           unsigned int __mask, struct statx *__restrict __buf)

> -  __THROW __nonnull ((2, 5));

> -

> -__END_DECLS

> +/* Use the generic definitions.  */

> +#include <bits/statx-generic.h>


OK.

> diff --git a/io/bits/types/struct_statx.h b/io/bits/types/struct_statx.h

> new file mode 100644

> index 0000000000..4f3ae3ece6

> --- /dev/null

> +++ b/io/bits/types/struct_statx.h

> @@ -0,0 +1,55 @@

> +/* Definition of the generic version of struct statx.

> +   Copyright (C) 2018-2019 Free Software Foundation, Inc.

> +   This file is part of the GNU C Library.

> +

> +   The GNU C Library is free software; you can redistribute it and/or

> +   modify it under the terms of the GNU Lesser General Public

> +   License as published by the Free Software Foundation; either

> +   version 2.1 of the License, or (at your option) any later version.

> +

> +   The GNU C Library is distributed in the hope that it will be useful,

> +   but WITHOUT ANY WARRANTY; without even the implied warranty of

> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

> +   Lesser General Public License for more details.

> +

> +   You should have received a copy of the GNU Lesser General Public

> +   License along with the GNU C Library; if not, see

> +   <http://www.gnu.org/licenses/>.  */

> +

> +#ifndef _SYS_STAT_H

> +# error Never include <bits/types/struct_statx.h> directly, include <sys/stat.h> instead.

> +#endif

> +

> +#ifndef __statx_defined

> +#define __statx_defined 1

> +

> +/* Warning: The kernel may add additional fields to this struct in the

> +   future.  Only use this struct for calling the statx function, not

> +   for storing data.  (Expansion will be controlled by the mask

> +   argument of the statx function.)  */

> +struct statx

> +{

> +  __uint32_t stx_mask;

> +  __uint32_t stx_blksize;

> +  __uint64_t stx_attributes;

> +  __uint32_t stx_nlink;

> +  __uint32_t stx_uid;

> +  __uint32_t stx_gid;

> +  __uint16_t stx_mode;

> +  __uint16_t __statx_pad1[1];

> +  __uint64_t stx_ino;

> +  __uint64_t stx_size;

> +  __uint64_t stx_blocks;

> +  __uint64_t stx_attributes_mask;

> +  struct statx_timestamp stx_atime;

> +  struct statx_timestamp stx_btime;

> +  struct statx_timestamp stx_ctime;

> +  struct statx_timestamp stx_mtime;

> +  __uint32_t stx_rdev_major;

> +  __uint32_t stx_rdev_minor;

> +  __uint32_t stx_dev_major;

> +  __uint32_t stx_dev_minor;

> +  __uint64_t __statx_pad2[14];

> +};


OK. Verified against linux 5.2-rc4 (d1fdb6d8f6a4109a4263176c84b899076a5f8008).

> +

> +#endif /* __statx_defined */

> diff --git a/io/bits/types/struct_statx_timestamp.h b/io/bits/types/struct_statx_timestamp.h

> new file mode 100644

> index 0000000000..0f104ef84e

> --- /dev/null

> +++ b/io/bits/types/struct_statx_timestamp.h

> @@ -0,0 +1,33 @@

> +/* Definition of the generic version of struct statx_timestamp.

> +   Copyright (C) 2018-2019 Free Software Foundation, Inc.

> +   This file is part of the GNU C Library.

> +

> +   The GNU C Library is free software; you can redistribute it and/or

> +   modify it under the terms of the GNU Lesser General Public

> +   License as published by the Free Software Foundation; either

> +   version 2.1 of the License, or (at your option) any later version.

> +

> +   The GNU C Library is distributed in the hope that it will be useful,

> +   but WITHOUT ANY WARRANTY; without even the implied warranty of

> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

> +   Lesser General Public License for more details.

> +

> +   You should have received a copy of the GNU Lesser General Public

> +   License along with the GNU C Library; if not, see

> +   <http://www.gnu.org/licenses/>.  */

> +

> +#ifndef _SYS_STAT_H

> +# error Never include <bits/types/struct_statx_timestamp.h> directly, include <sys/stat.h> instead.

> +#endif

> +

> +#ifndef __statx_timestamp_defined

> +#define __statx_timestamp_defined 1

> +

> +struct statx_timestamp

> +{

> +  __int64_t tv_sec;

> +  __uint32_t tv_nsec;

> +  __int32_t __statx_timestamp_pad1[1];

> +};


OK. Verified likewise.

> +

> +#endif /* __statx_timestamp_defined */

> diff --git a/io/statx_generic.c b/io/statx_generic.c

> index 10225ef5cb..ddc4097249 100644

> --- a/io/statx_generic.c

> +++ b/io/statx_generic.c

> @@ -18,9 +18,16 @@

>  

>  #include <errno.h>

>  #include <fcntl.h>

> +#include <string.h>

>  #include <sys/stat.h>

>  #include <sys/sysmacros.h>

>  

> +/* Obtain the original definition of struct statx.  */

> +#undef __statx_defined

> +#define statx original_statx

> +#include <bits/types/struct_statx.h>

> +#undef statx

> +

>  static inline struct statx_timestamp

>  statx_convert_timestamp (struct timespec tv)

>  {

> @@ -57,7 +64,7 @@ statx_generic (int fd, const char *path, int flags,

>    /* The interface is defined in such a way that unused (padding)

>       fields have to be cleared.  STATX_BASIC_STATS corresponds to the

>       data which is available via fstatat64.  */

> -  *buf = (struct statx)

> +  struct original_statx obuf =

>      {

>        .stx_mask = STATX_BASIC_STATS,

>        .stx_blksize = st.st_blksize,

> @@ -76,6 +83,8 @@ statx_generic (int fd, const char *path, int flags,

>        .stx_dev_major = major (st.st_dev),

>        .stx_dev_minor = minor (st.st_dev),

>      };

> +  _Static_assert (sizeof (*buf) >= sizeof (obuf), "struct statx size");

> +  memcpy (buf, &obuf, sizeof (obuf));


OK.

>  

>    return 0;

>  }

> diff --git a/sysdeps/unix/sysv/linux/bits/statx.h b/sysdeps/unix/sysv/linux/bits/statx.h

> new file mode 100644

> index 0000000000..d36f44efc6

> --- /dev/null

> +++ b/sysdeps/unix/sysv/linux/bits/statx.h

> @@ -0,0 +1,34 @@

> +/* statx-related definitions and declarations.  Linux version.

> +   Copyright (C) 2018-2019 Free Software Foundation, Inc.

> +   This file is part of the GNU C Library.

> +

> +   The GNU C Library is free software; you can redistribute it and/or

> +   modify it under the terms of the GNU Lesser General Public

> +   License as published by the Free Software Foundation; either

> +   version 2.1 of the License, or (at your option) any later version.

> +

> +   The GNU C Library is distributed in the hope that it will be useful,

> +   but WITHOUT ANY WARRANTY; without even the implied warranty of

> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU

> +   Lesser General Public License for more details.

> +

> +   You should have received a copy of the GNU Lesser General Public

> +   License along with the GNU C Library; if not, see

> +   <http://www.gnu.org/licenses/>.  */

> +

> +/* This interface is based on <linux/stat.h> in Linux.  */

> +

> +#ifndef _SYS_STAT_H

> +# error Never include <bits/statx.h> directly, include <sys/stat.h> instead.

> +#endif

> +

> +/* Use the Linux kernel header if available.  */

> +#if __glibc_has_include (<linux/stat.h>)


So if __has_include is not defined this is all #if 0, and removed.

So in the use case that the compiler is new enough:

- If sys/stat.h is included first we check to see if linux/stat.h file is
  includeable and then do so, and avoid including our own definitions.

- If linux/stat.h is included first we again detect it is includeable,
  include it again (idempotent), and avoid our own definitions.

So we reduce the problem to one of "always try to include the kernel
defines first."

Works for me.

> +# include <linux/stat.h>

> +# ifdef STATX_TYPE

> +#  define __statx_timestamp_defined 1

> +#  define __statx_defined 1

> +# endif

> +#endif

> +

> +#include <bits/statx-generic.h>


OK.

> 



-- 
Cheers,
Carlos.
Florian Weimer June 11, 2019, 9:06 p.m. | #30
* Carlos O'Donell:

>> diff --git a/io/bits/statx-generic.h b/io/bits/statx-generic.h

>> new file mode 100644

>> index 0000000000..1f5abbf148

>> --- /dev/null

>> +++ b/io/bits/statx-generic.h

>> @@ -0,0 +1,60 @@

>> +/* Generic statx-related definitions and declarations.

>> +   Copyright (C) 2018-2019 Free Software Foundation, Inc.

>

> s/2018-2019/2019/g

> 

> This looks like a new-from-scratch file even if it's based on the 

> other stat.h header.


I disagree, it contains more than twenty lines from the old file, not
counting the copyright header.

> So if __has_include is not defined this is all #if 0, and removed.

>

> So in the use case that the compiler is new enough:

>

> - If sys/stat.h is included first we check to see if linux/stat.h file is

>   includeable and then do so, and avoid including our own definitions.

>

> - If linux/stat.h is included first we again detect it is includeable,

>   include it again (idempotent), and avoid our own definitions.


Note that we also need to cover the case where <linux/stat.h> exists,
but does not contain the statx definitions.  Hence the STATX_TYPE check
below.

>> +# include <linux/stat.h>

>> +# ifdef STATX_TYPE

>> +#  define __statx_timestamp_defined 1

>> +#  define __statx_defined 1

>> +# endif

>> +#endif

>> +

>> +#include <bits/statx-generic.h>


Thanks,
Florian
Carlos O'Donell June 11, 2019, 9:18 p.m. | #31
On 6/11/19 5:06 PM, Florian Weimer wrote:
> * Carlos O'Donell:

> 

>>> diff --git a/io/bits/statx-generic.h b/io/bits/statx-generic.h

>>> new file mode 100644

>>> index 0000000000..1f5abbf148

>>> --- /dev/null

>>> +++ b/io/bits/statx-generic.h

>>> @@ -0,0 +1,60 @@

>>> +/* Generic statx-related definitions and declarations.

>>> +   Copyright (C) 2018-2019 Free Software Foundation, Inc.

>>

>> s/2018-2019/2019/g

>>

>> This looks like a new-from-scratch file even if it's based on the 

>> other stat.h header.

> 

> I disagree, it contains more than twenty lines from the old file, not

> counting the copyright header.


OK, I hadn't looked that closely at this, but if it's more than 15
lines, then sure.

>> So if __has_include is not defined this is all #if 0, and removed.

>>

>> So in the use case that the compiler is new enough:

>>

>> - If sys/stat.h is included first we check to see if linux/stat.h file is

>>   includeable and then do so, and avoid including our own definitions.

>>

>> - If linux/stat.h is included first we again detect it is includeable,

>>   include it again (idempotent), and avoid our own definitions.

> 

> Note that we also need to cover the case where <linux/stat.h> exists,

> but does not contain the statx definitions.  Hence the STATX_TYPE check

> below.


I had not considered that case. Nice that you had STATX_TYPE to test for.

>>> +# include <linux/stat.h>

>>> +# ifdef STATX_TYPE

>>> +#  define __statx_timestamp_defined 1

>>> +#  define __statx_defined 1

>>> +# endif

>>> +#endif

>>> +

>>> +#include <bits/statx-generic.h>

> 

> Thanks,

> Florian

> 



-- 
Cheers,
Carlos.
Andreas Schwab June 12, 2019, 7:43 a.m. | #32
On Jun 11 2019, Florian Weimer <fweimer@redhat.com> wrote:

> 	Linux: Use kernel headers for statx definitions if available.

> 	* include/bits/statx-generic.h: New file.

> 	* include/bits/types/struct_statx.h: Likewise.

> 	* include/bits/types/struct_statx_timestamp.h: Likewise.

> 	* io/Makefile (headers): Add bits/statx-generic.h.

> 	* io/bits/statx-generic.h: New file.  Partly copied from

> 	io/bits/statx.h.

> 	* io/statx_generic.c: Include <bits/types/struct_statx.h> to

> 	define original_statx.

> 	* io/bits/types/struct_statx.h: Likewise.

> 	* io/bits/types/struct_statx_timestamp.h: Likewise.

> 	(statx_generic): Use original_statx.

> 	* io/bits/statx.h: Rewrite to include <bits/statx-generic.h>.

> 	* sysdeps/unix/sysv/linux/bits/statx.h: New file.


This looks good.

Thanks, Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Patch

diff --git a/io/bits/statx.h b/io/bits/statx.h
index cff14b2543..d0dcc353ba 100644
--- a/io/bits/statx.h
+++ b/io/bits/statx.h
@@ -19,9 +19,14 @@ 
 /* This interface is based on <linux/stat.h> in Linux.  */
 
 #ifndef _SYS_STAT_H
-# error Never include <bits/stat.x.h> directly, include <sys/stat.h> instead.
+# error Never include <bits/statx.h> directly, include <sys/stat.h> instead.
 #endif
 
+#if __glibc_has_include (<linux/stat.h>)
+# include <linux/stat.h>
+#endif
+
+#ifndef STATX_ALL
 struct statx_timestamp
 {
   __int64_t tv_sec;
@@ -58,28 +63,29 @@  struct statx
   __uint64_t __statx_pad2[14];
 };
 
-#define STATX_TYPE 0x0001U
-#define STATX_MODE 0x0002U
-#define STATX_NLINK 0x0004U
-#define STATX_UID 0x0008U
-#define STATX_GID 0x0010U
-#define STATX_ATIME 0x0020U
-#define STATX_MTIME 0x0040U
-#define STATX_CTIME 0x0080U
-#define STATX_INO 0x0100U
-#define STATX_SIZE 0x0200U
-#define STATX_BLOCKS 0x0400U
-#define STATX_BASIC_STATS 0x07ffU
-#define STATX_ALL 0x0fffU
-#define STATX_BTIME 0x0800U
-#define STATX__RESERVED 0x80000000U
+# define STATX_TYPE 0x0001U
+# define STATX_MODE 0x0002U
+# define STATX_NLINK 0x0004U
+# define STATX_UID 0x0008U
+# define STATX_GID 0x0010U
+# define STATX_ATIME 0x0020U
+# define STATX_MTIME 0x0040U
+# define STATX_CTIME 0x0080U
+# define STATX_INO 0x0100U
+# define STATX_SIZE 0x0200U
+# define STATX_BLOCKS 0x0400U
+# define STATX_BASIC_STATS 0x07ffU
+# define STATX_ALL 0x0fffU
+# define STATX_BTIME 0x0800U
+# define STATX__RESERVED 0x80000000U
 
-#define STATX_ATTR_COMPRESSED 0x0004
-#define STATX_ATTR_IMMUTABLE 0x0010
-#define STATX_ATTR_APPEND 0x0020
-#define STATX_ATTR_NODUMP 0x0040
-#define STATX_ATTR_ENCRYPTED 0x0800
-#define STATX_ATTR_AUTOMOUNT 0x1000
+# define STATX_ATTR_COMPRESSED 0x0004
+# define STATX_ATTR_IMMUTABLE 0x0010
+# define STATX_ATTR_APPEND 0x0020
+# define STATX_ATTR_NODUMP 0x0040
+# define STATX_ATTR_ENCRYPTED 0x0800
+# define STATX_ATTR_AUTOMOUNT 0x1000
+#endif /* !STATX_ALL */
 
 __BEGIN_DECLS
 
diff --git a/sysdeps/unix/sysv/linux/statx.c b/sysdeps/unix/sysv/linux/statx.c
index b99e30dc3e..5d634960a6 100644
--- a/sysdeps/unix/sysv/linux/statx.c
+++ b/sysdeps/unix/sysv/linux/statx.c
@@ -21,6 +21,11 @@ 
 
 #include "statx_generic.c"
 
+/* Ensure that the kernel headers have not changed the struct size.
+   If this ever happens, it may be necessary to introduce a new symbol
+   version.  */
+_Static_assert (sizeof (struct statx) == 256, "struct statx size changed");
+
 int
 statx (int fd, const char *path, int flags,
        unsigned int mask, struct statx *buf)