Getting rid of cleanup parameter
Hi!
I was looking into the LTP API expecially the tst_res* part to make it cleaner and easier to use and one of the things I would like to change is how we execute the cleanup function.
Currenlty we pass the cleanup as function parameter. The 99% of the time we just pass the very same function to every call with the exception of the calls in the cleanup function itself. And the callback function is made in a way that it could be called at any point in the setup() as well.
The common mistakes with that API is that people pass cleanup() paramter to functions in cleanup() and to tst api in child processes. But as we use it now the cleanup can be set exactly once and called from the test library which could make sure that it's called exactly once, etc.
So my proposal is to add a call to set cleanup function, tst_set_cleanup(void (*cleanup)(void)) that would be called once in the setup and would store the function pointer, which would later be called either when test exits prematurely or on tst_exit(). When the cleanup function was set with this interface the cleanup paramter for all functions would be ignored (we may create static inline wrappers that sets it to NULL to be used from new code).
Does everybody agree with this change?
Signature · 2 lines
Cyril Hrubis [email protected]
----- Original Message -----
Quoted 21 lines
From: "Cyril Hrubis" <[email protected]>
To: [email protected]
Sent: Tuesday, 10 November, 2015 3:14:26 PM
Subject: [LTP] [RFC] Getting rid of cleanup parameterHi!
I was looking into the LTP API expecially the tst_res* part to make it cleaner and easier to use and one of the things I would like to change is how we execute the cleanup function.Currenlty we pass the cleanup as function parameter. The 99% of the time we just pass the very same function to every call with the exception of the calls in the cleanup function itself. And the callback function is made in a way that it could be called at any point in the
setup()as well.The common mistakes with that API is that people pass
cleanup()paramter to functions incleanup()and to tst api in child processes. But as we use it now the cleanup can be set exactly once and called from the test library which could make sure that it's called exactly once, etc.
Quoted 4 lines
So my proposal is to add a call to set cleanup function,
tst_set_cleanup(void (*cleanup)(void))that would be called once in the setup and would store the function pointer, which would later be called either when test exits prematurely or ontst_exit().
Just thinking loud, how this would work:
Is the scope of cleanup set with tst_set_cleanup() going to be per process? For example: If I call tst_set_cleanup() and then fork couple children, will they automatically ignore cleanup function set in parent? Can I use tst_set_cleanup() in child process to setup child-specific cleanup function?
Quoted 4 lines
When the cleanup
function was set with this interface the cleanup paramter for all functions would be ignored (we may create static inline wrappers that sets it to NULL to be used from new code).
And perhaps trigger a warning/TBROK if user tries to pass non-NULL value while he's using the new tst_set_cleanup() approach.
Regards,
Jan
Quoted 10 lines
Does everybody agree with this change?
--
Cyril Hrubis
[email protected]--
Mailing list info: http://lists.linux.it/listinfo/ltp
Hi!
Quoted 5 lines
Just thinking loud, how this would work:
Is the scope of cleanup set with
tst_set_cleanup()going to be per process? For example: If I calltst_set_cleanup()and then fork couple children, will they automatically ignore cleanup function set in parent?
It has to be per process, since othewise we would have the callback called twice problem again. I.e. the callback set in test setup() would be executed only if tst_* call that caused exit was called from the same process it has been set up.
Quoted 2 lines
Can I use
tst_set_cleanup()in child process to setup child-specific cleanup function?
Currently I would do it so that only the main test process can setup the callback in order to make it simpler. Since so far I haven't found a situation where the parent process cannot easily cleanup after it's children and doing cleanup once decreases the possibility of races in concurently executed cleanups. Imaginge 100 children cleaning after themselves, it's easy to mess up when 100 cleanup functions are running at the same time.
But if you found good usecase we may also allow per-child cleanups.
Quoted 7 lines
Quoted 4 lines
When the cleanup
function was set with this interface the cleanup paramter for all functions would be ignored (we may create static inline wrappers that sets it to NULL to be used from new code).And perhaps trigger a warning/TBROK if user tries to pass non-NULL value while he's using the new
tst_set_cleanup()approach.
Yep.
Signature · 2 lines
Cyril Hrubis [email protected]
Quoted 23 lines · Cyril Hrubis
Hi!
Quoted 5 lines
Just thinking loud, how this would work:
Is the scope of cleanup set with
tst_set_cleanup()going to be per process? For example: If I calltst_set_cleanup()and then fork couple children, will they automatically ignore cleanup function set in parent?It has to be per process, since othewise we would have the callback called twice problem again. I.e. the callback set in test
setup()would be executed only if tst_* call that caused exit was called from the same process it has been set up.Quoted 2 lines
Can I use
tst_set_cleanup()in child process to setup child-specific cleanup function?Currently I would do it so that only the main test process can setup the callback in order to make it simpler. Since so far I haven't found a situation where the parent process cannot easily cleanup after it's children and doing cleanup once decreases the possibility of races in concurently executed cleanups. Imaginge 100 children cleaning after themselves, it's easy to mess up when 100 cleanup functions are running at the same time.
Quoted 1 line
But if you found good usecase we may also allow per-child cleanups.
This may be the case when a test spawns multiple children to do the testing in parallel, with the children creating shared resources of unpredictable names/references (so the cleanup cannot be done from the parent).
I currently cannot think of any such resource,
* files can use tmpdir, removed recursively from parent
* spawned processes can be walked from the parent
* losetup-created devices can have a common prefix
* IPC shared resources can be pre-created by the parent (or be found via a unique key or a dedicated unix user)
* linux namespaces die automatically with the children
* ...
The general pattern is that the parent is always able to somehow use an identifier common for all children or pre-create the resources for them, collecting a list to be used for cleanup.
However it would be good to keep the use case in mind for the future.
Regarding tst_set_cleanup() - if it hooks the cleanup function into signal handlers as well, it will need to be called before sigaction setup in signal-testing tests. Nothing extraordinary, I guess, but still something to note.
About shell tests - maybe something similar would be useful, for consistency (same function name) as well as signal handling (trap) in shells that support it. Again - same signal handling limitations apply (if test re-defines trap, it may be an issue).
Quoted 11 lines
Quoted 8 lines
Quoted 5 lines
When the cleanup
function was set with this interface the cleanup paramter for all functions would be ignored (we may create static inline wrappers that sets it to NULL to be used from new code).And perhaps trigger a warning/TBROK if user tries to pass non-NULL value while he's using the new
tst_set_cleanup()approach.Yep.
Hi!
Quoted 15 lines
This may be the case when a test spawns multiple children to do the testing in parallel, with the children creating shared resources of unpredictable names/references (so the cleanup cannot be done from the parent).
I currently cannot think of any such resource,
* files can use tmpdir, removed recursively from parent
* spawned processes can be walked from the parent
* losetup-created devices can have a common prefix
* IPC shared resources can be pre-created by the parent (or be found via a unique key or a dedicated unix user)
* linux namespaces die automatically with the children
* ...
Quoted 5 lines
The general pattern is that the parent is always able to somehow use an identifier common for all children or pre-create the resources for them, collecting a list to be used for cleanup.
However it would be good to keep the use case in mind for the future.
That is the same reasoning as I did. So I would go for disabling the ability for cleanup function in child processes util we find that it's necessary or that it fairly simplifies a solution to a given problem.
Quoted 4 lines
Regarding
tst_set_cleanup()- if it hooks the cleanup function into signal handlers as well, it will need to be called before sigaction setup in signal-testing tests. Nothing extraordinary, I guess, but still something to note.
Right, I do not expect it to be called anywhere else than beginning of the setup fuction.
And we also have tst_sig() function that is able to install a cleanup for unexpected signals that 99% of the time installs the common test cleanup(). I guess that we should remove the tst_sig() in favor of the tst_set_cleanup() as well. We would have to be able to tell the tst_set_cleanup() not to install the the "poisoned" handler at least for SIGCHLD.
Or we should simply install only handlers that actually make sense to exit the test with TBROK. I find it silly that the LTP testcases break when you change the terminal size while they are running and the test exits because it does not expectet to get SIGWINCH.
On the other way as it is now we would catch situations where kernel send wrong signal to the process. Not sure if that actually had happened.
Quoted 4 lines
About shell tests - maybe something similar would be useful, for consistency (same function name) as well as signal handling (trap) in shells that support it. Again - same signal handling limitations apply (if test re-defines trap, it may be an issue).
Actually this change would get us closer to the shell interface, since the test.sh library currently has TST_CLEANUP variable that is evaluated before the test exits. So if we add the tst_set_cleanup() function that would do export TST_CLEANUP="$1" we will end up with the same interface.
The traps are not there at the moment but that would be just a few lines of code.
Signature · 2 lines
Cyril Hrubis [email protected]