[libcamera-devel] [RFC PATCH 04/10] libcamera: Process: Manage pipe fds by ScopedFD

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Thu, Apr 15, 2021 at 05:38:37PM +0900, Hirokazu Honda wrote:
> Process owns the file descriptors for pipe. They should be
> managed by ScopedFDs to avoid the leakage.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>

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

> ---
>  include/libcamera/internal/process.h |  4 +++-
>  src/libcamera/process.cpp            | 16 ++++++++++------
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h
> index 254cda85..b3bb24df 100644
> --- a/include/libcamera/internal/process.h
> +++ b/include/libcamera/internal/process.h
> @@ -11,6 +11,7 @@
>  #include <string>
>  #include <vector>
>  
> +#include <libcamera/scoped_file_descriptor.h>
>  #include <libcamera/signal.h>
>  
>  namespace libcamera {
> @@ -75,8 +76,9 @@ private:
>  	std::list<Process *> processes_;
>  
>  	struct sigaction oldsa_;
> +
>  	EventNotifier *sigEvent_;
> -	int pipe_[2];
> +	ScopedFD pipe_[2];
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> index 40a434a6..bb51d73b 100644
> --- a/src/libcamera/process.cpp
> +++ b/src/libcamera/process.cpp
> @@ -69,7 +69,7 @@ void sigact(int signal, siginfo_t *info, void *ucontext)
>  void ProcessManager::sighandler([[maybe_unused]] EventNotifier *notifier)
>  {
>  	char data;
> -	ssize_t ret = read(pipe_[0], &data, sizeof(data));
> +	ssize_t ret = read(pipe_[0].get(), &data, sizeof(data));
>  	if (ret < 0) {
>  		LOG(Process, Error)
>  			<< "Failed to read byte from signal handler pipe";
> @@ -129,10 +129,15 @@ ProcessManager::ProcessManager()
>  
>  	sigaction(SIGCHLD, &sa, NULL);
>  
> -	if (pipe2(pipe_, O_CLOEXEC | O_DIRECT | O_NONBLOCK))
> +	int pipe[2];
> +	if (pipe2(pipe, O_CLOEXEC | O_DIRECT | O_NONBLOCK))
>  		LOG(Process, Fatal)
>  			<< "Failed to initialize pipe for signal handling";
> -	sigEvent_ = new EventNotifier(pipe_[0], EventNotifier::Read);
> +
> +	pipe_[0] = ScopedFD(pipe[0]);
> +	pipe_[1] = ScopedFD(pipe[1]);
> +
> +	sigEvent_ = new EventNotifier(pipe_[0].get(), EventNotifier::Read);
>  	sigEvent_->activated.connect(this, &ProcessManager::sighandler);
>  
>  	self_ = this;
> @@ -141,9 +146,8 @@ ProcessManager::ProcessManager()
>  ProcessManager::~ProcessManager()
>  {
>  	sigaction(SIGCHLD, &oldsa_, NULL);
> +
>  	delete sigEvent_;
> -	close(pipe_[0]);
> -	close(pipe_[1]);
>  
>  	self_ = nullptr;
>  }
> @@ -170,7 +174,7 @@ ProcessManager *ProcessManager::instance()
>   */
>  int ProcessManager::writePipe() const
>  {
> -	return pipe_[1];
> +	return pipe_[1].get();
>  }
>  
>  /**

-- 
Regards,

Laurent Pinchart