| Message ID | 20190221112201.18324-1-chrubis@suse.cz |
|---|---|
| State | Changes Requested |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | syscalls/ustat: Move the syscall to lapi | expand |
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.
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...
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
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.
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.
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>
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?
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
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
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");
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(-)