Re: [LTP] [PATCH] Simple tests for cross memory attach (process_vm_[readv|writev] syscalls)

Cyril Hrubis
2012-05-18T12:38:24Z

Hi!
> I looked at testcases sent by Chris. Good news is, they
> do not overlap (in terms of functionality) with mine :-).
>
> The patch need some polishing to match the style guide:
> - indentation
> - I think output should be TINFO, rather than plain printf
> - single statement if without { }
> - I would split some code to more functions, for example process_vm_writev01.c
> main function seems complex
>
> As for test organization, I think we need to decide 2 issues:
> 1. directory structure
> a)
> testcases/kernel/syscalls/cma/
> testcases/kernel/syscalls/cma/process_vm_readv
> testcases/kernel/syscalls/cma/process_vm_writev
> b)
> testcases/kernel/syscalls/process_vm_readv
> testcases/kernel/syscalls/process_vm_writev
>
> I'm preferring a) as these 2 syscalls are closely related,
> and we already have need to share code.

Also please omit meaningless comments like:

/******************************************************
* main
******************************************************/

I'm also not a friend of keeping the HISTORY, FILENAME and others inside
of the file. We do have git that keeps this information in more
structured and useful manner. So this is a relict from a times
developers were distributing source code in tarballs. Simple credits
with your name in the top comment should be enough ;).

We use LKML coding style for LTP so you can use checkpatch.pl[1] to
check that your platches are okay.

1. https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl

> 2. how to invoke new syscalls
> a)
> Patch from Chris is defining new syscall numbers in test and
> using syscall(2).

Don't do that, use testcases/kernel/include/linux_syscall_numbers.h
instead (note that this file is generated and doesn't exists in the git
repo).

> b)
> My patch adds configure check for glibc wrappers process_vm_readv/writev
> and if those does not exists it checks for __NR_process_vm_readv.

That is generally what we should do, although sometimes there is a
difference between the libc wrapper and doing the syscall manually. In
some of the cases you can't get EFAULT because the pointer is
dereferenced in the userspace before it's passed to the kernel... Just
keep that in mind.

> I think we should not define syscall numbers in tests directly,
> but I'm relying on LTP maintainers to provide guidance here.
> In any case, if you run it on older kernel it should end with TCONF.

Yes, please use the generated header. ;)

> I would go with using Chris' process_vm.h, but remove syscall number
> defines and use names that wont conflict with glibc wrappers:
> test_process_vm_readv
> test_process_vm_writev
>
> These could be function pointers, that would get initialized with
> function using glibc wrapper
> function using syscall(__NR_process_vm_readv,..)
> or NULL
>
> each testcase could check in setup for NULL, and end with TCONF,
> if there is no way invoke that syscall.

Once you actually _did_ the configure check, this complications aren't
really needed. Or have I missed something?

> There was question from Caspar, if we should check for both glibc wrapper
> and syscall number (http://thread.gmane.org/gmane.linux.ltp/16055).
> I think we should, as we use glibc wrappers by default.
>
> Any thoughts?

As I said, once you are sure that glibc wrapper and bare syscall do the
same in your case preffering glibc wrapper is good idea as this covers
the codepath as whole. Otherwise just do the syscall and it either works
or you get ENOSYS.

--
Cyril Hrubis
chrubis@suse.cz