[{"id":5114,"web_url":"https://patchwork.libcamera.org/comment/5114/","msgid":"<20200607222436.GE22208@pendragon.ideasonboard.com>","date":"2020-06-07T22:24:36","subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Thu, May 21, 2020 at 01:54:27PM +0000, Umang Jain wrote:\n> This test checks the code-paths for camera's hotplugged and\n> unplugged support. It is based off bind/unbind of a UVC device\n\ns/off/on/\n\n> from sysfs. Hence, this tests required root permissions to run\n\ns/tests required/test requires/\n\n> and should have atleast one already bound UVC device present\n\ns/atleast/at least/\n\n> on the system.\n\ns/on/in/\n\n> \n> Signed-off-by: Umang Jain \n> ---\n> test/hotplug-cameras.cpp | 153 +++++++++++++++++++++++++++++++++++++++\n> test/meson.build | 1 +\n> 2 files changed, 154 insertions(+)\n> create mode 100644 test/hotplug-cameras.cpp\n> \n> diff --git a/test/hotplug-cameras.cpp b/test/hotplug-cameras.cpp\n> new file mode 100644\n> index 0000000..72c370f\n> --- /dev/null\n> +++ b/test/hotplug-cameras.cpp\n> @@ -0,0 +1,153 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Umang Jain \n> + *\n> + * hotplug-cameras.cpp - Emulate cameraAdded/cameraRemoved signals in CameraManager\n\nMaybe s/Emulate/Test/ ?\n\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n\nNo need for a blank line here, and you can then merge the two groups of\nheaders in alphabetical order.\n\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/file.h\"\n> +#include \"libcamera/internal/thread.h\"\n> +\n> +#include \"test.h\"\n> +\n> +using namespace std;\n\nAs you use the std namespace explicitly below, you can drop this line.\n\n> +using namespace libcamera;\n> +\n> +class HotplugTest : public Test\n> +{\n> +protected:\n> +\tvoid cameraAddedHandler(std::shared_ptr cam)\n> +\t{\n> +\t\tcameraAddedPass_ = true;\n> +\t}\n> +\n> +\tvoid cameraRemovedHandler(std::shared_ptr cam)\n> +\t{\n> +\t\tcameraRemovedPass_ = true;\n> +\t}\n> +\n> +\tint init()\n> +\t{\n> +\t\tif (!File::exists(\"/sys/module/uvcvideo\")) {\n> +\t\t\tstd::cout << \"uvcvideo driver is not loaded, skipping\" << std::endl;\n> +\t\t\treturn TestSkip;\n> +\t\t}\n> +\n> +\t\tif (geteuid() != 0) {\n> +\t\t\tstd::cout << \"This test requires root permissions, skipping\" << std::endl;\n> +\t\t\treturn TestSkip;\n> +\t\t}\n> +\n> +\t\tcm_ = new CameraManager();\n> +\t\tif (cm_->start()) {\n> +\t\t\tstd::cout << \"Failed to start camera manager\" << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tcameraAddedPass_ = false;\n> +\t\tcameraRemovedPass_ = false;\n> +\n> +\t\tcm_->newCameraAdded.connect(this, &HotplugTest::cameraAddedHandler);\n> +\t\tcm_->cameraRemoved.connect(this, &HotplugTest::cameraRemovedHandler);\n> +\n> +\t\tuvc_toplevel_ = \"/sys/module/uvcvideo/drivers/usb:uvcvideo/\";\n\nMaybe /sys/bus/usb/drivers/uvcvideo/ to avoid traversing symlinks ? It\nmakes little difference in practice I suppose.\n\ns/uvc_toplevel_/uvcTopLevel_/\n\nAnd I would even call this uvcDriverDir to make it more explicit. Now\nthat I think about it, as uvc_toplevel_ is constant, maybe we should\nhave\n\n\tstatic const std::string uvcDriverDir_;\n\nas a static class member.\n\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tint run()\n> +\t{\n> +\t\tDIR *dir;\n> +\t\tstruct dirent *dirent, *dirent2;\n> +\t\tstd::string uvc_driver_dir;\n> +\t\tbool uvc_driver_found = false;\n\ncamelCase please :-)\n\n> +\n> +\t\tdir = opendir(uvc_toplevel_.c_str());\n> +\t\t/* Find a UVC device driver symlink, which we can bind/unbind */\n\ns/unbind/unbind./ (same for the other comments below)\n\n> +\t\twhile ((dirent = readdir(dir)) != nullptr) {\n> +\t\t\tif (dirent->d_type != DT_LNK)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n> +\t\t\tDIR *device_driver = opendir(child_dir.c_str());\n> +\t\t\twhile ((dirent2 = readdir(device_driver)) != nullptr) {\n> +\t\t\t\tif (strncmp(dirent2->d_name, \"video4linux\", 11) == 0) {\n> +\t\t\t\t\tuvc_driver_dir = dirent->d_name;\n> +\t\t\t\t\tuvc_driver_found = true;\n> +\t\t\t\t\tbreak;\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t\tclosedir(device_driver);\n> +\n> +\t\t\tif (uvc_driver_found)\n> +\t\t\t\tbreak;\n\nCan't this be simplified with\n\n\t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n\t\t\tif (!File::exist(uvc_toplevel_ + dirent->d_name + \"/video4linux\"))\n\t\t\t\tcontinue;\n\n\t\t\tuvc_driver_dir = dirent->d_name;\n\t\t\tbreak;\n\n> +\t\t}\n> +\t\tclosedir(dir);\n> +\n> +\t\t/* If no UVC driver found, skip */\n> +\t\tif (!uvc_driver_found)\n\nAnd here,\n\n\t\tif (uvc_driver_dir.empty())\n\nto remove uvc_driver_found.\n\n> +\t\t\treturn TestSkip;\n> +\n> +\t\t/* Unbind a camera, process events */\n> +\t\tint fd1 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/unbind\", O_WRONLY);\n\nMaybe replacing the hardcoded string with uvc_toplevel_ + \"unbind\" ?\nThis would become\n\n\t\tint fd1 = open((uvcDriverDir_ + \"unbind\").c_str(), O_WRONLY);\n\n> +\t\twrite(fd1, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n> +\t\tclose(fd1);\n\nBlank line here ?\n\nI wonder if using the C++ file API would be simpler here.\n\n\t\tstd::ofstream(uvcDriverDir_ + \"unbind\", std::ios::binary)\n\t\t\t<< uvc_driver_dir;;\n\nand that's it.\n\n> +\t\tTimer timer;\n> +\t\ttimer.start(1000);\n> +\t\twhile (timer.isRunning())\n\n\t\twhile (timer.isRunning() && !cameraRemovedPass_)\n\nto shorten the wait time. Same below.\n\n> +\t\t\tThread::current()->eventDispatcher()->processEvents();\n> +\n\nHow about already testing for cameraRemovedPass_ here ?\n\n\t\tif (!cameraRemovedPass_)\n\t\t\treturn TestFail;\n\n> +\t\t/* \\todo: Fix this workaround of stopping and starting the camera-manager.\n> +\t\t * We need to do this, so that cm_ release all references to the uvc media symlinks.\n> +\t\t */\n\nWe can merge the series without fixing this, but I think it needs to be\nhandled sooner than latter.\n\n> +\t\tcm_->stop();\n> +\t\tif (cm_->start()) {\n> +\t\t\tstd::cout << \"Failed to restart camera-manager\" << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Bind the camera again, process events */\n> +\t\tint fd2 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/bind\", O_WRONLY);\n\nYou don't need fd2, you can reuse fd1 (which should be renamed fd).\n\n> +\t\twrite(fd2, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n> +\t\tclose(fd2);\n> +\n> +\t\ttimer.start(1000);\n> +\t\twhile (timer.isRunning())\n> +\t\t\tThread::current()->eventDispatcher()->processEvents();\n> +\n> +\t\tif (cameraAddedPass_ && cameraRemovedPass_)\n> +\t\t\treturn TestPass;\n> +\t\telse\n> +\t\t\treturn TestFail;\n\nThis would become\n\n\t\tif (!cameraAddedPass_)\n\t\t\treturn TestFail;\n\n\t\treturn TestPass;\n\n> +\t}\n> +\n> +\tvoid cleanup()\n> +\t{\n> +\t\tcm_->stop();\n> +\t\tdelete cm_;\n> +\t}\n> +\n> +private:\n> +\tCameraManager *cm_;\n> +\tstd::string uvc_toplevel_;\n> +\tbool cameraRemovedPass_;\n\nMaybe s/cameraRemovedPass_/cameraRemoved_/ ?\n\n> +\tbool cameraAddedPass_;\n\nSame here.\n\n> +};\n> +\n> +TEST_REGISTER(HotplugTest)\n> +\n> diff --git a/test/meson.build b/test/meson.build\n> index bd7da14..f7e27d7 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -31,6 +31,7 @@ internal_tests = [\n> ['file', 'file.cpp'],\n> ['file-descriptor', 'file-descriptor.cpp'],\n> ['message', 'message.cpp'],\n> + ['hotplug-cameras', 'hotplug-cameras.cpp'],\n\nAlphabetical order ?\n\n> ['object', 'object.cpp'],\n> ['object-invoke', 'object-invoke.cpp'],\n> ['signal-threads', 'signal-threads.cpp'],","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 5329161027\n\tfor ;\n\tMon, 8 Jun 2020 00:24:56 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BDFA42C9;\n\tMon, 8 Jun 2020 00:24:55 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"K8egYFPy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591568695;\n\tbh=dwvke96WhRIX1dFvz22NNRjD6khPXgKzddvYcpveIjE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=K8egYFPyCYNtAwj2g99seJr1lehJdneo78bu8xNRhFgvm8uEJp6eM0VELGWAnqX0v\n\tjLx52x+2zrg7TQj7l9pkugdzWYOqMVKaRVBF6/VtgxqZi30yaDlB78bJaMLFZW33ST\n\tUSdB7GT0StsPytQEsB1ytq5aMN0U4swxBdQ5b5WM=","Date":"Mon, 8 Jun 2020 01:24:36 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200607222436.GE22208@pendragon.ideasonboard.com>","References":"<20200521135416.13685-1-email@uajain.com>\n\t<20200521135416.13685-6-email@uajain.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200521135416.13685-6-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 07 Jun 2020 22:24:56 -0000"}},{"id":5115,"web_url":"https://patchwork.libcamera.org/comment/5115/","msgid":"<20200607222706.GF22208@pendragon.ideasonboard.com>","date":"2020-06-07T22:27:06","subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Mon, Jun 08, 2020 at 01:24:37AM +0300, Laurent Pinchart wrote:\n> On Thu, May 21, 2020 at 01:54:27PM +0000, Umang Jain wrote:\n> > This test checks the code-paths for camera's hotplugged and\n> > unplugged support. It is based off bind/unbind of a UVC device\n> \n> s/off/on/\n> \n> > from sysfs. Hence, this tests required root permissions to run\n> \n> s/tests required/test requires/\n> \n> > and should have atleast one already bound UVC device present\n> \n> s/atleast/at least/\n> \n> > on the system.\n> \n> s/on/in/\n> \n> > \n> > Signed-off-by: Umang Jain \n> > ---\n> > test/hotplug-cameras.cpp | 153 +++++++++++++++++++++++++++++++++++++++\n> > test/meson.build | 1 +\n> > 2 files changed, 154 insertions(+)\n> > create mode 100644 test/hotplug-cameras.cpp\n> > \n> > diff --git a/test/hotplug-cameras.cpp b/test/hotplug-cameras.cpp\n> > new file mode 100644\n> > index 0000000..72c370f\n> > --- /dev/null\n> > +++ b/test/hotplug-cameras.cpp\n> > @@ -0,0 +1,153 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Umang Jain \n> > + *\n> > + * hotplug-cameras.cpp - Emulate cameraAdded/cameraRemoved signals in CameraManager\n> \n> Maybe s/Emulate/Test/ ?\n> \n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> \n> No need for a blank line here, and you can then merge the two groups of\n> headers in alphabetical order.\n> \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/file.h\"\n> > +#include \"libcamera/internal/thread.h\"\n> > +\n> > +#include \"test.h\"\n> > +\n> > +using namespace std;\n> \n> As you use the std namespace explicitly below, you can drop this line.\n> \n> > +using namespace libcamera;\n> > +\n> > +class HotplugTest : public Test\n> > +{\n> > +protected:\n> > +\tvoid cameraAddedHandler(std::shared_ptr cam)\n> > +\t{\n> > +\t\tcameraAddedPass_ = true;\n> > +\t}\n> > +\n> > +\tvoid cameraRemovedHandler(std::shared_ptr cam)\n> > +\t{\n> > +\t\tcameraRemovedPass_ = true;\n> > +\t}\n> > +\n> > +\tint init()\n> > +\t{\n> > +\t\tif (!File::exists(\"/sys/module/uvcvideo\")) {\n> > +\t\t\tstd::cout << \"uvcvideo driver is not loaded, skipping\" << std::endl;\n> > +\t\t\treturn TestSkip;\n> > +\t\t}\n> > +\n> > +\t\tif (geteuid() != 0) {\n> > +\t\t\tstd::cout << \"This test requires root permissions, skipping\" << std::endl;\n> > +\t\t\treturn TestSkip;\n> > +\t\t}\n> > +\n> > +\t\tcm_ = new CameraManager();\n> > +\t\tif (cm_->start()) {\n> > +\t\t\tstd::cout << \"Failed to start camera manager\" << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tcameraAddedPass_ = false;\n> > +\t\tcameraRemovedPass_ = false;\n> > +\n> > +\t\tcm_->newCameraAdded.connect(this, &HotplugTest::cameraAddedHandler);\n> > +\t\tcm_->cameraRemoved.connect(this, &HotplugTest::cameraRemovedHandler);\n> > +\n> > +\t\tuvc_toplevel_ = \"/sys/module/uvcvideo/drivers/usb:uvcvideo/\";\n> \n> Maybe /sys/bus/usb/drivers/uvcvideo/ to avoid traversing symlinks ? It\n> makes little difference in practice I suppose.\n> \n> s/uvc_toplevel_/uvcTopLevel_/\n> \n> And I would even call this uvcDriverDir to make it more explicit. Now\n> that I think about it, as uvc_toplevel_ is constant, maybe we should\n> have\n> \n> \tstatic const std::string uvcDriverDir_;\n> \n> as a static class member.\n> \n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tint run()\n> > +\t{\n> > +\t\tDIR *dir;\n> > +\t\tstruct dirent *dirent, *dirent2;\n> > +\t\tstd::string uvc_driver_dir;\n> > +\t\tbool uvc_driver_found = false;\n> \n> camelCase please :-)\n> \n> > +\n> > +\t\tdir = opendir(uvc_toplevel_.c_str());\n> > +\t\t/* Find a UVC device driver symlink, which we can bind/unbind */\n> \n> s/unbind/unbind./ (same for the other comments below)\n> \n> > +\t\twhile ((dirent = readdir(dir)) != nullptr) {\n> > +\t\t\tif (dirent->d_type != DT_LNK)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n> > +\t\t\tDIR *device_driver = opendir(child_dir.c_str());\n> > +\t\t\twhile ((dirent2 = readdir(device_driver)) != nullptr) {\n> > +\t\t\t\tif (strncmp(dirent2->d_name, \"video4linux\", 11) == 0) {\n> > +\t\t\t\t\tuvc_driver_dir = dirent->d_name;\n> > +\t\t\t\t\tuvc_driver_found = true;\n> > +\t\t\t\t\tbreak;\n> > +\t\t\t\t}\n> > +\t\t\t}\n> > +\t\t\tclosedir(device_driver);\n> > +\n> > +\t\t\tif (uvc_driver_found)\n> > +\t\t\t\tbreak;\n> \n> Can't this be simplified with\n> \n> \t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n> \t\t\tif (!File::exist(uvc_toplevel_ + dirent->d_name + \"/video4linux\"))\n> \t\t\t\tcontinue;\n> \n> \t\t\tuvc_driver_dir = dirent->d_name;\n> \t\t\tbreak;\n> \n> > +\t\t}\n> > +\t\tclosedir(dir);\n> > +\n> > +\t\t/* If no UVC driver found, skip */\n> > +\t\tif (!uvc_driver_found)\n> \n> And here,\n> \n> \t\tif (uvc_driver_dir.empty())\n> \n> to remove uvc_driver_found.\n> \n> > +\t\t\treturn TestSkip;\n> > +\n> > +\t\t/* Unbind a camera, process events */\n> > +\t\tint fd1 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/unbind\", O_WRONLY);\n> \n> Maybe replacing the hardcoded string with uvc_toplevel_ + \"unbind\" ?\n> This would become\n> \n> \t\tint fd1 = open((uvcDriverDir_ + \"unbind\").c_str(), O_WRONLY);\n> \n> > +\t\twrite(fd1, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n> > +\t\tclose(fd1);\n> \n> Blank line here ?\n> \n> I wonder if using the C++ file API would be simpler here.\n> \n> \t\tstd::ofstream(uvcDriverDir_ + \"unbind\", std::ios::binary)\n> \t\t\t<< uvc_driver_dir;;\n> \n> and that's it.\n> \n> > +\t\tTimer timer;\n> > +\t\ttimer.start(1000);\n> > +\t\twhile (timer.isRunning())\n> \n> \t\twhile (timer.isRunning() && !cameraRemovedPass_)\n> \n> to shorten the wait time. Same below.\n> \n> > +\t\t\tThread::current()->eventDispatcher()->processEvents();\n> > +\n> \n> How about already testing for cameraRemovedPass_ here ?\n> \n> \t\tif (!cameraRemovedPass_)\n> \t\t\treturn TestFail;\n> \n> > +\t\t/* \\todo: Fix this workaround of stopping and starting the camera-manager.\n> > +\t\t * We need to do this, so that cm_ release all references to the uvc media symlinks.\n> > +\t\t */\n> \n> We can merge the series without fixing this, but I think it needs to be\n> handled sooner than latter.\n> \n> > +\t\tcm_->stop();\n> > +\t\tif (cm_->start()) {\n> > +\t\t\tstd::cout << \"Failed to restart camera-manager\" << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Bind the camera again, process events */\n> > +\t\tint fd2 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/bind\", O_WRONLY);\n> \n> You don't need fd2, you can reuse fd1 (which should be renamed fd).\n> \n> > +\t\twrite(fd2, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n> > +\t\tclose(fd2);\n> > +\n> > +\t\ttimer.start(1000);\n> > +\t\twhile (timer.isRunning())\n> > +\t\t\tThread::current()->eventDispatcher()->processEvents();\n> > +\n> > +\t\tif (cameraAddedPass_ && cameraRemovedPass_)\n> > +\t\t\treturn TestPass;\n> > +\t\telse\n> > +\t\t\treturn TestFail;\n> \n> This would become\n> \n> \t\tif (!cameraAddedPass_)\n> \t\t\treturn TestFail;\n> \n> \t\treturn TestPass;\n> \n> > +\t}\n> > +\n> > +\tvoid cleanup()\n> > +\t{\n> > +\t\tcm_->stop();\n> > +\t\tdelete cm_;\n> > +\t}\n> > +\n> > +private:\n> > +\tCameraManager *cm_;\n> > +\tstd::string uvc_toplevel_;\n> > +\tbool cameraRemovedPass_;\n> \n> Maybe s/cameraRemovedPass_/cameraRemoved_/ ?\n> \n> > +\tbool cameraAddedPass_;\n> \n> Same here.\n> \n> > +};\n> > +\n> > +TEST_REGISTER(HotplugTest)\n> > +\n\nI forgot to mention, there's an extra blank line here.\n\n> > diff --git a/test/meson.build b/test/meson.build\n> > index bd7da14..f7e27d7 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -31,6 +31,7 @@ internal_tests = [\n> > ['file', 'file.cpp'],\n> > ['file-descriptor', 'file-descriptor.cpp'],\n> > ['message', 'message.cpp'],\n> > + ['hotplug-cameras', 'hotplug-cameras.cpp'],\n> \n> Alphabetical order ?\n> \n> > ['object', 'object.cpp'],\n> > ['object-invoke', 'object-invoke.cpp'],\n> > ['signal-threads', 'signal-threads.cpp'],","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 A4FE2603C6\n\tfor ;\n\tMon, 8 Jun 2020 00:27:25 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EE0712C9;\n\tMon, 8 Jun 2020 00:27:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jTUTYxNC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591568845;\n\tbh=9jL6NmWQyAbOPdVU0KPvDf/l3UJeinuhGBENLhLqfuU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jTUTYxNCjbeBjBKIq6dil8sKtp6Nwk69/R/yoGONTortPIq1d7i2q7PSw2wfUyyVr\n\tUKNam32+ptoCjycre8so48yuwM1uqoxCERnS8aUwNa8dKt4BQckYN7yz+aZDK9y7t9\n\tYiHJFdJhuq+dP3NAxzlmm71onXAm5QysSf8AvNuI=","Date":"Mon, 8 Jun 2020 01:27:06 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200607222706.GF22208@pendragon.ideasonboard.com>","References":"<20200521135416.13685-1-email@uajain.com>\n\t<20200521135416.13685-6-email@uajain.com>\n\t<20200607222436.GE22208@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200607222436.GE22208@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 07 Jun 2020 22:27:26 -0000"}},{"id":5116,"web_url":"https://patchwork.libcamera.org/comment/5116/","msgid":"<20200607225440.GG22208@pendragon.ideasonboard.com>","date":"2020-06-07T22:54:40","subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Mon, Jun 08, 2020 at 01:24:37AM +0300, Laurent Pinchart wrote:\n> On Thu, May 21, 2020 at 01:54:27PM +0000, Umang Jain wrote:\n> > This test checks the code-paths for camera's hotplugged and\n> > unplugged support. It is based off bind/unbind of a UVC device\n> \n> s/off/on/\n> \n> > from sysfs. Hence, this tests required root permissions to run\n> \n> s/tests required/test requires/\n> \n> > and should have atleast one already bound UVC device present\n> \n> s/atleast/at least/\n> \n> > on the system.\n> \n> s/on/in/\n> \n> > Signed-off-by: Umang Jain \n> > ---\n> > test/hotplug-cameras.cpp | 153 +++++++++++++++++++++++++++++++++++++++\n> > test/meson.build | 1 +\n> > 2 files changed, 154 insertions(+)\n> > create mode 100644 test/hotplug-cameras.cpp\n> > \n> > diff --git a/test/hotplug-cameras.cpp b/test/hotplug-cameras.cpp\n> > new file mode 100644\n> > index 0000000..72c370f\n> > --- /dev/null\n> > +++ b/test/hotplug-cameras.cpp\n> > @@ -0,0 +1,153 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Umang Jain \n> > + *\n> > + * hotplug-cameras.cpp - Emulate cameraAdded/cameraRemoved signals in CameraManager\n> \n> Maybe s/Emulate/Test/ ?\n> \n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> \n> No need for a blank line here, and you can then merge the two groups of\n> headers in alphabetical order.\n> \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/file.h\"\n> > +#include \"libcamera/internal/thread.h\"\n> > +\n> > +#include \"test.h\"\n> > +\n> > +using namespace std;\n> \n> As you use the std namespace explicitly below, you can drop this line.\n> \n> > +using namespace libcamera;\n> > +\n> > +class HotplugTest : public Test\n> > +{\n> > +protected:\n> > +\tvoid cameraAddedHandler(std::shared_ptr cam)\n> > +\t{\n> > +\t\tcameraAddedPass_ = true;\n> > +\t}\n> > +\n> > +\tvoid cameraRemovedHandler(std::shared_ptr cam)\n> > +\t{\n> > +\t\tcameraRemovedPass_ = true;\n> > +\t}\n> > +\n> > +\tint init()\n> > +\t{\n> > +\t\tif (!File::exists(\"/sys/module/uvcvideo\")) {\n> > +\t\t\tstd::cout << \"uvcvideo driver is not loaded, skipping\" << std::endl;\n> > +\t\t\treturn TestSkip;\n> > +\t\t}\n> > +\n> > +\t\tif (geteuid() != 0) {\n> > +\t\t\tstd::cout << \"This test requires root permissions, skipping\" << std::endl;\n> > +\t\t\treturn TestSkip;\n> > +\t\t}\n> > +\n> > +\t\tcm_ = new CameraManager();\n> > +\t\tif (cm_->start()) {\n> > +\t\t\tstd::cout << \"Failed to start camera manager\" << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tcameraAddedPass_ = false;\n> > +\t\tcameraRemovedPass_ = false;\n> > +\n> > +\t\tcm_->newCameraAdded.connect(this, &HotplugTest::cameraAddedHandler);\n> > +\t\tcm_->cameraRemoved.connect(this, &HotplugTest::cameraRemovedHandler);\n> > +\n> > +\t\tuvc_toplevel_ = \"/sys/module/uvcvideo/drivers/usb:uvcvideo/\";\n> \n> Maybe /sys/bus/usb/drivers/uvcvideo/ to avoid traversing symlinks ? It\n> makes little difference in practice I suppose.\n> \n> s/uvc_toplevel_/uvcTopLevel_/\n> \n> And I would even call this uvcDriverDir to make it more explicit. Now\n> that I think about it, as uvc_toplevel_ is constant, maybe we should\n> have\n> \n> \tstatic const std::string uvcDriverDir_;\n> \n> as a static class member.\n> \n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tint run()\n> > +\t{\n> > +\t\tDIR *dir;\n> > +\t\tstruct dirent *dirent, *dirent2;\n> > +\t\tstd::string uvc_driver_dir;\n> > +\t\tbool uvc_driver_found = false;\n> \n> camelCase please :-)\n> \n> > +\n> > +\t\tdir = opendir(uvc_toplevel_.c_str());\n> > +\t\t/* Find a UVC device driver symlink, which we can bind/unbind */\n> \n> s/unbind/unbind./ (same for the other comments below)\n> \n> > +\t\twhile ((dirent = readdir(dir)) != nullptr) {\n> > +\t\t\tif (dirent->d_type != DT_LNK)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n> > +\t\t\tDIR *device_driver = opendir(child_dir.c_str());\n> > +\t\t\twhile ((dirent2 = readdir(device_driver)) != nullptr) {\n> > +\t\t\t\tif (strncmp(dirent2->d_name, \"video4linux\", 11) == 0) {\n> > +\t\t\t\t\tuvc_driver_dir = dirent->d_name;\n> > +\t\t\t\t\tuvc_driver_found = true;\n> > +\t\t\t\t\tbreak;\n> > +\t\t\t\t}\n> > +\t\t\t}\n> > +\t\t\tclosedir(device_driver);\n> > +\n> > +\t\t\tif (uvc_driver_found)\n> > +\t\t\t\tbreak;\n> \n> Can't this be simplified with\n> \n> \t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n> \t\t\tif (!File::exist(uvc_toplevel_ + dirent->d_name + \"/video4linux\"))\n> \t\t\t\tcontinue;\n> \n> \t\t\tuvc_driver_dir = dirent->d_name;\n> \t\t\tbreak;\n> \n> > +\t\t}\n> > +\t\tclosedir(dir);\n> > +\n> > +\t\t/* If no UVC driver found, skip */\n> > +\t\tif (!uvc_driver_found)\n> \n> And here,\n> \n> \t\tif (uvc_driver_dir.empty())\n> \n> to remove uvc_driver_found.\n> \n> > +\t\t\treturn TestSkip;\n> > +\n> > +\t\t/* Unbind a camera, process events */\n> > +\t\tint fd1 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/unbind\", O_WRONLY);\n> \n> Maybe replacing the hardcoded string with uvc_toplevel_ + \"unbind\" ?\n> This would become\n> \n> \t\tint fd1 = open((uvcDriverDir_ + \"unbind\").c_str(), O_WRONLY);\n> \n> > +\t\twrite(fd1, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n> > +\t\tclose(fd1);\n> \n> Blank line here ?\n> \n> I wonder if using the C++ file API would be simpler here.\n> \n> \t\tstd::ofstream(uvcDriverDir_ + \"unbind\", std::ios::binary)\n> \t\t\t<< uvc_driver_dir;;\n> \n> and that's it.\n> \n> > +\t\tTimer timer;\n> > +\t\ttimer.start(1000);\n> > +\t\twhile (timer.isRunning())\n> \n> \t\twhile (timer.isRunning() && !cameraRemovedPass_)\n> \n> to shorten the wait time. Same below.\n> \n> > +\t\t\tThread::current()->eventDispatcher()->processEvents();\n> > +\n> \n> How about already testing for cameraRemovedPass_ here ?\n> \n> \t\tif (!cameraRemovedPass_)\n> \t\t\treturn TestFail;\n> \n> > +\t\t/* \\todo: Fix this workaround of stopping and starting the camera-manager.\n> > +\t\t * We need to do this, so that cm_ release all references to the uvc media symlinks.\n> > +\t\t */\n> \n> We can merge the series without fixing this, but I think it needs to be\n> handled sooner than latter.\n\nI've had a quick look, and this seems to be caused by the pipeline\nhandler never getting deleted on hot-unplug. This is due to a reference\nto the pipeline handler being stored in the camera manager and never\ndropped. \n\nI'm not sure we actually need to store those references. Can I let you\ninvestigate ?\n\n> > +\t\tcm_->stop();\n> > +\t\tif (cm_->start()) {\n> > +\t\t\tstd::cout << \"Failed to restart camera-manager\" << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Bind the camera again, process events */\n> > +\t\tint fd2 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/bind\", O_WRONLY);\n> \n> You don't need fd2, you can reuse fd1 (which should be renamed fd).\n> \n> > +\t\twrite(fd2, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n> > +\t\tclose(fd2);\n> > +\n> > +\t\ttimer.start(1000);\n> > +\t\twhile (timer.isRunning())\n> > +\t\t\tThread::current()->eventDispatcher()->processEvents();\n> > +\n> > +\t\tif (cameraAddedPass_ && cameraRemovedPass_)\n> > +\t\t\treturn TestPass;\n> > +\t\telse\n> > +\t\t\treturn TestFail;\n> \n> This would become\n> \n> \t\tif (!cameraAddedPass_)\n> \t\t\treturn TestFail;\n> \n> \t\treturn TestPass;\n> \n> > +\t}\n> > +\n> > +\tvoid cleanup()\n> > +\t{\n> > +\t\tcm_->stop();\n> > +\t\tdelete cm_;\n> > +\t}\n> > +\n> > +private:\n> > +\tCameraManager *cm_;\n> > +\tstd::string uvc_toplevel_;\n> > +\tbool cameraRemovedPass_;\n> \n> Maybe s/cameraRemovedPass_/cameraRemoved_/ ?\n> \n> > +\tbool cameraAddedPass_;\n> \n> Same here.\n> \n> > +};\n> > +\n> > +TEST_REGISTER(HotplugTest)\n> > +\n> > diff --git a/test/meson.build b/test/meson.build\n> > index bd7da14..f7e27d7 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -31,6 +31,7 @@ internal_tests = [\n> > ['file', 'file.cpp'],\n> > ['file-descriptor', 'file-descriptor.cpp'],\n> > ['message', 'message.cpp'],\n> > + ['hotplug-cameras', 'hotplug-cameras.cpp'],\n> \n> Alphabetical order ?\n> \n> > ['object', 'object.cpp'],\n> > ['object-invoke', 'object-invoke.cpp'],\n> > ['signal-threads', 'signal-threads.cpp'],","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 722BA603C6\n\tfor ;\n\tMon, 8 Jun 2020 00:55:00 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DA1992C9;\n\tMon, 8 Jun 2020 00:54:59 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"RYbo5j/B\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591570500;\n\tbh=AT6ZtkTROiF/ctUdxwHL9GMH2MvRqI0c1sNtViqDPLw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RYbo5j/BROWTeyrRGhgntd8sXwXR/a38arBxqD4greuqZL/SxXdf4HFPPasCb9Nbu\n\tZF4rDXpB3jYyH72KjgIOno8PsREH0BjLIQHwzxzdFGyOxa6lUPrWpOP1Sm5QlICbAK\n\tV7nnGXWF8La5tCwRLVFaAew2wqZ1l3QyW/rhMbKQ=","Date":"Mon, 8 Jun 2020 01:54:40 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200607225440.GG22208@pendragon.ideasonboard.com>","References":"<20200521135416.13685-1-email@uajain.com>\n\t<20200521135416.13685-6-email@uajain.com>\n\t<20200607222436.GE22208@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200607222436.GE22208@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 07 Jun 2020 22:55:00 -0000"}},{"id":5135,"web_url":"https://patchwork.libcamera.org/comment/5135/","msgid":"<6af4bfa7-50d0-1f03-d4a5-a1f5dc5559db@uajain.com>","date":"2020-06-08T14:20:07","subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Laurent,\n\nThanks for the review.\n\nOn 6/8/20 4:24 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Mon, Jun 08, 2020 at 01:24:37AM +0300, Laurent Pinchart wrote:\n>> On Thu, May 21, 2020 at 01:54:27PM +0000, Umang Jain wrote:\n>>> This test checks the code-paths for camera's hotplugged and\n>>> unplugged support. It is based off bind/unbind of a UVC device\n>> s/off/on/\n>>\n>>> from sysfs. Hence, this tests required root permissions to run\n>> s/tests required/test requires/\n>>\n>>> and should have atleast one already bound UVC device present\n>> s/atleast/at least/\n>>\n>>> on the system.\n>> s/on/in/\n>>\n>>> Signed-off-by: Umang Jain \n>>> ---\n>>> test/hotplug-cameras.cpp | 153 +++++++++++++++++++++++++++++++++++++++\n>>> test/meson.build | 1 +\n>>> 2 files changed, 154 insertions(+)\n>>> create mode 100644 test/hotplug-cameras.cpp\n>>>\n>>> diff --git a/test/hotplug-cameras.cpp b/test/hotplug-cameras.cpp\n>>> new file mode 100644\n>>> index 0000000..72c370f\n>>> --- /dev/null\n>>> +++ b/test/hotplug-cameras.cpp\n>>> @@ -0,0 +1,153 @@\n>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>> +/*\n>>> + * Copyright (C) 2020, Umang Jain \n>>> + *\n>>> + * hotplug-cameras.cpp - Emulate cameraAdded/cameraRemoved signals in CameraManager\n>> Maybe s/Emulate/Test/ ?\n>>\n>>> + */\n>>> +\n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +\n>> No need for a blank line here, and you can then merge the two groups of\n>> headers in alphabetical order.\n>>\n>>> +#include \n>>> +#include \n>>> +#include \n>>> +\n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +\n>>> +#include \"libcamera/internal/file.h\"\n>>> +#include \"libcamera/internal/thread.h\"\n>>> +\n>>> +#include \"test.h\"\n>>> +\n>>> +using namespace std;\n>> As you use the std namespace explicitly below, you can drop this line.\n>>\n>>> +using namespace libcamera;\n>>> +\n>>> +class HotplugTest : public Test\n>>> +{\n>>> +protected:\n>>> +\tvoid cameraAddedHandler(std::shared_ptr cam)\n>>> +\t{\n>>> +\t\tcameraAddedPass_ = true;\n>>> +\t}\n>>> +\n>>> +\tvoid cameraRemovedHandler(std::shared_ptr cam)\n>>> +\t{\n>>> +\t\tcameraRemovedPass_ = true;\n>>> +\t}\n>>> +\n>>> +\tint init()\n>>> +\t{\n>>> +\t\tif (!File::exists(\"/sys/module/uvcvideo\")) {\n>>> +\t\t\tstd::cout << \"uvcvideo driver is not loaded, skipping\" << std::endl;\n>>> +\t\t\treturn TestSkip;\n>>> +\t\t}\n>>> +\n>>> +\t\tif (geteuid() != 0) {\n>>> +\t\t\tstd::cout << \"This test requires root permissions, skipping\" << std::endl;\n>>> +\t\t\treturn TestSkip;\n>>> +\t\t}\n>>> +\n>>> +\t\tcm_ = new CameraManager();\n>>> +\t\tif (cm_->start()) {\n>>> +\t\t\tstd::cout << \"Failed to start camera manager\" << std::endl;\n>>> +\t\t\treturn TestFail;\n>>> +\t\t}\n>>> +\n>>> +\t\tcameraAddedPass_ = false;\n>>> +\t\tcameraRemovedPass_ = false;\n>>> +\n>>> +\t\tcm_->newCameraAdded.connect(this, &HotplugTest::cameraAddedHandler);\n>>> +\t\tcm_->cameraRemoved.connect(this, &HotplugTest::cameraRemovedHandler);\n>>> +\n>>> +\t\tuvc_toplevel_ = \"/sys/module/uvcvideo/drivers/usb:uvcvideo/\";\n>> Maybe /sys/bus/usb/drivers/uvcvideo/ to avoid traversing symlinks ? It\n>> makes little difference in practice I suppose.\n>>\n>> s/uvc_toplevel_/uvcTopLevel_/\n>>\n>> And I would even call this uvcDriverDir to make it more explicit. Now\n>> that I think about it, as uvc_toplevel_ is constant, maybe we should\n>> have\n>>\n>> \tstatic const std::string uvcDriverDir_;\n>>\n>> as a static class member.\n>>\n>>> +\n>>> +\t\treturn 0;\n>>> +\t}\n>>> +\n>>> +\tint run()\n>>> +\t{\n>>> +\t\tDIR *dir;\n>>> +\t\tstruct dirent *dirent, *dirent2;\n>>> +\t\tstd::string uvc_driver_dir;\n>>> +\t\tbool uvc_driver_found = false;\n>> camelCase please :-)\n>>\n>>> +\n>>> +\t\tdir = opendir(uvc_toplevel_.c_str());\n>>> +\t\t/* Find a UVC device driver symlink, which we can bind/unbind */\n>> s/unbind/unbind./ (same for the other comments below)\n>>\n>>> +\t\twhile ((dirent = readdir(dir)) != nullptr) {\n>>> +\t\t\tif (dirent->d_type != DT_LNK)\n>>> +\t\t\t\tcontinue;\n>>> +\n>>> +\t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n>>> +\t\t\tDIR *device_driver = opendir(child_dir.c_str());\n>>> +\t\t\twhile ((dirent2 = readdir(device_driver)) != nullptr) {\n>>> +\t\t\t\tif (strncmp(dirent2->d_name, \"video4linux\", 11) == 0) {\n>>> +\t\t\t\t\tuvc_driver_dir = dirent->d_name;\n>>> +\t\t\t\t\tuvc_driver_found = true;\n>>> +\t\t\t\t\tbreak;\n>>> +\t\t\t\t}\n>>> +\t\t\t}\n>>> +\t\t\tclosedir(device_driver);\n>>> +\n>>> +\t\t\tif (uvc_driver_found)\n>>> +\t\t\t\tbreak;\n>> Can't this be simplified with\n>>\n>> \t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n>> \t\t\tif (!File::exist(uvc_toplevel_ + dirent->d_name + \"/video4linux\"))\n>> \t\t\t\tcontinue;\n>>\n>> \t\t\tuvc_driver_dir = dirent->d_name;\n>> \t\t\tbreak;\n>>\n>>> +\t\t}\n>>> +\t\tclosedir(dir);\n>>> +\n>>> +\t\t/* If no UVC driver found, skip */\n>>> +\t\tif (!uvc_driver_found)\n>> And here,\n>>\n>> \t\tif (uvc_driver_dir.empty())\n>>\n>> to remove uvc_driver_found.\n>>\n>>> +\t\t\treturn TestSkip;\n>>> +\n>>> +\t\t/* Unbind a camera, process events */\n>>> +\t\tint fd1 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/unbind\", O_WRONLY);\n>> Maybe replacing the hardcoded string with uvc_toplevel_ + \"unbind\" ?\n>> This would become\n>>\n>> \t\tint fd1 = open((uvcDriverDir_ + \"unbind\").c_str(), O_WRONLY);\n>>\n>>> +\t\twrite(fd1, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n>>> +\t\tclose(fd1);\n>> Blank line here ?\n>>\n>> I wonder if using the C++ file API would be simpler here.\n>>\n>> \t\tstd::ofstream(uvcDriverDir_ + \"unbind\", std::ios::binary)\n>> \t\t\t<< uvc_driver_dir;;\n>>\n>> and that's it.\n>>\n>>> +\t\tTimer timer;\n>>> +\t\ttimer.start(1000);\n>>> +\t\twhile (timer.isRunning())\n>> \t\twhile (timer.isRunning() && !cameraRemovedPass_)\n>>\n>> to shorten the wait time. Same below.\n>>\n>>> +\t\t\tThread::current()->eventDispatcher()->processEvents();\n>>> +\n>> How about already testing for cameraRemovedPass_ here ?\n>>\n>> \t\tif (!cameraRemovedPass_)\n>> \t\t\treturn TestFail;\n>>\n>>> +\t\t/* \\todo: Fix this workaround of stopping and starting the camera-manager.\n>>> +\t\t * We need to do this, so that cm_ release all references to the uvc media symlinks.\n>>> +\t\t */\n>> We can merge the series without fixing this, but I think it needs to be\n>> handled sooner than latter.\n> I've had a quick look, and this seems to be caused by the pipeline\n> handler never getting deleted on hot-unplug. This is due to a reference\n> to the pipeline handler being stored in the camera manager and never\n> dropped.\n>\n> I'm not sure we actually need to store those references. Can I let you\n> investigate ?\n\nSure.\n\nI am starting to address these review comments locally, so I will sort \nthis issue out before submitting another round.\n\nThe investigation has been pending for a while now from my side, thanks \nfor looking into it and finding out the problem.\n\n>\n>>> +\t\tcm_->stop();\n>>> +\t\tif (cm_->start()) {\n>>> +\t\t\tstd::cout << \"Failed to restart camera-manager\" << std::endl;\n>>> +\t\t\treturn TestFail;\n>>> +\t\t}\n>>> +\n>>> +\t\t/* Bind the camera again, process events */\n>>> +\t\tint fd2 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/bind\", O_WRONLY);\n>> You don't need fd2, you can reuse fd1 (which should be renamed fd).\n>>\n>>> +\t\twrite(fd2, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n>>> +\t\tclose(fd2);\n>>> +\n>>> +\t\ttimer.start(1000);\n>>> +\t\twhile (timer.isRunning())\n>>> +\t\t\tThread::current()->eventDispatcher()->processEvents();\n>>> +\n>>> +\t\tif (cameraAddedPass_ && cameraRemovedPass_)\n>>> +\t\t\treturn TestPass;\n>>> +\t\telse\n>>> +\t\t\treturn TestFail;\n>> This would become\n>>\n>> \t\tif (!cameraAddedPass_)\n>> \t\t\treturn TestFail;\n>>\n>> \t\treturn TestPass;\n>>\n>>> +\t}\n>>> +\n>>> +\tvoid cleanup()\n>>> +\t{\n>>> +\t\tcm_->stop();\n>>> +\t\tdelete cm_;\n>>> +\t}\n>>> +\n>>> +private:\n>>> +\tCameraManager *cm_;\n>>> +\tstd::string uvc_toplevel_;\n>>> +\tbool cameraRemovedPass_;\n>> Maybe s/cameraRemovedPass_/cameraRemoved_/ ?\n>>\n>>> +\tbool cameraAddedPass_;\n>> Same here.\n>>\n>>> +};\n>>> +\n>>> +TEST_REGISTER(HotplugTest)\n>>> +\n>>> diff --git a/test/meson.build b/test/meson.build\n>>> index bd7da14..f7e27d7 100644\n>>> --- a/test/meson.build\n>>> +++ b/test/meson.build\n>>> @@ -31,6 +31,7 @@ internal_tests = [\n>>> ['file', 'file.cpp'],\n>>> ['file-descriptor', 'file-descriptor.cpp'],\n>>> ['message', 'message.cpp'],\n>>> + ['hotplug-cameras', 'hotplug-cameras.cpp'],\n>> Alphabetical order ?\n>>\n>>> ['object', 'object.cpp'],\n>>> ['object-invoke', 'object-invoke.cpp'],\n>>> ['signal-threads', 'signal-threads.cpp'],","headers":{"Return-Path":"","Received":["from o1.f.az.sendgrid.net (o1.f.az.sendgrid.net [208.117.55.132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BBE7561027\n\tfor ;\n\tMon, 8 Jun 2020 16:20:09 +0200 (CEST)","by filter0084p3las1.sendgrid.net with SMTP id\n\tfilter0084p3las1-6626-5EDE4917-237\n\t2020-06-08 14:20:07.775727735 +0000 UTC m=+485841.171309706","from mail.uajain.com (unknown)\n\tby ismtpd0005p1maa1.sendgrid.net (SG) with ESMTP\n\tid l2WAGWqUSAG1dRgSzNQOgw Mon, 08 Jun 2020 14:20:07.329 +0000 (UTC)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=uajain.com\n\theader.i=@uajain.com header.b=\"HHoOb5Ko\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=uajain.com;\n\th=subject:references:from:mime-version:in-reply-to:to:cc:content-type:\n\tcontent-transfer-encoding;\n\ts=s1; bh=YgGei69Ed1Yx1bhI2azPwuB3oPte3tjEpC2lBBT1nfQ=;\n\tb=HHoOb5KopQDMtFkI3DS/hT+N6V9pUlpqm7y/4LRa58/BlMqY3n+/vCXdy433VQeCHy+s\n\tQ9YcrQC0p0WqOL9Lkm7o/rw7/SQu+SHeIWlAypWb4dgRJwLrH1AysTWGX0cOtb2w24QS/V\n\tSV0BWTm9UnVRf+2ykFJRSQ2+TPry5U6G4=","References":"<20200521135416.13685-1-email@uajain.com>\n\t<20200521135416.13685-6-email@uajain.com>\n\t<20200607222436.GE22208@pendragon.ideasonboard.com>\n\t<20200607225440.GG22208@pendragon.ideasonboard.com>","From":"Umang Jain ","Message-ID":"<6af4bfa7-50d0-1f03-d4a5-a1f5dc5559db@uajain.com>","Date":"Mon, 08 Jun 2020 14:20:07 +0000 (UTC)","Mime-Version":"1.0","In-Reply-To":"<20200607225440.GG22208@pendragon.ideasonboard.com>","X-SG-EID":"1Q40EQ7YGir8a9gjSIAdTjhngY657NMk9ckeo4dbHZDiOpywc/L3L9rFqlwE4KPc1jYMYoB2JcJvIPCLN/wEG9R5DdQY2iS7mlq+c421ybpsjuSIiu9OUHlPU/G2KGPrDV9hCoRSTd1mkDcm8xeUepCNWau3Mml0d0cfAoUcbkfB1Fck0RWgu8cQlFxLcJIVRYGR8sGyEZ1bL+rnt8y3b7tz/MVOaVcAjQEeMHHOMbIJQR2AT9QNaz3TaJe59HBC","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=us-ascii; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 08 Jun 2020 14:20:10 -0000"}},{"id":5137,"web_url":"https://patchwork.libcamera.org/comment/5137/","msgid":"<20200608142844.GH5896@pendragon.ideasonboard.com>","date":"2020-06-08T14:28:44","subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Mon, Jun 08, 2020 at 02:20:07PM +0000, Umang Jain wrote:\n> On 6/8/20 4:24 AM, Laurent Pinchart wrote:\n> > On Mon, Jun 08, 2020 at 01:24:37AM +0300, Laurent Pinchart wrote:\n> >> On Thu, May 21, 2020 at 01:54:27PM +0000, Umang Jain wrote:\n> >>> This test checks the code-paths for camera's hotplugged and\n> >>> unplugged support. It is based off bind/unbind of a UVC device\n> >> s/off/on/\n> >>\n> >>> from sysfs. Hence, this tests required root permissions to run\n> >> s/tests required/test requires/\n> >>\n> >>> and should have atleast one already bound UVC device present\n> >> s/atleast/at least/\n> >>\n> >>> on the system.\n> >> s/on/in/\n> >>\n> >>> Signed-off-by: Umang Jain \n> >>> ---\n> >>> test/hotplug-cameras.cpp | 153 +++++++++++++++++++++++++++++++++++++++\n> >>> test/meson.build | 1 +\n> >>> 2 files changed, 154 insertions(+)\n> >>> create mode 100644 test/hotplug-cameras.cpp\n> >>>\n> >>> diff --git a/test/hotplug-cameras.cpp b/test/hotplug-cameras.cpp\n> >>> new file mode 100644\n> >>> index 0000000..72c370f\n> >>> --- /dev/null\n> >>> +++ b/test/hotplug-cameras.cpp\n> >>> @@ -0,0 +1,153 @@\n> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2020, Umang Jain \n> >>> + *\n> >>> + * hotplug-cameras.cpp - Emulate cameraAdded/cameraRemoved signals in CameraManager\n> >> Maybe s/Emulate/Test/ ?\n> >>\n> >>> + */\n> >>> +\n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +\n> >> No need for a blank line here, and you can then merge the two groups of\n> >> headers in alphabetical order.\n> >>\n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +\n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +\n> >>> +#include \"libcamera/internal/file.h\"\n> >>> +#include \"libcamera/internal/thread.h\"\n> >>> +\n> >>> +#include \"test.h\"\n> >>> +\n> >>> +using namespace std;\n> >> As you use the std namespace explicitly below, you can drop this line.\n> >>\n> >>> +using namespace libcamera;\n> >>> +\n> >>> +class HotplugTest : public Test\n> >>> +{\n> >>> +protected:\n> >>> +\tvoid cameraAddedHandler(std::shared_ptr cam)\n> >>> +\t{\n> >>> +\t\tcameraAddedPass_ = true;\n> >>> +\t}\n> >>> +\n> >>> +\tvoid cameraRemovedHandler(std::shared_ptr cam)\n> >>> +\t{\n> >>> +\t\tcameraRemovedPass_ = true;\n> >>> +\t}\n> >>> +\n> >>> +\tint init()\n> >>> +\t{\n> >>> +\t\tif (!File::exists(\"/sys/module/uvcvideo\")) {\n> >>> +\t\t\tstd::cout << \"uvcvideo driver is not loaded, skipping\" << std::endl;\n> >>> +\t\t\treturn TestSkip;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tif (geteuid() != 0) {\n> >>> +\t\t\tstd::cout << \"This test requires root permissions, skipping\" << std::endl;\n> >>> +\t\t\treturn TestSkip;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tcm_ = new CameraManager();\n> >>> +\t\tif (cm_->start()) {\n> >>> +\t\t\tstd::cout << \"Failed to start camera manager\" << std::endl;\n> >>> +\t\t\treturn TestFail;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tcameraAddedPass_ = false;\n> >>> +\t\tcameraRemovedPass_ = false;\n> >>> +\n> >>> +\t\tcm_->newCameraAdded.connect(this, &HotplugTest::cameraAddedHandler);\n> >>> +\t\tcm_->cameraRemoved.connect(this, &HotplugTest::cameraRemovedHandler);\n> >>> +\n> >>> +\t\tuvc_toplevel_ = \"/sys/module/uvcvideo/drivers/usb:uvcvideo/\";\n> >> Maybe /sys/bus/usb/drivers/uvcvideo/ to avoid traversing symlinks ? It\n> >> makes little difference in practice I suppose.\n> >>\n> >> s/uvc_toplevel_/uvcTopLevel_/\n> >>\n> >> And I would even call this uvcDriverDir to make it more explicit. Now\n> >> that I think about it, as uvc_toplevel_ is constant, maybe we should\n> >> have\n> >>\n> >> \tstatic const std::string uvcDriverDir_;\n> >>\n> >> as a static class member.\n> >>\n> >>> +\n> >>> +\t\treturn 0;\n> >>> +\t}\n> >>> +\n> >>> +\tint run()\n> >>> +\t{\n> >>> +\t\tDIR *dir;\n> >>> +\t\tstruct dirent *dirent, *dirent2;\n> >>> +\t\tstd::string uvc_driver_dir;\n> >>> +\t\tbool uvc_driver_found = false;\n> >> camelCase please :-)\n> >>\n> >>> +\n> >>> +\t\tdir = opendir(uvc_toplevel_.c_str());\n> >>> +\t\t/* Find a UVC device driver symlink, which we can bind/unbind */\n> >> s/unbind/unbind./ (same for the other comments below)\n> >>\n> >>> +\t\twhile ((dirent = readdir(dir)) != nullptr) {\n> >>> +\t\t\tif (dirent->d_type != DT_LNK)\n> >>> +\t\t\t\tcontinue;\n> >>> +\n> >>> +\t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n> >>> +\t\t\tDIR *device_driver = opendir(child_dir.c_str());\n> >>> +\t\t\twhile ((dirent2 = readdir(device_driver)) != nullptr) {\n> >>> +\t\t\t\tif (strncmp(dirent2->d_name, \"video4linux\", 11) == 0) {\n> >>> +\t\t\t\t\tuvc_driver_dir = dirent->d_name;\n> >>> +\t\t\t\t\tuvc_driver_found = true;\n> >>> +\t\t\t\t\tbreak;\n> >>> +\t\t\t\t}\n> >>> +\t\t\t}\n> >>> +\t\t\tclosedir(device_driver);\n> >>> +\n> >>> +\t\t\tif (uvc_driver_found)\n> >>> +\t\t\t\tbreak;\n> >> Can't this be simplified with\n> >>\n> >> \t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n> >> \t\t\tif (!File::exist(uvc_toplevel_ + dirent->d_name + \"/video4linux\"))\n> >> \t\t\t\tcontinue;\n> >>\n> >> \t\t\tuvc_driver_dir = dirent->d_name;\n> >> \t\t\tbreak;\n> >>\n> >>> +\t\t}\n> >>> +\t\tclosedir(dir);\n> >>> +\n> >>> +\t\t/* If no UVC driver found, skip */\n> >>> +\t\tif (!uvc_driver_found)\n> >> And here,\n> >>\n> >> \t\tif (uvc_driver_dir.empty())\n> >>\n> >> to remove uvc_driver_found.\n> >>\n> >>> +\t\t\treturn TestSkip;\n> >>> +\n> >>> +\t\t/* Unbind a camera, process events */\n> >>> +\t\tint fd1 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/unbind\", O_WRONLY);\n> >> Maybe replacing the hardcoded string with uvc_toplevel_ + \"unbind\" ?\n> >> This would become\n> >>\n> >> \t\tint fd1 = open((uvcDriverDir_ + \"unbind\").c_str(), O_WRONLY);\n> >>\n> >>> +\t\twrite(fd1, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n> >>> +\t\tclose(fd1);\n> >> Blank line here ?\n> >>\n> >> I wonder if using the C++ file API would be simpler here.\n> >>\n> >> \t\tstd::ofstream(uvcDriverDir_ + \"unbind\", std::ios::binary)\n> >> \t\t\t<< uvc_driver_dir;;\n> >>\n> >> and that's it.\n> >>\n> >>> +\t\tTimer timer;\n> >>> +\t\ttimer.start(1000);\n> >>> +\t\twhile (timer.isRunning())\n> >> \t\twhile (timer.isRunning() && !cameraRemovedPass_)\n> >>\n> >> to shorten the wait time. Same below.\n> >>\n> >>> +\t\t\tThread::current()->eventDispatcher()->processEvents();\n> >>> +\n> >> How about already testing for cameraRemovedPass_ here ?\n> >>\n> >> \t\tif (!cameraRemovedPass_)\n> >> \t\t\treturn TestFail;\n> >>\n> >>> +\t\t/* \\todo: Fix this workaround of stopping and starting the camera-manager.\n> >>> +\t\t * We need to do this, so that cm_ release all references to the uvc media symlinks.\n> >>> +\t\t */\n> >> We can merge the series without fixing this, but I think it needs to be\n> >> handled sooner than latter.\n> > I've had a quick look, and this seems to be caused by the pipeline\n> > handler never getting deleted on hot-unplug. This is due to a reference\n> > to the pipeline handler being stored in the camera manager and never\n> > dropped.\n> >\n> > I'm not sure we actually need to store those references. Can I let you\n> > investigate ?\n> \n> Sure.\n> \n> I am starting to address these review comments locally, so I will sort \n> this issue out before submitting another round.\n> \n> The investigation has been pending for a while now from my side, thanks \n> for looking into it and finding out the problem.\n\nYou're welcome. Thank you for working on it in the first place :-)\n\nOnce you have a fix, I recommend running this test under valgrind, it\nshould help catching use-after-free issues. I've sent a patch yesterday\nthat avoids a valgrind crash with libcamera, I hope to get it merged\nsoon, but you may want to apply it locally in the meantime.\n\n> >>> +\t\tcm_->stop();\n> >>> +\t\tif (cm_->start()) {\n> >>> +\t\t\tstd::cout << \"Failed to restart camera-manager\" << std::endl;\n> >>> +\t\t\treturn TestFail;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\t/* Bind the camera again, process events */\n> >>> +\t\tint fd2 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/bind\", O_WRONLY);\n> >> You don't need fd2, you can reuse fd1 (which should be renamed fd).\n> >>\n> >>> +\t\twrite(fd2, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n> >>> +\t\tclose(fd2);\n> >>> +\n> >>> +\t\ttimer.start(1000);\n> >>> +\t\twhile (timer.isRunning())\n> >>> +\t\t\tThread::current()->eventDispatcher()->processEvents();\n> >>> +\n> >>> +\t\tif (cameraAddedPass_ && cameraRemovedPass_)\n> >>> +\t\t\treturn TestPass;\n> >>> +\t\telse\n> >>> +\t\t\treturn TestFail;\n> >> This would become\n> >>\n> >> \t\tif (!cameraAddedPass_)\n> >> \t\t\treturn TestFail;\n> >>\n> >> \t\treturn TestPass;\n> >>\n> >>> +\t}\n> >>> +\n> >>> +\tvoid cleanup()\n> >>> +\t{\n> >>> +\t\tcm_->stop();\n> >>> +\t\tdelete cm_;\n> >>> +\t}\n> >>> +\n> >>> +private:\n> >>> +\tCameraManager *cm_;\n> >>> +\tstd::string uvc_toplevel_;\n> >>> +\tbool cameraRemovedPass_;\n> >> Maybe s/cameraRemovedPass_/cameraRemoved_/ ?\n> >>\n> >>> +\tbool cameraAddedPass_;\n> >> Same here.\n> >>\n> >>> +};\n> >>> +\n> >>> +TEST_REGISTER(HotplugTest)\n> >>> +\n> >>> diff --git a/test/meson.build b/test/meson.build\n> >>> index bd7da14..f7e27d7 100644\n> >>> --- a/test/meson.build\n> >>> +++ b/test/meson.build\n> >>> @@ -31,6 +31,7 @@ internal_tests = [\n> >>> ['file', 'file.cpp'],\n> >>> ['file-descriptor', 'file-descriptor.cpp'],\n> >>> ['message', 'message.cpp'],\n> >>> + ['hotplug-cameras', 'hotplug-cameras.cpp'],\n> >> Alphabetical order ?\n> >>\n> >>> ['object', 'object.cpp'],\n> >>> ['object-invoke', 'object-invoke.cpp'],\n> >>> ['signal-threads', 'signal-threads.cpp'],","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 3F25161027\n\tfor ;\n\tMon, 8 Jun 2020 16:29:04 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B79F524F;\n\tMon, 8 Jun 2020 16:29:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"awPkH/0Q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591626543;\n\tbh=3NH1SIX8327/mY8MIAtx/Zlv5gd0i7/Hyok5PeW4koE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=awPkH/0QLRE6R8gd8us190hMzPEzcB/kilxDNZUMANq645LqWfzRFT1rVNWjn5FqW\n\tOQIb37blwkVUVEy8NtTplt+YIFnvOruE/qEKtoWpYJQrVz9i2rmb8b1eXg6l+I1m8c\n\tMJx4B5HMFBcuOgGncIGrrOJLezPC1JyI8ha3AH3Y=","Date":"Mon, 8 Jun 2020 17:28:44 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200608142844.GH5896@pendragon.ideasonboard.com>","References":"<20200521135416.13685-1-email@uajain.com>\n\t<20200521135416.13685-6-email@uajain.com>\n\t<20200607222436.GE22208@pendragon.ideasonboard.com>\n\t<20200607225440.GG22208@pendragon.ideasonboard.com>\n\t<6af4bfa7-50d0-1f03-d4a5-a1f5dc5559db@uajain.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<6af4bfa7-50d0-1f03-d4a5-a1f5dc5559db@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 08 Jun 2020 14:29:04 -0000"}},{"id":5167,"web_url":"https://patchwork.libcamera.org/comment/5167/","msgid":"","date":"2020-06-10T14:42:56","subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Laurent,\n\nOn 6/8/20 4:24 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Mon, Jun 08, 2020 at 01:24:37AM +0300, Laurent Pinchart wrote:\n>> On Thu, May 21, 2020 at 01:54:27PM +0000, Umang Jain wrote:\n>>> This test checks the code-paths for camera's hotplugged and\n>>> unplugged support. It is based off bind/unbind of a UVC device\n>> s/off/on/\n>>\n>>> from sysfs. Hence, this tests required root permissions to run\n>> s/tests required/test requires/\n>>\n>>> and should have atleast one already bound UVC device present\n>> s/atleast/at least/\n>>\n>>> on the system.\n>> s/on/in/\n>>\n>>> Signed-off-by: Umang Jain \n>>> ---\n>>> test/hotplug-cameras.cpp | 153 +++++++++++++++++++++++++++++++++++++++\n>>> test/meson.build | 1 +\n>>> 2 files changed, 154 insertions(+)\n>>> create mode 100644 test/hotplug-cameras.cpp\n>>>\n>>> diff --git a/test/hotplug-cameras.cpp b/test/hotplug-cameras.cpp\n>>> new file mode 100644\n>>> index 0000000..72c370f\n>>> --- /dev/null\n>>> +++ b/test/hotplug-cameras.cpp\n>>> @@ -0,0 +1,153 @@\n>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>> +/*\n>>> + * Copyright (C) 2020, Umang Jain \n>>> + *\n>>> + * hotplug-cameras.cpp - Emulate cameraAdded/cameraRemoved signals in CameraManager\n>> Maybe s/Emulate/Test/ ?\n>>\n>>> + */\n>>> +\n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +\n>> No need for a blank line here, and you can then merge the two groups of\n>> headers in alphabetical order.\n>>\n>>> +#include \n>>> +#include \n>>> +#include \n>>> +\n>>> +#include \n>>> +#include \n>>> +#include \n>>> +#include \n>>> +\n>>> +#include \"libcamera/internal/file.h\"\n>>> +#include \"libcamera/internal/thread.h\"\n>>> +\n>>> +#include \"test.h\"\n>>> +\n>>> +using namespace std;\n>> As you use the std namespace explicitly below, you can drop this line.\n>>\n>>> +using namespace libcamera;\n>>> +\n>>> +class HotplugTest : public Test\n>>> +{\n>>> +protected:\n>>> +\tvoid cameraAddedHandler(std::shared_ptr cam)\n>>> +\t{\n>>> +\t\tcameraAddedPass_ = true;\n>>> +\t}\n>>> +\n>>> +\tvoid cameraRemovedHandler(std::shared_ptr cam)\n>>> +\t{\n>>> +\t\tcameraRemovedPass_ = true;\n>>> +\t}\n>>> +\n>>> +\tint init()\n>>> +\t{\n>>> +\t\tif (!File::exists(\"/sys/module/uvcvideo\")) {\n>>> +\t\t\tstd::cout << \"uvcvideo driver is not loaded, skipping\" << std::endl;\n>>> +\t\t\treturn TestSkip;\n>>> +\t\t}\n>>> +\n>>> +\t\tif (geteuid() != 0) {\n>>> +\t\t\tstd::cout << \"This test requires root permissions, skipping\" << std::endl;\n>>> +\t\t\treturn TestSkip;\n>>> +\t\t}\n>>> +\n>>> +\t\tcm_ = new CameraManager();\n>>> +\t\tif (cm_->start()) {\n>>> +\t\t\tstd::cout << \"Failed to start camera manager\" << std::endl;\n>>> +\t\t\treturn TestFail;\n>>> +\t\t}\n>>> +\n>>> +\t\tcameraAddedPass_ = false;\n>>> +\t\tcameraRemovedPass_ = false;\n>>> +\n>>> +\t\tcm_->newCameraAdded.connect(this, &HotplugTest::cameraAddedHandler);\n>>> +\t\tcm_->cameraRemoved.connect(this, &HotplugTest::cameraRemovedHandler);\n>>> +\n>>> +\t\tuvc_toplevel_ = \"/sys/module/uvcvideo/drivers/usb:uvcvideo/\";\n>> Maybe /sys/bus/usb/drivers/uvcvideo/ to avoid traversing symlinks ? It\n>> makes little difference in practice I suppose.\n>>\n>> s/uvc_toplevel_/uvcTopLevel_/\n>>\n>> And I would even call this uvcDriverDir to make it more explicit. Now\n>> that I think about it, as uvc_toplevel_ is constant, maybe we should\n>> have\n>>\n>> \tstatic const std::string uvcDriverDir_;\n>>\n>> as a static class member.\n>>\n>>> +\n>>> +\t\treturn 0;\n>>> +\t}\n>>> +\n>>> +\tint run()\n>>> +\t{\n>>> +\t\tDIR *dir;\n>>> +\t\tstruct dirent *dirent, *dirent2;\n>>> +\t\tstd::string uvc_driver_dir;\n>>> +\t\tbool uvc_driver_found = false;\n>> camelCase please :-)\n>>\n>>> +\n>>> +\t\tdir = opendir(uvc_toplevel_.c_str());\n>>> +\t\t/* Find a UVC device driver symlink, which we can bind/unbind */\n>> s/unbind/unbind./ (same for the other comments below)\n>>\n>>> +\t\twhile ((dirent = readdir(dir)) != nullptr) {\n>>> +\t\t\tif (dirent->d_type != DT_LNK)\n>>> +\t\t\t\tcontinue;\n>>> +\n>>> +\t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n>>> +\t\t\tDIR *device_driver = opendir(child_dir.c_str());\n>>> +\t\t\twhile ((dirent2 = readdir(device_driver)) != nullptr) {\n>>> +\t\t\t\tif (strncmp(dirent2->d_name, \"video4linux\", 11) == 0) {\n>>> +\t\t\t\t\tuvc_driver_dir = dirent->d_name;\n>>> +\t\t\t\t\tuvc_driver_found = true;\n>>> +\t\t\t\t\tbreak;\n>>> +\t\t\t\t}\n>>> +\t\t\t}\n>>> +\t\t\tclosedir(device_driver);\n>>> +\n>>> +\t\t\tif (uvc_driver_found)\n>>> +\t\t\t\tbreak;\n>> Can't this be simplified with\n>>\n>> \t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n>> \t\t\tif (!File::exist(uvc_toplevel_ + dirent->d_name + \"/video4linux\"))\n>> \t\t\t\tcontinue;\n>>\n>> \t\t\tuvc_driver_dir = dirent->d_name;\n>> \t\t\tbreak;\n>>\n>>> +\t\t}\n>>> +\t\tclosedir(dir);\n>>> +\n>>> +\t\t/* If no UVC driver found, skip */\n>>> +\t\tif (!uvc_driver_found)\n>> And here,\n>>\n>> \t\tif (uvc_driver_dir.empty())\n>>\n>> to remove uvc_driver_found.\n>>\n>>> +\t\t\treturn TestSkip;\n>>> +\n>>> +\t\t/* Unbind a camera, process events */\n>>> +\t\tint fd1 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/unbind\", O_WRONLY);\n>> Maybe replacing the hardcoded string with uvc_toplevel_ + \"unbind\" ?\n>> This would become\n>>\n>> \t\tint fd1 = open((uvcDriverDir_ + \"unbind\").c_str(), O_WRONLY);\n>>\n>>> +\t\twrite(fd1, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n>>> +\t\tclose(fd1);\n>> Blank line here ?\n>>\n>> I wonder if using the C++ file API would be simpler here.\n>>\n>> \t\tstd::ofstream(uvcDriverDir_ + \"unbind\", std::ios::binary)\n>> \t\t\t<< uvc_driver_dir;;\n>>\n>> and that's it.\n>>\n>>> +\t\tTimer timer;\n>>> +\t\ttimer.start(1000);\n>>> +\t\twhile (timer.isRunning())\n>> \t\twhile (timer.isRunning() && !cameraRemovedPass_)\n>>\n>> to shorten the wait time. Same below.\n>>\n>>> +\t\t\tThread::current()->eventDispatcher()->processEvents();\n>>> +\n>> How about already testing for cameraRemovedPass_ here ?\n>>\n>> \t\tif (!cameraRemovedPass_)\n>> \t\t\treturn TestFail;\n>>\n>>> +\t\t/* \\todo: Fix this workaround of stopping and starting the camera-manager.\n>>> +\t\t * We need to do this, so that cm_ release all references to the uvc media symlinks.\n>>> +\t\t */\n>> We can merge the series without fixing this, but I think it needs to be\n>> handled sooner than latter.\n> I've had a quick look, and this seems to be caused by the pipeline\n> handler never getting deleted on hot-unplug. This is due to a reference\n> to the pipeline handler being stored in the camera manager and never\n> dropped.\n>\n> I'm not sure we actually need to store those references. Can I let you\n> investigate ?\n\nI see those references in CameraManager::pipes_ not getting dropped \nanywhere.\n\nAlso, I want to understand/handle this situation, in \nCameraManager::Private::init() (on master branch):\n\n                 std::shared_ptr pipe = \nfactory->create(cm_);\n\npipe has use_count() of 2 (I checked); and a bit below:\n\n                 pipes_.push_back(std::move(pipe));\n\npipes_ vector gets a created PipelineHander at refcount 2. Now, I can \ndrop \"a\" reference\n\nto this pipe that got added in pipes_ in maybe say, \nCameraManager::Private::removeCamera() on hot-unplug.\n\nWell, even then it will retain the refcount 1, so still the \nPipelineHandler is not destroyed on hot-unplug,\n\nwhich might be holding onto the media-device directory I pointed out as \n\\todo in the test code.\n\nDo you have any pointers on how can I make the refcount drop to 0, so \nthe PipelineHandler in pipes_ can be destroyed?\n\n>\n>>> +\t\tcm_->stop();\n>>> +\t\tif (cm_->start()) {\n>>> +\t\t\tstd::cout << \"Failed to restart camera-manager\" << std::endl;\n>>> +\t\t\treturn TestFail;\n>>> +\t\t}\n>>> +\n>>> +\t\t/* Bind the camera again, process events */\n>>> +\t\tint fd2 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/bind\", O_WRONLY);\n>> You don't need fd2, you can reuse fd1 (which should be renamed fd).\n>>\n>>> +\t\twrite(fd2, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n>>> +\t\tclose(fd2);\n>>> +\n>>> +\t\ttimer.start(1000);\n>>> +\t\twhile (timer.isRunning())\n>>> +\t\t\tThread::current()->eventDispatcher()->processEvents();\n>>> +\n>>> +\t\tif (cameraAddedPass_ && cameraRemovedPass_)\n>>> +\t\t\treturn TestPass;\n>>> +\t\telse\n>>> +\t\t\treturn TestFail;\n>> This would become\n>>\n>> \t\tif (!cameraAddedPass_)\n>> \t\t\treturn TestFail;\n>>\n>> \t\treturn TestPass;\n>>\n>>> +\t}\n>>> +\n>>> +\tvoid cleanup()\n>>> +\t{\n>>> +\t\tcm_->stop();\n>>> +\t\tdelete cm_;\n>>> +\t}\n>>> +\n>>> +private:\n>>> +\tCameraManager *cm_;\n>>> +\tstd::string uvc_toplevel_;\n>>> +\tbool cameraRemovedPass_;\n>> Maybe s/cameraRemovedPass_/cameraRemoved_/ ?\n>>\n>>> +\tbool cameraAddedPass_;\n>> Same here.\n>>\n>>> +};\n>>> +\n>>> +TEST_REGISTER(HotplugTest)\n>>> +\n>>> diff --git a/test/meson.build b/test/meson.build\n>>> index bd7da14..f7e27d7 100644\n>>> --- a/test/meson.build\n>>> +++ b/test/meson.build\n>>> @@ -31,6 +31,7 @@ internal_tests = [\n>>> ['file', 'file.cpp'],\n>>> ['file-descriptor', 'file-descriptor.cpp'],\n>>> ['message', 'message.cpp'],\n>>> + ['hotplug-cameras', 'hotplug-cameras.cpp'],\n>> Alphabetical order ?\n>>\n>>> ['object', 'object.cpp'],\n>>> ['object-invoke', 'object-invoke.cpp'],\n>>> ['signal-threads', 'signal-threads.cpp'],","headers":{"Return-Path":"","Received":["from o1.f.az.sendgrid.net (o1.f.az.sendgrid.net [208.117.55.132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C4FF0600F7\n\tfor ;\n\tWed, 10 Jun 2020 16:42:57 +0200 (CEST)","by filterdrecv-p3iad2-784dbb6bd8-z6chg with SMTP id\n\tfilterdrecv-p3iad2-784dbb6bd8-z6chg-19-5EE0F16F-A6\n\t2020-06-10 14:42:56.149203644 +0000 UTC m=+585367.182451810","from mail.uajain.com (unknown)\n\tby ismtpd0001p1maa1.sendgrid.net (SG) with ESMTP\n\tid f48ojCsKRCKQj94Di56FfA Wed, 10 Jun 2020 14:42:55.441 +0000 (UTC)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=uajain.com\n\theader.i=@uajain.com header.b=\"sWljxs1k\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=uajain.com;\n\th=subject:references:from:mime-version:in-reply-to:to:cc:content-type:\n\tcontent-transfer-encoding;\n\ts=s1; bh=9ufFsM/ZYmv02oClku0Y7APe6FzvTPxNzYuJuOSQlHM=;\n\tb=sWljxs1kZpDGTpQ0FjOchJQ7yvHoiUURQHX4FomRyolY/O2fUflAqDZWX68nODEvWJ2G\n\tZbElYwpeNZGMTUV6LMajBHgqpymYGwOSvoZOHYyfEWXXGlYzib2LAqnsT+lQWCyXhfaxF0\n\tmmdFXTIlbLGfGBa7HmkE1yUPtzr30P1y0=","References":"<20200521135416.13685-1-email@uajain.com>\n\t<20200521135416.13685-6-email@uajain.com>\n\t<20200607222436.GE22208@pendragon.ideasonboard.com>\n\t<20200607225440.GG22208@pendragon.ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Wed, 10 Jun 2020 14:42:56 +0000 (UTC)","Mime-Version":"1.0","In-Reply-To":"<20200607225440.GG22208@pendragon.ideasonboard.com>","X-SG-EID":"1Q40EQ7YGir8a9gjSIAdTjhngY657NMk9ckeo4dbHZDiOpywc/L3L9rFqlwE4KPcwyK5CDkYX8/iiotPdJ0nvrQxGiSPoKV/vtNH4yYzUCygJqc/jhHHsGEA1wAGBQzWdbbsLbRw6RzHAJEujb7z4XYiVT/vYpat7erj+98EtuRWHJVxgw1ORShHtqEsT54ROvtBswAZE9IFHN1sqlvaXrlupaj+Bfhg728u90ctySGszyEL+1bu+uGzT1Jt7s2P73ccewYoK1qkzcNGj4HtAw==","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=iso-8859-1; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 10 Jun 2020 14:42:58 -0000"}},{"id":5175,"web_url":"https://patchwork.libcamera.org/comment/5175/","msgid":"<20200611082723.GA5827@pendragon.ideasonboard.com>","date":"2020-06-11T08:27:23","subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Wed, Jun 10, 2020 at 02:42:56PM +0000, Umang Jain wrote:\n> On 6/8/20 4:24 AM, Laurent Pinchart wrote:\n> > On Mon, Jun 08, 2020 at 01:24:37AM +0300, Laurent Pinchart wrote:\n> >> On Thu, May 21, 2020 at 01:54:27PM +0000, Umang Jain wrote:\n> >>> This test checks the code-paths for camera's hotplugged and\n> >>> unplugged support. It is based off bind/unbind of a UVC device\n> >>\n> >> s/off/on/\n> >>\n> >>> from sysfs. Hence, this tests required root permissions to run\n> >>\n> >> s/tests required/test requires/\n> >>\n> >>> and should have atleast one already bound UVC device present\n> >>\n> >> s/atleast/at least/\n> >>\n> >>> on the system.\n> >>\n> >> s/on/in/\n> >>\n> >>> Signed-off-by: Umang Jain \n> >>> ---\n> >>> test/hotplug-cameras.cpp | 153 +++++++++++++++++++++++++++++++++++++++\n> >>> test/meson.build | 1 +\n> >>> 2 files changed, 154 insertions(+)\n> >>> create mode 100644 test/hotplug-cameras.cpp\n> >>>\n> >>> diff --git a/test/hotplug-cameras.cpp b/test/hotplug-cameras.cpp\n> >>> new file mode 100644\n> >>> index 0000000..72c370f\n> >>> --- /dev/null\n> >>> +++ b/test/hotplug-cameras.cpp\n> >>> @@ -0,0 +1,153 @@\n> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2020, Umang Jain \n> >>> + *\n> >>> + * hotplug-cameras.cpp - Emulate cameraAdded/cameraRemoved signals in CameraManager\n> >>\n> >> Maybe s/Emulate/Test/ ?\n> >>\n> >>> + */\n> >>> +\n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +\n> >>\n> >> No need for a blank line here, and you can then merge the two groups of\n> >> headers in alphabetical order.\n> >>\n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +\n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +\n> >>> +#include \"libcamera/internal/file.h\"\n> >>> +#include \"libcamera/internal/thread.h\"\n> >>> +\n> >>> +#include \"test.h\"\n> >>> +\n> >>> +using namespace std;\n> >>\n> >> As you use the std namespace explicitly below, you can drop this line.\n> >>\n> >>> +using namespace libcamera;\n> >>> +\n> >>> +class HotplugTest : public Test\n> >>> +{\n> >>> +protected:\n> >>> +\tvoid cameraAddedHandler(std::shared_ptr cam)\n> >>> +\t{\n> >>> +\t\tcameraAddedPass_ = true;\n> >>> +\t}\n> >>> +\n> >>> +\tvoid cameraRemovedHandler(std::shared_ptr cam)\n> >>> +\t{\n> >>> +\t\tcameraRemovedPass_ = true;\n> >>> +\t}\n> >>> +\n> >>> +\tint init()\n> >>> +\t{\n> >>> +\t\tif (!File::exists(\"/sys/module/uvcvideo\")) {\n> >>> +\t\t\tstd::cout << \"uvcvideo driver is not loaded, skipping\" << std::endl;\n> >>> +\t\t\treturn TestSkip;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tif (geteuid() != 0) {\n> >>> +\t\t\tstd::cout << \"This test requires root permissions, skipping\" << std::endl;\n> >>> +\t\t\treturn TestSkip;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tcm_ = new CameraManager();\n> >>> +\t\tif (cm_->start()) {\n> >>> +\t\t\tstd::cout << \"Failed to start camera manager\" << std::endl;\n> >>> +\t\t\treturn TestFail;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tcameraAddedPass_ = false;\n> >>> +\t\tcameraRemovedPass_ = false;\n> >>> +\n> >>> +\t\tcm_->newCameraAdded.connect(this, &HotplugTest::cameraAddedHandler);\n> >>> +\t\tcm_->cameraRemoved.connect(this, &HotplugTest::cameraRemovedHandler);\n> >>> +\n> >>> +\t\tuvc_toplevel_ = \"/sys/module/uvcvideo/drivers/usb:uvcvideo/\";\n> >>\n> >> Maybe /sys/bus/usb/drivers/uvcvideo/ to avoid traversing symlinks ? It\n> >> makes little difference in practice I suppose.\n> >>\n> >> s/uvc_toplevel_/uvcTopLevel_/\n> >>\n> >> And I would even call this uvcDriverDir to make it more explicit. Now\n> >> that I think about it, as uvc_toplevel_ is constant, maybe we should\n> >> have\n> >>\n> >> \tstatic const std::string uvcDriverDir_;\n> >>\n> >> as a static class member.\n> >>\n> >>> +\n> >>> +\t\treturn 0;\n> >>> +\t}\n> >>> +\n> >>> +\tint run()\n> >>> +\t{\n> >>> +\t\tDIR *dir;\n> >>> +\t\tstruct dirent *dirent, *dirent2;\n> >>> +\t\tstd::string uvc_driver_dir;\n> >>> +\t\tbool uvc_driver_found = false;\n> >>\n> >> camelCase please :-)\n> >>\n> >>> +\n> >>> +\t\tdir = opendir(uvc_toplevel_.c_str());\n> >>> +\t\t/* Find a UVC device driver symlink, which we can bind/unbind */\n> >>\n> >> s/unbind/unbind./ (same for the other comments below)\n> >>\n> >>> +\t\twhile ((dirent = readdir(dir)) != nullptr) {\n> >>> +\t\t\tif (dirent->d_type != DT_LNK)\n> >>> +\t\t\t\tcontinue;\n> >>> +\n> >>> +\t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n> >>> +\t\t\tDIR *device_driver = opendir(child_dir.c_str());\n> >>> +\t\t\twhile ((dirent2 = readdir(device_driver)) != nullptr) {\n> >>> +\t\t\t\tif (strncmp(dirent2->d_name, \"video4linux\", 11) == 0) {\n> >>> +\t\t\t\t\tuvc_driver_dir = dirent->d_name;\n> >>> +\t\t\t\t\tuvc_driver_found = true;\n> >>> +\t\t\t\t\tbreak;\n> >>> +\t\t\t\t}\n> >>> +\t\t\t}\n> >>> +\t\t\tclosedir(device_driver);\n> >>> +\n> >>> +\t\t\tif (uvc_driver_found)\n> >>> +\t\t\t\tbreak;\n> >>\n> >> Can't this be simplified with\n> >>\n> >> \t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n> >> \t\t\tif (!File::exist(uvc_toplevel_ + dirent->d_name + \"/video4linux\"))\n> >> \t\t\t\tcontinue;\n> >>\n> >> \t\t\tuvc_driver_dir = dirent->d_name;\n> >> \t\t\tbreak;\n> >>\n> >>> +\t\t}\n> >>> +\t\tclosedir(dir);\n> >>> +\n> >>> +\t\t/* If no UVC driver found, skip */\n> >>> +\t\tif (!uvc_driver_found)\n> >>\n> >> And here,\n> >>\n> >> \t\tif (uvc_driver_dir.empty())\n> >>\n> >> to remove uvc_driver_found.\n> >>\n> >>> +\t\t\treturn TestSkip;\n> >>> +\n> >>> +\t\t/* Unbind a camera, process events */\n> >>> +\t\tint fd1 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/unbind\", O_WRONLY);\n> >>\n> >> Maybe replacing the hardcoded string with uvc_toplevel_ + \"unbind\" ?\n> >> This would become\n> >>\n> >> \t\tint fd1 = open((uvcDriverDir_ + \"unbind\").c_str(), O_WRONLY);\n> >>\n> >>> +\t\twrite(fd1, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n> >>> +\t\tclose(fd1);\n> >>\n> >> Blank line here ?\n> >>\n> >> I wonder if using the C++ file API would be simpler here.\n> >>\n> >> \t\tstd::ofstream(uvcDriverDir_ + \"unbind\", std::ios::binary)\n> >> \t\t\t<< uvc_driver_dir;;\n> >>\n> >> and that's it.\n> >>\n> >>> +\t\tTimer timer;\n> >>> +\t\ttimer.start(1000);\n> >>> +\t\twhile (timer.isRunning())\n> >>\n> >> \t\twhile (timer.isRunning() && !cameraRemovedPass_)\n> >>\n> >> to shorten the wait time. Same below.\n> >>\n> >>> +\t\t\tThread::current()->eventDispatcher()->processEvents();\n> >>> +\n> >>\n> >> How about already testing for cameraRemovedPass_ here ?\n> >>\n> >> \t\tif (!cameraRemovedPass_)\n> >> \t\t\treturn TestFail;\n> >>\n> >>> +\t\t/* \\todo: Fix this workaround of stopping and starting the camera-manager.\n> >>> +\t\t * We need to do this, so that cm_ release all references to the uvc media symlinks.\n> >>> +\t\t */\n> >>\n> >> We can merge the series without fixing this, but I think it needs to be\n> >> handled sooner than latter.\n> >\n> > I've had a quick look, and this seems to be caused by the pipeline\n> > handler never getting deleted on hot-unplug. This is due to a reference\n> > to the pipeline handler being stored in the camera manager and never\n> > dropped.\n> >\n> > I'm not sure we actually need to store those references. Can I let you\n> > investigate ?\n> \n> I see those references in CameraManager::pipes_ not getting dropped \n> anywhere.\n> \n> Also, I want to understand/handle this situation, in \n> CameraManager::Private::init() (on master branch):\n> \n>                 std::shared_ptr pipe = factory->create(cm_);\n> \n> pipe has use_count() of 2 (I checked); and a bit below:\n\n2 ? I wonder where the second one comes from. Is it right after that\nline, or after the pipe->match() call ? Camera object retain a reference\nto the PipelineHandler, so that would explain it (one reference in the\nlocal pipe variable here, one reference in Camera::Private::pipe_).\n\n>                 pipes_.push_back(std::move(pipe));\n> \n> pipes_ vector gets a created PipelineHander at refcount 2. Now, I can \n> drop \"a\" reference to this pipe that got added in pipes_ in maybe say, \n> CameraManager::Private::removeCamera() on hot-unplug.\n\nI was wondering if we could actually remove the pipes_ vector\ncompletely. It would require being careful in the unplug path, as the\npipeline handler will be deleted as soon as the last camera is deleted,\nwhich can happen in PipelineHandler::disconnect(). The cameras._clear()\ncall at the end of the function may happen after the pipeline object is\ndeleted. Yes, objects can be deleted while their functions are running,\nit's scary, but allowed if you ensure that they don't access data :-)\nRunning the test in valgrind will help catch problems.\n\n> Well, even then it will retain the refcount 1, so still the \n> PipelineHandler is not destroyed on hot-unplug, which might be holding\n> onto the media-device directory I pointed out as \\todo in the test code.\n> \n> Do you have any pointers on how can I make the refcount drop to 0, so \n> the PipelineHandler in pipes_ can be destroyed?\n> \n> >>> +\t\tcm_->stop();\n> >>> +\t\tif (cm_->start()) {\n> >>> +\t\t\tstd::cout << \"Failed to restart camera-manager\" << std::endl;\n> >>> +\t\t\treturn TestFail;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\t/* Bind the camera again, process events */\n> >>> +\t\tint fd2 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/bind\", O_WRONLY);\n> >>\n> >> You don't need fd2, you can reuse fd1 (which should be renamed fd).\n> >>\n> >>> +\t\twrite(fd2, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n> >>> +\t\tclose(fd2);\n> >>> +\n> >>> +\t\ttimer.start(1000);\n> >>> +\t\twhile (timer.isRunning())\n> >>> +\t\t\tThread::current()->eventDispatcher()->processEvents();\n> >>> +\n> >>> +\t\tif (cameraAddedPass_ && cameraRemovedPass_)\n> >>> +\t\t\treturn TestPass;\n> >>> +\t\telse\n> >>> +\t\t\treturn TestFail;\n> >>\n> >> This would become\n> >>\n> >> \t\tif (!cameraAddedPass_)\n> >> \t\t\treturn TestFail;\n> >>\n> >> \t\treturn TestPass;\n> >>\n> >>> +\t}\n> >>> +\n> >>> +\tvoid cleanup()\n> >>> +\t{\n> >>> +\t\tcm_->stop();\n> >>> +\t\tdelete cm_;\n> >>> +\t}\n> >>> +\n> >>> +private:\n> >>> +\tCameraManager *cm_;\n> >>> +\tstd::string uvc_toplevel_;\n> >>> +\tbool cameraRemovedPass_;\n> >>\n> >> Maybe s/cameraRemovedPass_/cameraRemoved_/ ?\n> >>\n> >>> +\tbool cameraAddedPass_;\n> >>\n> >> Same here.\n> >>\n> >>> +};\n> >>> +\n> >>> +TEST_REGISTER(HotplugTest)\n> >>> +\n> >>> diff --git a/test/meson.build b/test/meson.build\n> >>> index bd7da14..f7e27d7 100644\n> >>> --- a/test/meson.build\n> >>> +++ b/test/meson.build\n> >>> @@ -31,6 +31,7 @@ internal_tests = [\n> >>> ['file', 'file.cpp'],\n> >>> ['file-descriptor', 'file-descriptor.cpp'],\n> >>> ['message', 'message.cpp'],\n> >>> + ['hotplug-cameras', 'hotplug-cameras.cpp'],\n> >>\n> >> Alphabetical order ?\n> >>\n> >>> ['object', 'object.cpp'],\n> >>> ['object-invoke', 'object-invoke.cpp'],\n> >>> ['signal-threads', 'signal-threads.cpp'],","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 F1DC161027\n\tfor ;\n\tThu, 11 Jun 2020 10:27:44 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 664702C9;\n\tThu, 11 Jun 2020 10:27:44 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"R9D/J2+v\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591864064;\n\tbh=C85wQgV2DNGoI2IRd5ZARLz0jhcq5PebZVcdmVMy17c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R9D/J2+v3jrsEuy7kJPkylghwMai6E+FEii0fTEZTRfRIwpTzdoPDMgLClQdB+A67\n\tOiHdY1Gt/mHYIrouitStL9qdJf0/JzJY+uxlgqBy8CKL45n1PNfm1Neqxfs+cF0NSJ\n\tHGvmlEQVW1MDRysQouHCMIJewiNdgBnYLfy83taI=","Date":"Thu, 11 Jun 2020 11:27:23 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200611082723.GA5827@pendragon.ideasonboard.com>","References":"<20200521135416.13685-1-email@uajain.com>\n\t<20200521135416.13685-6-email@uajain.com>\n\t<20200607222436.GE22208@pendragon.ideasonboard.com>\n\t<20200607225440.GG22208@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 11 Jun 2020 08:27:45 -0000"}},{"id":5181,"web_url":"https://patchwork.libcamera.org/comment/5181/","msgid":"<1161a3be-f06d-84ca-f5aa-e2355b8ab1d4@uajain.com>","date":"2020-06-11T12:29:48","subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Laurent,\n\nOn 6/11/20 1:57 PM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Wed, Jun 10, 2020 at 02:42:56PM +0000, Umang Jain wrote:\n>> On 6/8/20 4:24 AM, Laurent Pinchart wrote:\n>>> On Mon, Jun 08, 2020 at 01:24:37AM +0300, Laurent Pinchart wrote:\n>>>> On Thu, May 21, 2020 at 01:54:27PM +0000, Umang Jain wrote:\n>>>>> This test checks the code-paths for camera's hotplugged and\n>>>>> unplugged support. It is based off bind/unbind of a UVC device\n>>>> s/off/on/\n>>>>\n>>>>> from sysfs. Hence, this tests required root permissions to run\n>>>> s/tests required/test requires/\n>>>>\n>>>>> and should have atleast one already bound UVC device present\n>>>> s/atleast/at least/\n>>>>\n>>>>> on the system.\n>>>> s/on/in/\n>>>>\n>>>>> Signed-off-by: Umang Jain \n>>>>> ---\n>>>>> test/hotplug-cameras.cpp | 153 +++++++++++++++++++++++++++++++++++++++\n>>>>> test/meson.build | 1 +\n>>>>> 2 files changed, 154 insertions(+)\n>>>>> create mode 100644 test/hotplug-cameras.cpp\n>>>>>\n>>>>> diff --git a/test/hotplug-cameras.cpp b/test/hotplug-cameras.cpp\n>>>>> new file mode 100644\n>>>>> index 0000000..72c370f\n>>>>> --- /dev/null\n>>>>> +++ b/test/hotplug-cameras.cpp\n>>>>> @@ -0,0 +1,153 @@\n>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>>>> +/*\n>>>>> + * Copyright (C) 2020, Umang Jain \n>>>>> + *\n>>>>> + * hotplug-cameras.cpp - Emulate cameraAdded/cameraRemoved signals in CameraManager\n>>>> Maybe s/Emulate/Test/ ?\n>>>>\n>>>>> + */\n>>>>> +\n>>>>> +#include \n>>>>> +#include \n>>>>> +#include \n>>>>> +#include \n>>>>> +#include \n>>>>> +\n>>>> No need for a blank line here, and you can then merge the two groups of\n>>>> headers in alphabetical order.\n>>>>\n>>>>> +#include \n>>>>> +#include \n>>>>> +#include \n>>>>> +\n>>>>> +#include \n>>>>> +#include \n>>>>> +#include \n>>>>> +#include \n>>>>> +\n>>>>> +#include \"libcamera/internal/file.h\"\n>>>>> +#include \"libcamera/internal/thread.h\"\n>>>>> +\n>>>>> +#include \"test.h\"\n>>>>> +\n>>>>> +using namespace std;\n>>>> As you use the std namespace explicitly below, you can drop this line.\n>>>>\n>>>>> +using namespace libcamera;\n>>>>> +\n>>>>> +class HotplugTest : public Test\n>>>>> +{\n>>>>> +protected:\n>>>>> +\tvoid cameraAddedHandler(std::shared_ptr cam)\n>>>>> +\t{\n>>>>> +\t\tcameraAddedPass_ = true;\n>>>>> +\t}\n>>>>> +\n>>>>> +\tvoid cameraRemovedHandler(std::shared_ptr cam)\n>>>>> +\t{\n>>>>> +\t\tcameraRemovedPass_ = true;\n>>>>> +\t}\n>>>>> +\n>>>>> +\tint init()\n>>>>> +\t{\n>>>>> +\t\tif (!File::exists(\"/sys/module/uvcvideo\")) {\n>>>>> +\t\t\tstd::cout << \"uvcvideo driver is not loaded, skipping\" << std::endl;\n>>>>> +\t\t\treturn TestSkip;\n>>>>> +\t\t}\n>>>>> +\n>>>>> +\t\tif (geteuid() != 0) {\n>>>>> +\t\t\tstd::cout << \"This test requires root permissions, skipping\" << std::endl;\n>>>>> +\t\t\treturn TestSkip;\n>>>>> +\t\t}\n>>>>> +\n>>>>> +\t\tcm_ = new CameraManager();\n>>>>> +\t\tif (cm_->start()) {\n>>>>> +\t\t\tstd::cout << \"Failed to start camera manager\" << std::endl;\n>>>>> +\t\t\treturn TestFail;\n>>>>> +\t\t}\n>>>>> +\n>>>>> +\t\tcameraAddedPass_ = false;\n>>>>> +\t\tcameraRemovedPass_ = false;\n>>>>> +\n>>>>> +\t\tcm_->newCameraAdded.connect(this, &HotplugTest::cameraAddedHandler);\n>>>>> +\t\tcm_->cameraRemoved.connect(this, &HotplugTest::cameraRemovedHandler);\n>>>>> +\n>>>>> +\t\tuvc_toplevel_ = \"/sys/module/uvcvideo/drivers/usb:uvcvideo/\";\n>>>> Maybe /sys/bus/usb/drivers/uvcvideo/ to avoid traversing symlinks ? It\n>>>> makes little difference in practice I suppose.\n>>>>\n>>>> s/uvc_toplevel_/uvcTopLevel_/\n>>>>\n>>>> And I would even call this uvcDriverDir to make it more explicit. Now\n>>>> that I think about it, as uvc_toplevel_ is constant, maybe we should\n>>>> have\n>>>>\n>>>> \tstatic const std::string uvcDriverDir_;\n>>>>\n>>>> as a static class member.\n>>>>\n>>>>> +\n>>>>> +\t\treturn 0;\n>>>>> +\t}\n>>>>> +\n>>>>> +\tint run()\n>>>>> +\t{\n>>>>> +\t\tDIR *dir;\n>>>>> +\t\tstruct dirent *dirent, *dirent2;\n>>>>> +\t\tstd::string uvc_driver_dir;\n>>>>> +\t\tbool uvc_driver_found = false;\n>>>> camelCase please :-)\n>>>>\n>>>>> +\n>>>>> +\t\tdir = opendir(uvc_toplevel_.c_str());\n>>>>> +\t\t/* Find a UVC device driver symlink, which we can bind/unbind */\n>>>> s/unbind/unbind./ (same for the other comments below)\n>>>>\n>>>>> +\t\twhile ((dirent = readdir(dir)) != nullptr) {\n>>>>> +\t\t\tif (dirent->d_type != DT_LNK)\n>>>>> +\t\t\t\tcontinue;\n>>>>> +\n>>>>> +\t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n>>>>> +\t\t\tDIR *device_driver = opendir(child_dir.c_str());\n>>>>> +\t\t\twhile ((dirent2 = readdir(device_driver)) != nullptr) {\n>>>>> +\t\t\t\tif (strncmp(dirent2->d_name, \"video4linux\", 11) == 0) {\n>>>>> +\t\t\t\t\tuvc_driver_dir = dirent->d_name;\n>>>>> +\t\t\t\t\tuvc_driver_found = true;\n>>>>> +\t\t\t\t\tbreak;\n>>>>> +\t\t\t\t}\n>>>>> +\t\t\t}\n>>>>> +\t\t\tclosedir(device_driver);\n>>>>> +\n>>>>> +\t\t\tif (uvc_driver_found)\n>>>>> +\t\t\t\tbreak;\n>>>> Can't this be simplified with\n>>>>\n>>>> \t\t\tstd::string child_dir = uvc_toplevel_ + dirent->d_name;\n>>>> \t\t\tif (!File::exist(uvc_toplevel_ + dirent->d_name + \"/video4linux\"))\n>>>> \t\t\t\tcontinue;\n>>>>\n>>>> \t\t\tuvc_driver_dir = dirent->d_name;\n>>>> \t\t\tbreak;\n>>>>\n>>>>> +\t\t}\n>>>>> +\t\tclosedir(dir);\n>>>>> +\n>>>>> +\t\t/* If no UVC driver found, skip */\n>>>>> +\t\tif (!uvc_driver_found)\n>>>> And here,\n>>>>\n>>>> \t\tif (uvc_driver_dir.empty())\n>>>>\n>>>> to remove uvc_driver_found.\n>>>>\n>>>>> +\t\t\treturn TestSkip;\n>>>>> +\n>>>>> +\t\t/* Unbind a camera, process events */\n>>>>> +\t\tint fd1 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/unbind\", O_WRONLY);\n>>>> Maybe replacing the hardcoded string with uvc_toplevel_ + \"unbind\" ?\n>>>> This would become\n>>>>\n>>>> \t\tint fd1 = open((uvcDriverDir_ + \"unbind\").c_str(), O_WRONLY);\n>>>>\n>>>>> +\t\twrite(fd1, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n>>>>> +\t\tclose(fd1);\n>>>> Blank line here ?\n>>>>\n>>>> I wonder if using the C++ file API would be simpler here.\n>>>>\n>>>> \t\tstd::ofstream(uvcDriverDir_ + \"unbind\", std::ios::binary)\n>>>> \t\t\t<< uvc_driver_dir;;\n>>>>\n>>>> and that's it.\n>>>>\n>>>>> +\t\tTimer timer;\n>>>>> +\t\ttimer.start(1000);\n>>>>> +\t\twhile (timer.isRunning())\n>>>> \t\twhile (timer.isRunning() && !cameraRemovedPass_)\n>>>>\n>>>> to shorten the wait time. Same below.\n>>>>\n>>>>> +\t\t\tThread::current()->eventDispatcher()->processEvents();\n>>>>> +\n>>>> How about already testing for cameraRemovedPass_ here ?\n>>>>\n>>>> \t\tif (!cameraRemovedPass_)\n>>>> \t\t\treturn TestFail;\n>>>>\n>>>>> +\t\t/* \\todo: Fix this workaround of stopping and starting the camera-manager.\n>>>>> +\t\t * We need to do this, so that cm_ release all references to the uvc media symlinks.\n>>>>> +\t\t */\n>>>> We can merge the series without fixing this, but I think it needs to be\n>>>> handled sooner than latter.\n>>> I've had a quick look, and this seems to be caused by the pipeline\n>>> handler never getting deleted on hot-unplug. This is due to a reference\n>>> to the pipeline handler being stored in the camera manager and never\n>>> dropped.\n>>>\n>>> I'm not sure we actually need to store those references. Can I let you\n>>> investigate ?\n>> I see those references in CameraManager::pipes_ not getting dropped\n>> anywhere.\n>>\n>> Also, I want to understand/handle this situation, in\n>> CameraManager::Private::init() (on master branch):\n>>\n>>                 std::shared_ptr pipe = factory->create(cm_);\n>>\n>> pipe has use_count() of 2 (I checked); and a bit below:\n> 2 ? I wonder where the second one comes from. Is it right after that\n> line, or after the pipe->match() call ? Camera object retain a reference\n> to the PipelineHandler, so that would explain it (one reference in the\n> local pipe variable here, one reference in Camera::Private::pipe_).\n>\n>>                 pipes_.push_back(std::move(pipe));\n>>\n>> pipes_ vector gets a created PipelineHander at refcount 2. Now, I can\n>> drop \"a\" reference to this pipe that got added in pipes_ in maybe say,\n>> CameraManager::Private::removeCamera() on hot-unplug.\n> I was wondering if we could actually remove the pipes_ vector\n> completely. It would require being careful in the unplug path, as the\n> pipeline handler will be deleted as soon as the last camera is deleted,\n> which can happen in PipelineHandler::disconnect(). The cameras._clear()\n> call at the end of the function may happen after the pipeline object is\n> deleted. Yes, objects can be deleted while their functions are running,\n> it's scary, but allowed if you ensure that they don't access data :-)\n> Running the test in valgrind will help catch problems.\n\nI see. Yes, I initially thought pipes_ vector can be removed completely as\n\nI couldn't see how it is useful in the manager.But felt a bit against the\n\nintuition that a CameraManager has no placeholdersfor managing created \npipelines.\n\nAs for the camera deletion, we do (in this series) pass a reference to \nthe camera\n\nwhen we emit a camera[Added | Removed]. So my understanding, in context\n\nof this patch series, since applications will get another reference on \ncameraRemoved,\n\nthey still can access the camera data safely if they want (possibly in \ntheir signal handler),\n\nbefore the camera gets destroyed and subsequently the pipeline handler. \nFor now,\n\nI am considering your suggestion of removing the pipes_ vector from \nCameraManager to\n\nfinally put an end to this rabbithole. Thanks for your pointer.\n\n>\n>> Well, even then it will retain the refcount 1, so still the\n>> PipelineHandler is not destroyed on hot-unplug, which might be holding\n>> onto the media-device directory I pointed out as \\todo in the test code.\n>>\n>> Do you have any pointers on how can I make the refcount drop to 0, so\n>> the PipelineHandler in pipes_ can be destroyed?\n>>\n>>>>> +\t\tcm_->stop();\n>>>>> +\t\tif (cm_->start()) {\n>>>>> +\t\t\tstd::cout << \"Failed to restart camera-manager\" << std::endl;\n>>>>> +\t\t\treturn TestFail;\n>>>>> +\t\t}\n>>>>> +\n>>>>> +\t\t/* Bind the camera again, process events */\n>>>>> +\t\tint fd2 = open(\"/sys/module/uvcvideo/drivers/usb:uvcvideo/bind\", O_WRONLY);\n>>>> You don't need fd2, you can reuse fd1 (which should be renamed fd).\n>>>>\n>>>>> +\t\twrite(fd2, uvc_driver_dir.c_str(), uvc_driver_dir.size());\n>>>>> +\t\tclose(fd2);\n>>>>> +\n>>>>> +\t\ttimer.start(1000);\n>>>>> +\t\twhile (timer.isRunning())\n>>>>> +\t\t\tThread::current()->eventDispatcher()->processEvents();\n>>>>> +\n>>>>> +\t\tif (cameraAddedPass_ && cameraRemovedPass_)\n>>>>> +\t\t\treturn TestPass;\n>>>>> +\t\telse\n>>>>> +\t\t\treturn TestFail;\n>>>> This would become\n>>>>\n>>>> \t\tif (!cameraAddedPass_)\n>>>> \t\t\treturn TestFail;\n>>>>\n>>>> \t\treturn TestPass;\n>>>>\n>>>>> +\t}\n>>>>> +\n>>>>> +\tvoid cleanup()\n>>>>> +\t{\n>>>>> +\t\tcm_->stop();\n>>>>> +\t\tdelete cm_;\n>>>>> +\t}\n>>>>> +\n>>>>> +private:\n>>>>> +\tCameraManager *cm_;\n>>>>> +\tstd::string uvc_toplevel_;\n>>>>> +\tbool cameraRemovedPass_;\n>>>> Maybe s/cameraRemovedPass_/cameraRemoved_/ ?\n>>>>\n>>>>> +\tbool cameraAddedPass_;\n>>>> Same here.\n>>>>\n>>>>> +};\n>>>>> +\n>>>>> +TEST_REGISTER(HotplugTest)\n>>>>> +\n>>>>> diff --git a/test/meson.build b/test/meson.build\n>>>>> index bd7da14..f7e27d7 100644\n>>>>> --- a/test/meson.build\n>>>>> +++ b/test/meson.build\n>>>>> @@ -31,6 +31,7 @@ internal_tests = [\n>>>>> ['file', 'file.cpp'],\n>>>>> ['file-descriptor', 'file-descriptor.cpp'],\n>>>>> ['message', 'message.cpp'],\n>>>>> + ['hotplug-cameras', 'hotplug-cameras.cpp'],\n>>>> Alphabetical order ?\n>>>>\n>>>>> ['object', 'object.cpp'],\n>>>>> ['object-invoke', 'object-invoke.cpp'],\n>>>>> ['signal-threads', 'signal-threads.cpp'],","headers":{"Return-Path":"","Received":["from o1.f.az.sendgrid.net (o1.f.az.sendgrid.net [208.117.55.132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2B96D610A5\n\tfor ;\n\tThu, 11 Jun 2020 14:29:50 +0200 (CEST)","by filter0072p3las1.sendgrid.net with SMTP id\n\tfilter0072p3las1-4518-5EE223BC-6F\n\t2020-06-11 12:29:48.934850514 +0000 UTC m=+238826.681894940","from mail.uajain.com (unknown)\n\tby ismtpd0004p1maa1.sendgrid.net (SG) with ESMTP\n\tid x1Ul9-5pTa2mh9P2RwDhVQ Thu, 11 Jun 2020 12:29:48.311 +0000 (UTC)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=uajain.com\n\theader.i=@uajain.com header.b=\"NLw9e2IR\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=uajain.com;\n\th=subject:references:from:mime-version:in-reply-to:to:cc:content-type:\n\tcontent-transfer-encoding;\n\ts=s1; bh=L3IEXpDyLBb06vIiPzfcadDJZc1WXQOYnZI9Cb+6g1g=;\n\tb=NLw9e2IRTS4oMFDBZ8BYZ8gWBAoSqZoudjI6GNprIu5D/N7cqtY9gyTzZLkQxczU9quK\n\tgk+kY6pZK6bN4YcLvqvySYGTnVueSWrDGMQ/W5hL3mog0XTo2rZRu4MqlRzMe9iOQj2Cms\n\tAOZIJbzon1CzTSpGEuXrbMk9V4lO+dR2E=","References":"<20200521135416.13685-1-email@uajain.com>\n\t<20200521135416.13685-6-email@uajain.com>\n\t<20200607222436.GE22208@pendragon.ideasonboard.com>\n\t<20200607225440.GG22208@pendragon.ideasonboard.com>\n\t\n\t<20200611082723.GA5827@pendragon.ideasonboard.com>","From":"Umang Jain ","Message-ID":"<1161a3be-f06d-84ca-f5aa-e2355b8ab1d4@uajain.com>","Date":"Thu, 11 Jun 2020 12:29:48 +0000 (UTC)","Mime-Version":"1.0","In-Reply-To":"<20200611082723.GA5827@pendragon.ideasonboard.com>","X-SG-EID":"1Q40EQ7YGir8a9gjSIAdTjhngY657NMk9ckeo4dbHZDiOpywc/L3L9rFqlwE4KPcw8DXVBj03b84G8OPZYg80/sQhgnf+0vr8tc8rj6G3ya7TO8s3+IYLFxaR0z2UHEeqcPVdvC4/T1oavFIEURzCnaIWYcVr3RGP2as8UkfqRxFjQBQu+77wWmgrCxHSKPBMwo9oN8TbjL9FB+oYpambRHUEpUsg790ECLErxSte/my6Qd/UaqSY1O9jQu/gERY","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=iso-8859-1; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug\n\thot-unplug unit test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 11 Jun 2020 12:29:52 -0000"}}]