[libcamera-devel] test: process: Test Process::kill()

9016 diff mbox series

Add a test to ensure that Process::kill() on an unstarted process is
safe. This aims at ensuring we don't kill other processes unexpectedly.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 test/process/process_test.cpp | 4 ++++
 1 file changed, 4 insertions(+)

If you want to try this at home *without* the corresponding fix to the
Process class ("libcamera: process: Fix killing innocent processes
unexpectedly"), make sure to save all your work first. It's a very
effective implementation of an ejection seat.

Hi Laurent,

On 27/07/2020 13:53, Laurent Pinchart wrote:
> Add a test to ensure that Process::kill() on an unstarted process is
> safe. This aims at ensuring we don't kill other processes unexpectedly.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  test/process/process_test.cpp | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> If you want to try this at home *without* the corresponding fix to the
> Process class ("libcamera: process: Fix killing innocent processes
> unexpectedly"), make sure to save all your work first. It's a very
> effective implementation of an ejection seat.
> 
> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
> index ce0cc7c972cd..721a7c9d46ff 100644
> --- a/test/process/process_test.cpp
> +++ b/test/process/process_test.cpp
> @@ -51,6 +51,10 @@ protected:
>  		args.push_back(to_string(exitCode));
>  		proc_.finished.connect(this, &ProcessTest::procFinished);
>  
> +		/* Test that kill() on an unstarted process is safe. */
> +		proc_.kill();
> +

Perhaps this is one test where we should merge the fix *first* :-)

Do we need to do anything to actually make sure no other process is
killed? I presume it will become quite self evident, as probably all of
the test environment might disappear to succinctly fail the test ;-)

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

(As long as I don't have to provide a Tested-by: tag...)



> +		/* Test starting the process and retrieving the exit code. */
>  		int ret = proc_.start("/proc/self/exe", args);
>  		if (ret) {
>  			cerr << "failed to start process" << endl;
>

Hi Kieran,

On Mon, Jul 27, 2020 at 02:15:14PM +0100, Kieran Bingham wrote:
> On 27/07/2020 13:53, Laurent Pinchart wrote:
> > Add a test to ensure that Process::kill() on an unstarted process is
> > safe. This aims at ensuring we don't kill other processes unexpectedly.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  test/process/process_test.cpp | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > If you want to try this at home *without* the corresponding fix to the
> > Process class ("libcamera: process: Fix killing innocent processes
> > unexpectedly"), make sure to save all your work first. It's a very
> > effective implementation of an ejection seat.
> > 
> > diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
> > index ce0cc7c972cd..721a7c9d46ff 100644
> > --- a/test/process/process_test.cpp
> > +++ b/test/process/process_test.cpp
> > @@ -51,6 +51,10 @@ protected:
> >  		args.push_back(to_string(exitCode));
> >  		proc_.finished.connect(this, &ProcessTest::procFinished);
> >  
> > +		/* Test that kill() on an unstarted process is safe. */
> > +		proc_.kill();
> > +
> 
> Perhaps this is one test where we should merge the fix *first* :-)

It's merged already :-)

> Do we need to do anything to actually make sure no other process is
> killed? I presume it will become quite self evident, as probably all of
> the test environment might disappear to succinctly fail the test ;-)

Manually checking that no other process was impacted would be difficult.
I think making sure that we don't kill(-1) is enough. That one is very
noticeable :-)

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> (As long as I don't have to provide a Tested-by: tag...)
> 
> > +		/* Test starting the process and retrieving the exit code. */
> >  		int ret = proc_.start("/proc/self/exe", args);
> >  		if (ret) {
> >  			cerr << "failed to start process" << endl;