[libcamera-devel,v3,2/7] libcamera: Add Process and ProcessManager classes

1635 diff mbox series

Add a Process class to abstract a process, and a ProcessManager singleton
to monitor and manage the processes.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
Changes in v3:
- add Process test
- move ProcessManager header to process.cpp
- make Process final
- add a bunch of things for monitoring and signals on process
  termination

New in v2

 src/libcamera/include/process.h   |  61 +++++
 src/libcamera/meson.build         |   3 +
 src/libcamera/process.cpp         | 359 ++++++++++++++++++++++++++++++
 src/libcamera/process_manager.cpp |   0
 test/meson.build                  |   1 +
 test/process/meson.build          |  12 +
 test/process/process_test.cpp     |  95 ++++++++
 7 files changed, 531 insertions(+)
 create mode 100644 src/libcamera/include/process.h
 create mode 100644 src/libcamera/process.cpp
 create mode 100644 src/libcamera/process_manager.cpp
 create mode 100644 test/process/meson.build
 create mode 100644 test/process/process_test.cpp

Hi Paul,

Thank you for the patch.

On Wed, Jul 10, 2019 at 03:44:45AM +0900, Paul Elder wrote:
> Add a Process class to abstract a process, and a ProcessManager singleton
> to monitor and manage the processes.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> Changes in v3:
> - add Process test
> - move ProcessManager header to process.cpp
> - make Process final
> - add a bunch of things for monitoring and signals on process
>   termination
> 
> New in v2
> 
>  src/libcamera/include/process.h   |  61 +++++
>  src/libcamera/meson.build         |   3 +
>  src/libcamera/process.cpp         | 359 ++++++++++++++++++++++++++++++
>  src/libcamera/process_manager.cpp |   0
>  test/meson.build                  |   1 +
>  test/process/meson.build          |  12 +
>  test/process/process_test.cpp     |  95 ++++++++
>  7 files changed, 531 insertions(+)
>  create mode 100644 src/libcamera/include/process.h
>  create mode 100644 src/libcamera/process.cpp
>  create mode 100644 src/libcamera/process_manager.cpp
>  create mode 100644 test/process/meson.build
>  create mode 100644 test/process/process_test.cpp
> 
> diff --git a/src/libcamera/include/process.h b/src/libcamera/include/process.h
> new file mode 100644
> index 0000000..3933f58
> --- /dev/null
> +++ b/src/libcamera/include/process.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * process.h - Process object
> + */
> +#ifndef __LIBCAMERA_PROCESS_H__
> +#define __LIBCAMERA_PROCESS_H__
> +
> +#include <string>
> +#include <vector>
> +
> +#include <libcamera/event_notifier.h>
> +
> +namespace libcamera {
> +
> +class Process final
> +{
> +public:
> +	enum ExitStatus {
> +		NotExited,
> +		Exited,
> +		Signaled,

When the exit status is Signaled the process has exited, so I think that
should be visible in the name. How about NotExited, NormalExit,
SignalExit ? Or maybe NoExit for the first one to avoid Exited vs. Exit
?

> +	};
> +
> +	Process();
> +	~Process();
> +
> +	int start(const std::string &path,
> +		  const std::vector<std::string> &args = std::vector<std::string>(),
> +		  const std::vector<int> &fds = std::vector<int>());
> +
> +	enum ExitStatus exitStatus() const { return exitStatus_; }

s/enum //

> +

You can remove this blank line, it makes sense to group exitStatus() and
exitCode() together.

> +	int exitCode() const { return exitCode_; }
> +
> +	void kill() const;

I think killing a process changes its state, even if it doesn't write
any field of the Process class. The function should thus not be const.

> +
> +	Signal<Process *, enum ExitStatus, int> finished;
> +
> +private:
> +	enum Status {
> +		NotRunning,
> +		Running,
> +	};
> +
> +	void closefrom_except(int from, const std::vector<int> &fds);

	closeFromExcept(...);

As you always pass 0 as the from argument I would remove it, so maybe
the function could be called closeAllFdsExcept() ?

> +	int isolate();
> +	void died(int wstatus);
> +
> +	pid_t pid_;
> +	enum Status status_;

	bool running_;

> +	enum ExitStatus exitStatus_;
> +	int exitCode_;
> +
> +	friend class ProcessManager;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_PROCESS_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 97ff86e..a364f68 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -20,6 +20,8 @@ libcamera_sources = files([
>      'media_object.cpp',
>      'object.cpp',
>      'pipeline_handler.cpp',
> +    'process.cpp',
> +    'process_manager.cpp',
>      'request.cpp',
>      'signal.cpp',
>      'stream.cpp',
> @@ -45,6 +47,7 @@ libcamera_headers = files([
>      'include/media_device.h',
>      'include/media_object.h',
>      'include/pipeline_handler.h',
> +    'include/process.h',
>      'include/utils.h',
>      'include/v4l2_device.h',
>      'include/v4l2_subdevice.h',
> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> new file mode 100644
> index 0000000..7df9138
> --- /dev/null
> +++ b/src/libcamera/process.cpp
> @@ -0,0 +1,359 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * process.cpp - Process object
> + */
> +
> +#include "process.h"
> +
> +#include <algorithm>
> +#include <dirent.h>
> +#include <fcntl.h>
> +#include <iostream>
> +#include <list>
> +#include <signal.h>
> +#include <string.h>
> +#include <sys/socket.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +#include <vector>
> +
> +#include <libcamera/event_notifier.h>
> +
> +#include "log.h"
> +#include "utils.h"
> +
> +/**
> + * \file process.h
> + * \brief Process object
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Process)
> +
> +/**
> + * \class ProcessManager
> + * \brief Manager of processes
> + *
> + * The ProcessManager singleton keeps track of all created Process instances,
> + * and manages the signal handling involved in terminating processes.
> + */
> +class ProcessManager
> +{
> +public:
> +	void registerProcess(Process *proc);
> +
> +	static ProcessManager *instance();
> +
> +	int writePipe() const;
> +
> +	const struct sigaction &oldsa() const;
> +
> +private:
> +	void sighandler(EventNotifier *notifier);
> +	ProcessManager();
> +	~ProcessManager();
> +
> +	std::list<Process *> processes_;
> +
> +	struct sigaction oldsa_;
> +	EventNotifier *sigEvent_;
> +	int pipe_[2];
> +};
> +
> +namespace {
> +
> +void sigact(int signal, siginfo_t *info, void *ucontext)
> +{
> +	char data = 0;
> +	write(ProcessManager::instance()->writePipe(), &data, sizeof(data));
> +
> +	struct sigaction oldsa = ProcessManager::instance()->oldsa();

	const struct sigaction &oldsa = ...;

> +	if (oldsa.sa_flags & SA_SIGINFO) {
> +		oldsa.sa_sigaction(signal, info, ucontext);
> +	} else {
> +		if (oldsa.sa_handler != SIG_IGN && oldsa.sa_handler != SIG_DFL)
> +			oldsa.sa_handler(signal);
> +	}
> +}
> +
> +} /* namespace */
> +
> +void ProcessManager::sighandler(EventNotifier *notifier)
> +{
> +	char data;
> +	read(pipe_[0], &data, sizeof(data));
> +
> +	for (auto it = processes_.begin(); it != processes_.end(); ) {
> +		Process *process = *it;
> +
> +		int wstatus;
> +		pid_t pid = waitpid(process->pid_, &wstatus, WNOHANG);
> +		if (process->pid_ != pid) {
> +			++it;
> +			continue;
> +		}
> +
> +		it = processes_.erase(it);
> +		process->died(wstatus);
> +	}
> +}
> +
> +/**
> + * \brief Register process with process manager
> + * \param[in] proc Process to register
> + *
> + * Add \a proc to the process manager to manage.
> + *

I think you can drop this line, the next paragraph is enough.

> + * This method registers the \a proc with the process manager. It
> + * shall be called by the parent process after successfully forking, in
> + * order to let the parent signal process termination.
> + */
> +void ProcessManager::registerProcess(Process *proc)
> +{
> +	processes_.push_back(proc);
> +}
> +
> +ProcessManager::ProcessManager()
> +{
> +	sigaction(SIGCHLD, NULL, &oldsa_);
> +
> +	struct sigaction sa;
> +	memset(&sa, 0, sizeof(sa));
> +	sa.sa_sigaction = &sigact;
> +	sigset_t mask;
> +	memcpy(&mask, &oldsa_.sa_mask, sizeof(mask));
> +	sigaddset(&mask, SIGCHLD);

Shouldn't mask be sa.sa_mask ?

> +	sa.sa_flags = oldsa_.sa_flags | SA_SIGINFO;
> +
> +	sigaction(SIGCHLD, &sa, NULL);
> +
> +	pipe2(pipe_, O_CLOEXEC | O_DIRECT | O_NONBLOCK);
> +	sigEvent_ = new EventNotifier(pipe_[0], EventNotifier::Read);
> +	sigEvent_->activated.connect(this, &ProcessManager::sighandler);
> +}
> +
> +ProcessManager::~ProcessManager()
> +{
> +	delete sigEvent_;
> +	close(pipe_[0]);
> +	close(pipe_[1]);

You should here restore the old signal handler with sigaction().

> +}
> +
> +/**
> + * \brief Retrieve the Process manager instance
> + *
> + * The ProcessManager is a singleton and can't be constructed manually. This
> + * method shall instead be used to retrieve the single global instance of the
> + * manager.
> + *
> + * \return The Process manager instance
> + */
> +ProcessManager *ProcessManager::instance()
> +{
> +	static ProcessManager processManager;
> +	return &processManager;
> +}
> +
> +/**
> + * \brief Retrieve the Process manager's write pipe
> + *
> + * This method is meant only to be used by the static signal handler.
> + *
> + * \return Pipe for writing
> + */
> +int ProcessManager::writePipe() const
> +{
> +	return pipe_[1];
> +}
> +
> +/**
> + * \brief Retrive the old signal action data
> + *
> + * This method is meant only to be used by the static signal handler.
> + *
> + * \return The old signal action data
> + */
> +const struct sigaction &ProcessManager::oldsa() const
> +{
> +	return oldsa_;
> +}
> +
> +
> +/**
> + * \class Process
> + * \brief Process object
> + *
> + * The Process class models a process, and simplifies spawning new processes
> + * and monitoring the exiting of a process.
> + */
> +
> +/**
> + * \enum Process::ExitStatus
> + * \brief Exit status of process
> + * \var Process::NotExited
> + * The process hasn't exited yet.

s/\.$//

and below too.

> + * \var Process::Exited
> + * The process exited normally, either via exit() or returning from main.
> + * \var Process::Signaled
> + * The process was terminated by signal.

s/by signal/by a signal (this includes crashing)/

> + */
> +
> +Process::Process()
> +	: pid_(-1), status_(NotRunning), exitStatus_(NotExited), exitCode_(0)
> +{
> +}
> +
> +Process::~Process()
> +{
> +	kill();
> +	/* \todo wait for child process to exit */
> +}
> +
> +/**
> + * \brief Fork and exec a process, and close fds
> + * \param[in] path Path to executable
> + * \param[in] args Arguments to pass to executable (optional)
> + * \param[in] fds Vector of file descriptors to keep open (optional)
> + *
> + * Fork a process, and exec the executable specified by path. Prior to
> + * exec'ing, but after forking, all file descriptors except for those
> + * specified in fds will be closed.
> + *
> + * All indexes of args will be incremented by 1 before being fed to exec(),
> + * so args[0] should not need to be equal to path.
> + *
> + * \return zero on successful fork, exec, and closing the file descriptors,
> + * or a negative error code otherwise
> + */
> +int Process::start(const std::string &path,
> +		   const std::vector<std::string> &args,
> +		   const std::vector<int> &fds)
> +{
> +	int ret;
> +
> +	if (status_ == Running)
> +		return 0;
> +
> +	int childPid = fork();
> +	if (childPid == -1) {
> +		ret = errno;

errno is positive, so this should be ret = -errno.

> +		LOG(Process, Error) << "Failed to fork: " << strerror(ret);

And strerror(-ret).

> +		return ret;
> +	} else if (childPid) {
> +		pid_ = childPid;
> +		ProcessManager::instance()->registerProcess(this);
> +
> +		status_ = Running;
> +
> +		return 0;
> +	} else {
> +		if (isolate())
> +			_exit(EXIT_FAILURE);
> +
> +		closefrom_except(0, fds);
> +
> +		const char **argv = new const char *[args.size() + 2];
> +		unsigned int len = args.size();
> +		argv[0] = path.c_str();
> +		for (unsigned int i = 0; i < len; i++)
> +			argv[i+1] = args[i].c_str();
> +		argv[len+1] = nullptr;
> +
> +		execv(path.c_str(), (char **)argv);
> +
> +		exit(EXIT_FAILURE);
> +	}
> +}
> +
> +void Process::closefrom_except(int from, const std::vector<int> &fds)
> +{
> +	std::vector<int> v(fds);
> +	sort(v.begin(), v.end());
> +
> +	DIR *dir = opendir("/proc/self/fd");
> +	if (!dir)
> +		return;
> +
> +	int dfd = dirfd(dir);
> +
> +	struct dirent *ent;
> +	while ((ent = readdir(dir)) != nullptr) {
> +		char *endp;
> +		int fd = strtoul(ent->d_name, &endp, 10);
> +		if (*endp)
> +			continue;
> +
> +		if (fd >= from && fd != dfd &&
> +		    !std::binary_search(v.begin(), v.end(), fd))
> +			close(fd);
> +	}
> +
> +	closedir(dir);
> +}
> +
> +int Process::isolate()
> +{
> +	return unshare(CLONE_NEWUSER | CLONE_NEWNET);
> +}
> +
> +/*

/**

> + * \brief SIGCHLD handler
> + * \param[in] wstatus The status as output by waitpid()
> + *
> + * This method is called when the process associated with Process terminates.
> + * Emits the Process::finished signal.

s/Emits/It emits/

> + */
> +void Process::died(int wstatus)
> +{
> +	status_ = NotRunning;
> +	exitStatus_ = WIFEXITED(wstatus) ? Exited : Signaled;
> +	exitCode_ = exitStatus_ == Exited ? WEXITSTATUS(wstatus) : -1;
> +
> +	finished.emit(this, exitStatus_, exitCode_);
> +}
> +
> +/**
> + * \fn Process::exitStatus()
> + * \brief Return the exit status of the process

s/Return/Retrieve/

> + *
> + * Return the exit status of the process, that is, whether the process
> + * has exited via exit() or returning from main, or if the process was
> + * exited by signaling.

s/exited by signaling/was terminated by a signal/

> + *
> + * \sa ExitStatus
> + *
> + * \return Exit status

s/Exit status/The process exit status/

> + */
> +
> +/**
> + * \fn Process::exitCode()
> + * \brief Return the exit code of the process

s/Return/Retrieve/

> + *
> + * Return the exit code of the process.

You can drop this as it's the same as the brief.

> + *
> + * This method is only valid if exitStatus() returned Exited.
> + *
> + * \return Exit code
> + */
> +
> +/**
> + * \var Process::finished
> + *
> + * Signal that is emitted when the process is confirmed to have terminated.
> + */
> +
> +/**
> + * \brief Kill the process
> + *
> + * Sends SIGKILL to the process.
> + */
> +void Process::kill() const
> +{
> +	::kill(pid_, SIGKILL);
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/process_manager.cpp b/src/libcamera/process_manager.cpp
> new file mode 100644
> index 0000000..e69de29
> diff --git a/test/meson.build b/test/meson.build
> index 41b54ff..045e95a 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -6,6 +6,7 @@ subdir('ipa')
>  subdir('ipc')
>  subdir('media_device')
>  subdir('pipeline')
> +subdir('process')
>  subdir('stream')
>  subdir('v4l2_subdevice')
>  subdir('v4l2_videodevice')
> diff --git a/test/process/meson.build b/test/process/meson.build
> new file mode 100644
> index 0000000..c4d83d6
> --- /dev/null
> +++ b/test/process/meson.build
> @@ -0,0 +1,12 @@
> +process_tests = [
> +    [ 'process_test',  'process_test.cpp' ],
> +]
> +
> +foreach t : process_tests
> +    exe = executable(t[0], t[1],
> +                     dependencies : libcamera_dep,
> +                     link_with : test_libraries,
> +                     include_directories : test_includes_internal)
> +
> +    test(t[0], exe, suite : 'process', is_parallel : false)
> +endforeach
> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
> new file mode 100644
> index 0000000..39f77c1
> --- /dev/null
> +++ b/test/process/process_test.cpp
> @@ -0,0 +1,95 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * process_test.cpp - Process test
> + */
> +
> +#include <iostream>
> +#include <unistd.h>
> +#include <vector>
> +
> +#include <libcamera/camera_manager.h>
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/timer.h>
> +
> +#include "process.h"
> +#include "test.h"
> +#include "utils.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +class ProcessTestChild
> +{
> +public:
> +	int run(int status)
> +	{
> +		usleep(2000);

That's a bit short, I would increase it to 50-100ms to avoid the child
process exiting before the parent has a chance to do anything.

> +
> +		return status;
> +	}
> +};
> +
> +class ProcessTest : public Test
> +{
> +public:
> +	ProcessTest()
> +		: actualExitCode_(-1)
> +	{
> +	}
> +
> +protected:
> +	int init()
> +	{
> +		return 0;
> +	}

No need for an empty init() function, you can drop it.

> +
> +	int run()
> +	{
> +		EventDispatcher *dispatcher = CameraManager::instance()->eventDispatcher();
> +		Timer timeout;
> +
> +		exitCode_ = 42;

This can be a local variable.

> +		vector<std::string> args;
> +		args.push_back(to_string(exitCode_));
> +		int ret = proc_.start("/proc/self/exe", args);
> +		if (ret)
> +			return TestFail;
> +		proc_.finished.connect(this, &ProcessTest::procFinished);
> +
> +		timeout.start(6);
> +		while (timeout.isRunning())
> +			dispatcher->processEvents();
> +
> +		if (exitCode_ != actualExitCode_)
> +			cout << "exit code should be " << exitCode_ << ", actual is " << actualExitCode_ << endl;

Please wrap the line.

			return TestFail;
		}

		return TestPass;

> +		return exitCode_ == actualExitCode_ ? TestPass : TestFail;
> +	}
> +
> +private:
> +	void procFinished(Process *proc, enum Process::ExitStatus exitStatus, int exitCode)
> +	{
> +		if (exitStatus == Process::Exited)
> +			actualExitCode_ = exitCode;

Let's just store both the exit status and the exit code in order to
check them both in the run() function.

> +	}
> +
> +	Process proc_;
> +	int exitCode_;
> +	int actualExitCode_;
> +};
> +
> +/*
> + * 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 == 2) {
> +		int status = std::stoi(argv[1]);
> +		ProcessTestChild child;
> +		return child.run(status);
> +	}
> +
> +	return ProcessTest().execute();
> +}