[libcamera-devel] [PATCH 1/3] test: process: Fix forking race

[Laurent Pinchart]

Hi Kieran,

On Wed, Sep 11, 2019 at 09:19:16AM +0100, Kieran Bingham wrote:
> On 10/09/2019 11:08, Laurent Pinchart wrote:
> > On Tue, Sep 10, 2019 at 11:02:56AM +0100, Kieran Bingham wrote:
> >>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>
> >> Thanks, I'll update the commit message here as there is indeed no actual
> >> race.
> >>
> >> But it does make me feel a lot better to hook up the dependencies first :-)
> 
> Is this rewrite acceptable to you ?
> 
> test: process: Connect signal before launching process
> 
> The procFinished event handler is registered after the process is
> started. This doesn't actually create any race, as the finished signal
> is emitted after a SIGCHLD is caught and handled through the
> ProcessManager and processed by the event loop.
> 
> To make it clear that resources are connected before the process runs,
> register the handler before the process is started.

I would say something along the lines of "However, to follow the best
practice that resources should be acquired before performing an action,
connect the finished signal before starting the process.".

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> >>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>>> ---
> >>>>  test/process/process_test.cpp | 3 ++-
> >>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
> >>>> index d264555e545f..701f156b5053 100644
> >>>> --- a/test/process/process_test.cpp
> >>>> +++ b/test/process/process_test.cpp
> >>>> @@ -47,12 +47,13 @@ protected:
> >>>>  		int exitCode = 42;
> >>>>  		vector<std::string> args;
> >>>>  		args.push_back(to_string(exitCode));
> >>>> +		proc_.finished.connect(this, &ProcessTest::procFinished);
> >>>> +
> >>>>  		int ret = proc_.start("/proc/self/exe", args);
> >>>>  		if (ret) {
> >>>>  			cerr << "failed to start process" << endl;
> >>>>  			return TestFail;
> >>>>  		}
> >>>> -		proc_.finished.connect(this, &ProcessTest::procFinished);
> >>>>  
> >>>>  		timeout.start(100);
> >>>>  		while (timeout.isRunning())

-- 
Regards,

Laurent Pinchart