[LTP] [RFC] Getting rid of cleanup parameter

[LTP] [RFC] Getting rid of cleanup parameter

[Cyril Hrubis]

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?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [RFC] Getting rid of cleanup parameter

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: ltp@lists.linux.it
> Sent: Tuesday, 10 November, 2015 3:14:26 PM
> Subject: [LTP] [RFC] 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().

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?

> 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

> 
> Does everybody agree with this change?
> 
> --
> Cyril Hrubis
> chrubis@suse.cz
> 
> --
> Mailing list info: http://lists.linux.it/listinfo/ltp
> 

[LTP] [RFC] Getting rid of cleanup parameter

[Cyril Hrubis]

Hi!
> 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?

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.

> 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.

> > 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.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [RFC] Getting rid of cleanup parameter

[Jiri Jaburek]

On 11/11/2015 02:30 PM, Cyril Hrubis wrote:
> Hi!
>> 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?
> 
> 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.
> 
>> 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.

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).

> 
>>> 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.
> 

[LTP] [RFC] Getting rid of cleanup parameter

[Cyril Hrubis]

Hi!
> 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.

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.

> 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.

> 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.

-- 
Cyril Hrubis
chrubis@suse.cz