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

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

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.

> +		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