syscalls/ustat: Move the syscall to lapi
diff mbox series

Message ID 20190221112201.18324-1-chrubis@suse.cz
State New
Headers show
Series
  • syscalls/ustat: Move the syscall to lapi
Related show

Commit Message

Cyril Hrubis Feb. 21, 2019, 11:22 a.m. UTC
The dev parameter needs to be casted to unsigned in some cases, let's
move call to tst_syscall() from the tests to lapi so that the tests does
not have to worry about the low level details.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
CC: Steve Muckle <smuckle@google.com>
CC: Richard Palethorpe <rpalethorpe@suse.com>
---
 include/lapi/ustat.h                      | 7 +++++++
 testcases/kernel/syscalls/ustat/ustat01.c | 5 ++---
 testcases/kernel/syscalls/ustat/ustat02.c | 5 ++---
 3 files changed, 11 insertions(+), 6 deletions(-)

Comments

Richard Palethorpe Feb. 21, 2019, 12:34 p.m. UTC | #1
Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> The dev parameter needs to be casted to unsigned in some cases, let's
> move call to tst_syscall() from the tests to lapi so that the tests does
> not have to worry about the low level details.
>
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> CC: Steve Muckle <smuckle@google.com>
> CC: Richard Palethorpe <rpalethorpe@suse.com>
> ---
>  include/lapi/ustat.h                      | 7 +++++++
>  testcases/kernel/syscalls/ustat/ustat01.c | 5 ++---
>  testcases/kernel/syscalls/ustat/ustat02.c | 5 ++---
>  3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/include/lapi/ustat.h b/include/lapi/ustat.h
> index 12c073582..6365b2e92 100644
> --- a/include/lapi/ustat.h
> +++ b/include/lapi/ustat.h
> @@ -10,12 +10,19 @@
>  #ifdef HAVE_SYS_USTAT_H
>  # include <sys/ustat.h>

Just a thought, but this is potentially a problem if lib C implementes
ustat in user land, but the system call still exists. Which I think is
more likely with an obsolete system call.

>  #else
> +# include "lapi/syscalls.h"
> +
>  struct ustat {
>  	daddr_t f_tfree;
>  	ino_t f_tinode;
>  	char f_fname[6];
>  	char f_fpack[6];
>  };
> +
> +static inline int ustat(dev_t dev, struct ustat *ubuf)
> +{
> +	return tst_syscall(__NR_ustat, (unsigned int)dev, ubuf);
> +}
>  #endif
>
>  #endif /* LAPI_USTAT_H */
> diff --git a/testcases/kernel/syscalls/ustat/ustat01.c b/testcases/kernel/syscalls/ustat/ustat01.c
> index 2e7dcc9d7..729f8c106 100644
> --- a/testcases/kernel/syscalls/ustat/ustat01.c
> +++ b/testcases/kernel/syscalls/ustat/ustat01.c
> @@ -10,9 +10,8 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>
> -#include "lapi/syscalls.h"
> -#include "lapi/ustat.h"
>  #include "tst_test.h"
> +#include "lapi/ustat.h"
>
>  static dev_t dev_num;
>
> @@ -20,7 +19,7 @@ void run(void)
>  {
>  	struct ustat ubuf;
>
> -	TEST(tst_syscall(__NR_ustat, (unsigned int)dev_num, &ubuf));
> +	TEST(ustat(dev_num, &ubuf));
>
>  	if (TST_RET == -1)
>  		tst_res(TFAIL | TTERRNO, "ustat(2) failed");
> diff --git a/testcases/kernel/syscalls/ustat/ustat02.c b/testcases/kernel/syscalls/ustat/ustat02.c
> index 9bbe4f3f5..3c9236200 100644
> --- a/testcases/kernel/syscalls/ustat/ustat02.c
> +++ b/testcases/kernel/syscalls/ustat/ustat02.c
> @@ -11,9 +11,8 @@
>  #include <sys/stat.h>
>  #include <sys/types.h>
>
> -#include "lapi/syscalls.h"
> -#include "lapi/ustat.h"
>  #include "tst_test.h"
> +#include "lapi/ustat.h"
>
>  static dev_t invalid_dev = -1;
>  static dev_t root_dev;
> @@ -36,7 +35,7 @@ int TST_TOTAL = ARRAY_SIZE(tc);
>
>  void run(unsigned int test)
>  {
> -	TEST(tst_syscall(__NR_ustat, *tc[test].dev, tc[test].buf));
> +	TEST(ustat(*tc[test].dev, tc[test].buf));
>
>  	if ((TST_RET == -1) && (TST_ERR == tc[test].exp_errno))
>  		tst_res(TPASS | TTERRNO, "ustat(2) expected failure");


--
Thank you,
Richard.
Cyril Hrubis Feb. 21, 2019, 2:30 p.m. UTC | #2
Hi!
> > diff --git a/include/lapi/ustat.h b/include/lapi/ustat.h
> > index 12c073582..6365b2e92 100644
> > --- a/include/lapi/ustat.h
> > +++ b/include/lapi/ustat.h
> > @@ -10,12 +10,19 @@
> >  #ifdef HAVE_SYS_USTAT_H
> >  # include <sys/ustat.h>
> 
> Just a thought, but this is potentially a problem if lib C implementes
> ustat in user land, but the system call still exists. Which I think is
> more likely with an obsolete system call.

Good point. So it all depends on what we want to focus on, if we are
after kernel, we should call the syscall directly, if we look at system
functionality we should go after the libc one by default.

I guess that ideally we should test both, not sure how to achiveve that
reasonably easily...
pvorel Feb. 21, 2019, 2:50 p.m. UTC | #3
Hi,

>> Just a thought, but this is potentially a problem if lib C implementes
>> ustat in user land, but the system call still exists. Which I think is
>> more likely with an obsolete system call.
> 
> Good point. So it all depends on what we want to focus on, if we are
> after kernel, we should call the syscall directly, if we look at system
> functionality we should go after the libc one by default.
> 
> I guess that ideally we should test both, not sure how to achiveve that
> reasonably easily...
+1.

Petr
Richard Palethorpe Feb. 22, 2019, 7:39 a.m. UTC | #4
Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> > diff --git a/include/lapi/ustat.h b/include/lapi/ustat.h
>> > index 12c073582..6365b2e92 100644
>> > --- a/include/lapi/ustat.h
>> > +++ b/include/lapi/ustat.h
>> > @@ -10,12 +10,19 @@
>> >  #ifdef HAVE_SYS_USTAT_H
>> >  # include <sys/ustat.h>
>>
>> Just a thought, but this is potentially a problem if lib C implementes
>> ustat in user land, but the system call still exists. Which I think is
>> more likely with an obsolete system call.
>
> Good point. So it all depends on what we want to focus on, if we are
> after kernel, we should call the syscall directly, if we look at system
> functionality we should go after the libc one by default.
>
> I guess that ideally we should test both, not sure how to achiveve that
> reasonably easily...

Possibly we could create a config option which forcibly sets (almost)
all the HAVE_* macros to zero. This will probably result in a lot of
tests being skipped as well, but it might be good enough.

--
Thank you,
Richard.
Cyril Hrubis Feb. 22, 2019, 3 p.m. UTC | #5
Hi!
> >> > diff --git a/include/lapi/ustat.h b/include/lapi/ustat.h
> >> > index 12c073582..6365b2e92 100644
> >> > --- a/include/lapi/ustat.h
> >> > +++ b/include/lapi/ustat.h
> >> > @@ -10,12 +10,19 @@
> >> >  #ifdef HAVE_SYS_USTAT_H
> >> >  # include <sys/ustat.h>
> >>
> >> Just a thought, but this is potentially a problem if lib C implementes
> >> ustat in user land, but the system call still exists. Which I think is
> >> more likely with an obsolete system call.
> >
> > Good point. So it all depends on what we want to focus on, if we are
> > after kernel, we should call the syscall directly, if we look at system
> > functionality we should go after the libc one by default.
> >
> > I guess that ideally we should test both, not sure how to achiveve that
> > reasonably easily...
> 
> Possibly we could create a config option which forcibly sets (almost)
> all the HAVE_* macros to zero. This will probably result in a lot of
> tests being skipped as well, but it might be good enough.

I don't think that this will actully get past linking, I suppose we
would end up with two confilicting syscall wrappers in most of the
cases.
Steve Muckle Feb. 22, 2019, 7:29 p.m. UTC | #6
On 02/21/2019 03:22 AM, Cyril Hrubis wrote:
> The dev parameter needs to be casted to unsigned in some cases, let's
> move call to tst_syscall() from the tests to lapi so that the tests does
> not have to worry about the low level details.
> 
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> CC: Steve Muckle <smuckle@google.com>
> CC: Richard Palethorpe <rpalethorpe@suse.com>
> ---
>   include/lapi/ustat.h                      | 7 +++++++
>   testcases/kernel/syscalls/ustat/ustat01.c | 5 ++---
>   testcases/kernel/syscalls/ustat/ustat02.c | 5 ++---
>   3 files changed, 11 insertions(+), 6 deletions(-)

Reviewed-by: Steve Muckle <smuckle@google.com>
Steve Muckle Feb. 22, 2019, 7:43 p.m. UTC | #7
On 02/22/2019 07:00 AM, Cyril Hrubis wrote:
> Hi!
>>>>> diff --git a/include/lapi/ustat.h b/include/lapi/ustat.h
>>>>> index 12c073582..6365b2e92 100644
>>>>> --- a/include/lapi/ustat.h
>>>>> +++ b/include/lapi/ustat.h
>>>>> @@ -10,12 +10,19 @@
>>>>>   #ifdef HAVE_SYS_USTAT_H
>>>>>   # include <sys/ustat.h>
>>>>
>>>> Just a thought, but this is potentially a problem if lib C implementes
>>>> ustat in user land, but the system call still exists. Which I think is
>>>> more likely with an obsolete system call.
>>>
>>> Good point. So it all depends on what we want to focus on, if we are
>>> after kernel, we should call the syscall directly, if we look at system
>>> functionality we should go after the libc one by default.
>>>
>>> I guess that ideally we should test both, not sure how to achiveve that
>>> reasonably easily...

This seems to me a great feature to have as well - it would address the 
concern that was just recently raised about having to choose between 
focusing on testing at the libc level or the kernel syscall level.
>> Possibly we could create a config option which forcibly sets (almost)
>> all the HAVE_* macros to zero. This will probably result in a lot of
>> tests being skipped as well, but it might be good enough.
> 
> I don't think that this will actully get past linking, I suppose we
> would end up with two confilicting syscall wrappers in most of the
> cases.

Perhaps something like how the compat_16 syscalls are handled? Test libc 
wrappers by default if the host has them, and for syscalls that can 
easily be called directly, add an extra bit in the test's Makefile that 
causes a "<testname>_direct" version of the test to be built using a 
direct syscall?
Cyril Hrubis Feb. 25, 2019, 12:18 p.m. UTC | #8
Hi!
> >>> Good point. So it all depends on what we want to focus on, if we are
> >>> after kernel, we should call the syscall directly, if we look at system
> >>> functionality we should go after the libc one by default.
> >>>
> >>> I guess that ideally we should test both, not sure how to achiveve that
> >>> reasonably easily...
> 
> This seems to me a great feature to have as well - it would address the 
> concern that was just recently raised about having to choose between 
> focusing on testing at the libc level or the kernel syscall level.
> >> Possibly we could create a config option which forcibly sets (almost)
> >> all the HAVE_* macros to zero. This will probably result in a lot of
> >> tests being skipped as well, but it might be good enough.
> > 
> > I don't think that this will actully get past linking, I suppose we
> > would end up with two confilicting syscall wrappers in most of the
> > cases.
> 
> Perhaps something like how the compat_16 syscalls are handled? Test libc 
> wrappers by default if the host has them, and for syscalls that can 
> easily be called directly, add an extra bit in the test's Makefile that 
> causes a "<testname>_direct" version of the test to be built using a 
> direct syscall?

Either that or we can add a parameter to switch between them on runtime.
I will look into this later on.

Maybe we can even add a .raw_syscal bit into the tst_test structure and
add an infrastructure to choose what to call on runtime, e.g.
tst_ustat() will call either libc or syscall based on some global flag
and the test library would execute the run function twice, for each
possiblity. The tst_ustat() function could be in fact generated based
just on the function signature, so we can add a script to generate these
as well.

I've created an issue, so that we can track the discussion and progress
on GitHub as well:

https://github.com/linux-test-project/ltp/issues/506

Patch
diff mbox series

diff --git a/include/lapi/ustat.h b/include/lapi/ustat.h
index 12c073582..6365b2e92 100644
--- a/include/lapi/ustat.h
+++ b/include/lapi/ustat.h
@@ -10,12 +10,19 @@ 
 #ifdef HAVE_SYS_USTAT_H
 # include <sys/ustat.h>
 #else
+# include "lapi/syscalls.h"
+
 struct ustat {
 	daddr_t f_tfree;
 	ino_t f_tinode;
 	char f_fname[6];
 	char f_fpack[6];
 };
+
+static inline int ustat(dev_t dev, struct ustat *ubuf)
+{
+	return tst_syscall(__NR_ustat, (unsigned int)dev, ubuf);
+}
 #endif
 
 #endif /* LAPI_USTAT_H */
diff --git a/testcases/kernel/syscalls/ustat/ustat01.c b/testcases/kernel/syscalls/ustat/ustat01.c
index 2e7dcc9d7..729f8c106 100644
--- a/testcases/kernel/syscalls/ustat/ustat01.c
+++ b/testcases/kernel/syscalls/ustat/ustat01.c
@@ -10,9 +10,8 @@ 
 #include <sys/types.h>
 #include <sys/stat.h>
 
-#include "lapi/syscalls.h"
-#include "lapi/ustat.h"
 #include "tst_test.h"
+#include "lapi/ustat.h"
 
 static dev_t dev_num;
 
@@ -20,7 +19,7 @@  void run(void)
 {
 	struct ustat ubuf;
 
-	TEST(tst_syscall(__NR_ustat, (unsigned int)dev_num, &ubuf));
+	TEST(ustat(dev_num, &ubuf));
 
 	if (TST_RET == -1)
 		tst_res(TFAIL | TTERRNO, "ustat(2) failed");
diff --git a/testcases/kernel/syscalls/ustat/ustat02.c b/testcases/kernel/syscalls/ustat/ustat02.c
index 9bbe4f3f5..3c9236200 100644
--- a/testcases/kernel/syscalls/ustat/ustat02.c
+++ b/testcases/kernel/syscalls/ustat/ustat02.c
@@ -11,9 +11,8 @@ 
 #include <sys/stat.h>
 #include <sys/types.h>
 
-#include "lapi/syscalls.h"
-#include "lapi/ustat.h"
 #include "tst_test.h"
+#include "lapi/ustat.h"
 
 static dev_t invalid_dev = -1;
 static dev_t root_dev;
@@ -36,7 +35,7 @@  int TST_TOTAL = ARRAY_SIZE(tc);
 
 void run(unsigned int test)
 {
-	TEST(tst_syscall(__NR_ustat, *tc[test].dev, tc[test].buf));
+	TEST(ustat(*tc[test].dev, tc[test].buf));
 
 	if ((TST_RET == -1) && (TST_ERR == tc[test].exp_errno))
 		tst_res(TPASS | TTERRNO, "ustat(2) expected failure");