diff mbox series

[1/2] getrlimit/getrlimit03: add configure-time check for struct ulimit64

Message ID 20181203190148.93108-1-ghackmann@google.com
State Superseded
Delegated to: Petr Vorel
Headers show
Series [1/2] getrlimit/getrlimit03: add configure-time check for struct ulimit64 | expand

Commit Message

Greg Hackmann Dec. 3, 2018, 7:01 p.m. UTC 
The kernel's UAPI headers export a definition of struct ulimit64 which
can conflict with the open-coded one in getrlimit03.  Fix this by moving
getrlimit03's definition behind a configure-time check.

Signed-off-by: Greg Hackmann <ghackmann@google.com>
---
 configure.ac                                      |  1 +
 m4/ltp-rlimit64.m4                                | 10 ++++++++++
 testcases/kernel/syscalls/getrlimit/getrlimit03.c |  2 ++
 3 files changed, 13 insertions(+)
 create mode 100644 m4/ltp-rlimit64.m4

Comments

Petr Vorel Dec. 3, 2018, 9:25 p.m. UTC | #1 
Hi Greg,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> +++ b/m4/ltp-rlimit64.m4
> @@ -0,0 +1,10 @@
> +dnl SPDX-License-Identifier: GPL-2.0-or-later
> +dnl Copyright (c) 2018 Google, Inc.
> +
> +AC_DEFUN([LTP_CHECK_RLIMIT64],[
> +AC_CHECK_TYPES([struct rlimit64],,,[
> +#include <sys/time.h>
> +#include <sys/resource.h>
> +])
> +])
Glibc and uclibc-ng define struct rlimit64 only #ifdef __USE_LARGEFILE64.
Would it make sense to change autoconf test to AC_COMPILE_IFELSE and pass -D_LARGEFILE64_SOURCE to it?
+ Use it in Makefile, of course.


Kind regards,
Petr
Greg Hackmann Dec. 3, 2018, 10:20 p.m. UTC | #2 
On 12/03/2018 01:25 PM, Petr Vorel wrote:
> Glibc and uclibc-ng define struct rlimit64 only #ifdef __USE_LARGEFILE64.

Makes sense.  I ran into this issue with bionic, which doesn't require 
-D_LARGEFILE64_SOURCE for these kinds of definitions.

> Would it make sense to change autoconf test to AC_COMPILE_IFELSE and pass -D_LARGEFILE64_SOURCE to it?
> + Use it in Makefile, of course.
> 
> 
> Kind regards,
> Petr
> 

I'm honestly not that familiar with autotools, so I'm not sure I follow 
this.  Are you suggesting that we assume struct rlimit64 is defined 
(possibly conditionally on -D_LARGEFILE64_SOURCE), and we really ought 
to probe whether -D_LARGEFILE64_SOURCE is required to make it visible?
Petr Vorel Dec. 4, 2018, 9:01 a.m. UTC | #3 
Hi Greg,

> On 12/03/2018 01:25 PM, Petr Vorel wrote:
> > Glibc and uclibc-ng define struct rlimit64 only #ifdef __USE_LARGEFILE64.

> Makes sense.  I ran into this issue with bionic, which doesn't require
> -D_LARGEFILE64_SOURCE for these kinds of definitions.
Out of curiosity: how do you use it on bionic? Looking into git, struct rlimit64
is defined in libc/kernel/uapi/linux/resource.h (generated file form kernel
headers), not in libc/include/sys/resource.h. And you include in getrlimit03.c
<sys/resource.h> (not <linux/resource.h>).

> > Would it make sense to change autoconf test to AC_COMPILE_IFELSE and pass -D_LARGEFILE64_SOURCE to it?
> > + Use it in Makefile, of course.


> I'm honestly not that familiar with autotools, so I'm not sure I follow
> this.  Are you suggesting that we assume struct rlimit64 is defined
> (possibly conditionally on -D_LARGEFILE64_SOURCE), and we really ought to
> probe whether -D_LARGEFILE64_SOURCE is required to make it visible?
I propose to define _LARGEFILE64_SOURCE anyway, as it allows us to use structures on
glibc[1] and uclibc{,-ng} while it does not harm musl and bionic.
your way to check for it before using it makes sense. I just pointed out,

+ <sys/time.h> is no t needed in LTP_CHECK_RLIMIT64.

So I propose following changes to your patch:

diff --git m4/ltp-rlimit64.m4 m4/ltp-rlimit64.m4
index 6513e65e5..dccb40188 100644
--- m4/ltp-rlimit64.m4
+++ m4/ltp-rlimit64.m4
@@ -3,7 +3,7 @@ dnl Copyright (c) 2018 Google, Inc.
 
 AC_DEFUN([LTP_CHECK_RLIMIT64],[
 AC_CHECK_TYPES([struct rlimit64],,,[
-#include <sys/time.h>
+#define _LARGEFILE64_SOURCE
 #include <sys/resource.h>
 ])
 ])
diff --git testcases/kernel/syscalls/getrlimit/Makefile testcases/kernel/syscalls/getrlimit/Makefile
index bd617d806..4a776e7b1 100644
--- testcases/kernel/syscalls/getrlimit/Makefile
+++ testcases/kernel/syscalls/getrlimit/Makefile
@@ -18,6 +18,8 @@
 
 top_srcdir		?= ../../../..
 
+getrlimit03: CFLAGS += -D_LARGEFILE64_SOURCE
+
 include $(top_srcdir)/include/mk/testcases.mk
 
 include $(top_srcdir)/include/mk/generic_leaf_target.mk



Kind regards,
Petr

[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=bits/resource.h;h=7693d20fed284ee1bae5cba8884da319f58e262e;hb=HEAD#l112
Greg Hackmann Dec. 4, 2018, 5:34 p.m. UTC | #4 
On 12/04/2018 01:01 AM, Petr Vorel wrote:
> Hi Greg,
> 
>> On 12/03/2018 01:25 PM, Petr Vorel wrote:
>>> Glibc and uclibc-ng define struct rlimit64 only #ifdef __USE_LARGEFILE64.
> 
>> Makes sense.  I ran into this issue with bionic, which doesn't require
>> -D_LARGEFILE64_SOURCE for these kinds of definitions.
> Out of curiosity: how do you use it on bionic? Looking into git, struct rlimit64
> is defined in libc/kernel/uapi/linux/resource.h (generated file form kernel
> headers), not in libc/include/sys/resource.h. And you include in getrlimit03.c
> <sys/resource.h> (not <linux/resource.h>).
> 

bionic prefers to grab kernel-facing definitions directly from the UAPI 
headers, so sys/resource.h includes linux/resource.h.

I could switch the test to explicitly use linux/resource.h, if you feel 
that makes more sense.  I originally used sys/resource.h to get all the 
other rlimit-related constants.  I actually didn't realize that the 
kernel already exported a struct rlimit64 definition that we could use 
in place of open-coding something.

>>> Would it make sense to change autoconf test to AC_COMPILE_IFELSE and pass -D_LARGEFILE64_SOURCE to it?
>>> + Use it in Makefile, of course.
> 
> 
>> I'm honestly not that familiar with autotools, so I'm not sure I follow
>> this.  Are you suggesting that we assume struct rlimit64 is defined
>> (possibly conditionally on -D_LARGEFILE64_SOURCE), and we really ought to
>> probe whether -D_LARGEFILE64_SOURCE is required to make it visible?
> I propose to define _LARGEFILE64_SOURCE anyway, as it allows us to use structures on
> glibc[1] and uclibc{,-ng} while it does not harm musl and bionic.
> your way to check for it before using it makes sense. I just pointed out,
> 
> + <sys/time.h> is no t needed in LTP_CHECK_RLIMIT64.
> 
> So I propose following changes to your patch:
> 
> diff --git m4/ltp-rlimit64.m4 m4/ltp-rlimit64.m4
> index 6513e65e5..dccb40188 100644
> --- m4/ltp-rlimit64.m4
> +++ m4/ltp-rlimit64.m4
> @@ -3,7 +3,7 @@ dnl Copyright (c) 2018 Google, Inc.
>   
>   AC_DEFUN([LTP_CHECK_RLIMIT64],[
>   AC_CHECK_TYPES([struct rlimit64],,,[
> -#include <sys/time.h>
> +#define _LARGEFILE64_SOURCE
>   #include <sys/resource.h>
>   ])
>   ])
> diff --git testcases/kernel/syscalls/getrlimit/Makefile testcases/kernel/syscalls/getrlimit/Makefile
> index bd617d806..4a776e7b1 100644
> --- testcases/kernel/syscalls/getrlimit/Makefile
> +++ testcases/kernel/syscalls/getrlimit/Makefile
> @@ -18,6 +18,8 @@
>   
>   top_srcdir		?= ../../../..
>   
> +getrlimit03: CFLAGS += -D_LARGEFILE64_SOURCE
> +
>   include $(top_srcdir)/include/mk/testcases.mk
>   
>   include $(top_srcdir)/include/mk/generic_leaf_target.mk
> 
> 
> 
> Kind regards,
> Petr
> 
> [1] https://sourceware.org/git/?p=glibc.git;a=blob;f=bits/resource.h;h=7693d20fed284ee1bae5cba8884da319f58e262e;hb=HEAD#l112
> 

Sounds reasonable.  Thanks for your help here.
Petr Vorel Dec. 5, 2018, 4:47 p.m. UTC | #5 
Hi Greg,

> bionic prefers to grab kernel-facing definitions directly from the UAPI
> headers, so sys/resource.h includes linux/resource.h.
Thanks for info.

> I could switch the test to explicitly use linux/resource.h, if you feel that
> makes more sense.  I originally used sys/resource.h to get all the other
> rlimit-related constants.  I actually didn't realize that the kernel already
> exported a struct rlimit64 definition that we could use in place of
> open-coding something.
You answer yourself - RLIM_NLIMITS and other rlimit-related constants are
needed.

+ using <linux/resource.h> conflicts with other <time.h> usage in LTP library headers.

In file included from /usr/include/time.h:48,
                 from ../../../../include/safe_file_ops_fn.h:22,
                 from ../../../../include/tst_safe_file_ops.h:27,
                 from ../../../../include/tst_test.h:87,
                 from getrlimit03.c:29:
/usr/include/bits/types/struct_itimerspec.h: At top level:
/usr/include/bits/types/struct_itimerspec.h:8:8: error: redefinition of ‘struct itimerspec’


Kind regards,
Petr
diff mbox series

Patch

diff --git a/configure.ac b/configure.ac
index e81d2add5..caea34462 100644
--- a/configure.ac
+++ b/configure.ac
@@ -228,6 +228,7 @@  LTP_CHECK_UNAME_DOMAINNAME
 LTP_CHECK_X_TABLES
 LTP_CHECK_ATOMIC_MEMORY_MODEL
 LTP_CHECK_TPACKET_V3
+LTP_CHECK_RLIMIT64
 LTP_DETECT_HOST_CPU
 LTP_CHECK_PERF_EVENT
 
diff --git a/m4/ltp-rlimit64.m4 b/m4/ltp-rlimit64.m4
new file mode 100644
index 000000000..f2e6c526a
--- /dev/null
+++ b/m4/ltp-rlimit64.m4
@@ -0,0 +1,10 @@ 
+dnl SPDX-License-Identifier: GPL-2.0-or-later
+dnl Copyright (c) 2018 Google, Inc.
+
+AC_DEFUN([LTP_CHECK_RLIMIT64],[
+AC_CHECK_TYPES([struct rlimit64],,,[
+#include <sys/time.h>
+#include <sys/resource.h>
+])
+])
+
diff --git a/testcases/kernel/syscalls/getrlimit/getrlimit03.c b/testcases/kernel/syscalls/getrlimit/getrlimit03.c
index 9ff28acb6..376ef7241 100644
--- a/testcases/kernel/syscalls/getrlimit/getrlimit03.c
+++ b/testcases/kernel/syscalls/getrlimit/getrlimit03.c
@@ -44,10 +44,12 @@ 
 #define __NR_getrlimit_ulong_str	"__NR_getrlimit"
 #endif
 
+#ifndef HAVE_STRUCT_RLIMIT64
 struct rlimit64 {
 	uint64_t rlim_cur;
 	uint64_t rlim_max;
 };
+#endif
 const uint64_t RLIM_INFINITY_U64 = UINT64_MAX;
 
 static int getrlimit_u64(int resource, struct rlimit64 *rlim)