[2/3] tst_device.h: Use lapi/syscalls.h instead of <sys/syscall.h>
diff mbox series

Message ID 20200117113715.22786-3-pvorel@suse.cz
State Accepted
Delegated to: Petr Vorel
Headers show
Series
  • Fixes for old distros
Related show

Commit Message

Petr Vorel Jan. 17, 2020, 11:37 a.m. UTC
As a fallback for old distros which does not define __NR_syncfs
(the dependency is in the library itself).

This was needed on SLES 11 (kernel 2.6.32).

Fixes: 74aeb88c9 ("tst_device: use raw syscall in the tst_device.h")

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 include/tst_device.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Petr Vorel Jan. 20, 2020, 10:31 a.m. UTC | #1
Hi,

> As a fallback for old distros which does not define __NR_syncfs
> (the dependency is in the library itself).

> This was needed on SLES 11 (kernel 2.6.32).

> Fixes: 74aeb88c9 ("tst_device: use raw syscall in the tst_device.h")

> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  include/tst_device.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

> diff --git a/include/tst_device.h b/include/tst_device.h
> index 3db5275c9..13d92ee54 100644
> --- a/include/tst_device.h
> +++ b/include/tst_device.h
> @@ -19,7 +19,7 @@
>  #define TST_DEVICE_H__

>  #include <unistd.h>
> -#include <sys/syscall.h>
> +#include "lapi/syscalls.h"
Hm, maybe this fix wasn't a good idea.
It effectively uses lapi/syscalls.h everywhere instead of <sys/syscall.h>.
Not sure if this is what we want.

Example of the error is #634 [1], which is caused by __NR_socketcall being -1
instead of not defined (socketcall is not defined on some archs, e.g. x86-64 and ARM).
We can fix the condition, but it will lead to numerous not obvious errors, so I
suggest to revert this (and thus get LTP broken on very old distros).

I'm sorry to break LTP just before release.

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/issues/634
Jan Stancek Jan. 20, 2020, 10:56 a.m. UTC | #2
----- Original Message -----
> >  #include <unistd.h>
> > -#include <sys/syscall.h>
> > +#include "lapi/syscalls.h"
> Hm, maybe this fix wasn't a good idea.
> It effectively uses lapi/syscalls.h everywhere instead of <sys/syscall.h>.
> Not sure if this is what we want.

We already include lapi/syscalls.h at several places, so I wouldn't expect
this to be as bad.

> 
> Example of the error is #634 [1], which is caused by __NR_socketcall being -1
> instead of not defined (socketcall is not defined on some archs, e.g. x86-64
> and ARM).
> We can fix the condition

Tests using tst_syscall or ltp_syscall should be fine, since those check
for ENOSYS.

>, but it will lead to numerous not obvious errors, so
> I
> suggest to revert this (and thus get LTP broken on very old distros).
> 

Cyril, any thoughts?
Petr Vorel Jan. 20, 2020, 12:03 p.m. UTC | #3
Hi,

> ----- Original Message -----
> > >  #include <unistd.h>
> > > -#include <sys/syscall.h>
> > > +#include "lapi/syscalls.h"
> > Hm, maybe this fix wasn't a good idea.
> > It effectively uses lapi/syscalls.h everywhere instead of <sys/syscall.h>.
> > Not sure if this is what we want.

> We already include lapi/syscalls.h at several places, so I wouldn't expect
> this to be as bad.

Yes, it's in many places, the same as lapi/syscalls.h:
git grep -l sys/syscall.h |wc -l
136

git grep -l lapi/syscalls.h |wc -l
206

But none of lapi/syscalls.h use is in the API headers (only in API C and tests)

> > Example of the error is #634 [1], which is caused by __NR_socketcall being -1
> > instead of not defined (socketcall is not defined on some archs, e.g. x86-64
> > and ARM).
> > We can fix the condition

> Tests using tst_syscall or ltp_syscall should be fine, since those check
> for ENOSYS.
OK, if lapi/syscalls.h include is ok, using tst_syscall() in socketcall01.c is
trivial fix. But I'd worry about these uses:

$ git grep -e '^#if.*\bSYS_' $(git grep -l sys/syscall.h)
testcases/kernel/containers/libclone/libclone.h:#ifndef SYS_unshare
testcases/kernel/containers/libclone/libclone.h:#ifdef __NR_unshare
testcases/kernel/containers/libclone/libclone.h:#ifndef __NR_unshare
testcases/kernel/fs/scsi/ltpscsi/llseek.c:#ifdef __NR_lseek
testcases/kernel/fs/scsi/ltpscsi/llseek.c:#ifndef __NR_llseek
testcases/kernel/hotplug/memory_hotplug/commands.c:#ifndef __NR_migrate_pages
testcases/kernel/mem/vma/vma03.c:#ifdef __NR_mmap2
testcases/kernel/security/tomoyo/include.h:#ifdef __NR_uselib
testcases/kernel/security/tomoyo/include.h:#ifdef __NR_pivot_root
testcases/kernel/syscalls/accept4/accept4_01.c:#if defined(SYS_ACCEPT4) /* the socketcall() number */
testcases/kernel/syscalls/readahead/readahead01.c:#if defined(__NR_readahead)
testcases/kernel/syscalls/set_robust_list/set_robust_list01.c:#ifdef __NR_set_robust_list
testcases/kernel/syscalls/set_robust_list/set_robust_list01.c:#ifdef __NR_set_robust_list
testcases/kernel/syscalls/set_robust_list/set_robust_list01.c:#ifdef __NR_set_robust_list
testcases/kernel/syscalls/set_robust_list/set_robust_list01.c:#ifdef __NR_set_robust_list
testcases/kernel/syscalls/setns/setns01.c:#if defined(__NR_setns)
testcases/kernel/syscalls/setns/setns02.c:#if defined(__NR_setns) && defined(CLONE_NEWIPC) && defined(CLONE_NEWUTS)
testcases/kernel/syscalls/socketcall/socketcall01.c:#ifdef __NR_socketcall
testcases/kernel/syscalls/socketcall/socketcall02.c:#ifdef __NR_socketcall
testcases/kernel/syscalls/socketcall/socketcall03.c:#ifdef __NR_socketcall
testcases/kernel/syscalls/socketcall/socketcall04.c:#ifdef __NR_socketcall
testcases/kernel/syscalls/timerfd/timerfd01.c:#ifdef __NR_timerfd_create
testcases/misc/crash/crash02.c:#if defined(__NR_vfork) && __NR_vfork
testcases/misc/crash/crash02.c:#if defined(__NR_clone) && __NR_clone

which IMHO fail unless transformed into tst_syscall/ltp_syscall().

That's why I'd apply my fix https://patchwork.ozlabs.org/patch/1225869/.

Kind regards,
Petr
Cyril Hrubis Jan. 20, 2020, 12:28 p.m. UTC | #4
Hi!
> > Example of the error is #634 [1], which is caused by __NR_socketcall being -1
> > instead of not defined (socketcall is not defined on some archs, e.g. x86-64
> > and ARM).
> > We can fix the condition
> 
> Tests using tst_syscall or ltp_syscall should be fine, since those check
> for ENOSYS.
> 
> >, but it will lead to numerous not obvious errors, so
> > I
> > suggest to revert this (and thus get LTP broken on very old distros).
> > 
> 
> Cyril, any thoughts?

I would say that doing #ifdef __NR_* in test source to disable it is
wrong to begin with.

Doing 'git grep ifdef __NR' shows only socketcall, set_robust_list and
timerfd tests.

The timerfd tests include lapi/syscalls.h already and uses ltp_syscall()
so that shouldn't break. I guess we can safely remove the ifdef there as
well.

The set_robust_list includes the old test.h so thi change to
tst_device.h should not affect it either.

So as far as I can tell the only thing that breaks are the socketcall
tests and I guess that the actual fix would be using tst_syscall()
instead of syscall() and we can remove the ifdefs as well.
Martin Doucha Jan. 20, 2020, 12:30 p.m. UTC | #5
On 1/20/20 1:03 PM, Petr Vorel wrote:
> But none of lapi/syscalls.h use is in the API headers (only in API C and tests)

And you could avoid #including lapi/syscalls.h in API headers by simply
moving the implementation of tst_dev_sync() to lib/tst_device.c.

Why do you guys use static inline functions so much anyway? There are no
technical reasons to do that except for default main().
Petr Vorel Jan. 20, 2020, 1:04 p.m. UTC | #6
Hi,

> On 1/20/20 1:03 PM, Petr Vorel wrote:
> > But none of lapi/syscalls.h use is in the API headers (only in API C and tests)

> And you could avoid #including lapi/syscalls.h in API headers by simply
> moving the implementation of tst_dev_sync() to lib/tst_device.c.

> Why do you guys use static inline functions so much anyway? There are no
> technical reasons to do that except for default main().

+1. I still think it's not good to include lapi/syscalls.h even it does not
break anything. I'll send a patch which uses tst_syscall in socketcall01.c and
second commit which moves tst_dev_sync() to lib/tst_device.c.
It's up to you if agree on second one.

Kind regards,
Petr

Patch
diff mbox series

diff --git a/include/tst_device.h b/include/tst_device.h
index 3db5275c9..13d92ee54 100644
--- a/include/tst_device.h
+++ b/include/tst_device.h
@@ -19,7 +19,7 @@ 
 #define TST_DEVICE_H__
 
 #include <unistd.h>
-#include <sys/syscall.h>
+#include "lapi/syscalls.h"
 
 struct tst_device {
 	const char *dev;