Test library API changes

From: "Cyril Hrubis" <[email protected]>
To: [email protected]
Sent: Tuesday, 5 January, 2016 12:11:36 PM
Subject: [LTP] Test library API changes

Hi!
I'm back after my vacation and I had a some time to experiment with the LTP test API. My intention is to make the changes in the next development cycle. I.e. we will concentrate on the release in upcoming weeks and once that is taken care of I would like to discuss these changes.

I've pushed one cleanup that removes tst_res() and tst_brk() from the codebase today since there was only one test using this interface and that was a typo anyway.

Two patches that actually implement sketch of the new API are in forked LTP repository at:

https://github.com/metan-ucw/ltp

The interesting files are:

https://github.com/metan-ucw/ltp/blob/master/include/tst_test.h
and
https://github.com/metan-ucw/ltp/blob/master/lib/tst_test.c
and one testcase that has been converted to the new library:
https://github.com/metan-ucw/ltp/blob/master/testcases/kernel/syscalls/preadv/preadv01.c

The general intention is to move as much code to the library as possible in order to avoid common mistakes and also to let the programmer concentrate on the test itself rather than spending time figuring out the test library API.

One of the important changes is that the library functionality is exported as bitflags in the test structure rather than function calls, this makes the library much easier to use since this eliminates all problems with ordering of the resource inits/cleanups.

Another big change is that the process no longer runs the test function itself but rather than that export pointer to the function that runs the test. This would allow us to finally fix problems with test timeouts etc. since we can easily fork in the test library before we start execution the test code and watch for segfaults/timeouts etc.

The code I've wrote just a proof of concept and by no means complete. It's intended to start a conversation (I can send it as a patch here as well if desired, but I do not think that it makes sense for so early prototype).

--
Cyril Hrubis
[email protected]

--
Mailing list info: http://lists.linux.it/listinfo/ltp

1) https://github.com/metan-ucw/ltp/blob/master/include/tst_test.h#L65
I did like there is option for test_all(). One concern I had was that forcing everybody to use test(int) was too restrictive. For example, you want to pass more parameters than just testcase #. Also not all tests follow "static struct tcase {" idiom, with single test function that is fed different arguments. I think test_all() gives us more flexibility. I was also thinking if library should care about "tcnt".

2) https://github.com/metan-ucw/ltp/blob/master/lib/tst_test.c#L218-L219
This also looks more strict requirement than what we had until now. It requires to count exactly how many PASS/FAIL there are. Is extra PASS or SKIP message a reason to fail the test with brk?

3) cleanup and children
I know, it's a proof of concept :-).

4) After first reading, I'm not sure I have clear picture of changes in API. From what I gathered these are main changes:
test.h -> tst_test.h
tst_resm -> tst_res
tst_brkm -> tst_brk
safe_macros.h -> tst_safe_macros.h (now included via tst_test.h)

This time I've added functionality to handle child processes:

https://github.com/metan-ucw/ltp/blob/master/include/tst_test.h#L48

This function forks a child, runs the function and propagates the results to the parent process via pipe.

The function is implemented at:

https://github.com/metan-ucw/ltp/blob/master/lib/tst_test.c#L129

And used in newly converted test at:

https://github.com/metan-ucw/ltp/blob/master/testcases/kernel/syscalls/creat/creat04.c

Whith this the core test library API is more or less sketched.

And I was writing faster than thinking. We will need another one where starting and reaping child is split between two functions. I will work on that tomorrow.

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Tuesday, 9 February, 2016 6:46:18 PM
Subject: Re: [LTP] Test library API changes

Hi!

Quoted 3 lines

And I was writing faster than thinking. We will need another one where starting and reaping child is split between two functions. I will work on that tomorrow.

And I've split the function into two now:

https://github.com/metan-ucw/ltp/blob/master/include/tst_test.h#L50

--
Cyril Hrubis
[email protected]

I had a look and have some comments/questions?

1. Why is tcnt not ARRAY_SIZE(tcases)?
https://github.com/metan-ucw/ltp/blob/master/testcases/kernel/syscalls/creat/creat04.c#L83

static struct tst_test test = {
	.tid = "creat04",
	.tcnt = 2,

2. It would be nice if we could pass any parameters to child. Atm. child func accepts only int and only way to share something else appears to be passing it via global variables, which may become confusing for bigger tests.

3. Sum based on tcnt still worries me because number of assertions per test may vary:
if (sum != tcnt * acnt * iterations)

	tst_brk(TBROK, "Number passes/fails/skipps does not match number of tests");

What if we verified instead, that after every testcase, at least one of pass/fail/skip increased?

4. suggestion: improved safeguard for cleanup
Record pid of main process at start and call cleanup only if current pid is recorded pid. This doesn't make it depend on using specific API call to run child process.

4. suggestion: improved safeguard for cleanup
Record pid of main process at start and call cleanup only if current pid is recorded pid. This doesn't make it depend on using specific API call to run child process.

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Thursday, 11 February, 2016 5:03:13 PM
Subject: Re: [LTP] Test library API changes

Hi!
I've redone the child handling and now it does:

* If test sets forks_child flag in test structure pipe is opened at the start of the test which is used to propagate test results from a child to parent

* Child processes are created via SAFE_FORK(), which flushes stdout, just in case there is something there, checks for for fork() failure and clears the result structure.

* After each test run (call to test() function), wait() is called until it returns ECHILD.

If child was succesfully waited(), pipe is examined and if it contains test results, these are added to parent results.

We look into the pipe after each sucessful wait() in order not to fill up the pipe capacity with many children.

* If child calls tst_brk(), exit() with non-zero value is called which is handled in parent wait() and the parent exits with tst_brk() as well.

Techincaly any child that does not exit with 0 cause main test process to report TCONF/TBROK.

At the moment the tst_brk() semantics is to exit the whole test since something unexpected happened. Which seems to be right course of action since failing SAFE_MACRO() in child should really cause main test process to exit.

So if tst_brk() stays as it is we would need to add another call that can exit the child (would do the same action as returning from the test() function, i.e. write results and do exit(0)).

The code is at the same place at:

https://github.com/metan-ucw/ltp

Few test/example programs:

https://github.com/metan-ucw/ltp/blob/master/lib/newlib_tests/test05.c
https://github.com/metan-ucw/ltp/blob/master/lib/newlib_tests/test06.c
https://github.com/metan-ucw/ltp/blob/master/lib/newlib_tests/test07.c

There are probably stil a few rough edges: children forked from children, waiting for rest of the children after one of them called tst_brk(), etc. But the basic functionality seems to work fine.

As usuall comments are welcome.

--
Cyril Hrubis
[email protected]

I was thinking about mapping a piece of shared memory for "struct results", then we wouldn't have to worry about how/when child terminates and how big buffer for resutls we need.

And if it was always there, it is one less parameter (forks_child) user needs to provide.

Quoted 16 lines

* Child processes are created via SAFE_FORK(), which flushes stdout, just in case there is something there, checks for for fork() failure and clears the result structure.

* After each test run (call to test() function), wait() is called until it returns ECHILD.

If child was succesfully waited(), pipe is examined and if it contains test results, these are added to parent results.

We look into the pipe after each sucessful wait() in order not to fill up the pipe capacity with many children.

* If child calls tst_brk(), exit() with non-zero value is called which is handled in parent wait() and the parent exits with tst_brk() as well.

Quoted 7 lines

Techincaly any child that does not exit with 0 cause main test process to report TCONF/TBROK.

At the moment the tst_brk() semantics is to exit the whole test since something unexpected happened. Which seems to be right course of action since failing SAFE_MACRO() in child should really cause main test process to exit.

We have tests that crash/kill child on purpose. As I recall some examples are mprotect and oom tests.

These tests would need to wait for child themselves, since non-zero exit code is expected and we don't want reap_children() to see them and stop the test.

Quoted 3 lines

So if tst_brk() stays as it is we would need to add another call that can exit the child (would do the same action as returning from the test() function, i.e. write results and do exit(0)).

Other notes:

write_result calls "exit(0);", and run_tests calls exit as well after it calls write_result.

If we stay with pipe and write_result, can we use atexit() to call it? That would allow child to exit anywhere with "exit()" instead of making sure it returns all the way to run_tests function.

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Tuesday, 16 February, 2016 10:19:58 PM
Subject: Re: [LTP] Test library API changes

Hi!
The up to date code is at the same place as usual.
(https://github.com/metan-ucw/ltp)

Apart from added support of acquiring a device, checkpoints and resource files the IPC was rewritten to shared memory.

Now the library, before the test is started, mmaps a page backed by a file on /dev/shm/. The first few bytes are used for shared struct results, the rest is used by checkpoints if checkpoints are needed. The struct result values are incremented by gcc atomic operations (which is thread safe as well), the open question is if we need a fallback to inline assembler if these are not supported. Then there is a special function to open checkpoint shm file from a process started by a exec (a few of the testcases does so) since it must open the shm rather than a file in a temporary directory.

There are still some rough edges and the code should be reviewed, but at least 90% of the functionality is there and working.

As usuall comments are welcome :)

--
Cyril Hrubis
[email protected]

My first impression of this version is that it looks simpler, I find it easier to follow, but that could also be explained by fact that I'm reading it for 3rd time :-).

Here are some comments/questions/ideas:

1. acnt appears to be unused now

2. Can we keep ltp_syscall() and call correct brk func with some magic?

3. new/old files names do not follow obvious pattern
For example: tst_dev.h vs tst_device.h
I'm thinking if we can give all old same kind of prefix, like tst1_* to distinguish them from new api files.

4. ipc_fd = open(name, O_CREAT | O_EXCL | O_RDWR, 0600); Should we allow other users to read/write too? For example, what if I change euid and then fork? Or do we expect to always fork and then change euid?

5. setup_ipc ideas/comments (some of these are connected)

5a) Why not always setup results and checkpoints?
If we always map file, it doesn't cost us anything to initialize checkpoints. needs_checkpoints could be removed.

Also processes spawned via exec could report results easily to main process if we always initialized both.

5b) split setup_ipc into setup_ipc and open_ipc,
That should eliminate some code from tst_checkpoint_open()

5c) What if we stored ipc path to env variable?

setup_ipc
generates tmp name based on test name: ltp_ipc_path for convenience will initialize also envp array:

    ltp_only_ipc_env[] = { "LTP_IPC_PATH="$ltp_ipc_path, NULL }

creates ipc file

open_ipc(ipc_path)
if results and checkpoints already initialized
return
if ipc_path == NULL
ipc_path = getenv("LTP_IPC_PATH")
if ipc_path == NULL

    brk()

open ipc file and initialize both results and checkpoints

TST_CHECKPOINT_INIT calls open_ipc(NULL)
process spawned via exec could call either of them, effect would be the same

5d) Always unlink ipc file in setup if we have /proc
Main pid keeps file open and unlinks it. ltp_ipc_path would be set to "/proc/$main_pid/fd/$fd"

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Wednesday, 17 February, 2016 4:54:06 PM
Subject: Re: [LTP] Test library API changes

Hi!

Quoted 3 lines

My first impression of this version is that it looks simpler, I find it easier to follow, but that could also be explained by fact that I'm reading it for 3rd time :-).

Well the SHM code is a bit easier than the pipes but not that much easier, so I would say it's a combination of both.

Quoted 3 lines

Here are some comments/questions/ideas:

1. acnt appears to be unused now

I haven't removed it yet... I'm still thinking how we can generate things like TAP output that expects us to print exact number of tests on the first line followed by exact number of result lines and make as little rules for result reporting as possible.

Maybe we can just say that testcases that define test_all do one just one test and produce one result per such test and do more detailed reporting only for testcases that define tcnt.

Or we can turn on the detailed reporting only if acnt has been set.

Quoted 1 line

2. Can we keep ltp_syscall() and call correct brk func with some magic?

Well we can split the header as we did with the rest of them, do you think that it's worth of it?

Quoted 5 lines

3. new/old files names do not follow obvious pattern
For example: tst_dev.h vs tst_device.h
I'm thinking if we can give all old same kind of prefix, like tst1_* to distinguish them from new api files.

I would go for tst_old_ prefix in this case.

We can easily change all but safe_macros.h that is explicitly included in ~200 testcases.

Quoted 4 lines

4. ipc_fd = open(name, O_CREAT | O_EXCL | O_RDWR, 0600); Should we allow other users to read/write too? For example, what if I change euid and then fork? Or do we expect to always fork and then change euid?

We have four tests that uses checkpoint from child processes, none of them does change euid, so I've just put 0600 there. We can always change that if needed. Or do you think that we should just enable it now since it does not matter much?

Quoted 7 lines

5. setup_ipc ideas/comments (some of these are connected)

5a) Why not always setup results and checkpoints?
If we always map file, it doesn't cost us anything to initialize checkpoints. needs_checkpoints could be removed.

That depends. If we want to be able to change the library internals in the future we have to force the test to explicitly list what it's using, otherwise we would have to anotate all tests that use checkpoints in the futere in case that they wouldn't be initialized as a byproduct of result propagation.

In short if we decide that shared memory is what we will use from now on till the end of the world we can remove it. :)

Quoted 2 lines

Also processes spawned via exec could report results easily to main process if we always initialized both.

Well we can but I see much value in this since execed() test processes are quite rare (~10) and all of them seems to report PASS/FAIL using the return value.

Quoted 2 lines

5b) split setup_ipc into setup_ipc and open_ipc,
That should eliminate some code from tst_checkpoint_open()

Sure, there are still some leftovers since I've commited first working version late in the evening :).

Quoted 2 lines

5c) What if we stored ipc path to env variable?

Quoted 5 lines

setup_ipc
generates tmp name based on test name: ltp_ipc_path for convenience will initialize also envp array:

    ltp_only_ipc_env[] = { "LTP_IPC_PATH="$ltp_ipc_path, NULL }

creates ipc file

Hmm, that way the test would have to explicitly pass it to the execve().

I would rather make it reasonably unique but decideable without explicitly passing variables around.

The current approach would not work when the the process is execed by an child of the test process, which is not ideal either.

Quoted 9 lines

open_ipc(ipc_path)
if results and checkpoints already initialized
return
if ipc_path == NULL
ipc_path = getenv("LTP_IPC_PATH")
if ipc_path == NULL

    brk()

open ipc file and initialize both results and checkpoints

Quoted 7 lines

TST_CHECKPOINT_INIT calls open_ipc(NULL)
process spawned via exec could call either of them, effect would be the same

5d) Always unlink ipc file in setup if we have /proc
Main pid keeps file open and unlinks it. ltp_ipc_path would be set to "/proc/$main_pid/fd/$fd"

This is a good idea, that way the memory would be freed even when the main test process fails to do cleanup.

--
Cyril Hrubis
[email protected]

Quoted 4 lines

Quoted 1 line

2. Can we keep ltp_syscall() and call correct brk func with some magic?

Well we can split the header as we did with the rest of them, do you think that it's worth of it?

I was thinking some ifdef magic. It has same signature in both versions of API, so adding new function with different name, that does pretty much the same seems like unnecessary complication.

Quoted 9 lines

Quoted 7 lines

5c) What if we stored ipc path to env variable?

setup_ipc
generates tmp name based on test name: ltp_ipc_path for convenience will initialize also envp array:

    ltp_only_ipc_env[] = { "LTP_IPC_PATH="$ltp_ipc_path, NULL }

creates ipc file

Hmm, that way the test would have to explicitly pass it to the execve().

True, but it would be rare, as you said it's for ~10 testcases.

Quoted 1 line

Quoted 2 lines

I would rather make it reasonably unique but decideable without explicitly passing variables around.

Should we consider multiple instances running at a time? I do recall that tools/pounder21 allows running things in parallel. (Not sure if anyone runs more instances of same test though)

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Thursday, 18 February, 2016 12:07:53 PM
Subject: Re: [LTP] Test library API changes

Hi!

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

2. Can we keep ltp_syscall() and call correct brk func with some magic?

Well we can split the header as we did with the rest of them, do you think that it's worth of it?

I was thinking some ifdef magic. It has same signature in both versions of API, so adding new function with different name, that does pretty much the same seems like unnecessary complication.

My reasoning was different. There are only two functions that start with ltp_ in the library the ltp_syscall and ltp_clone. So the reason for rename is unifying on tst_ and it was convinient in this case as well. But I do not care that much about this. If you think keeping ltp_syscall() is better, then we can go for it.

Quoted 13 lines

Quoted 9 lines

Quoted 7 lines

5c) What if we stored ipc path to env variable?

setup_ipc
generates tmp name based on test name: ltp_ipc_path for convenience will initialize also envp array:

    ltp_only_ipc_env[] = { "LTP_IPC_PATH="$ltp_ipc_path, NULL }

creates ipc file

Hmm, that way the test would have to explicitly pass it to the execve().

True, but it would be rare, as you said it's for ~10 testcases.

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

I would rather make it reasonably unique but decideable without explicitly passing variables around.

Should we consider multiple instances running at a time? I do recall that tools/pounder21 allows running things in parallel. (Not sure if anyone runs more instances of same test though)

I think that disabling the possibility just to make writing the test library a bit easier is pretty bad idea. Most of the testcases we have can run in parallel just fine. There are only a few that stress the system to the limit or use global resources (devices, IPC, change system time, ...) and if we anotate these tests we can easily speed up the test run five times just by running most of the testcases in parallel.

--
Cyril Hrubis
[email protected]

Quoted 6 lines

I think that disabling the possibility just to make writing the test library a bit easier is pretty bad idea. Most of the testcases we have can run in parallel just fine. There are only a few that stress the system to the limit or use global resources (devices, IPC, change system time, ...) and if we anotate these tests we can easily speed up the test run five times just by running most of the testcases in parallel.

I wasn't suggesting we do that. I was thinking about making ipc filename more unique for each instance, in case we wouldn't have /proc and test cleanup doesn't run for whatever reason. That of course would make ipc file names less predictable.

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>, "Alexey Kodanev" <[email protected]>
Cc: [email protected]
Sent: Wednesday, 2 March, 2016 3:44:21 PM
Subject: Re: [LTP] Test library API changes

Hi!
The up to date version is at usuall place at:

https://github.com/metan-ucw/ltp

The changes are:

* acnt was removed completly

* the bitflags in struct tst_test were fixed to be 1 bit wide

* the headers were changed so that most of the old headers starts with old_ prefix

The safe_macros.h and test.h were left intact at the moment since these are included in numerous testcases. We can rename them in a separate patch if needed.

* Added macro TST_TEST_TCONF() to ease writing testcases that are guarded by #ifdefs, see newly converted llistxattr testcases

* Few more converted testcases

* Started to update Test-Writing-Guidelines for new API

The open issues:

* The SHM base file path propagation
- we will probably go with the env variable

* Finishing rewrite of Test Writing Guidelines
- needs just a bit more work

* Should we allow SAFE_MACROS() in cleanup?

- we can easily do that since cleanup is executed from the test library and we can change tst_brk() to return to the caller if cleanup was entered

- plus point is that writing cleanup is bit easier

- minus point is that tst_brk() can now return and as a side effect tst_brkm() has to return as well (since tst_brkm() from safe macros is redirected to tst_brk()) and because of that we have to drop the __attribute__((noreturn)) since that causes gcc to optimize out return statements from the function which generates compilation warnings in some cases

Is there anything else?

--
Cyril Hrubis
[email protected]

In that case, tst_vbrk_() should also call update_results(), so TBROK during cleanup doesn't get lost/ignored.

Quoted 9 lines

- plus point is that writing cleanup is bit easier

- minus point is that tst_brk() can now return and as a side effect tst_brkm() has to return as well (since tst_brkm() from safe macros is redirected to tst_brk()) and because of that we have to drop the __attribute__((noreturn)) since that causes gcc to optimize out return statements from the function which generates compilation warnings in some cases

I assume if we drop noreturn, we still have a different way to suppress those warnings, right?

Quoted 3 lines

Is there anything else?

I ran into number of compilation errors on RHEL5.6, but they will likely happen on other distros too:

1. lib/ltp_priv.h and va_list
Adding "#include <stdarg.h>" fixed it.

2. preadv/preadv.h:27: undefined reference to `tst_brkm' pread01 and pread02 share predv.h, which is using ltp_syscall

3. recvmsg02.c:68: error: ???SOCK_CLOEXEC??? undeclared (first use in this function) I'm guessing some old api header used to include also lapi/fcntl.h

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Thursday, 10 March, 2016 5:57:21 PM
Subject: Re: [LTP] Test library API changes

Hi!
Up to date version is at usuall place:

https://github.com/metan-ucw/ltp

Changes are:

* Now we use env variable for SHM path

- I was thinking about setting it to the /proc path if proc is mounted but that way we would have to keep the fd open (and it would occupy fd=3) and I think that the less resources the library occupy the better

* The rest of the default test options -I and -C are now working

* I've given up on SAFE_MACROS in cleaup for now

- We may add them later, but I would like to get this merged ideally before we do another release and doing this would take some time

* All relevant parts of test-writing-guidelines should be now rewritten

- There are couple of functions that still take cleanup as fist parameter and that are not exported to the new library but that should be all that is missing

Anything else that should be done before we can merge it?

--
Cyril Hrubis
[email protected]

Quoted 2 lines

- We may add them later, but I would like to get this merged ideally before we do another release and doing this would take some time

I think we have this problem already. tst_brk() will call tst_vbrk_(), which calls cleanup_ipc(), which is using SAFE_* macros. It looks like we'll get infinite loop if any of those fail.

Quoted 5 lines

* All relevant parts of test-writing-guidelines should be now rewritten

- There are couple of functions that still take cleanup as fist parameter and that are not exported to the new library but that should be all that is missing

"Child processes started via exec*, or any other processes must initialize the checkpoint by calling 'TST_CHECKPOINT_INIT()' before usage."
This doesn't sound correct for newlib. We now have "tst_checkpoint_open()".

---

'To make sure cleanup() was called only once LTP has a helper "TST_DECLARE_ONCE_FN"'
TST_DECLARE_ONCE_FN is declared in test.h, it doesn't look like newlib can use it at the moment.

---

I'm not sure it is enough to make cleanup() call only once. It looks like we still can get into situation where we call "do_cleanup();cleanup_ipc();" more than once (if we hit tst_brk in multiple threads with newlib)

---

Can we remove "SAFE_CLOSE(ipc_fd);" from cleanup_ipc()? This fd is already closed in setup_ipc().

---

Should "struct tst_test *tst_test" be initialized to NULL? Some tst_res.c oldlib functions are using it, but it is initialized only if you use newlib, at tst_run_tcases().

Quoted 3 lines

Anything else that should be done before we can merge it?

I only made a compile test on some old/recent RHEL distros and ran some newlib testcases by hand. This worked OK.

Have you tried running all of the (oldlib) tests on various arches/releases to see if we haven't regressed?

I could run it against various RHEL majors, it
would take couple days.

Quoted 3 lines

I'm not sure it is enough to make cleanup() call only once. It looks like we still can get into situation where we call "do_cleanup();cleanup_ipc();" more than once (if we hit tst_brk in multiple threads with newlib)

Right this macro is purely for guarding the user supplied callback.

Unfortunately the pthread_once() does not compile to a stub without -pthread, otherwise we could just use it on the do_cleanup() in library and get rid of that part in the testcase.

I guess that we can emulate it with mutexes as well and guard the library cleanup hence the user supplied cleanup would end up thread safe as well without any additional work.

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Monday, 14 March, 2016 5:00:37 PM
Subject: Re: [LTP] Test library API changes

Hi!

Quoted 7 lines

Quoted 4 lines

I'm not sure it is enough to make cleanup() call only once. It looks like we still can get into situation where we call "do_cleanup();cleanup_ipc();"
more than once (if we hit tst_brk in multiple threads with newlib)

Right this macro is purely for guarding the user supplied callback.

Quoted 7 lines

Unfortunately the pthread_once() does not compile to a stub without -pthread, otherwise we could just use it on the do_cleanup() in library and get rid of that part in the testcase.

I guess that we can emulate it with mutexes as well and guard the library cleanup hence the user supplied cleanup would end up thread safe as well without any additional work.

Thinking of thread safe callback it gets more complicated than that.

We should not execute the cleanup while other threads are still running as well since the cleanup may remove resources these threads are using and we may end up segfaulting.

The ideal solution would be killing/stopping rest of the threads at this point. I guess that we can kill them looping over /proc/self/task/ directory.

I.e.:

mutex_lock()

for tid in /proc/self/task/* {
	if tid != mytid
		tgkill(pid, tid, 9);
}

mutex_unlock()

do_cleanup()
do_exit();

What do you think?

--
Cyril Hrubis
[email protected]

That it will only cause signal to be delivered to that thread, but handler will still kill entire process.

We could use different signal and define a signal handler, that would just loop, but that still doesn't guarantee when pending signal is delivered. Also threads could mask the signal.

Then I was thinking about blocking threads using affinity, but if test changed priority we could lock ourselves out.

We do say in style guide, that cleanup can be called at any time. Should we leave it up to test author? We had the same problem with oldlib, right?

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Thursday, 17 March, 2016 5:06:06 PM
Subject: Re: [LTP] Test library API changes

Hi!
I've rethinked the thread safety and LTP library and rewrote the documentation, there are no changes to the library as the ideal place to deal with thread safety is the user supplied cleanup.

Can you please review:

https://github.com/metan-ucw/ltp/blob/master/doc/test-writing-guidelines.txt#L732

There is also a new test at:

https://github.com/metan-ucw/ltp/blob/master/lib/newlib_tests/test08.c

--
Cyril Hrubis
[email protected]

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Thursday, 31 March, 2016 12:01:46 PM
Subject: Re: [LTP] Test library API changes

Hi!
Latest version is at usuall place:

https://github.com/metan-ucw/ltp

Now rest of the library calls should be wired to the new library as well, see for example this header:

https://github.com/metan-ucw/ltp/blob/master/include/tst_fs.h

The documentation was updated as well (I've even added a few functions that were not documented previously).

I've also compared build logs to make sure that the changes haven't introduced new warnings and did a few syscall testruns to make sure everything works fine. Both looks good to me.

We are not that far from next LTP release. As a matter of fact we should start preparing for it anytime soon. Ideally I would like to get this merged before the release freeze and testing. What do you think?

--
Cyril Hrubis
[email protected]

Quoted 4 lines

Now rest of the library calls should be wired to the new library as well, see for example this header:

https://github.com/metan-ucw/ltp/blob/master/include/tst_fs.h

Isn't it the other way around? Newlib interface wired to old lib implementation? Do the names with "_" at the end have any special meaning?

Was "#ifdef TST_TEST_H__" used in previous versions? Is there a difference between ifdef approach and splitting interface to old_* header?

Quoted 2 lines

The documentation was updated as well (I've even added a few functions that were not documented previously).

"resource_files" still appears to be undocumented.

Quoted 7 lines

I've also compared build logs to make sure that the changes haven't introduced new warnings and did a few syscall testruns to make sure everything works fine. Both looks good to me.

We are not that far from next LTP release. As a matter of fact we should start preparing for it anytime soon. Ideally I would like to get this merged before the release freeze and testing. What do you think?

I think technically it's going to work fine. My only worry is how much are old/new API mixed together when I'm looking at ltp/include directory. And I keep thinking: "As someone editing testcase that is using old/new API, what headers should/shouldn't I use?"

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Monday, 4 April, 2016 2:04:45 PM
Subject: Re: [LTP] Test library API changes

Hi!

Quoted 8 lines

Quoted 4 lines

Now rest of the library calls should be wired to the new library as well, see for example this header:

https://github.com/metan-ucw/ltp/blob/master/include/tst_fs.h

Isn't it the other way around? Newlib interface wired to old lib implementation?
Do the names with "_" at the end have any special meaning?

What I was trying to describe is that the rest of the functionality is available there i.e. wired to the new library.

The names with _ are the original prototypes so that we can have functions with and without the cleanup parameter depending on which proxy static inline function is selected.

Quoted 2 lines

Was "#ifdef TST_TEST_H__" used in previous versions? Is there a difference between ifdef approach and splitting interface to old_* header?

Well the old headers are there for two cases. Either the functions does not need to be exported to the testcases as the functionality is provided by the library, or the header contains some kind of SAFE_MACROS (minus the safe_macros.h that wasn't renamed beacause it's included directly in many testcases). I could have split the rest of the headers the very same way but we would need three files for each. One with the internal functions with underscore and one for oldlib and one for newlib inline functions, which looked to me like unnecessary many files.

Looking at it now we can easily stick the static inline functions for oldlib to the test.h. But we would still need to split the tst_ headers into two, one for the prototypes with underscore ending in _fn.h and one with the static inline for new library.

Or do you have a better idea how to proceed?

Quoted 5 lines

Quoted 3 lines

The documentation was updated as well (I've even added a few functions that were not documented previously).

"resource_files" still appears to be undocumented.

Right. Will fix that.

Quoted 8 lines

Quoted 7 lines

I've also compared build logs to make sure that the changes haven't introduced new warnings and did a few syscall testruns to make sure everything works fine. Both looks good to me.

We are not that far from next LTP release. As a matter of fact we should start preparing for it anytime soon. Ideally I would like to get this merged before the release freeze and testing. What do you think?

Quoted 5 lines

I think technically it's going to work fine. My only worry is how much are old/new API mixed together when I'm looking at ltp/include directory. And I keep thinking: "As someone editing testcase that is using old/new API, what headers should/shouldn't I use?"

Currently everything test needs is included in tst_test.h which is all that should be included in the newlib testcase. Which is IMHO better than requiring to include new header for each couple of functions.

And
given that everything that the library exports should start either with TST_ tst_ SAFE_ or safe_ there shouldn't be any unexpected collisions either.

--
Cyril Hrubis
[email protected]

Quoted 1 line

Or do you have a better idea how to proceed?

I don't think I have. I was thinking about macros to generate both prototypes, but that would probably get messy soon.

One related idea I had for more "visual separation", was to create subdir, e.g. include/oldlib and move all oldlib exclusive headers there. And then add that dir to include dirs in CFLAGS, so nothing changes for tests.

Then (if I get the big picture), we could tell: "if you're looking for a function for your newlib test, just avoid _fn headers and anything in oldlib subdir". And same would apply for adding new functions to newlib.

What I image people do to discover these functions is search in ltp/include, and I'm guessing they'll get up to 3 hits: old header (which may or may not have old_ in name), _fn header and tst_ header.

This is what made me think if we can somehow steer them to look at newlib headers first.

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Tuesday, 5 April, 2016 4:16:28 PM
Subject: Re: [LTP] Test library API changes

Hi!

Quoted 20 lines

Quoted 1 line

Or do you have a better idea how to proceed?

I don't think I have. I was thinking about macros to generate both prototypes, but that would probably get messy soon.

One related idea I had for more "visual separation", was to create subdir, e.g. include/oldlib and move all oldlib exclusive headers there. And then add that dir to include dirs in CFLAGS, so nothing changes for tests.

Then (if I get the big picture), we could tell: "if you're looking for a function for your newlib test, just avoid _fn headers and anything in oldlib subdir". And same would apply for adding new functions to newlib.

What I image people do to discover these functions is search in ltp/include,
and I'm guessing they'll get up to 3 hits: old header (which may or may not have old_ in name), _fn header and tst_ header.

Quoted 2 lines

This is what made me think if we can somehow steer them to look at newlib headers first.

I've added a patch on the top of the one that adds the new library that moves the old headers to include/old. Now only config.h, tst_* headers and safe_*fn.h are directly in include.

Overall, I think we can merge it. I'd suggest to extend our usual test/freeze time before release to make sure people have enough time to try it.

Quoted 4 lines

* I've given up on SAFE_MACROS in cleaup for now

- We may add them later, but I would like to get this merged ideally before we do another release and doing this would take some time

I think we have this problem already. tst_brk() will call tst_vbrk_(), which calls cleanup_ipc(), which is using SAFE_* macros. It looks like we'll get infinite loop if any of those fail.

Hi!

Quoted 3 lines

Here are some comments/questions/ideas:

1. acnt appears to be unused now

I haven't removed it yet... I'm still thinking how we can generate things like TAP output that expects us to print exact number of tests on the first line followed by exact number of result lines and make as little rules for result reporting as possible.

Maybe we can just say that testcases that define test_all do one just one test and produce one result per such test and do more detailed reporting only for testcases that define tcnt.

Or we can turn on the detailed reporting only if acnt has been set.

Quoted 9 lines

I haven't removed it yet... I'm still thinking how we can generate things like TAP output that expects us to print exact number of tests on the first line followed by exact number of result lines and make as little rules for result reporting as possible.

Maybe we can just say that testcases that define test_all do one just one test and produce one result per such test and do more detailed reporting only for testcases that define tcnt.

Quoted 1 line

Or we can turn on the detailed reporting only if acnt has been set.

Not sure, but if I read TAP spec correctly, it doesn't strictly define that it should beon the first line, rather appear only once at the start or at the end (if the number of test points is not known).

So if tcnt for the test plan, what is 'acnt' for? ... I didn't find the analogy in the TAP spec.

Looking at the 'struct tst_test',

* the names like 'tcnt' and 'acnt' seem not clear. May be we should name 'tcnt' as
test_planor test_count?

* also, why bitfields in the struct tst_test have different number of storage bits?

   int needs_tmpdir:1;
   int needs_root:2;
   int forks_child:3;
   int needs_device:4;
   int needs_checkpoints:5;

if their value is just '0' or '1', ':1' - should be instead.