[LTP] [PATCH] syscalls/ustat: Move the syscall to lapi

[LTP] [PATCH] syscalls/ustat: Move the syscall to lapi

[Cyril Hrubis]

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>
 #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");
-- 
2.19.2

[LTP] [PATCH] syscalls/ustat: Move the syscall to lapi

[Richard Palethorpe]

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.

[LTP] [PATCH] syscalls/ustat: Move the syscall to lapi

[Cyril Hrubis]

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...

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/ustat: Move the syscall to lapi

[pvorel]

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

[LTP] [PATCH] syscalls/ustat: Move the syscall to lapi

[Richard Palethorpe]

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.

[LTP] [PATCH] syscalls/ustat: Move the syscall to lapi

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/ustat: Move the syscall to lapi

[Steve Muckle]

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>

[LTP] [PATCH] syscalls/ustat: Move the syscall to lapi

[Steve Muckle]

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?

[LTP] [PATCH] syscalls/ustat: Move the syscall to lapi

[Cyril Hrubis]

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

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/ustat: Move the syscall to lapi

[Petr Vorel]

HI Cyril,

> 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>
I guess you send a v2 with test variants, so closing this in patchwork.


Kind regards,
Petr