[libcamera-devel] [PATCH 01/23] libcamera: ProcessManager: make ProcessManager lifetime explicitly managed

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Tue, Sep 15, 2020 at 11:20:16PM +0900, Paul Elder wrote:
> 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 unique
> pointer) the ProcessManager.

I can't see a unique pointer. The code looks fine, only the commit
message seems to need an update.

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

> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  include/libcamera/internal/process.h | 27 ++++++++++++++++
>  src/libcamera/camera_manager.cpp     |  2 ++
>  src/libcamera/process.cpp            | 46 ++++++++++++----------------
>  3 files changed, 48 insertions(+), 27 deletions(-)
> 
> diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h
> index 36595106..df697c7f 100644
> --- a/include/libcamera/internal/process.h
> +++ b/include/libcamera/internal/process.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_INTERNAL_PROCESS_H__
>  #define __LIBCAMERA_INTERNAL_PROCESS_H__
>  
> +#include <signal.h>
>  #include <string>
>  #include <vector>
>  
> @@ -50,6 +51,32 @@ private:
>  	friend class ProcessManager;
>  };
>  
> +class ProcessManager
> +{
> +public:
> +	ProcessManager();
> +	~ProcessManager();
> +
> +	void registerProcess(Process *proc);
> +
> +	static ProcessManager *instance();
> +
> +	int writePipe() const;
> +
> +	const struct sigaction &oldsa() const;
> +
> +private:
> +	static ProcessManager *self_;
> +
> +	void sighandler(EventNotifier *notifier);
> +
> +	std::list<Process *> processes_;
> +
> +	struct sigaction oldsa_;
> +	EventNotifier *sigEvent_;
> +	int pipe_[2];
> +};
> +
>  } /* namespace libcamera */
>  
>  #endif /* __LIBCAMERA_INTERNAL_PROCESS_H__ */
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 47d56256..b8d3ccc8 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -18,6 +18,7 @@
>  #include "libcamera/internal/ipa_manager.h"
>  #include "libcamera/internal/log.h"
>  #include "libcamera/internal/pipeline_handler.h"
> +#include "libcamera/internal/process.h"
>  #include "libcamera/internal/thread.h"
>  #include "libcamera/internal/utils.h"
>  
> @@ -67,6 +68,7 @@ private:
>  	std::unique_ptr<DeviceEnumerator> enumerator_;
>  
>  	IPAManager ipaManager_;
> +	ProcessManager processManager_;
>  };
>  
>  CameraManager::Private::Private(CameraManager *cm)
> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> index 994190dc..72b5afe2 100644
> --- a/src/libcamera/process.cpp
> +++ b/src/libcamera/process.cpp
> @@ -41,28 +41,6 @@ LOG_DEFINE_CATEGORY(Process)
>   * 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 {
>  
> @@ -127,8 +105,20 @@ void ProcessManager::registerProcess(Process *proc)
>  	processes_.push_back(proc);
>  }
>  
> +ProcessManager *ProcessManager::self_ = nullptr;
> +
> +/**
> + * \brief Construct a ProcessManager instance
> + *
> + * The ProcessManager class is meant to only be instantiated once, by the
> + * CameraManager.
> + */
>  ProcessManager::ProcessManager()
>  {
> +	if (self_)
> +		LOG(Process, Fatal)
> +			<< "Multiple ProcessManager objects are not allowed";
> +
>  	sigaction(SIGCHLD, NULL, &oldsa_);
>  
>  	struct sigaction sa;
> @@ -145,6 +135,8 @@ ProcessManager::ProcessManager()
>  			<< "Failed to initialize pipe for signal handling";
>  	sigEvent_ = new EventNotifier(pipe_[0], EventNotifier::Read);
>  	sigEvent_->activated.connect(this, &ProcessManager::sighandler);
> +
> +	self_ = this;
>  }
>  
>  ProcessManager::~ProcessManager()
> @@ -153,21 +145,21 @@ ProcessManager::~ProcessManager()
>  	delete sigEvent_;
>  	close(pipe_[0]);
>  	close(pipe_[1]);
> +
> +	self_ = nullptr;
>  }
>  
>  /**
>   * \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.
> + * The ProcessManager is constructed by the CameraManager. This function shall
> + * be used to retrieve the single instance of the manager.
>   *
>   * \return The Process manager instance
>   */
>  ProcessManager *ProcessManager::instance()
>  {
> -	static ProcessManager processManager;
> -	return &processManager;
> +	return self_;
>  }
>  
>  /**

-- 
Regards,

Laurent Pinchart