[{"id":2134,"web_url":"https://patchwork.libcamera.org/comment/2134/","msgid":"<20190703163309.GG5007@pendragon.ideasonboard.com>","date":"2019-07-03T16:33:09","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/7] libcamera: process,\n\tprocess manager: create process and manager classes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nI would write the subject line as \"libcamera: Add Process and\nProcessManager classes\".\n\nOn Wed, Jul 03, 2019 at 05:00:02PM +0900, Paul Elder wrote:\n> Add a Process class to abstract a process, and a ProcessManager singleton\n> to monitor and manage the processes.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> ---\n> New in v2\n> \n>  src/libcamera/include/process.h         |  35 ++++++\n>  src/libcamera/include/process_manager.h |  40 +++++++\n>  src/libcamera/meson.build               |   4 +\n>  src/libcamera/process.cpp               | 140 ++++++++++++++++++++++++\n>  src/libcamera/process_manager.cpp       | 104 ++++++++++++++++++\n>  5 files changed, 323 insertions(+)\n>  create mode 100644 src/libcamera/include/process.h\n>  create mode 100644 src/libcamera/include/process_manager.h\n>  create mode 100644 src/libcamera/process.cpp\n>  create mode 100644 src/libcamera/process_manager.cpp\n> \n> diff --git a/src/libcamera/include/process.h b/src/libcamera/include/process.h\n> new file mode 100644\n> index 0000000..85c0163\n> --- /dev/null\n> +++ b/src/libcamera/include/process.h\n> @@ -0,0 +1,35 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * process.h - Process object\n> + */\n> +#ifndef __LIBCAMERA_PROCESS_H__\n> +#define __LIBCAMERA_PROCESS_H__\n> +\n> +#include <string>\n> +#include <vector>\n> +\n> +namespace libcamera {\n> +\n> +class Process\n> +{\n> +public:\n> +\tProcess();\n> +\tvirtual ~Process();\n\nI don't think anything needs to derive from the Process class, it thus\ndoesn't require any virtual method (neither destructor, nor\nsigchldHandler). They can all be normal methods. I would even make the\nclass final to enforce this.\n\n> +\n> +\tint exec(const std::string &path, const std::vector<std::string> &args, const std::vector<int> &fds);\n\nLet's wrap this line to avoid making it too long.\n\nShould we provide default values (empty vectors) for args and fds ? This\nwould especially be useful for fds as passing fds is not always needed.\nAnother option is to add a separate method to specify the fds that\nshould be preserved, but that's probably overkill for now. If we later\nneed to execute processes and capture their stdout or stderr I think\nwe'll revisit this API.\n\nI would name the function start() to mimick the QProcess API.\n\n> +\n> +private:\n> +\tpid_t pid_;\n> +\tbool execed_;\n\nI would go for an enum State { NotRunning, Running } as we may later\nneed to add more states (Starting, Stopping, Dead, ...)\n\n> +\n> +\t/* TODO better prototype, and implementation; emit finished signal */\n> +\tvirtual void sigchldHandler() { };\n\nYou can already move the implementation to the .cpp file as it will grow\ntoo big for an inline function. Let's already pick a good name, maybe\nprocessDied() ? Or just died() ?\n\nI would already add a Signal<Process*, int> finished signal to the\nProcess class to indicate termination, with the exit code being passed\nas the second signal argument. The tests should be updated accordingly.\n\n> +\n> +\tfriend class ProcessManager;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_PROCESS_H__ */\n> diff --git a/src/libcamera/include/process_manager.h b/src/libcamera/include/process_manager.h\n> new file mode 100644\n> index 0000000..9b4bf25\n> --- /dev/null\n> +++ b/src/libcamera/include/process_manager.h\n> @@ -0,0 +1,40 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * process_manager.h - Process manager\n> + */\n> +#ifndef __LIBCAMERA_PROCESS_MANAGER_H__\n> +#define __LIBCAMERA_PROCESS_MANAGER_H__\n> +\n> +#include \"process.h\"\n> +\n> +#include <string>\n> +#include <vector>\n> +\n> +namespace libcamera {\n> +\n> +class EventNotifier;\n> +\n> +class ProcessManager\n\nThis class should not be used directly outside of process.cpp, how about\nmoving its definition to process.cpp ?\n\n> +{\n> +public:\n> +\tint registerProcess(Process *proc);\n> +\n> +\tstatic ProcessManager *instance();\n> +\n> +private:\n> +\tstd::vector<Process *> processes_;\n> +\n> +\tProcessManager();\n> +\t~ProcessManager();\n> +\tvoid sigchldHandler(int sig);\n\nWe usually put methods first and variables second.\n\n> +\n> +\tint signalfd_;\n> +\n> +\tEventNotifier *fdEvent_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_PROCESS_MANAGER_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 8075b1f..087b578 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -20,6 +20,8 @@ libcamera_sources = files([\n>      'media_object.cpp',\n>      'object.cpp',\n>      'pipeline_handler.cpp',\n> +    'process.cpp',\n> +    'process_manager.cpp',\n>      'request.cpp',\n>      'signal.cpp',\n>      'stream.cpp',\n> @@ -45,6 +47,8 @@ libcamera_headers = files([\n>      'include/media_device.h',\n>      'include/media_object.h',\n>      'include/pipeline_handler.h',\n> +    'include/process.h',\n> +    'include/process_manager.h',\n>      'include/utils.h',\n>      'include/v4l2_device.h',\n>      'include/v4l2_subdevice.h',\n> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp\n> new file mode 100644\n> index 0000000..ea7b58d\n> --- /dev/null\n> +++ b/src/libcamera/process.cpp\n> @@ -0,0 +1,140 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * process.cpp - Process object\n> + */\n> +\n> +#include \"process.h\"\n> +\n> +#include <algorithm>\n> +#include <iostream>\n> +#include <vector>\n> +\n> +#include <dirent.h>\n> +#include <string.h>\n> +#include <sys/socket.h>\n> +#include <sys/types.h>\n> +#include <unistd.h>\n\nYou can mix the C and C++ headers.\n\n> +\n> +#include \"ipa_module.h\"\n\nThis should not be needed, the Process class should not depend on IPA\n\n> +#include \"log.h\"\n> +#include \"process_manager.h\"\n> +#include \"utils.h\"\n> +\n> +/**\n> + * \\file process.h\n> + * \\brief Process object\n> + *\n> + * TODO add stuff here\n\nPlease do :-)\n\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Process)\n> +\n> +namespace {\n> +\n> +void closefrom_except(int from, const std::vector<int> &fds)\n\nYou can make this a private member of the Process class.\n\n> +{\n> +\tstd::vector<int> v(fds);\n> +\tsort(v.begin(), v.end());\n> +\n> +\tDIR *dir = opendir(\"/proc/self/fd\");\n> +\tif (!dir)\n> +\t\treturn;\n> +\n> +\tstruct dirent *ent;\n> +\twhile ((ent = readdir(dir)) != nullptr) {\n> +\t\tint fd;\n> +\t\tif (sscanf(ent->d_name, \"%d\", &fd) == 1 && fd >= from &&\n> +\t\t    fd != dirfd(dir) && !std::binary_search(v.begin(), v.end(), fd))\n> +\t\t\tclose(fd);\n\nWould strtoul be simpler than sscanf ?\n\n\t\tchar *endp;\n\t\tint fd = strtoul(ent->d_name, &endp, 10);\n\t\tif (*endp)\n\t\t\tcontinue;\n\n\t\tif (fd >= from && fd != dirfd(dir) &&\n\t\t    !std::binary_search(v.begin(), v.end(), fd)\n\t\t\tclose(fd);\n\nYou could also store the result of dirfd() to a local variable outside\nof the loop to avoid calling the function repeatedly (and btw good idea\nabout dirfd, I wouldn't have thought about that myself).\n\n> +\t}\n> +\n> +\tclosedir(dir);\n> +\treturn;\n\nNo need for an explicit return.\n\n> +}\n> +\n> +}\n> +\n> +/**\n> + * \\class Process\n> + * \\brief Manager for processes\n> + *\n> + * TODO write this\n\nPlease :-)\n\n> + */\n> +\n> +Process::Process()\n> +\t: pid_(-1), execed_(false)\n> +{\n> +}\n> +\n> +Process::~Process()\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Fork and exec a process, and close fds\n> + * \\param[in] path Path to executable\n> + * \\param[in] args Arguments to pass to executable\n> + * \\param[in] fds Vector of file descriptors to keep open\n> + *\n> + * Fork a process, and exec the executable specified by path. Prior to\n> + * exec'ing, but after forking, all file descriptors except for those\n> + * specified in fds will be closed.\n> + *\n> + * All indexes of args will be incremented by 1 before being fed to exec(),\n> + * so args[0] should not need to be equal to path.\n> + *\n> + * \\return a positive socket file descriptor on successful fork, exec, and\n> + * closing the file descriptors, or a negative error code otherwise\n> + */\n> +int Process::exec(const std::string &path, const std::vector<std::string> &args, const std::vector<int> &fds)\n> +{\n> +\tint childPid;\n> +\n> +\tif (execed_)\n> +\t\treturn 0;\n> +\n> +\tif ((childPid = fork()) == -1) {\n\n\tint childPid = fork();\n\tif (childPid == -1) {\n\nis more readable, especially with the second branch testing childPid\ntoo.\n\n> +\t\tint err = errno;\n> +\t\tLOG(Process, Error) << \"Failed to fork: \" << strerror(err);\n> +\t\treturn err;\n\nWe usually use ret instead of err. I would declare ret at the top of the\nfunction, as it's used in several branches.\n\n> +\t} else if (childPid) {\n> +\t\tstd::cout << \"parent uid = \" << getuid() << std::endl;\n\n\tLOG(Process, Debug) << \"Child pid \" << childPid;\n\nor just get rid of it :-)\n\n> +\t\tpid_ = childPid;\n> +\t\tProcessManager::instance()->registerProcess(this);\n> +\n> +\t\texeced_ = true;\n> +\n> +\t\treturn 0;\n> +\t} else {\n> +\t\tint ret;\n> +\t\tif (unshare(CLONE_NEWUSER|CLONE_NEWNET)) {\n\ns/|/ | /\n\n> +\t\t\tret = -errno;\n> +\t\t\tLOG(Process, Error)\n> +\t\t\t\t<< \"Failed to isolate IPA: \" << strerror(-ret);\n\nWhere will the log go ? :-) If the log is redirected to the log file,\nwe'll have two processes trying to write to the same file, that will be\npainful. I think we need to drop this message.\n\n> +\t\t\texit(ret);\n\nLet's use _exit() to avoid calling the cleanup functions registered with\natexit() by the parent.\n\nI wouldn't return -errno, as the process exit code is or'ed ith 0377\nbefore being returned to the parent, so we will likely end up with a\nmeaningless value. I would just return EXIT_FAILURE.\n\n> +\t\t}\n\nI expect we'll do more than just calling unshare(), so I would use this\nto a private method (called unshare() or isolate()).\n\n> +\n> +\t\tstd::cout << \"child uid = \" << getuid() << std::endl;\n> +\n\nSimilarly I think you need to drop this.\n\n> +\t\tclosefrom_except(3, fds);\n\nDo we need to keep stdin, stdout and stderr open ?\n\n> +\n> +\t\tconst char **argv = new const char *[args.size() + 2];\n> +\t\tint len = args.size();\n\nunsigned int len\n\n> +\t\targv[0] = path.c_str();\n> +\t\tfor (int i = 0; i < len; i++)\n\nunsigned int i\n\n> +\t\t\targv[i+1] = args[i].c_str();\n> +\t\targv[len+1] = NULL;\n\ns/NULL/nullptr/\n\n> +\n> +\t\texecv(path.c_str(), (char **)argv);\n> +\n> +\t\tret = -errno;\n> +\t\tLOG(Process, Error) << \"Failed to exec: \" << strerror(-ret);\n> +\t\texit(ret);\n\nSame here, EXIT_FAILURE, and dropping the error message.\n\n> +\t}\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/process_manager.cpp b/src/libcamera/process_manager.cpp\n> new file mode 100644\n> index 0000000..1ba0cfb\n> --- /dev/null\n> +++ b/src/libcamera/process_manager.cpp\n> @@ -0,0 +1,104 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * process_manager.cpp - Process manager\n> + */\n> +\n> +#include \"process_manager.h\"\n> +\n> +#include <algorithm>\n> +#include <iostream>\n> +#include <vector>\n> +\n> +#include <dirent.h>\n> +#include <signal.h>\n> +#include <string.h>\n> +#include <sys/signalfd.h>\n> +#include <sys/socket.h>\n> +#include <sys/types.h>\n> +#include <sys/wait.h>\n> +#include <unistd.h>\n\nYou can merge the two sets of includes.\n\n> +\n> +#include <libcamera/event_notifier.h>\n> +\n> +#include \"ipa_module.h\"\n> +#include \"log.h\"\n> +#include \"utils.h\"\n> +\n> +/**\n> + * \\file process_manager.h\n> + * \\brief Process manager\n> + *\n> + * TODO add stuff here\n\nBe my guest ;-)\n\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(ProcessManager)\n> +\n> +/**\n> + * \\class ProcessManager\n> + * \\brief Manager for processes\n> + *\n> + * TODO make this nicer\n> + */\n> +\n> +void ProcessManager::sigchldHandler(int sig)\n> +{\n> +\t/* TODO give the process the status? */\n> +\tfor (Process *p : processes_)\n> +\t\tif ((p->pid_ = waitpid(p->pid_, NULL, WNOHANG)))\n\nDid you mean == instead of = ? This should have been caught by a test,\nso please add one :-) Starting two processes that will exit after a\ndifferent duration (500ms and 1000ms for instance) and verifying that\nyou receive the finished signal for both of them should do the trick.\n\n> +\t\t\tp->sigchldHandler();\n\nHow about capturing the exit code and passing it to sigchldHandler() ?\n\nYou should remove the process from the processes_ list here. This will\nrequire changing the loop structure though, as the iterator will be\ninvalidated.\n\n\tfor (auto it = processes_.begin(); it != processes_.end(); ) {\n\t\tProcess *process = *it;\n\n\t\tint exitCode;\n\t\tpid_t pid = waitpid(process->pid_, &exitCode, WNOHANG);\n\t\tif (process->pid_ != pid) {\n\t\t\t++it;\n\t\t\tcontinue;\n\t\t}\n\n\t\tit = processes_.erase(it);\n\t\tp->processDied(exitCode);\n\t}\n\nI also wonder if you should replace the std::vector with a container\nthat has a less costly erase operation, such as std::list, or std::set\n(std::unordered_set isn't an option as the order of unerased elements is\npreserved starting in C++14 only).\n\n> +}\n> +\n> +\n> +/**\n> + * \\brief Register process with process manager\n> + * \\param[in] proc Process to register\n\ns/proc/process/\n\n> + *\n> + * Add proc to the process manager to manage.\n> + *\n> + * \\todo add things to manage\n\n * This method registers the \\a process with the process manager. It\n * shall be called by the process after successfully forking, in order\n * to let the parent signal process termination.\n\n> + *\n> + * \\return zero on success, or negative error value\n> + */\n> +int ProcessManager::registerProcess(Process *proc)\n> +{\n> +\tprocesses_.push_back(proc);\n> +\n> +\treturn 0;\n\nThe function never returns an error, you can make it void.\n\n> +}\n> +\n> +ProcessManager::ProcessManager()\n> +{\n> +\tsigset_t mask;\n> +\tsigemptyset(&mask);\n> +\tsigaddset(&mask, SIGCHLD);\n> +\n> +\tsignalfd_ = signalfd(-1, &mask, SFD_NONBLOCK);\n> +\tfdEvent_ = new EventNotifier(signalfd_, EventNotifier::Read);\n\nI know I recommended usage of signalfd(), but I think it may lead to an\nissue. See https://ldpreload.com/blog/signalfd-is-useless for an\nexplanation.\n\nAnother issue is that the application using libcamera may create\nprocesses itself, and may rely on SIGCHLD. Blocking the signal will\nprevent the application from working correctly.\n\nI think the best option is to use sigaction() to install a SIGCHLD\nsignal handler, storing the old handler in a private variable of the\nprocess manager. The signal handler should then process the signal, and\npass it to the old handler. You should copy the current sa_mask when\ninstalling the new handler to preserve the behaviour expected by the\napplication (you can retrieve the current handler and its sa_mask with a\ncall to sigaction() with the second argument set to NULL before calling\nit again to install the handler). Please read the sigaction() man page\nand the sa_flags description to set the appropriate flags. In particular\nI think we should set SA_SIGINFO in order to use the extended\n3-parameters API, and forward that to the old handler with sa_handler or\nsa_sigaction based on whether it had set SA_SIGINFO itself.\n\nAs the signal can come at any time, we can't call\nProcessManager::sigchldHandler() synchronously. You should instead\ncreate a pipe and have the signal handler writing one byte to one end of\nthe pipe, and the ProcessManager fdEvent_ listening to the other end.\nDon't forget to read the byte on the receiving side.\n\nBy the way it seems you don't connect the fdEvent_ notifier :-)\n\n> +}\n> +\n> +ProcessManager::~ProcessManager()\n> +{\n> +\tdelete fdEvent_;\n> +\tclose(signalfd_);\n\nYou should here restore the old signal handler with sigaction(), as\nexplained above.\n\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the Process manager instance\n> + *\n> + * The ProcessManager is a singleton and can't be constructed manually. This\n> + * function shall instead be used to retrieve the single global instance of the\n\ns/function/method/\n\n> + * manager.\n> + *\n> + * \\return The Process manager instance\n> + */\n> +ProcessManager *ProcessManager::instance()\n> +{\n> +\tstatic ProcessManager processManager;\n> +\treturn &processManager;\n> +}\n> +\n> +} /* namespace libcamera */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9886E60C01\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  3 Jul 2019 18:33:30 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0B48E24B;\n\tWed,  3 Jul 2019 18:33:29 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1562171610;\n\tbh=xRnKMXBECotypSr42fPKcLUWHaQ7dieFk05Ze/byuGE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r8RNbnhyaXiCDKGdNcqE6f6uSsND2Vx/K5UJ2Dg056oLKSUU2fGCeKp5qyCmNqKnC\n\trPO1aAf3MimvD+ysr7BuIyscxQ0MD7N/Bii2Q6xt7ESPjkenkGmWfJe7VHIeJPwtlF\n\txXRr+Hn6OiVxxOckT4rU3B+rZzDKWExhPp6ADIic=","Date":"Wed, 3 Jul 2019 19:33:09 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190703163309.GG5007@pendragon.ideasonboard.com>","References":"<20190703080007.21376-1-paul.elder@ideasonboard.com>\n\t<20190703080007.21376-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190703080007.21376-3-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/7] libcamera: process,\n\tprocess manager: create process and manager classes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 03 Jul 2019 16:33:30 -0000"}}]