[libcamera-devel,RFC,v2,2/7] libcamera: process, process manager: create process and manager classes

1590 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>
---
New in v2

 src/libcamera/include/process.h         |  35 ++++++
 src/libcamera/include/process_manager.h |  40 +++++++
 src/libcamera/meson.build               |   4 +
 src/libcamera/process.cpp               | 140 ++++++++++++++++++++++++
 src/libcamera/process_manager.cpp       | 104 ++++++++++++++++++
 5 files changed, 323 insertions(+)
 create mode 100644 src/libcamera/include/process.h
 create mode 100644 src/libcamera/include/process_manager.h
 create mode 100644 src/libcamera/process.cpp
 create mode 100644 src/libcamera/process_manager.cpp

Hi Paul,

Thank you for the patch.

I would write the subject line as "libcamera: Add Process and
ProcessManager classes".

On Wed, Jul 03, 2019 at 05:00:02PM +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>
> ---
> New in v2
> 
>  src/libcamera/include/process.h         |  35 ++++++
>  src/libcamera/include/process_manager.h |  40 +++++++
>  src/libcamera/meson.build               |   4 +
>  src/libcamera/process.cpp               | 140 ++++++++++++++++++++++++
>  src/libcamera/process_manager.cpp       | 104 ++++++++++++++++++
>  5 files changed, 323 insertions(+)
>  create mode 100644 src/libcamera/include/process.h
>  create mode 100644 src/libcamera/include/process_manager.h
>  create mode 100644 src/libcamera/process.cpp
>  create mode 100644 src/libcamera/process_manager.cpp
> 
> diff --git a/src/libcamera/include/process.h b/src/libcamera/include/process.h
> new file mode 100644
> index 0000000..85c0163
> --- /dev/null
> +++ b/src/libcamera/include/process.h
> @@ -0,0 +1,35 @@
> +/* 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>
> +
> +namespace libcamera {
> +
> +class Process
> +{
> +public:
> +	Process();
> +	virtual ~Process();

I don't think anything needs to derive from the Process class, it thus
doesn't require any virtual method (neither destructor, nor
sigchldHandler). They can all be normal methods. I would even make the
class final to enforce this.

> +
> +	int exec(const std::string &path, const std::vector<std::string> &args, const std::vector<int> &fds);

Let's wrap this line to avoid making it too long.

Should we provide default values (empty vectors) for args and fds ? This
would especially be useful for fds as passing fds is not always needed.
Another option is to add a separate method to specify the fds that
should be preserved, but that's probably overkill for now. If we later
need to execute processes and capture their stdout or stderr I think
we'll revisit this API.

I would name the function start() to mimick the QProcess API.

> +
> +private:
> +	pid_t pid_;
> +	bool execed_;

I would go for an enum State { NotRunning, Running } as we may later
need to add more states (Starting, Stopping, Dead, ...)

> +
> +	/* TODO better prototype, and implementation; emit finished signal */
> +	virtual void sigchldHandler() { };

You can already move the implementation to the .cpp file as it will grow
too big for an inline function. Let's already pick a good name, maybe
processDied() ? Or just died() ?

I would already add a Signal<Process*, int> finished signal to the
Process class to indicate termination, with the exit code being passed
as the second signal argument. The tests should be updated accordingly.

> +
> +	friend class ProcessManager;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_PROCESS_H__ */
> diff --git a/src/libcamera/include/process_manager.h b/src/libcamera/include/process_manager.h
> new file mode 100644
> index 0000000..9b4bf25
> --- /dev/null
> +++ b/src/libcamera/include/process_manager.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * process_manager.h - Process manager
> + */
> +#ifndef __LIBCAMERA_PROCESS_MANAGER_H__
> +#define __LIBCAMERA_PROCESS_MANAGER_H__
> +
> +#include "process.h"
> +
> +#include <string>
> +#include <vector>
> +
> +namespace libcamera {
> +
> +class EventNotifier;
> +
> +class ProcessManager

This class should not be used directly outside of process.cpp, how about
moving its definition to process.cpp ?

> +{
> +public:
> +	int registerProcess(Process *proc);
> +
> +	static ProcessManager *instance();
> +
> +private:
> +	std::vector<Process *> processes_;
> +
> +	ProcessManager();
> +	~ProcessManager();
> +	void sigchldHandler(int sig);

We usually put methods first and variables second.

> +
> +	int signalfd_;
> +
> +	EventNotifier *fdEvent_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_PROCESS_MANAGER_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 8075b1f..087b578 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,8 @@ libcamera_headers = files([
>      'include/media_device.h',
>      'include/media_object.h',
>      'include/pipeline_handler.h',
> +    'include/process.h',
> +    'include/process_manager.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..ea7b58d
> --- /dev/null
> +++ b/src/libcamera/process.cpp
> @@ -0,0 +1,140 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * process.cpp - Process object
> + */
> +
> +#include "process.h"
> +
> +#include <algorithm>
> +#include <iostream>
> +#include <vector>
> +
> +#include <dirent.h>
> +#include <string.h>
> +#include <sys/socket.h>
> +#include <sys/types.h>
> +#include <unistd.h>

You can mix the C and C++ headers.

> +
> +#include "ipa_module.h"

This should not be needed, the Process class should not depend on IPA

> +#include "log.h"
> +#include "process_manager.h"
> +#include "utils.h"
> +
> +/**
> + * \file process.h
> + * \brief Process object
> + *
> + * TODO add stuff here

Please do :-)

> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Process)
> +
> +namespace {
> +
> +void closefrom_except(int from, const std::vector<int> &fds)

You can make this a private member of the Process class.

> +{
> +	std::vector<int> v(fds);
> +	sort(v.begin(), v.end());
> +
> +	DIR *dir = opendir("/proc/self/fd");
> +	if (!dir)
> +		return;
> +
> +	struct dirent *ent;
> +	while ((ent = readdir(dir)) != nullptr) {
> +		int fd;
> +		if (sscanf(ent->d_name, "%d", &fd) == 1 && fd >= from &&
> +		    fd != dirfd(dir) && !std::binary_search(v.begin(), v.end(), fd))
> +			close(fd);

Would strtoul be simpler than sscanf ?

		char *endp;
		int fd = strtoul(ent->d_name, &endp, 10);
		if (*endp)
			continue;

		if (fd >= from && fd != dirfd(dir) &&
		    !std::binary_search(v.begin(), v.end(), fd)
			close(fd);

You could also store the result of dirfd() to a local variable outside
of the loop to avoid calling the function repeatedly (and btw good idea
about dirfd, I wouldn't have thought about that myself).

> +	}
> +
> +	closedir(dir);
> +	return;

No need for an explicit return.

> +}
> +
> +}
> +
> +/**
> + * \class Process
> + * \brief Manager for processes
> + *
> + * TODO write this

Please :-)

> + */
> +
> +Process::Process()
> +	: pid_(-1), execed_(false)
> +{
> +}
> +
> +Process::~Process()
> +{
> +}
> +
> +/**
> + * \brief Fork and exec a process, and close fds
> + * \param[in] path Path to executable
> + * \param[in] args Arguments to pass to executable
> + * \param[in] fds Vector of file descriptors to keep open
> + *
> + * 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 a positive socket file descriptor on successful fork, exec, and
> + * closing the file descriptors, or a negative error code otherwise
> + */
> +int Process::exec(const std::string &path, const std::vector<std::string> &args, const std::vector<int> &fds)
> +{
> +	int childPid;
> +
> +	if (execed_)
> +		return 0;
> +
> +	if ((childPid = fork()) == -1) {

	int childPid = fork();
	if (childPid == -1) {

is more readable, especially with the second branch testing childPid
too.

> +		int err = errno;
> +		LOG(Process, Error) << "Failed to fork: " << strerror(err);
> +		return err;

We usually use ret instead of err. I would declare ret at the top of the
function, as it's used in several branches.

> +	} else if (childPid) {
> +		std::cout << "parent uid = " << getuid() << std::endl;

	LOG(Process, Debug) << "Child pid " << childPid;

or just get rid of it :-)

> +		pid_ = childPid;
> +		ProcessManager::instance()->registerProcess(this);
> +
> +		execed_ = true;
> +
> +		return 0;
> +	} else {
> +		int ret;
> +		if (unshare(CLONE_NEWUSER|CLONE_NEWNET)) {

s/|/ | /

> +			ret = -errno;
> +			LOG(Process, Error)
> +				<< "Failed to isolate IPA: " << strerror(-ret);

Where will the log go ? :-) If the log is redirected to the log file,
we'll have two processes trying to write to the same file, that will be
painful. I think we need to drop this message.

> +			exit(ret);

Let's use _exit() to avoid calling the cleanup functions registered with
atexit() by the parent.

I wouldn't return -errno, as the process exit code is or'ed ith 0377
before being returned to the parent, so we will likely end up with a
meaningless value. I would just return EXIT_FAILURE.

> +		}

I expect we'll do more than just calling unshare(), so I would use this
to a private method (called unshare() or isolate()).

> +
> +		std::cout << "child uid = " << getuid() << std::endl;
> +

Similarly I think you need to drop this.

> +		closefrom_except(3, fds);

Do we need to keep stdin, stdout and stderr open ?

> +
> +		const char **argv = new const char *[args.size() + 2];
> +		int len = args.size();

unsigned int len

> +		argv[0] = path.c_str();
> +		for (int i = 0; i < len; i++)

unsigned int i

> +			argv[i+1] = args[i].c_str();
> +		argv[len+1] = NULL;

s/NULL/nullptr/

> +
> +		execv(path.c_str(), (char **)argv);
> +
> +		ret = -errno;
> +		LOG(Process, Error) << "Failed to exec: " << strerror(-ret);
> +		exit(ret);

Same here, EXIT_FAILURE, and dropping the error message.

> +	}
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/process_manager.cpp b/src/libcamera/process_manager.cpp
> new file mode 100644
> index 0000000..1ba0cfb
> --- /dev/null
> +++ b/src/libcamera/process_manager.cpp
> @@ -0,0 +1,104 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * process_manager.cpp - Process manager
> + */
> +
> +#include "process_manager.h"
> +
> +#include <algorithm>
> +#include <iostream>
> +#include <vector>
> +
> +#include <dirent.h>
> +#include <signal.h>
> +#include <string.h>
> +#include <sys/signalfd.h>
> +#include <sys/socket.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <unistd.h>

You can merge the two sets of includes.

> +
> +#include <libcamera/event_notifier.h>
> +
> +#include "ipa_module.h"
> +#include "log.h"
> +#include "utils.h"
> +
> +/**
> + * \file process_manager.h
> + * \brief Process manager
> + *
> + * TODO add stuff here

Be my guest ;-)

> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(ProcessManager)
> +
> +/**
> + * \class ProcessManager
> + * \brief Manager for processes
> + *
> + * TODO make this nicer
> + */
> +
> +void ProcessManager::sigchldHandler(int sig)
> +{
> +	/* TODO give the process the status? */
> +	for (Process *p : processes_)
> +		if ((p->pid_ = waitpid(p->pid_, NULL, WNOHANG)))

Did you mean == instead of = ? This should have been caught by a test,
so please add one :-) Starting two processes that will exit after a
different duration (500ms and 1000ms for instance) and verifying that
you receive the finished signal for both of them should do the trick.

> +			p->sigchldHandler();

How about capturing the exit code and passing it to sigchldHandler() ?

You should remove the process from the processes_ list here. This will
require changing the loop structure though, as the iterator will be
invalidated.

	for (auto it = processes_.begin(); it != processes_.end(); ) {
		Process *process = *it;

		int exitCode;
		pid_t pid = waitpid(process->pid_, &exitCode, WNOHANG);
		if (process->pid_ != pid) {
			++it;
			continue;
		}

		it = processes_.erase(it);
		p->processDied(exitCode);
	}

I also wonder if you should replace the std::vector with a container
that has a less costly erase operation, such as std::list, or std::set
(std::unordered_set isn't an option as the order of unerased elements is
preserved starting in C++14 only).

> +}
> +
> +
> +/**
> + * \brief Register process with process manager
> + * \param[in] proc Process to register

s/proc/process/

> + *
> + * Add proc to the process manager to manage.
> + *
> + * \todo add things to manage

 * This method registers the \a process with the process manager. It
 * shall be called by the process after successfully forking, in order
 * to let the parent signal process termination.

> + *
> + * \return zero on success, or negative error value
> + */
> +int ProcessManager::registerProcess(Process *proc)
> +{
> +	processes_.push_back(proc);
> +
> +	return 0;

The function never returns an error, you can make it void.

> +}
> +
> +ProcessManager::ProcessManager()
> +{
> +	sigset_t mask;
> +	sigemptyset(&mask);
> +	sigaddset(&mask, SIGCHLD);
> +
> +	signalfd_ = signalfd(-1, &mask, SFD_NONBLOCK);
> +	fdEvent_ = new EventNotifier(signalfd_, EventNotifier::Read);

I know I recommended usage of signalfd(), but I think it may lead to an
issue. See https://ldpreload.com/blog/signalfd-is-useless for an
explanation.

Another issue is that the application using libcamera may create
processes itself, and may rely on SIGCHLD. Blocking the signal will
prevent the application from working correctly.

I think the best option is to use sigaction() to install a SIGCHLD
signal handler, storing the old handler in a private variable of the
process manager. The signal handler should then process the signal, and
pass it to the old handler. You should copy the current sa_mask when
installing the new handler to preserve the behaviour expected by the
application (you can retrieve the current handler and its sa_mask with a
call to sigaction() with the second argument set to NULL before calling
it again to install the handler). Please read the sigaction() man page
and the sa_flags description to set the appropriate flags. In particular
I think we should set SA_SIGINFO in order to use the extended
3-parameters API, and forward that to the old handler with sa_handler or
sa_sigaction based on whether it had set SA_SIGINFO itself.

As the signal can come at any time, we can't call
ProcessManager::sigchldHandler() synchronously. You should instead
create a pipe and have the signal handler writing one byte to one end of
the pipe, and the ProcessManager fdEvent_ listening to the other end.
Don't forget to read the byte on the receiving side.

By the way it seems you don't connect the fdEvent_ notifier :-)

> +}
> +
> +ProcessManager::~ProcessManager()
> +{
> +	delete fdEvent_;
> +	close(signalfd_);

You should here restore the old signal handler with sigaction(), as
explained above.

> +}
> +
> +/**
> + * \brief Retrieve the Process manager instance
> + *
> + * The ProcessManager is a singleton and can't be constructed manually. This
> + * function shall instead be used to retrieve the single global instance of the

s/function/method/

> + * manager.
> + *
> + * \return The Process manager instance
> + */
> +ProcessManager *ProcessManager::instance()
> +{
> +	static ProcessManager processManager;
> +	return &processManager;
> +}
> +
> +} /* namespace libcamera */