[{"id":2632,"web_url":"https://patchwork.libcamera.org/comment/2632/","msgid":"<20190910094040.GC4799@pendragon.ideasonboard.com>","date":"2019-09-10T09:40:40","subject":"Re: [libcamera-devel] [PATCH 1/3] test: process: Fix forking race","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Tue, Sep 10, 2019 at 10:04:16AM +0100, Kieran Bingham wrote:\n> The procFinished event handler is registered after the process is\n> started, leading to the opportunity of a missed race.\n\nThat shouldn't be the case, as the procFinished signal is emitted from\nthe event loop. Still, connecting the signal before starting the process\nis a good practice, so this patch is fine. You may want to mention in\nthe commit message that there's no actual race though.\n\nReviewed-by: Laurent Pinchart \n\n> Register the handler before the process is launched.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> test/process/process_test.cpp | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp\n> index d264555e545f..701f156b5053 100644\n> --- a/test/process/process_test.cpp\n> +++ b/test/process/process_test.cpp\n> @@ -47,12 +47,13 @@ protected:\n> \t\tint exitCode = 42;\n> \t\tvector args;\n> \t\targs.push_back(to_string(exitCode));\n> +\t\tproc_.finished.connect(this, &ProcessTest::procFinished);\n> +\n> \t\tint ret = proc_.start(\"/proc/self/exe\", args);\n> \t\tif (ret) {\n> \t\t\tcerr << \"failed to start process\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> -\t\tproc_.finished.connect(this, &ProcessTest::procFinished);\n> \n> \t\ttimeout.start(100);\n> \t\twhile (timeout.isRunning())","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CFD0260BCF\n\tfor ;\n\tTue, 10 Sep 2019 11:40:46 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [148.69.85.38])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 457132F9;\n\tTue, 10 Sep 2019 11:40:46 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1568108446;\n\tbh=54Pd1oeIi8TqJEKbZBpoktL64IdVOtvNeIQG2rmMy5Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=V3jr+c1wxlFpffykafU8aHZXMGTxiY63iwtDP0mBANqQ2qzORA/7KtKPSZY+9Oz6S\n\tP7080tOBZ3DP8SLdhC5BfNsAfJLjimSXegcYpCdcDMJL8hkZJz2vUS5PWmf7atuSnk\n\tXsWBWR/AiWflfgxDqBPLjHPZK9Xz1Mdoi7uSc2S0=","Date":"Tue, 10 Sep 2019 12:40:40 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190910094040.GC4799@pendragon.ideasonboard.com>","References":"<20190910090418.30502-1-kieran.bingham@ideasonboard.com>\n\t<20190910090418.30502-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190910090418.30502-2-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/3] test: process: Fix forking race","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 10 Sep 2019 09:40:47 -0000"}},{"id":2635,"web_url":"https://patchwork.libcamera.org/comment/2635/","msgid":"","date":"2019-09-10T10:02:56","subject":"Re: [libcamera-devel] [PATCH 1/3] test: process: Fix forking race","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 10/09/2019 10:40, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Tue, Sep 10, 2019 at 10:04:16AM +0100, Kieran Bingham wrote:\n>> The procFinished event handler is registered after the process is\n>> started, leading to the opportunity of a missed race.\n> \n> That shouldn't be the case, as the procFinished signal is emitted from\n> the event loop. Still, connecting the signal before starting the process\n> is a good practice, so this patch is fine. You may want to mention in\n> the commit message that there's no actual race though.\n\nAha, OK so I misunderstood the code.\n\nDo the events get 'cached' and stored then ? If a slot is not connected,\ncan we accumulate an unlimited amount of events?\n\nSo if we connect a signal/slot at any arbitrary point in time later -\nwill we receive all of the historical calls? Or just the latest?\n\n\nI've confirmed there is no race here by putting an arbitrary 5 second\nsleep after calling .start(), and before connecting the signal and it\nstill works.\n\n\n\n> Reviewed-by: Laurent Pinchart \n\nThanks, I'll update the commit message here as there is indeed no actual\nrace.\n\nBut it does make me feel a lot better to hook up the dependencies first :-)\n\n> \n>> Register the handler before the process is launched.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> test/process/process_test.cpp | 3 ++-\n>> 1 file changed, 2 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp\n>> index d264555e545f..701f156b5053 100644\n>> --- a/test/process/process_test.cpp\n>> +++ b/test/process/process_test.cpp\n>> @@ -47,12 +47,13 @@ protected:\n>> \t\tint exitCode = 42;\n>> \t\tvector args;\n>> \t\targs.push_back(to_string(exitCode));\n>> +\t\tproc_.finished.connect(this, &ProcessTest::procFinished);\n>> +\n>> \t\tint ret = proc_.start(\"/proc/self/exe\", args);\n>> \t\tif (ret) {\n>> \t\t\tcerr << \"failed to start process\" << endl;\n>> \t\t\treturn TestFail;\n>> \t\t}\n>> -\t\tproc_.finished.connect(this, &ProcessTest::procFinished);\n>> \n>> \t\ttimeout.start(100);\n>> \t\twhile (timeout.isRunning())\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A73ED60BE6\n\tfor ;\n\tTue, 10 Sep 2019 12:02:59 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 09AA94FF;\n\tTue, 10 Sep 2019 12:02:58 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1568109779;\n\tbh=0/sjQYLmRViOHUlagrYgumv/StGwtpEvfyDQJgCPTvM=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=USM9Y9Y89N/rOuNVbaIYrKDWpfrD7tbEKSzhERVQu255bWR70bPYEAbS8hoN/AFGc\n\tXHJDlE1Tu9ffMf3iUDjb7oH39xPKMeoavmQnBptfq59w5y9ZQ/m5bk5pHjXBmNDJNE\n\tIZjVbFNftc0AfVLvRScGDNHoB+s7sdOn4gTq9kPQ=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","References":"<20190910090418.30502-1-kieran.bingham@ideasonboard.com>\n\t<20190910090418.30502-2-kieran.bingham@ideasonboard.com>\n\t<20190910094040.GC4799@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCJQQYAQoADwIbDAUCWcOUawUJ\n\tB4D+AgAKCRChHkZyEKRh/XJhEACr5iidt/0MZ0rWRMCbZFMWD7D2g6nZeOp+F2zY8CEUW+sd\n\tCDVd9BH9QX9KN5SZo6YtJzMzSzpcx45VwTvtQW0n/6Eujg9EUqblfU9xqvqDmbjEapr5d/OL\n\t21GTALb0owKhA5qDUGEcKGCphpQffKhTNo/BP99jvmJUj7IPSKH97qPypi8/ym8bAxB+uY31\n\tgHTMHf1jMJJ1pRo2tYYPeIIHGDqXBI4sp5GHHF+JcIhgR/e/A6w/dgzHYmQPl2ix5eZYEZbV\n\tTRP+gkX4NV8oHqa/lR+xPOlWElGB57viOSOoWriqxQbFy8XbG1GR8cWlkNtGBGVWaJaSoORP\n\tiowD7irXL91bCyFIqL+7BVk3Jy4uzP744PzE80KwxOp5SQAp9sPzFbgsJrLev90PZySjFHG0\n\twP144DK7nBjOj/J0g9OHVASP1JjK+nw7SDoKnETDIdRC0XmiHXk7TXzPdkvO0UkpHdEPjZUp\n\tWyuc0MqehjR/hTTPt4m/Y14XzEcy6JREIjOrFfUZVho2QpOdv9CNryGdieRTNjUtz463CIaZ\n\tdPBiw9mOMBoNffkn9FIoCjLnAaj9gUAnEHWBZOEviQ5NuyqpeP0YtzI4iaRbSUkYZHej99X3\n\tVmHrdLlMqd/ZgYYbPGSL4AN3FVACb5CxuxEHwo029VcE5U3CSjzqtCoX12tm7A==","Organization":"Ideas on Board","Message-ID":"","Date":"Tue, 10 Sep 2019 11:02:56 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.8.0","MIME-Version":"1.0","In-Reply-To":"<20190910094040.GC4799@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 1/3] test: process: Fix forking race","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 10 Sep 2019 10:02:59 -0000"}},{"id":2636,"web_url":"https://patchwork.libcamera.org/comment/2636/","msgid":"<20190910100848.GF4799@pendragon.ideasonboard.com>","date":"2019-09-10T10:08:48","subject":"Re: [libcamera-devel] [PATCH 1/3] test: process: Fix forking race","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Sep 10, 2019 at 11:02:56AM +0100, Kieran Bingham wrote:\n> On 10/09/2019 10:40, Laurent Pinchart wrote:\n> > On Tue, Sep 10, 2019 at 10:04:16AM +0100, Kieran Bingham wrote:\n> >> The procFinished event handler is registered after the process is\n> >> started, leading to the opportunity of a missed race.\n> > \n> > That shouldn't be the case, as the procFinished signal is emitted from\n> > the event loop. Still, connecting the signal before starting the process\n> > is a good practice, so this patch is fine. You may want to mention in\n> > the commit message that there's no actual race though.\n> \n> Aha, OK so I misunderstood the code.\n> \n> Do the events get 'cached' and stored then ? If a slot is not connected,\n> can we accumulate an unlimited amount of events?\n\nIt's not about signals/slots, it's about listening to events. The\nfinished signal is emitted from Process::died(), called by\nProcessManager::sighandler(), itself a slot connected to the activated\nsignal of the sigEvent_ event notifier. The event notifier listens for\nread events on the pipe used to communicate between the unix signal\nhandler (sigact() in process.cpp). Thus, when SIGCHLD is caught, we\nwrite to the pipe, and the notification on the other side goes through\nthe event notifier, which is processed by the event loop.\n\n> So if we connect a signal/slot at any arbitrary point in time later -\n> will we receive all of the historical calls? Or just the latest?\n\nSignals are not buffered, we keep no history.\n\n> I've confirmed there is no race here by putting an arbitrary 5 second\n> sleep after calling .start(), and before connecting the signal and it\n> still works.\n> \n> > Reviewed-by: Laurent Pinchart \n> \n> Thanks, I'll update the commit message here as there is indeed no actual\n> race.\n> \n> But it does make me feel a lot better to hook up the dependencies first :-)\n> \n> >> Register the handler before the process is launched.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> test/process/process_test.cpp | 3 ++-\n> >> 1 file changed, 2 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp\n> >> index d264555e545f..701f156b5053 100644\n> >> --- a/test/process/process_test.cpp\n> >> +++ b/test/process/process_test.cpp\n> >> @@ -47,12 +47,13 @@ protected:\n> >> \t\tint exitCode = 42;\n> >> \t\tvector args;\n> >> \t\targs.push_back(to_string(exitCode));\n> >> +\t\tproc_.finished.connect(this, &ProcessTest::procFinished);\n> >> +\n> >> \t\tint ret = proc_.start(\"/proc/self/exe\", args);\n> >> \t\tif (ret) {\n> >> \t\t\tcerr << \"failed to start process\" << endl;\n> >> \t\t\treturn TestFail;\n> >> \t\t}\n> >> -\t\tproc_.finished.connect(this, &ProcessTest::procFinished);\n> >> \n> >> \t\ttimeout.start(100);\n> >> \t\twhile (timeout.isRunning())","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2DEC960BE6\n\tfor ;\n\tTue, 10 Sep 2019 12:08:55 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [148.69.85.38])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9A4074FF;\n\tTue, 10 Sep 2019 12:08:54 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1568110134;\n\tbh=Qe2RXEJbv0sqHsXVg+vk8qWQgz1K9azGxE5FUbbScUU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=k08vhPgN7s6ASKP+j+YqMASNcqfoxyCv/N4sfiAQh/0KPA7Xjm1Nt8nvzG7D0A7sf\n\tdBPcfZlVisaWp1QIgwisKpe6443dkuM7LFqz1vwhH3mag2aa56Zhkhb9I8+pVrIbRi\n\tnrofS/E+O/jTZJaOmT+QI9ezf411HG3E3yHlJbgk=","Date":"Tue, 10 Sep 2019 13:08:48 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190910100848.GF4799@pendragon.ideasonboard.com>","References":"<20190910090418.30502-1-kieran.bingham@ideasonboard.com>\n\t<20190910090418.30502-2-kieran.bingham@ideasonboard.com>\n\t<20190910094040.GC4799@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/3] test: process: Fix forking race","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 10 Sep 2019 10:08:55 -0000"}},{"id":2637,"web_url":"https://patchwork.libcamera.org/comment/2637/","msgid":"","date":"2019-09-11T08:19:16","subject":"Re: [libcamera-devel] [PATCH 1/3] test: process: Fix forking race","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 10/09/2019 11:08, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Tue, Sep 10, 2019 at 11:02:56AM +0100, Kieran Bingham wrote:\n>>\n>>> Reviewed-by: Laurent Pinchart \n>>\n>> Thanks, I'll update the commit message here as there is indeed no actual\n>> race.\n>>\n>> But it does make me feel a lot better to hook up the dependencies first :-)\n\nIs this rewrite acceptable to you ?\n\n\ntest: process: Connect signal before launching process\n\nThe procFinished event handler is registered after the process is\nstarted. This doesn't actually create any race, as the finished signal\nis emitted after a SIGCHLD is caught and handled through the\nProcessManager and processed by the event loop.\n\nTo make it clear that resources are connected before the process runs,\nregister the handler before the process is started.\n\n\n\n>>>> Signed-off-by: Kieran Bingham \n>>>> ---\n>>>> test/process/process_test.cpp | 3 ++-\n>>>> 1 file changed, 2 insertions(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp\n>>>> index d264555e545f..701f156b5053 100644\n>>>> --- a/test/process/process_test.cpp\n>>>> +++ b/test/process/process_test.cpp\n>>>> @@ -47,12 +47,13 @@ protected:\n>>>> \t\tint exitCode = 42;\n>>>> \t\tvector args;\n>>>> \t\targs.push_back(to_string(exitCode));\n>>>> +\t\tproc_.finished.connect(this, &ProcessTest::procFinished);\n>>>> +\n>>>> \t\tint ret = proc_.start(\"/proc/self/exe\", args);\n>>>> \t\tif (ret) {\n>>>> \t\t\tcerr << \"failed to start process\" << endl;\n>>>> \t\t\treturn TestFail;\n>>>> \t\t}\n>>>> -\t\tproc_.finished.connect(this, &ProcessTest::procFinished);\n>>>> \n>>>> \t\ttimeout.start(100);\n>>>> \t\twhile (timeout.isRunning())\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9669A600E9\n\tfor ;\n\tWed, 11 Sep 2019 10:19:20 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0115A33A;\n\tWed, 11 Sep 2019 10:19:19 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1568189960;\n\tbh=NEBFFjrIg+KqMubCUSpzFx2FmirL17ZCAI20Fc/t13g=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=RUB8ju10S5o7cF57mAmgBmUcHVSGSoBzA9S5DahVFYdfEpuQE5tDOoes0bIZF25g2\n\tBglaVc00mfm85T0jyzfPWKrkx2e8A0iz0MvEzSzT2ANIBT/1wQ9f/hnj8b+Q/H2/T/\n\tw+kwB/lB3tNmpeZoD5UEZZfUOA/n/x6gBuOMcm34=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","References":"<20190910090418.30502-1-kieran.bingham@ideasonboard.com>\n\t<20190910090418.30502-2-kieran.bingham@ideasonboard.com>\n\t<20190910094040.GC4799@pendragon.ideasonboard.com>\n\t\n\t<20190910100848.GF4799@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCJQQYAQoADwIbDAUCWcOUawUJ\n\tB4D+AgAKCRChHkZyEKRh/XJhEACr5iidt/0MZ0rWRMCbZFMWD7D2g6nZeOp+F2zY8CEUW+sd\n\tCDVd9BH9QX9KN5SZo6YtJzMzSzpcx45VwTvtQW0n/6Eujg9EUqblfU9xqvqDmbjEapr5d/OL\n\t21GTALb0owKhA5qDUGEcKGCphpQffKhTNo/BP99jvmJUj7IPSKH97qPypi8/ym8bAxB+uY31\n\tgHTMHf1jMJJ1pRo2tYYPeIIHGDqXBI4sp5GHHF+JcIhgR/e/A6w/dgzHYmQPl2ix5eZYEZbV\n\tTRP+gkX4NV8oHqa/lR+xPOlWElGB57viOSOoWriqxQbFy8XbG1GR8cWlkNtGBGVWaJaSoORP\n\tiowD7irXL91bCyFIqL+7BVk3Jy4uzP744PzE80KwxOp5SQAp9sPzFbgsJrLev90PZySjFHG0\n\twP144DK7nBjOj/J0g9OHVASP1JjK+nw7SDoKnETDIdRC0XmiHXk7TXzPdkvO0UkpHdEPjZUp\n\tWyuc0MqehjR/hTTPt4m/Y14XzEcy6JREIjOrFfUZVho2QpOdv9CNryGdieRTNjUtz463CIaZ\n\tdPBiw9mOMBoNffkn9FIoCjLnAaj9gUAnEHWBZOEviQ5NuyqpeP0YtzI4iaRbSUkYZHej99X3\n\tVmHrdLlMqd/ZgYYbPGSL4AN3FVACb5CxuxEHwo029VcE5U3CSjzqtCoX12tm7A==","Organization":"Ideas on Board","Message-ID":"","Date":"Wed, 11 Sep 2019 09:19:16 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.8.0","MIME-Version":"1.0","In-Reply-To":"<20190910100848.GF4799@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 1/3] test: process: Fix forking race","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 11 Sep 2019 08:19:20 -0000"}},{"id":2638,"web_url":"https://patchwork.libcamera.org/comment/2638/","msgid":"<20190912195033.GC6006@pendragon.ideasonboard.com>","date":"2019-09-12T19:50:33","subject":"Re: [libcamera-devel] [PATCH 1/3] test: process: Fix forking race","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Sep 11, 2019 at 09:19:16AM +0100, Kieran Bingham wrote:\n> On 10/09/2019 11:08, Laurent Pinchart wrote:\n> > On Tue, Sep 10, 2019 at 11:02:56AM +0100, Kieran Bingham wrote:\n> >>\n> >>> Reviewed-by: Laurent Pinchart \n> >>\n> >> Thanks, I'll update the commit message here as there is indeed no actual\n> >> race.\n> >>\n> >> But it does make me feel a lot better to hook up the dependencies first :-)\n> \n> Is this rewrite acceptable to you ?\n> \n> test: process: Connect signal before launching process\n> \n> The procFinished event handler is registered after the process is\n> started. This doesn't actually create any race, as the finished signal\n> is emitted after a SIGCHLD is caught and handled through the\n> ProcessManager and processed by the event loop.\n> \n> To make it clear that resources are connected before the process runs,\n> register the handler before the process is started.\n\nI would say something along the lines of \"However, to follow the best\npractice that resources should be acquired before performing an action,\nconnect the finished signal before starting the process.\".\n\nReviewed-by: Laurent Pinchart \n\n> >>>> Signed-off-by: Kieran Bingham \n> >>>> ---\n> >>>> test/process/process_test.cpp | 3 ++-\n> >>>> 1 file changed, 2 insertions(+), 1 deletion(-)\n> >>>>\n> >>>> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp\n> >>>> index d264555e545f..701f156b5053 100644\n> >>>> --- a/test/process/process_test.cpp\n> >>>> +++ b/test/process/process_test.cpp\n> >>>> @@ -47,12 +47,13 @@ protected:\n> >>>> \t\tint exitCode = 42;\n> >>>> \t\tvector args;\n> >>>> \t\targs.push_back(to_string(exitCode));\n> >>>> +\t\tproc_.finished.connect(this, &ProcessTest::procFinished);\n> >>>> +\n> >>>> \t\tint ret = proc_.start(\"/proc/self/exe\", args);\n> >>>> \t\tif (ret) {\n> >>>> \t\t\tcerr << \"failed to start process\" << endl;\n> >>>> \t\t\treturn TestFail;\n> >>>> \t\t}\n> >>>> -\t\tproc_.finished.connect(this, &ProcessTest::procFinished);\n> >>>> \n> >>>> \t\ttimeout.start(100);\n> >>>> \t\twhile (timeout.isRunning())","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CCF2D600E9\n\tfor ;\n\tThu, 12 Sep 2019 21:50:40 +0200 (CEST)","from pendragon.ideasonboard.com (bl10-204-24.dsl.telepac.pt\n\t[85.243.204.24])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E7DF133A;\n\tThu, 12 Sep 2019 21:50:39 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1568317840;\n\tbh=m5yGjtG/kVObHxWoo5NkhHvE8PgSODD3Q7qAU3AFtOs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uu9sOivI/dcBABA4QCJH2C4vX3ZI7dm7GeWiEdzRkLJfZsYm4by1EXjc66Sh7urBd\n\tiolnkG3XwD/3zje9pEMOpY+rNK6vY640Wjp92YbbTxfXcY9vl7dUkLASw7d9yhgrbz\n\tLs0LMQTJ45pDuFqm+m1Mg/Y4reM9Gjx4jpT672gI=","Date":"Thu, 12 Sep 2019 22:50:33 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190912195033.GC6006@pendragon.ideasonboard.com>","References":"<20190910090418.30502-1-kieran.bingham@ideasonboard.com>\n\t<20190910090418.30502-2-kieran.bingham@ideasonboard.com>\n\t<20190910094040.GC4799@pendragon.ideasonboard.com>\n\t\n\t<20190910100848.GF4799@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/3] test: process: Fix forking race","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 12 Sep 2019 19:50:41 -0000"}}]