[{"id":18904,"web_url":"https://patchwork.libcamera.org/comment/18904/","msgid":"<20210818094155.GJ1733965@pyrite.rasen.tech>","date":"2021-08-18T09:41:55","subject":"Re: [libcamera-devel] [PATCH v2 2/2] test: camera: Camera\n\treconfiguration and fd-leak test","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Umang,\n\nOn Wed, Aug 18, 2021 at 02:32:47PM +0530, Umang Jain wrote:\n> This tests basically checks for two things:\n> - Camera reconfigurations without stopping CameraManager\n> - Fd leaks across IPA IPC boundary [1]\n> \n> Currently, it uses vimc, but can be easily changed to using another\n> platform (for e.g. IPU3) by changing kCamId_ and kIpaProxyName_.\n> \n> The test performs kNumOfReconfigures_ (currently set to 10)\n> reconfigurations of the camera. Each reconfiguration runs\n> start(), capture(100ms), stop() of the camera. Hence, the test\n> runs approximately for 1 second.\n> \n> For checking the fd leaks, the test monitors the /proc/$PROXY_PID/fd\n> directory for open fds. It compares the number of open fds after each\n> run to the number of open fds before the first run. If those two are\n> found to be mis-matched, the test shall report failure.\n> \n> Since, the test needs to test the IPA IPC code paths, it is meant to\n> always run with LIBCAMERA_IPA_FORCE_ISOLATION=1 environment variable.\n> \n> [1] https://bugs.libcamera.org/show_bug.cgi?id=63\n> \n> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n\nReviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n\n> ---\n>  test/camera/camera_reconfigure.cpp | 255 +++++++++++++++++++++++++++++\n>  test/camera/meson.build            |   1 +\n>  2 files changed, 256 insertions(+)\n>  create mode 100644 test/camera/camera_reconfigure.cpp\n> \n> diff --git a/test/camera/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp\n> new file mode 100644\n> index 00000000..90f798fe\n> --- /dev/null\n> +++ b/test/camera/camera_reconfigure.cpp\n> @@ -0,0 +1,255 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * Test:\n> + * - Camera's multiple reconfigurations without stopping CameraManager\n> + * - leaking fds across IPA IPC boundary\n> + */\n> +\n> +#include <dirent.h>\n> +#include <fstream>\n> +#include <iostream>\n> +\n> +#include <libcamera/base/event_dispatcher.h>\n> +#include <libcamera/base/file.h>\n> +#include <libcamera/base/thread.h>\n> +#include <libcamera/base/timer.h>\n> +\n> +#include <libcamera/framebuffer_allocator.h>\n> +\n> +#include \"camera_test.h\"\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +\n> +namespace {\n> +\n> +class CameraReconfigure : public CameraTest, public Test\n> +{\n> +public:\n> +\tCameraReconfigure()\n> +\t\t: CameraTest(kCamId_, true)\n> +\t{\n> +\t}\n> +\n> +private:\n> +\tstatic constexpr const char *kCamId_ = \"platform/vimc.0 Sensor B\";\n> +\tstatic constexpr const char *kIpaProxyName_ = \"vimc_ipa_proxy\";\n> +\tstatic constexpr unsigned int kNumOfReconfigures_ = 10;\n> +\n> +\tvoid requestComplete(Request *request)\n> +\t{\n> +\t\tif (request->status() != Request::RequestComplete)\n> +\t\t\treturn;\n> +\n> +\t\tconst Request::BufferMap &buffers = request->buffers();\n> +\n> +\t\t/* Create a new request. */\n> +\t\tconst Stream *stream = buffers.begin()->first;\n> +\t\tFrameBuffer *buffer = buffers.begin()->second;\n> +\n> +\t\trequest->reuse();\n> +\t\trequest->addBuffer(stream, buffer);\n> +\t\tcamera_->queueRequest(request);\n> +\t}\n> +\n> +\tint startAndStop()\n> +\t{\n> +\t\tStreamConfiguration &cfg = config_->at(0);\n> +\n> +\t\tif (camera_->acquire()) {\n> +\t\t\tcerr << \"Failed to acquire the camera\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (camera_->configure(config_.get())) {\n> +\t\t\tcerr << \"Failed to set default configuration\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tStream *stream = cfg.stream();\n> +\n> +\t\tif (!allocated_) {\n> +\t\t\tint ret = allocator_->allocate(stream);\n> +\t\t\tif (ret < 0)\n> +\t\t\t\treturn TestFail;\n> +\t\t\tallocated_ = true;\n> +\t\t}\n> +\n> +\t\tfor (const unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {\n> +\t\t\tunique_ptr<Request> request = camera_->createRequest();\n> +\t\t\tif (!request) {\n> +\t\t\t\tcerr << \"Failed to create request\" << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\n> +\t\t\tif (request->addBuffer(stream, buffer.get())) {\n> +\t\t\t\tcerr << \"Failed to associating buffer with request\" << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\n> +\t\t\trequests_.push_back(move(request));\n> +\t\t}\n> +\n> +\t\tcamera_->requestCompleted.connect(this, &CameraReconfigure::requestComplete);\n> +\n> +\t\tif (camera_->start()) {\n> +\t\t\tcerr << \"Failed to start camera\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tfor (unique_ptr<Request> &request : requests_) {\n> +\t\t\tif (camera_->queueRequest(request.get())) {\n> +\t\t\t\tcerr << \"Failed to queue request\" << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tEventDispatcher *dispatcher = Thread::current()->eventDispatcher();\n> +\n> +\t\tTimer timer;\n> +\t\ttimer.start(100);\n> +\t\twhile (timer.isRunning())\n> +\t\t\tdispatcher->processEvents();\n> +\n> +\t\tif (camera_->stop()) {\n> +\t\t\tcerr << \"Failed to stop camera\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (camera_->release()) {\n> +\t\t\tcerr << \"Failed to release camera\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tcamera_->requestCompleted.disconnect(this, &CameraReconfigure::requestComplete);\n> +\n> +\t\trequests_.clear();\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tint fdsOpen(pid_t pid)\n> +\t{\n> +\t\tstring proxyFdPath = \"/proc/\" + to_string(pid) + \"/fd\";\n> +\t\tDIR *dir;\n> +\t\tstruct dirent *ptr;\n> +\t\tunsigned int openFds = 0;\n> +\n> +\t\tdir = opendir(proxyFdPath.c_str());\n> +\t\tif (dir == nullptr) {\n> +\t\t\tint err = errno;\n> +\t\t\tcerr << \"Error opening \" << proxyFdPath << \": \"\n> +\t\t\t     << strerror(-err) << endl;\n> +\t\t\treturn 0;\n> +\t\t}\n> +\n> +\t\twhile ((ptr = readdir(dir)) != nullptr) {\n> +\t\t\tif ((strcmp(ptr->d_name, \".\") == 0) ||\n> +\t\t\t    (strcmp(ptr->d_name, \"..\") == 0))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\topenFds++;\n> +\t\t}\n> +\t\tclosedir(dir);\n> +\n> +\t\treturn openFds;\n> +\t}\n> +\n> +\tpid_t findProxyPid()\n> +\t{\n> +\t\tstring proxyPid;\n> +\t\tstring proxyName(kIpaProxyName_);\n> +\t\tDIR *dir;\n> +\t\tstruct dirent *ptr;\n> +\n> +\t\tdir = opendir(\"/proc\");\n> +\t\twhile ((ptr = readdir(dir)) != nullptr) {\n> +\t\t\tif (ptr->d_type != DT_DIR)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tstring pname(\"/proc/\" + string(ptr->d_name) + \"/comm\");\n> +\t\t\tif (File::exists(pname.c_str())) {\n> +\t\t\t\tifstream pfile(pname.c_str());\n> +\t\t\t\tstring comm;\n> +\t\t\t\tgetline(pfile, comm);\n> +\t\t\t\tpfile.close();\n> +\n> +\t\t\t\tproxyPid = comm == proxyName ? string(ptr->d_name) : \"\";\n> +\t\t\t}\n> +\n> +\t\t\tif (!proxyPid.empty())\n> +\t\t\t\tbreak;\n> +\t\t}\n> +\t\tclosedir(dir);\n> +\n> +\t\tif (!proxyPid.empty())\n> +\t\t\treturn atoi(proxyPid.c_str());\n> +\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tint init() override\n> +\t{\n> +\t\tif (status_ != TestPass)\n> +\t\t\treturn status_;\n> +\n> +\t\tconfig_ = camera_->generateConfiguration({ StreamRole::StillCapture });\n> +\t\tif (!config_ || config_->size() != 1) {\n> +\t\t\tcerr << \"Failed to generate default configuration\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tallocator_ = make_unique<FrameBufferAllocator>(camera_);\n> +\t\tallocated_ = false;\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint run() override\n> +\t{\n> +\t\tunsigned int openFdsAtStart = 0;\n> +\t\tunsigned int openFds = 0;\n> +\n> +\t\tpid_t proxyPid = findProxyPid();\n> +\t\tif (proxyPid < 0) {\n> +\t\t\tcerr << \"Cannot find \" << kIpaProxyName_\n> +\t\t\t     << \" pid, exiting\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\topenFdsAtStart = fdsOpen(proxyPid);\n> +\t\tfor (unsigned int i = 0; i < kNumOfReconfigures_; i++) {\n> +\t\t\tstartAndStop();\n> +\t\t\topenFds = fdsOpen(proxyPid);\n> +\t\t\tif (openFds == 0) {\n> +\t\t\t\tcerr << \"No open fds found whereas \"\n> +\t\t\t\t     << \"open fds at start: \" << openFdsAtStart\n> +\t\t\t\t     << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\n> +\t\t\tif (openFds != openFdsAtStart) {\n> +\t\t\t\tcerr << \"Leaking fds for \" << kIpaProxyName_\n> +\t\t\t\t     << \" - Open fds: \" << openFds << \" vs \"\n> +\t\t\t\t     << \"Open fds at start: \" << openFdsAtStart\n> +\t\t\t\t     << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tbool allocated_;\n> +\n> +\tvector<unique_ptr<Request>> requests_;\n> +\n> +\tunique_ptr<CameraConfiguration> config_;\n> +\tunique_ptr<FrameBufferAllocator> allocator_;\n> +};\n> +\n> +} /* namespace */\n> +\n> +TEST_REGISTER(CameraReconfigure)\n> diff --git a/test/camera/meson.build b/test/camera/meson.build\n> index 002a87b5..668d5c03 100644\n> --- a/test/camera/meson.build\n> +++ b/test/camera/meson.build\n> @@ -8,6 +8,7 @@ camera_tests = [\n>      ['buffer_import',           'buffer_import.cpp'],\n>      ['statemachine',            'statemachine.cpp'],\n>      ['capture',                 'capture.cpp'],\n> +    ['camera_reconfigure',      'camera_reconfigure.cpp'],\n>  ]\n>  \n>  foreach t : camera_tests\n> -- \n> 2.31.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D3EAABD87D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 18 Aug 2021 09:42:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 24740688A3;\n\tWed, 18 Aug 2021 11:42:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AA96D6025E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 18 Aug 2021 11:42:03 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3488D466;\n\tWed, 18 Aug 2021 11:42:01 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mP4iX+V0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629279723;\n\tbh=tkjsPg60KBK/6sNV0UDov456FmB7emp1QAR3HxwPD68=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mP4iX+V0kqcRtRFmfRJV6kHZeNKUsb8CgiOFBpPQbTdi0l/mW0kzj+qyxC5irtBnv\n\tva19qP1WPRxOZTTXuDe2+Gg/f8dPfxMRB6GZz8Av/6wdlqbJ7bt0U0z0pwG9knUzQO\n\tcaFy1YhEqnfa5MEenBXmy6tEoPL8OWhwbLmJZO8c=","Date":"Wed, 18 Aug 2021 18:41:55 +0900","From":"paul.elder@ideasonboard.com","To":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<20210818094155.GJ1733965@pyrite.rasen.tech>","References":"<20210818090247.33838-1-umang.jain@ideasonboard.com>\n\t<20210818090247.33838-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210818090247.33838-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] test: camera: Camera\n\treconfiguration and fd-leak test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18908,"web_url":"https://patchwork.libcamera.org/comment/18908/","msgid":"<671f20a3-fafd-d0b6-0049-55a6fb4828ff@ideasonboard.com>","date":"2021-08-18T10:49:35","subject":"Re: [libcamera-devel] [PATCH v2 2/2] test: camera: Camera\n\treconfiguration and fd-leak test","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 18/08/2021 10:02, Umang Jain wrote:\n\nI think we can introduce this patch at the beginning of the commit message:\n\n\"\"\"\nDevelopment and usage of isolated IPA's has identified bugs in the\ntransmission of the file descriptors between the pipeline handler and\nthe IPA through.\n\nAdd tests to identify these bugs and prevent regressions.\n\"\"\"\n\n\n> This tests basically checks for two things:\n> - Camera reconfigurations without stopping CameraManager\n> - Fd leaks across IPA IPC boundary [1]\n> \n> Currently, it uses vimc, but can be easily changed to using another\n> platform (for e.g. IPU3) by changing kCamId_ and kIpaProxyName_.\n> \n> The test performs kNumOfReconfigures_ (currently set to 10)\n> reconfigurations of the camera. Each reconfiguration runs\n> start(), capture(100ms), stop() of the camera. Hence, the test\n> runs approximately for 1 second.\n> \n> For checking the fd leaks, the test monitors the /proc/$PROXY_PID/fd\n> directory for open fds. It compares the number of open fds after each\n> run to the number of open fds before the first run. If those two are\n> found to be mis-matched, the test shall report failure.\n> \n> Since, the test needs to test the IPA IPC code paths, it is meant to\n> always run with LIBCAMERA_IPA_FORCE_ISOLATION=1 environment variable.\n\n\nThat last statement could be rephrased as LIBCAMERA_IPA_FORCE_ISOLATION\nisn't visibly being set in this code path:\n\n\"\"\"\nThe test validates IPA IPC code paths which are used when the IPA is run\nin isolated mode. The CameraTest is constructed with the isolate flag\nset to enforce this.\n\"\"\"\n\n\n> \n> [1] https://bugs.libcamera.org/show_bug.cgi?id=63\n> \n> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> ---\n>  test/camera/camera_reconfigure.cpp | 255 +++++++++++++++++++++++++++++\n>  test/camera/meson.build            |   1 +\n>  2 files changed, 256 insertions(+)\n>  create mode 100644 test/camera/camera_reconfigure.cpp\n> \n> diff --git a/test/camera/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp\n> new file mode 100644\n> index 00000000..90f798fe\n> --- /dev/null\n> +++ b/test/camera/camera_reconfigure.cpp\n> @@ -0,0 +1,255 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * Test:\n> + * - Camera's multiple reconfigurations without stopping CameraManager\n\n\"Multiple reconfigurations of the Camera without stopping the CameraManager\"\n\n> + * - leaking fds across IPA IPC boundary\n\n\"Validate there are no file descriptor leaks when using IPC\"\n\n> + */\n> +\n> +#include <dirent.h>\n> +#include <fstream>\n> +#include <iostream>\n> +\n> +#include <libcamera/base/event_dispatcher.h>\n> +#include <libcamera/base/file.h>\n> +#include <libcamera/base/thread.h>\n> +#include <libcamera/base/timer.h>\n> +\n> +#include <libcamera/framebuffer_allocator.h>\n> +\n> +#include \"camera_test.h\"\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +\n> +namespace {\n> +\n> +class CameraReconfigure : public CameraTest, public Test\n> +{\n> +public:\n> +\tCameraReconfigure()\n> +\t\t: CameraTest(kCamId_, true)\n\nIf we're not going to make the bool::isolate an enum, perhaps we need to\nadd a comment above the constructor to say \"Initialise the CameraTest\nwith an Isolated IPA\"\n\n  \"CameraTest(kCamId_, true)\"\n\ndoesn't really convey that without having to go look up the CameraTest\nconstructor.\n\n\n> +\t{\n> +\t}\n> +\n> +private:\n> +\tstatic constexpr const char *kCamId_ = \"platform/vimc.0 Sensor B\";\n> +\tstatic constexpr const char *kIpaProxyName_ = \"vimc_ipa_proxy\";\n> +\tstatic constexpr unsigned int kNumOfReconfigures_ = 10;\n> +\n> +\tvoid requestComplete(Request *request)\n> +\t{\n> +\t\tif (request->status() != Request::RequestComplete)\n> +\t\t\treturn;\n> +\n> +\t\tconst Request::BufferMap &buffers = request->buffers();\n> +\n> +\t\t/* Create a new request. */\n\nIs this really creating a new request?\n\nIt's more\n\"Reuse the request, and re-queue it with the same buffers.\"\n\nI think that can also be done by setting the ReuseBuffers flag on\nrequest->reuse() which would be clearer.\n\n> +\t\tconst Stream *stream = buffers.begin()->first;\n> +\t\tFrameBuffer *buffer = buffers.begin()->second;\n> +\n> +\t\trequest->reuse();\n> +\t\trequest->addBuffer(stream, buffer);\n> +\t\tcamera_->queueRequest(request);\n> +\t}\n> +\n> +\tint startAndStop()\n> +\t{\n> +\t\tStreamConfiguration &cfg = config_->at(0);\n> +\n> +\t\tif (camera_->acquire()) {\n> +\t\t\tcerr << \"Failed to acquire the camera\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (camera_->configure(config_.get())) {\n> +\t\t\tcerr << \"Failed to set default configuration\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tStream *stream = cfg.stream();\n> +\n\nI would add a comment here to say,\n\n\t\t/*\n\t\t * The configuration is consistent so we can re-use the\n\t\t * same buffer allocation for each run.\n\t\t */\n\n> +\t\tif (!allocated_) {\n> +\t\t\tint ret = allocator_->allocate(stream);\n> +\t\t\tif (ret < 0)\n\nShould we print a failure reason here?\n\n> +\t\t\t\treturn TestFail;\n> +\t\t\tallocated_ = true;\n> +\t\t}\n> +\n> +\t\tfor (const unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {\n> +\t\t\tunique_ptr<Request> request = camera_->createRequest();\n> +\t\t\tif (!request) {\n> +\t\t\t\tcerr << \"Failed to create request\" << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\n> +\t\t\tif (request->addBuffer(stream, buffer.get())) {\n> +\t\t\t\tcerr << \"Failed to associating buffer with request\" << endl;\n\ns/associating/associate/\n\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\n> +\t\t\trequests_.push_back(move(request));\n> +\t\t}\n> +\n> +\t\tcamera_->requestCompleted.connect(this, &CameraReconfigure::requestComplete);\n> +\n> +\t\tif (camera_->start()) {\n> +\t\t\tcerr << \"Failed to start camera\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tfor (unique_ptr<Request> &request : requests_) {\n> +\t\t\tif (camera_->queueRequest(request.get())) {\n> +\t\t\t\tcerr << \"Failed to queue request\" << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tEventDispatcher *dispatcher = Thread::current()->eventDispatcher();\n> +\n> +\t\tTimer timer;\n> +\t\ttimer.start(100);\n> +\t\twhile (timer.isRunning())\n> +\t\t\tdispatcher->processEvents();\n> +\n> +\t\tif (camera_->stop()) {\n> +\t\t\tcerr << \"Failed to stop camera\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (camera_->release()) {\n> +\t\t\tcerr << \"Failed to release camera\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tcamera_->requestCompleted.disconnect(this, &CameraReconfigure::requestComplete);\n> +\n> +\t\trequests_.clear();\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tint fdsOpen(pid_t pid)\n> +\t{\n> +\t\tstring proxyFdPath = \"/proc/\" + to_string(pid) + \"/fd\";\n> +\t\tDIR *dir;\n> +\t\tstruct dirent *ptr;\n> +\t\tunsigned int openFds = 0;\n> +\n> +\t\tdir = opendir(proxyFdPath.c_str());\n> +\t\tif (dir == nullptr) {\n> +\t\t\tint err = errno;\n> +\t\t\tcerr << \"Error opening \" << proxyFdPath << \": \"\n> +\t\t\t     << strerror(-err) << endl;\n> +\t\t\treturn 0;\n> +\t\t}\n> +\n> +\t\twhile ((ptr = readdir(dir)) != nullptr) {\n> +\t\t\tif ((strcmp(ptr->d_name, \".\") == 0) ||\n> +\t\t\t    (strcmp(ptr->d_name, \"..\") == 0))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\topenFds++;\n> +\t\t}\n> +\t\tclosedir(dir);\n> +\n> +\t\treturn openFds;\n> +\t}\n> +\n> +\tpid_t findProxyPid()\n> +\t{\n> +\t\tstring proxyPid;\n> +\t\tstring proxyName(kIpaProxyName_);\n> +\t\tDIR *dir;\n> +\t\tstruct dirent *ptr;\n> +\n> +\t\tdir = opendir(\"/proc\");\n> +\t\twhile ((ptr = readdir(dir)) != nullptr) {\n> +\t\t\tif (ptr->d_type != DT_DIR)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tstring pname(\"/proc/\" + string(ptr->d_name) + \"/comm\");\n> +\t\t\tif (File::exists(pname.c_str())) {\n> +\t\t\t\tifstream pfile(pname.c_str());\n> +\t\t\t\tstring comm;\n> +\t\t\t\tgetline(pfile, comm);\n> +\t\t\t\tpfile.close();\n> +\n> +\t\t\t\tproxyPid = comm == proxyName ? string(ptr->d_name) : \"\";\n> +\t\t\t}\n> +\n> +\t\t\tif (!proxyPid.empty())\n> +\t\t\t\tbreak;\n> +\t\t}\n> +\t\tclosedir(dir);\n> +\n> +\t\tif (!proxyPid.empty())\n> +\t\t\treturn atoi(proxyPid.c_str());\n> +\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tint init() override\n> +\t{\n> +\t\tif (status_ != TestPass)\n> +\t\t\treturn status_;\n> +\n> +\t\tconfig_ = camera_->generateConfiguration({ StreamRole::StillCapture });\n> +\t\tif (!config_ || config_->size() != 1) {\n> +\t\t\tcerr << \"Failed to generate default configuration\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tallocator_ = make_unique<FrameBufferAllocator>(camera_);\n> +\t\tallocated_ = false;\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint run() override\n> +\t{\n> +\t\tunsigned int openFdsAtStart = 0;\n> +\t\tunsigned int openFds = 0;\n> +\n> +\t\tpid_t proxyPid = findProxyPid();\n> +\t\tif (proxyPid < 0) {\n> +\t\t\tcerr << \"Cannot find \" << kIpaProxyName_\n> +\t\t\t     << \" pid, exiting\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\topenFdsAtStart = fdsOpen(proxyPid);\n> +\t\tfor (unsigned int i = 0; i < kNumOfReconfigures_; i++) {\n> +\t\t\tstartAndStop();\n> +\t\t\topenFds = fdsOpen(proxyPid);\n> +\t\t\tif (openFds == 0) {\n> +\t\t\t\tcerr << \"No open fds found whereas \"\n> +\t\t\t\t     << \"open fds at start: \" << openFdsAtStart\n> +\t\t\t\t     << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n\nI think the condition below would be enough to catch the errors, but I'm\nnot opposed to the check above as well.\n\nWith the minors resolved as you see fit:\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n\n\n> +\n> +\t\t\tif (openFds != openFdsAtStart) {\n> +\t\t\t\tcerr << \"Leaking fds for \" << kIpaProxyName_\n> +\t\t\t\t     << \" - Open fds: \" << openFds << \" vs \"\n> +\t\t\t\t     << \"Open fds at start: \" << openFdsAtStart\n> +\t\t\t\t     << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tbool allocated_;\n> +\n> +\tvector<unique_ptr<Request>> requests_;\n> +\n> +\tunique_ptr<CameraConfiguration> config_;\n> +\tunique_ptr<FrameBufferAllocator> allocator_;\n> +};\n> +\n> +} /* namespace */\n> +\n> +TEST_REGISTER(CameraReconfigure)\n> diff --git a/test/camera/meson.build b/test/camera/meson.build\n> index 002a87b5..668d5c03 100644\n> --- a/test/camera/meson.build\n> +++ b/test/camera/meson.build\n> @@ -8,6 +8,7 @@ camera_tests = [\n>      ['buffer_import',           'buffer_import.cpp'],\n>      ['statemachine',            'statemachine.cpp'],\n>      ['capture',                 'capture.cpp'],\n> +    ['camera_reconfigure',      'camera_reconfigure.cpp'],\n>  ]\n>  \n>  foreach t : camera_tests\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7D883BD87D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 18 Aug 2021 10:49:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D545668895;\n\tWed, 18 Aug 2021 12:49:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D64CF6888A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 18 Aug 2021 12:49:38 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4983C466;\n\tWed, 18 Aug 2021 12:49:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ecBeufBh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629283778;\n\tbh=W9ufHZEK6ekndiyV4r1nkKKnNw0qDhNNKTnfzZjmQLE=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=ecBeufBh1v5i2T/HBP4+H5ohzHmuQHsQU2595BnSvNw7V1jiQaRePDQ4wzqe30r3V\n\tNXUVPq5an6eHsvUuX52qSIXSlObIS/zzpOVMQEee9VzjR3eauw1gSDnvfwfw9Xwpvc\n\ttntUovjVM2uidoMw9QOOkW6zIkXqz/iyUXZSE5fY=","To":"Umang Jain <umang.jain@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210818090247.33838-1-umang.jain@ideasonboard.com>\n\t<20210818090247.33838-3-umang.jain@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<671f20a3-fafd-d0b6-0049-55a6fb4828ff@ideasonboard.com>","Date":"Wed, 18 Aug 2021 11:49:35 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210818090247.33838-3-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] test: camera: Camera\n\treconfiguration and fd-leak test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]