[libcamera-devel] [PATCH v2 3/4] test: logging: add logging process test

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Tue, Jul 16, 2019 at 04:05:07PM +0900, Paul Elder wrote:
> Add a test to test that logging works in isolated child processes,

s/,$/./

> Only logSetFile is tested, because stdout and stderr are closed for
> isolated child processes, and syslog and the none logging destinations
> are expected to be the same as non-isolated processes.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Changes in v2:
> - unlink the log file
> - move random number generation and process completion connection to
>   init
> - other minor changes
> 
>  test/log_process.cpp | 153 +++++++++++++++++++++++++++++++++++++++++++
>  test/meson.build     |   1 +
>  2 files changed, 154 insertions(+)
>  create mode 100644 test/log_process.cpp
> 
> diff --git a/test/log_process.cpp b/test/log_process.cpp
> new file mode 100644
> index 0000000..499a5a8
> --- /dev/null
> +++ b/test/log_process.cpp
> @@ -0,0 +1,153 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * log_process.cpp - Logging in isolated child process test
> + */
> +
> +#include <fcntl.h>
> +#include <iostream>
> +#include <random>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <vector>
> +
> +#include <libcamera/camera_manager.h>
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/logging.h>
> +#include <libcamera/timer.h>
> +
> +#include "log.h"
> +#include "process.h"
> +#include "test.h"
> +#include "utils.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +static string message("hello from the child");

s/static/static const/

> +
> +LOG_DEFINE_CATEGORY(LogProcessTest)
> +
> +class LogProcessTestChild
> +{
> +public:
> +	int run(int status, int num)
> +	{
> +		usleep(50000);
> +
> +		string logPath = "/tmp/libcamera.worker.test." +
> +				 to_string(num) + ".log";
> +		if (logSetFile(logPath.c_str()) < 0)
> +			return TestSkip;
> +
> +		LOG(LogProcessTest, Warning) << message;
> +
> +		return status;
> +	}
> +};
> +
> +class LogProcessTest : public Test
> +{
> +protected:
> +	int init()
> +	{
> +		random_device random;
> +		num_ = random();
> +
> +		proc_.finished.connect(this, &LogProcessTest::procFinished);
> +		return 0;
> +	}
> +
> +	int run()
> +	{
> +		EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
> +		Timer timeout;
> +
> +		int exitCode = 42;
> +		vector<std::string> args;
> +		args.push_back(to_string(exitCode));
> +		args.push_back(to_string(num_));
> +		int ret = proc_.start("/proc/self/exe", args);
> +		if (ret) {
> +			cerr << "failed to start process" << endl;
> +			return TestFail;
> +		}
> +
> +		timeout.start(200);
> +		while (timeout.isRunning())
> +			dispatcher->processEvents();
> +
> +		if (exitStatus_ != Process::NormalExit) {
> +			cerr << "process did not exit normally" << endl;
> +			return TestFail;
> +		}
> +
> +		if (exitCode_ == TestSkip)
> +			return TestSkip;
> +
> +		if (exitCode_ != exitCode) {
> +			cerr << "exit code should be " << exitCode
> +			     << ", actual is " << exitCode_ << endl;
> +			return TestFail;
> +		}
> +
> +		logPath_ = "/tmp/libcamera.worker.test." +
> +				 to_string(num_) + ".log";

Please set logPath_ in the init() function, otherwise you may end up
calling unlink("") in cleanup().

> +		int fd = open(logPath_.c_str(), O_RDONLY, S_IRUSR);
> +		if (fd < 0) {
> +			cerr << "failed to open tmp log file" << endl;
> +			return TestFail;
> +		}
> +
> +		char buf[200];
> +		memset(buf, 0, sizeof(buf));
> +		if (read(fd, buf, sizeof(buf)) < 0) {
> +			cerr << "Failed to read tmp log file" << endl;
> +			return TestFail;
> +		}
> +		close(fd);
> +
> +		string str(buf);
> +		if (str.find(message) == string::npos)
> +			return TestFail;
> +
> +		return TestPass;
> +	}
> +
> +	void cleanup()
> +	{
> +		unlink(logPath_.c_str());
> +	}
> +
> +private:
> +	void procFinished(Process *proc, enum Process::ExitStatus exitStatus, int exitCode)
> +	{
> +		exitStatus_ = exitStatus;
> +		exitCode_ = exitCode;
> +	}
> +
> +	Process proc_;
> +	enum Process::ExitStatus exitStatus_;

s/enum //

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +	string logPath_;
> +	int exitCode_;
> +	int num_;
> +};
> +
> +/*
> + * Can't use TEST_REGISTER() as single binary needs to act as both
> + * parent and child processes.
> + */
> +int main(int argc, char **argv)
> +{
> +	if (argc == 3) {
> +		int status = std::stoi(argv[1]);
> +		int num = std::stoi(argv[2]);
> +		LogProcessTestChild child;
> +		return child.run(status, num);
> +	}
> +
> +	return LogProcessTest().execute();
> +}
> diff --git a/test/meson.build b/test/meson.build
> index ad1a2f2..658f283 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -23,6 +23,7 @@ public_tests = [
>  internal_tests = [
>      ['camera-sensor',                   'camera-sensor.cpp'],
>      ['log',                             'log.cpp'],
> +    ['log_process',                     'log_process.cpp'],
>      ['message',                         'message.cpp'],
>      ['signal-threads',                  'signal-threads.cpp'],
>      ['threads',                         'threads.cpp'],

-- 
Regards,

Laurent Pinchart