| Message ID | 20200117113715.22786-3-pvorel@suse.cz |
|---|---|
| State | Accepted |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | Fixes for old distros | expand |
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
----- 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?
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
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.
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().
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
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;
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(-)