[libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug hot-unplug unit test

[Laurent Pinchart]

Hi Umang,

On Mon, Jun 08, 2020 at 02:20:07PM +0000, Umang Jain wrote:
> On 6/8/20 4:24 AM, Laurent Pinchart wrote:
> > On Mon, Jun 08, 2020 at 01:24:37AM +0300, Laurent Pinchart wrote:
> >> On Thu, May 21, 2020 at 01:54:27PM +0000, Umang Jain wrote:
> >>> This test checks the code-paths for camera's hotplugged and
> >>> unplugged support. It is based off bind/unbind of a UVC device
> >> s/off/on/
> >>
> >>> from sysfs. Hence, this tests required root permissions to run
> >> s/tests required/test requires/
> >>
> >>> and should have atleast one already bound UVC device present
> >> s/atleast/at least/
> >>
> >>> on the system.
> >> s/on/in/
> >>
> >>> Signed-off-by: Umang Jain <email at uajain.com>
> >>> ---
> >>>   test/hotplug-cameras.cpp | 153 +++++++++++++++++++++++++++++++++++++++
> >>>   test/meson.build         |   1 +
> >>>   2 files changed, 154 insertions(+)
> >>>   create mode 100644 test/hotplug-cameras.cpp
> >>>
> >>> diff --git a/test/hotplug-cameras.cpp b/test/hotplug-cameras.cpp
> >>> new file mode 100644
> >>> index 0000000..72c370f
> >>> --- /dev/null
> >>> +++ b/test/hotplug-cameras.cpp
> >>> @@ -0,0 +1,153 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >>> +/*
> >>> + * Copyright (C) 2020, Umang Jain <email at uajain.com>
> >>> + *
> >>> + * hotplug-cameras.cpp - Emulate cameraAdded/cameraRemoved signals in CameraManager
> >> Maybe s/Emulate/Test/ ?
> >>
> >>> + */
> >>> +
> >>> +#include <dirent.h>
> >>> +#include <fstream>
> >>> +#include <iostream>
> >>> +#include <string.h>
> >>> +#include <unistd.h>
> >>> +
> >> No need for a blank line here, and you can then merge the two groups of
> >> headers in alphabetical order.
> >>
> >>> +#include <fcntl.h>
> >>> +#include <sys/stat.h>
> >>> +#include <sys/types.h>
> >>> +
> >>> +#include <libcamera/camera.h>
> >>> +#include <libcamera/camera_manager.h>
> >>> +#include <libcamera/event_dispatcher.h>
> >>> +#include <libcamera/timer.h>
> >>> +
> >>> +#include "libcamera/internal/file.h"
> >>> +#include "libcamera/internal/thread.h"
> >>> +
> >>> +#include "test.h"
> >>> +
> >>> +using namespace std;
> >> As you use the std namespace explicitly below, you can drop this line.
> >>
> >>> +using namespace libcamera;
> >>> +
> >>> +class HotplugTest : public Test
> >>> +{
> >>> +protected:
> >>> +	void cameraAddedHandler(std::shared_ptr<Camera> cam)
> >>> +	{
> >>> +		cameraAddedPass_ = true;
> >>> +	}
> >>> +
> >>> +	void cameraRemovedHandler(std::shared_ptr<Camera> cam)
> >>> +	{
> >>> +		cameraRemovedPass_ = true;
> >>> +	}
> >>> +
> >>> +	int init()
> >>> +	{
> >>> +		if (!File::exists("/sys/module/uvcvideo")) {
> >>> +			std::cout << "uvcvideo driver is not loaded, skipping" << std::endl;
> >>> +			return TestSkip;
> >>> +		}
> >>> +
> >>> +		if (geteuid() != 0) {
> >>> +			std::cout << "This test requires root permissions, skipping" << std::endl;
> >>> +			return TestSkip;
> >>> +		}
> >>> +
> >>> +		cm_ = new CameraManager();
> >>> +		if (cm_->start()) {
> >>> +			std::cout << "Failed to start camera manager" << std::endl;
> >>> +			return TestFail;
> >>> +		}
> >>> +
> >>> +		cameraAddedPass_ = false;
> >>> +		cameraRemovedPass_ = false;
> >>> +
> >>> +		cm_->newCameraAdded.connect(this, &HotplugTest::cameraAddedHandler);
> >>> +		cm_->cameraRemoved.connect(this, &HotplugTest::cameraRemovedHandler);
> >>> +
> >>> +		uvc_toplevel_ = "/sys/module/uvcvideo/drivers/usb:uvcvideo/";
> >> Maybe /sys/bus/usb/drivers/uvcvideo/ to avoid traversing symlinks ? It
> >> makes little difference in practice I suppose.
> >>
> >> s/uvc_toplevel_/uvcTopLevel_/
> >>
> >> And I would even call this uvcDriverDir to make it more explicit. Now
> >> that I think about it, as uvc_toplevel_ is constant, maybe we should
> >> have
> >>
> >> 	static const std::string uvcDriverDir_;
> >>
> >> as a static class member.
> >>
> >>> +
> >>> +		return 0;
> >>> +	}
> >>> +
> >>> +	int run()
> >>> +	{
> >>> +		DIR *dir;
> >>> +		struct dirent *dirent, *dirent2;
> >>> +		std::string uvc_driver_dir;
> >>> +		bool uvc_driver_found = false;
> >> camelCase please :-)
> >>
> >>> +
> >>> +		dir = opendir(uvc_toplevel_.c_str());
> >>> +		/* Find a UVC device driver symlink, which we can bind/unbind */
> >> s/unbind/unbind./ (same for the other comments below)
> >>
> >>> +		while ((dirent = readdir(dir)) != nullptr) {
> >>> +			if (dirent->d_type != DT_LNK)
> >>> +				continue;
> >>> +
> >>> +			std::string child_dir = uvc_toplevel_ + dirent->d_name;
> >>> +			DIR *device_driver = opendir(child_dir.c_str());
> >>> +			while ((dirent2 = readdir(device_driver)) != nullptr) {
> >>> +				if (strncmp(dirent2->d_name, "video4linux", 11) == 0) {
> >>> +					uvc_driver_dir = dirent->d_name;
> >>> +					uvc_driver_found = true;
> >>> +					break;
> >>> +				}
> >>> +			}
> >>> +			closedir(device_driver);
> >>> +
> >>> +			if (uvc_driver_found)
> >>> +				break;
> >> Can't this be simplified with
> >>
> >> 			std::string child_dir = uvc_toplevel_ + dirent->d_name;
> >> 			if (!File::exist(uvc_toplevel_ + dirent->d_name + "/video4linux"))
> >> 				continue;
> >>
> >> 			uvc_driver_dir = dirent->d_name;
> >> 			break;
> >>
> >>> +		}
> >>> +		closedir(dir);
> >>> +
> >>> +		/* If no UVC driver found, skip */
> >>> +		if (!uvc_driver_found)
> >> And here,
> >>
> >> 		if (uvc_driver_dir.empty())
> >>
> >> to remove uvc_driver_found.
> >>
> >>> +			return TestSkip;
> >>> +
> >>> +		/* Unbind a camera, process events */
> >>> +		int fd1 = open("/sys/module/uvcvideo/drivers/usb:uvcvideo/unbind", O_WRONLY);
> >> Maybe replacing the hardcoded string with uvc_toplevel_ + "unbind" ?
> >> This would become
> >>
> >> 		int fd1 = open((uvcDriverDir_ + "unbind").c_str(), O_WRONLY);
> >>
> >>> +		write(fd1, uvc_driver_dir.c_str(), uvc_driver_dir.size());
> >>> +		close(fd1);
> >> Blank line here ?
> >>
> >> I wonder if using the C++ file API would be simpler here.
> >>
> >> 		std::ofstream(uvcDriverDir_ + "unbind", std::ios::binary)
> >> 			<< uvc_driver_dir;;
> >>
> >> and that's it.
> >>
> >>> +		Timer timer;
> >>> +		timer.start(1000);
> >>> +		while (timer.isRunning())
> >> 		while (timer.isRunning() && !cameraRemovedPass_)
> >>
> >> to shorten the wait time. Same below.
> >>
> >>> +			Thread::current()->eventDispatcher()->processEvents();
> >>> +
> >> How about already testing for cameraRemovedPass_ here ?
> >>
> >> 		if (!cameraRemovedPass_)
> >> 			return TestFail;
> >>
> >>> +		/* \todo: Fix this workaround of stopping and starting the camera-manager.
> >>> +		 *  We need to do this, so that cm_ release all references to the uvc media symlinks.
> >>> +		 */
> >> We can merge the series without fixing this, but I think it needs to be
> >> handled sooner than latter.
> > I've had a quick look, and this seems to be caused by the pipeline
> > handler never getting deleted on hot-unplug. This is due to a reference
> > to the pipeline handler being stored in the camera manager and never
> > dropped.
> >
> > I'm not sure we actually need to store those references. Can I let you
> > investigate ?
> 
> Sure.
> 
> I am starting to address these review comments locally, so I will sort 
> this issue out before submitting another round.
> 
> The investigation has been pending for a while now from my side, thanks 
> for looking into it and finding out the problem.

You're welcome. Thank you for working on it in the first place :-)

Once you have a fix, I recommend running this test under valgrind, it
should help catching use-after-free issues. I've sent a patch yesterday
that avoids a valgrind crash with libcamera, I hope to get it merged
soon, but you may want to apply it locally in the meantime.

> >>> +		cm_->stop();
> >>> +		if (cm_->start()) {
> >>> +			std::cout << "Failed to restart camera-manager" << std::endl;
> >>> +			return TestFail;
> >>> +		}
> >>> +
> >>> +		/* Bind the camera again, process events */
> >>> +		int fd2 = open("/sys/module/uvcvideo/drivers/usb:uvcvideo/bind", O_WRONLY);
> >> You don't need fd2, you can reuse fd1 (which should be renamed fd).
> >>
> >>> +		write(fd2, uvc_driver_dir.c_str(), uvc_driver_dir.size());
> >>> +		close(fd2);
> >>> +
> >>> +		timer.start(1000);
> >>> +		while (timer.isRunning())
> >>> +			Thread::current()->eventDispatcher()->processEvents();
> >>> +
> >>> +		if (cameraAddedPass_ && cameraRemovedPass_)
> >>> +			return TestPass;
> >>> +		else
> >>> +			return TestFail;
> >> This would become
> >>
> >> 		if (!cameraAddedPass_)
> >> 			return TestFail;
> >>
> >> 		return TestPass;
> >>
> >>> +	}
> >>> +
> >>> +	void cleanup()
> >>> +	{
> >>> +		cm_->stop();
> >>> +		delete cm_;
> >>> +	}
> >>> +
> >>> +private:
> >>> +	CameraManager *cm_;
> >>> +	std::string uvc_toplevel_;
> >>> +	bool cameraRemovedPass_;
> >> Maybe s/cameraRemovedPass_/cameraRemoved_/ ?
> >>
> >>> +	bool cameraAddedPass_;
> >> Same here.
> >>
> >>> +};
> >>> +
> >>> +TEST_REGISTER(HotplugTest)
> >>> +
> >>> diff --git a/test/meson.build b/test/meson.build
> >>> index bd7da14..f7e27d7 100644
> >>> --- a/test/meson.build
> >>> +++ b/test/meson.build
> >>> @@ -31,6 +31,7 @@ internal_tests = [
> >>>       ['file',                            'file.cpp'],
> >>>       ['file-descriptor',                 'file-descriptor.cpp'],
> >>>       ['message',                         'message.cpp'],
> >>> +    ['hotplug-cameras',                 'hotplug-cameras.cpp'],
> >> Alphabetical order ?
> >>
> >>>       ['object',                          'object.cpp'],
> >>>       ['object-invoke',                   'object-invoke.cpp'],
> >>>       ['signal-threads',                  'signal-threads.cpp'],

-- 
Regards,

Laurent Pinchart