| Message ID | 20200915142038.28757-2-paul.elder@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> > Signed-off-by: Paul Elder <paul.elder@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_; > } > > /**
Hi Paul, Thanks for your work. On 2020-09-15 23:20:16 +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. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> With the comment about commit message pointed out by Laurent fixed, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > 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_; > } > > /** > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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_; } /**
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. Signed-off-by: Paul Elder <paul.elder@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(-)