The `ProcessManager` is a singleton class to handle `SIGCHLD` signals
and report the exit status to the particular `Process` instance.
However, having a singleton in a library is not favourable and it is
even less favourable if it installs a signal handler.
Using pidfd it is possible to avoid the need for the signal handler;
and the `Process` objects can watch their pidfd themselves, eliminating
the need for the `ProcessManager` class altogether.
`P_PIDFD` for `waitid()` was introduced in Linux 5.4, so this change
raises the minimum supported kernel version. `clone3()`, `CLONE_PIDFD`,
`pidfd_send_signal()` were all introduced earlier.
Furthermore, the call to the `unshare()` system call can be removed
as those options can be passed to `clone3()` directly.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Instead of creating a new vector, take the vector by value to make it
possible for the caller to use move construction when calling the function.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Instead of using a separate member variable, use `pid_ > 0` to determine
if the process is still running. Previously the value of `pid_` was not
reset to -1 when the process terminated, but since it is only meaningful
while the process is running, reset it to -1 in `Process::died()`.
Neither `pid_` nor `running_` are exposed, so this change has no effect
on the public interface or observable behaviour.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Returning 0 when a running process is already managed can be confusing
since the parameters might be completely different, causing the caller
to mistakenly assume that the program it specified has been started.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Firstly, get the number of arguments first, and use that to determine the
size of the allocation instead of retrieving it twice.
Secondly, use `const_cast` instead of a C-style cast when calling `execv()`.
Third, use `size_t` to match the type of `args.size()`.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
When a child process is started from Process::start(), the file
descriptors inherited from the parent process are closed, except
the ones explicitly listed in the fds[] argument.
One issue is that the file descriptors for stdin, stdout and stderr
being closed, the subsequent file descriptors created by the child
process will reuse the values 0, 1 and 2 that are now available.
Thus, usage of printf(), assert() or alike may direct its output
to the new resource bound to one of these reused file descriptors.
The other issue is that the child process can no longer log on
the console because stderr has been closed.
To address the 2 issues, Process:start() is amended as below:
- Child process inherits from parent's stderr fd in order to share
the same logging descriptor
- Child process stdin, stdout and stderr fds are bound to /dev/null
if not inherited from parent. That is to prevent those descriptors
to be reused for any other resource, that could be corrupted by
the presence of printf(), assert() or alike.
Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
The LSP autoformatter doesn't like some of the current formatting, let's
make it happier. Note that not all of its suggestions were accepted
because readability is preferred and adjusting .clang-format may not be
easy or possible.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Source files in libcamera start by a comment block header, which
includes the file name and a one-line description of the file contents.
While the latter is useful to get a quick overview of the file contents
at a glance, the former is mostly a source of inconvenience. The name in
the comments can easily get out of sync with the file name when files
are renamed, and copy & paste during development have often lead to
incorrect names being used to start with.
Readers of the source code are expected to know which file they're
looking it. Drop the file name from the header comment block.
The change was generated with the following script:
----------------------------------------
dirs="include/libcamera src test utils"
declare -rA patterns=(
['c']=' \* '
['cpp']=' \* '
['h']=' \* '
['py']='# '
['sh']='# '
)
for ext in ${!patterns[@]} ; do
files=$(for dir in $dirs ; do find $dir -name "*.${ext}" ; done)
pattern=${patterns[${ext}]}
for file in $files ; do
name=$(basename ${file})
sed -i "s/^\(${pattern}\)${name} - /\1/" "$file"
done
done
----------------------------------------
This misses several files that are out of sync with the comment block
header. Those will be addressed separately and manually.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Currently it is not possible to display debug output from an isolated IPA
module. The standard descriptors are all closed and any specified log
file is explicitly deactivated for the IPA module. Since libcamera and the
isolated IPA module are separate processes, they cannot write to the same
file. However, if syslog is used, then this would be possible.
If syslog is specified as a log file, then this is left as it is for the
isolated IPA module.
Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Many signals used in internal and public APIs carry the emitter pointer
as a signal argument. This was done to allow slots connected to multiple
signal instances to differentiate between emitters. While starting from
a good intention of facilitating the implementation of slots, it turned
out to be a bad API design as the signal isn't meant to know what it
will be connected to, and thus shouldn't carry parameters that are
solely meant to support a use case specific to the connected slot.
These pointers turn out to be unused in all slots but one. In the only
case where it is needed, it can be obtained by wrapping the slot in a
lambda function when connecting the signal. Do so, and drop the emitter
pointer from all signals.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
Usage of 'method' to refer to member functions comes from Java. The C++
standard uses the term 'function' only. Replace 'method' with 'function'
or 'member function' through the whole code base and documentation.
While at it, fix two typos (s/backeng/backend/).
The BoundMethod and Object::invokeMethod() are left as-is here, and will
be addressed separately.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Move the functionality for the following components to the new
base support library:
- BoundMethod
- EventDispatcher
- EventDispatcherPoll
- Log
- Message
- Object
- Signal
- Semaphore
- Thread
- Timer
While it would be preferable to see these split to move one component
per commit, these components are all interdependent upon each other,
which leaves us with one big change performing the move for all of them.
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
If any Process instances are destroyed after the ProcessManager is
destroyed, then a segfault will occur.
Fix this by making the lifetime of the ProcessManager explicit, and make
the CameraManager construct and deconstruct (automatically, via a member
variable) the ProcessManager.
Update the tests accordingly.
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
We build libcamera with -Wno-unused-parameter and this doesn't cause
much issue internally. However, it prevents catching unused parameters
in inline functions defined in public headers. This can lead to
compilation warnings for applications compiled without
-Wno-unused-parameter.
To catch those issues, remove -Wno-unused-parameter and fix all the
related warnings with [[maybe_unused]].
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
The libcamera internal headers are located in src/libcamera/include/.
The directory is added to the compiler headers search path with a meson
include_directories() directive, and internal headers are included with
(e.g. for the internal semaphore.h header)
#include "semaphore.h"
All was well, until libcxx decided to implement the C++20
synchronization library. The __threading_support header gained a
#include <semaphore.h>
to include the pthread's semaphore support. As include_directories()
adds src/libcamera/include/ to the compiler search path with -I, the
internal semaphore.h is included instead of the pthread version.
Needless to say, the compiler isn't happy.
Three options have been considered to fix this issue:
- Use -iquote instead of -I. The -iquote option instructs gcc to only
consider the header search path for headers included with the ""
version. Meson unfortunately doesn't support this option.
- Rename the internal semaphore.h header. This was deemed to be the
beginning of a long whack-a-mole game, where namespace clashes with
system libraries would appear over time (possibly dependent on
particular system configurations) and would need to be constantly
fixed.
- Move the internal headers to another directory to create a unique
namespace through path components. This causes lots of churn in all
the existing source files through the all project.
The first option would be best, but isn't available to us due to missing
support in meson. Even if -iquote support was added, we would need to
fix the problem before a new version of meson containing the required
support would be released.
The third option is thus the only practical solution available. Bite the
bullet, and do it, moving headers to include/libcamera/internal/.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Jacopo Mondi <jacopo@jmondi.org>
Casting the return value of a function to (void) doesn't ignore the
unused result warning with gcc. Use a #pragma to fix this properly, to
fix compilation with _FORTIFY_SOURCE.
Fixes: df23ab95f3 ("libcamera: process: fix compilation on Chromium OS")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Add an error debug message when disassociating part of a process
execution context using unshare fails.
As this is currently used to isolate a child process which is
immediately terminated silently if unshare fails, add a debug printout
and propagate up the error code to make the failure more visible.
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
The return value of a read() call is mistakenly checked for nonzero
rather than less than zero. Fix this.
Fixes: df23ab95f3 ("libcamera: process: fix compilation on Chromium OS")
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Commit 3d20beca66 ("libcamera: Add Process and ProcessManager
classes") causes the build to fail in the Chromium OS build environment,
because the return values of some function calls marked with the
__warn_unused_result__ attribute are ignored. Fix this.
Fixes: 3d20beca66 ("libcamera: Add Process and ProcessManager classes")
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
