[RFC,v1] apps: cam: Use signalfd
diff mbox series

Message ID 20250815141237.2235085-1-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • [RFC,v1] apps: cam: Use signalfd
Related show

Commit Message

Barnabás Pőcze Aug. 15, 2025, 2:12 p.m. UTC
Use a signalfd to detect `SIGINT` instead of registering a signal handler in
order to remove the `CamApp` singleton. This is better than using a normal
signal handler: a signal can arrive after the `CamApp` object has been destroyed,
leading to a use-after-free. Modifying the `CamApp::app_` in the destructor is
not a good alternative because that would not be async signal safe (unless it
is made atomic).

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/apps/cam/main.cpp | 66 +++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 37 deletions(-)

--
2.50.1

Comments

Laurent Pinchart Aug. 15, 2025, 10:21 p.m. UTC | #1
Hi Barnabás,

Thank you for the patch.

On Fri, Aug 15, 2025 at 04:12:37PM +0200, Barnabás Pőcze wrote:
> Use a signalfd to detect `SIGINT` instead of registering a signal handler in
> order to remove the `CamApp` singleton. This is better than using a normal
> signal handler: a signal can arrive after the `CamApp` object has been destroyed,
> leading to a use-after-free. Modifying the `CamApp::app_` in the destructor is
> not a good alternative because that would not be async signal safe (unless it
> is made atomic).

While this is true, it's also not really much of an issue in my opinion,
or at least a very minor one. Now that the patch exists lets get it
merged, but you don't have to hunt for these kind of problems in cam or
qcam :-)

> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/apps/cam/main.cpp | 66 +++++++++++++++++++------------------------
>  1 file changed, 29 insertions(+), 37 deletions(-)
> 
> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp
> index fa266eca6..cbc85b59f 100644
> --- a/src/apps/cam/main.cpp
> +++ b/src/apps/cam/main.cpp
> @@ -13,6 +13,8 @@
>  #include <signal.h>
>  #include <string.h>
> 
> +#include <sys/signalfd.h>

You can keep this grouped with the previous headers.

> +
>  #include <libcamera/libcamera.h>
>  #include <libcamera/property_ids.h>
> 
> @@ -27,15 +29,12 @@ using namespace libcamera;
>  class CamApp
>  {
>  public:
> -	CamApp();
> -
> -	static CamApp *instance();
> +	CamApp(EventLoop &loop);
> 
>  	int init(int argc, char **argv);
>  	void cleanup();
> 
>  	int exec();
> -	void quit();
> 
>  private:
>  	void cameraAdded(std::shared_ptr<Camera> cam);
> @@ -46,26 +45,17 @@ private:
> 
>  	static std::string cameraName(const Camera *camera);
> 
> -	static CamApp *app_;
>  	OptionsParser::Options options_;
> 
>  	std::unique_ptr<CameraManager> cm_;
> 
>  	std::atomic_uint loopUsers_;
> -	EventLoop loop_;
> +	EventLoop *loop_ = nullptr;
>  };
> 
> -CamApp *CamApp::app_ = nullptr;
> -
> -CamApp::CamApp()
> -	: loopUsers_(0)
> -{
> -	CamApp::app_ = this;
> -}
> -
> -CamApp *CamApp::instance()
> +CamApp::CamApp(EventLoop &loop)
> +	: loopUsers_(0), loop_(&loop)
>  {
> -	return CamApp::app_;
>  }
> 
>  int CamApp::init(int argc, char **argv)
> @@ -103,11 +93,6 @@ int CamApp::exec()
>  	return ret;
>  }
> 
> -void CamApp::quit()
> -{
> -	loop_.exit();
> -}
> -
>  int CamApp::parseOptions(int argc, char *argv[])
>  {
>  	StreamKeyValueParser streamKeyValue;
> @@ -205,7 +190,7 @@ void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
>  void CamApp::captureDone()
>  {
>  	if (--loopUsers_ == 0)
> -		EventLoop::instance()->exit(0);
> +		loop_->exit(0);
>  }
> 
>  int CamApp::run()
> @@ -290,7 +275,7 @@ int CamApp::run()
>  	}
> 
>  	if (loopUsers_)
> -		loop_.exec();
> +		loop_->exec();
> 
>  	/* 6. Stop capture. */
>  	for (const auto &session : sessions) {
> @@ -345,29 +330,36 @@ std::string CamApp::cameraName(const Camera *camera)
>  	return name;
>  }
> 
> -namespace {
> -
> -void signalHandler([[maybe_unused]] int signal)
> +int main(int argc, char **argv)
>  {
> -	std::cout << "Exiting" << std::endl;
> -	CamApp::instance()->quit();
> -}
> +	auto sigfd = [] {
> +		sigset_t ss;
> 
> -} /* namespace */
> +		sigemptyset(&ss);
> +		sigaddset(&ss, SIGINT);
> +		sigprocmask(SIG_BLOCK, &ss, nullptr);
> 
> -int main(int argc, char **argv)
> -{
> -	CamApp app;
> +		return UniqueFD(signalfd(-1, &ss, SFD_CLOEXEC | SFD_NONBLOCK));
> +	}();

Why a lambda ?

> +	if (!sigfd.isValid())
> +		return EXIT_FAILURE;
> +
> +	EventLoop loop;
> +	CamApp app(loop);
>  	int ret;
> 
> +	loop.addFdEvent(sigfd.get(), EventLoop::Read, [&] {
> +		signalfd_siginfo si;
> +		std::ignore = read(sigfd.get(), &si, sizeof(si));
> +
> +		std::cout << "Exiting" << std::endl;
> +		loop.exit(0);
> +	});

It could make things simpler to handle this in the CamApp class, you
won't have to change where the EventLoop is instantiated.

> +
>  	ret = app.init(argc, argv);
>  	if (ret)
>  		return ret == -EINTR ? 0 : EXIT_FAILURE;
> 
> -	struct sigaction sa = {};
> -	sa.sa_handler = &signalHandler;
> -	sigaction(SIGINT, &sa, nullptr);
> -
>  	if (app.exec())
>  		return EXIT_FAILURE;
>
Barnabás Pőcze Aug. 15, 2025, 10:28 p.m. UTC | #2
Hi

2025. 08. 16. 0:21 keltezéssel, Laurent Pinchart írta:
> Hi Barnabás,
> 
> Thank you for the patch.
> 
> On Fri, Aug 15, 2025 at 04:12:37PM +0200, Barnabás Pőcze wrote:
>> Use a signalfd to detect `SIGINT` instead of registering a signal handler in
>> order to remove the `CamApp` singleton. This is better than using a normal
>> signal handler: a signal can arrive after the `CamApp` object has been destroyed,
>> leading to a use-after-free. Modifying the `CamApp::app_` in the destructor is
>> not a good alternative because that would not be async signal safe (unless it
>> is made atomic).
> 
> While this is true, it's also not really much of an issue in my opinion,
> or at least a very minor one. Now that the patch exists lets get it
> merged, but you don't have to hunt for these kind of problems in cam or
> qcam :-)

I actually did run into this on multiple occasions. :(


> 
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/apps/cam/main.cpp | 66 +++++++++++++++++++------------------------
>>   1 file changed, 29 insertions(+), 37 deletions(-)
>>
>> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp
>> index fa266eca6..cbc85b59f 100644
>> --- a/src/apps/cam/main.cpp
>> +++ b/src/apps/cam/main.cpp
>> @@ -13,6 +13,8 @@
>>   #include <signal.h>
>>   #include <string.h>
>>
>> +#include <sys/signalfd.h>
> 
> You can keep this grouped with the previous headers.
> 
>> +
>>   #include <libcamera/libcamera.h>
>>   #include <libcamera/property_ids.h>
>>
>> @@ -27,15 +29,12 @@ using namespace libcamera;
>>   class CamApp
>>   {
>>   public:
>> -	CamApp();
>> -
>> -	static CamApp *instance();
>> +	CamApp(EventLoop &loop);
>>
>>   	int init(int argc, char **argv);
>>   	void cleanup();
>>
>>   	int exec();
>> -	void quit();
>>
>>   private:
>>   	void cameraAdded(std::shared_ptr<Camera> cam);
>> @@ -46,26 +45,17 @@ private:
>>
>>   	static std::string cameraName(const Camera *camera);
>>
>> -	static CamApp *app_;
>>   	OptionsParser::Options options_;
>>
>>   	std::unique_ptr<CameraManager> cm_;
>>
>>   	std::atomic_uint loopUsers_;
>> -	EventLoop loop_;
>> +	EventLoop *loop_ = nullptr;
>>   };
>>
>> -CamApp *CamApp::app_ = nullptr;
>> -
>> -CamApp::CamApp()
>> -	: loopUsers_(0)
>> -{
>> -	CamApp::app_ = this;
>> -}
>> -
>> -CamApp *CamApp::instance()
>> +CamApp::CamApp(EventLoop &loop)
>> +	: loopUsers_(0), loop_(&loop)
>>   {
>> -	return CamApp::app_;
>>   }
>>
>>   int CamApp::init(int argc, char **argv)
>> @@ -103,11 +93,6 @@ int CamApp::exec()
>>   	return ret;
>>   }
>>
>> -void CamApp::quit()
>> -{
>> -	loop_.exit();
>> -}
>> -
>>   int CamApp::parseOptions(int argc, char *argv[])
>>   {
>>   	StreamKeyValueParser streamKeyValue;
>> @@ -205,7 +190,7 @@ void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
>>   void CamApp::captureDone()
>>   {
>>   	if (--loopUsers_ == 0)
>> -		EventLoop::instance()->exit(0);
>> +		loop_->exit(0);
>>   }
>>
>>   int CamApp::run()
>> @@ -290,7 +275,7 @@ int CamApp::run()
>>   	}
>>
>>   	if (loopUsers_)
>> -		loop_.exec();
>> +		loop_->exec();
>>
>>   	/* 6. Stop capture. */
>>   	for (const auto &session : sessions) {
>> @@ -345,29 +330,36 @@ std::string CamApp::cameraName(const Camera *camera)
>>   	return name;
>>   }
>>
>> -namespace {
>> -
>> -void signalHandler([[maybe_unused]] int signal)
>> +int main(int argc, char **argv)
>>   {
>> -	std::cout << "Exiting" << std::endl;
>> -	CamApp::instance()->quit();
>> -}
>> +	auto sigfd = [] {
>> +		sigset_t ss;
>>
>> -} /* namespace */
>> +		sigemptyset(&ss);
>> +		sigaddset(&ss, SIGINT);
>> +		sigprocmask(SIG_BLOCK, &ss, nullptr);
>>
>> -int main(int argc, char **argv)
>> -{
>> -	CamApp app;
>> +		return UniqueFD(signalfd(-1, &ss, SFD_CLOEXEC | SFD_NONBLOCK));
>> +	}();
> 
> Why a lambda ?
> 
>> +	if (!sigfd.isValid())
>> +		return EXIT_FAILURE;
>> +
>> +	EventLoop loop;
>> +	CamApp app(loop);
>>   	int ret;
>>
>> +	loop.addFdEvent(sigfd.get(), EventLoop::Read, [&] {
>> +		signalfd_siginfo si;
>> +		std::ignore = read(sigfd.get(), &si, sizeof(si));
>> +
>> +		std::cout << "Exiting" << std::endl;
>> +		loop.exit(0);
>> +	});
> 
> It could make things simpler to handle this in the CamApp class, you
> won't have to change where the EventLoop is instantiated.
> 
>> +
>>   	ret = app.init(argc, argv);
>>   	if (ret)
>>   		return ret == -EINTR ? 0 : EXIT_FAILURE;
>>
>> -	struct sigaction sa = {};
>> -	sa.sa_handler = &signalHandler;
>> -	sigaction(SIGINT, &sa, nullptr);
>> -
>>   	if (app.exec())
>>   		return EXIT_FAILURE;
>>
Laurent Pinchart Aug. 15, 2025, 11:02 p.m. UTC | #3
On Sat, Aug 16, 2025 at 12:28:49AM +0200, Barnabás Pőcze wrote:
> 2025. 08. 16. 0:21 keltezéssel, Laurent Pinchart írta:
> > On Fri, Aug 15, 2025 at 04:12:37PM +0200, Barnabás Pőcze wrote:
> >> Use a signalfd to detect `SIGINT` instead of registering a signal handler in
> >> order to remove the `CamApp` singleton. This is better than using a normal
> >> signal handler: a signal can arrive after the `CamApp` object has been destroyed,
> >> leading to a use-after-free. Modifying the `CamApp::app_` in the destructor is
> >> not a good alternative because that would not be async signal safe (unless it
> >> is made atomic).
> > 
> > While this is true, it's also not really much of an issue in my opinion,
> > or at least a very minor one. Now that the patch exists lets get it
> > merged, but you don't have to hunt for these kind of problems in cam or
> > qcam :-)
> 
> I actually did run into this on multiple occasions. :(

Do you mean crashes because a signal arrived when the application was
closing ? How did that happen ?

> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >> ---
> >>   src/apps/cam/main.cpp | 66 +++++++++++++++++++------------------------
> >>   1 file changed, 29 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp
> >> index fa266eca6..cbc85b59f 100644
> >> --- a/src/apps/cam/main.cpp
> >> +++ b/src/apps/cam/main.cpp
> >> @@ -13,6 +13,8 @@
> >>   #include <signal.h>
> >>   #include <string.h>
> >>
> >> +#include <sys/signalfd.h>
> > 
> > You can keep this grouped with the previous headers.
> > 
> >> +
> >>   #include <libcamera/libcamera.h>
> >>   #include <libcamera/property_ids.h>
> >>
> >> @@ -27,15 +29,12 @@ using namespace libcamera;
> >>   class CamApp
> >>   {
> >>   public:
> >> -	CamApp();
> >> -
> >> -	static CamApp *instance();
> >> +	CamApp(EventLoop &loop);
> >>
> >>   	int init(int argc, char **argv);
> >>   	void cleanup();
> >>
> >>   	int exec();
> >> -	void quit();
> >>
> >>   private:
> >>   	void cameraAdded(std::shared_ptr<Camera> cam);
> >> @@ -46,26 +45,17 @@ private:
> >>
> >>   	static std::string cameraName(const Camera *camera);
> >>
> >> -	static CamApp *app_;
> >>   	OptionsParser::Options options_;
> >>
> >>   	std::unique_ptr<CameraManager> cm_;
> >>
> >>   	std::atomic_uint loopUsers_;
> >> -	EventLoop loop_;
> >> +	EventLoop *loop_ = nullptr;
> >>   };
> >>
> >> -CamApp *CamApp::app_ = nullptr;
> >> -
> >> -CamApp::CamApp()
> >> -	: loopUsers_(0)
> >> -{
> >> -	CamApp::app_ = this;
> >> -}
> >> -
> >> -CamApp *CamApp::instance()
> >> +CamApp::CamApp(EventLoop &loop)
> >> +	: loopUsers_(0), loop_(&loop)
> >>   {
> >> -	return CamApp::app_;
> >>   }
> >>
> >>   int CamApp::init(int argc, char **argv)
> >> @@ -103,11 +93,6 @@ int CamApp::exec()
> >>   	return ret;
> >>   }
> >>
> >> -void CamApp::quit()
> >> -{
> >> -	loop_.exit();
> >> -}
> >> -
> >>   int CamApp::parseOptions(int argc, char *argv[])
> >>   {
> >>   	StreamKeyValueParser streamKeyValue;
> >> @@ -205,7 +190,7 @@ void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
> >>   void CamApp::captureDone()
> >>   {
> >>   	if (--loopUsers_ == 0)
> >> -		EventLoop::instance()->exit(0);
> >> +		loop_->exit(0);
> >>   }
> >>
> >>   int CamApp::run()
> >> @@ -290,7 +275,7 @@ int CamApp::run()
> >>   	}
> >>
> >>   	if (loopUsers_)
> >> -		loop_.exec();
> >> +		loop_->exec();
> >>
> >>   	/* 6. Stop capture. */
> >>   	for (const auto &session : sessions) {
> >> @@ -345,29 +330,36 @@ std::string CamApp::cameraName(const Camera *camera)
> >>   	return name;
> >>   }
> >>
> >> -namespace {
> >> -
> >> -void signalHandler([[maybe_unused]] int signal)
> >> +int main(int argc, char **argv)
> >>   {
> >> -	std::cout << "Exiting" << std::endl;
> >> -	CamApp::instance()->quit();
> >> -}
> >> +	auto sigfd = [] {
> >> +		sigset_t ss;
> >>
> >> -} /* namespace */
> >> +		sigemptyset(&ss);
> >> +		sigaddset(&ss, SIGINT);
> >> +		sigprocmask(SIG_BLOCK, &ss, nullptr);
> >>
> >> -int main(int argc, char **argv)
> >> -{
> >> -	CamApp app;
> >> +		return UniqueFD(signalfd(-1, &ss, SFD_CLOEXEC | SFD_NONBLOCK));
> >> +	}();
> > 
> > Why a lambda ?
> > 
> >> +	if (!sigfd.isValid())
> >> +		return EXIT_FAILURE;
> >> +
> >> +	EventLoop loop;
> >> +	CamApp app(loop);
> >>   	int ret;
> >>
> >> +	loop.addFdEvent(sigfd.get(), EventLoop::Read, [&] {
> >> +		signalfd_siginfo si;
> >> +		std::ignore = read(sigfd.get(), &si, sizeof(si));
> >> +
> >> +		std::cout << "Exiting" << std::endl;
> >> +		loop.exit(0);
> >> +	});
> > 
> > It could make things simpler to handle this in the CamApp class, you
> > won't have to change where the EventLoop is instantiated.
> > 
> >> +
> >>   	ret = app.init(argc, argv);
> >>   	if (ret)
> >>   		return ret == -EINTR ? 0 : EXIT_FAILURE;
> >>
> >> -	struct sigaction sa = {};
> >> -	sa.sa_handler = &signalHandler;
> >> -	sigaction(SIGINT, &sa, nullptr);
> >> -
> >>   	if (app.exec())
> >>   		return EXIT_FAILURE;
> >>
Barnabás Pőcze Aug. 15, 2025, 11:06 p.m. UTC | #4
2025. 08. 16. 1:02 keltezéssel, Laurent Pinchart írta:
> On Sat, Aug 16, 2025 at 12:28:49AM +0200, Barnabás Pőcze wrote:
>> 2025. 08. 16. 0:21 keltezéssel, Laurent Pinchart írta:
>>> On Fri, Aug 15, 2025 at 04:12:37PM +0200, Barnabás Pőcze wrote:
>>>> Use a signalfd to detect `SIGINT` instead of registering a signal handler in
>>>> order to remove the `CamApp` singleton. This is better than using a normal
>>>> signal handler: a signal can arrive after the `CamApp` object has been destroyed,
>>>> leading to a use-after-free. Modifying the `CamApp::app_` in the destructor is
>>>> not a good alternative because that would not be async signal safe (unless it
>>>> is made atomic).
>>>
>>> While this is true, it's also not really much of an issue in my opinion,
>>> or at least a very minor one. Now that the patch exists lets get it
>>> merged, but you don't have to hunt for these kind of problems in cam or
>>> qcam :-)
>>
>> I actually did run into this on multiple occasions. :(
> 
> Do you mean crashes because a signal arrived when the application was
> closing ? How did that happen ?

Yes. Spamming ctrl+c to stop `cam`. I usually test with asan, and with that it is
easy to trigger this issue; eventually it became a bit too annoying, hence this change.


> 
>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>> ---
>>>>    src/apps/cam/main.cpp | 66 +++++++++++++++++++------------------------
>>>>    1 file changed, 29 insertions(+), 37 deletions(-)
>>>>
>>>> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp
>>>> index fa266eca6..cbc85b59f 100644
>>>> --- a/src/apps/cam/main.cpp
>>>> +++ b/src/apps/cam/main.cpp
>>>> @@ -13,6 +13,8 @@
>>>>    #include <signal.h>
>>>>    #include <string.h>
>>>>
>>>> +#include <sys/signalfd.h>
>>>
>>> You can keep this grouped with the previous headers.
>>>
>>>> +
>>>>    #include <libcamera/libcamera.h>
>>>>    #include <libcamera/property_ids.h>
>>>>
>>>> @@ -27,15 +29,12 @@ using namespace libcamera;
>>>>    class CamApp
>>>>    {
>>>>    public:
>>>> -	CamApp();
>>>> -
>>>> -	static CamApp *instance();
>>>> +	CamApp(EventLoop &loop);
>>>>
>>>>    	int init(int argc, char **argv);
>>>>    	void cleanup();
>>>>
>>>>    	int exec();
>>>> -	void quit();
>>>>
>>>>    private:
>>>>    	void cameraAdded(std::shared_ptr<Camera> cam);
>>>> @@ -46,26 +45,17 @@ private:
>>>>
>>>>    	static std::string cameraName(const Camera *camera);
>>>>
>>>> -	static CamApp *app_;
>>>>    	OptionsParser::Options options_;
>>>>
>>>>    	std::unique_ptr<CameraManager> cm_;
>>>>
>>>>    	std::atomic_uint loopUsers_;
>>>> -	EventLoop loop_;
>>>> +	EventLoop *loop_ = nullptr;
>>>>    };
>>>>
>>>> -CamApp *CamApp::app_ = nullptr;
>>>> -
>>>> -CamApp::CamApp()
>>>> -	: loopUsers_(0)
>>>> -{
>>>> -	CamApp::app_ = this;
>>>> -}
>>>> -
>>>> -CamApp *CamApp::instance()
>>>> +CamApp::CamApp(EventLoop &loop)
>>>> +	: loopUsers_(0), loop_(&loop)
>>>>    {
>>>> -	return CamApp::app_;
>>>>    }
>>>>
>>>>    int CamApp::init(int argc, char **argv)
>>>> @@ -103,11 +93,6 @@ int CamApp::exec()
>>>>    	return ret;
>>>>    }
>>>>
>>>> -void CamApp::quit()
>>>> -{
>>>> -	loop_.exit();
>>>> -}
>>>> -
>>>>    int CamApp::parseOptions(int argc, char *argv[])
>>>>    {
>>>>    	StreamKeyValueParser streamKeyValue;
>>>> @@ -205,7 +190,7 @@ void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
>>>>    void CamApp::captureDone()
>>>>    {
>>>>    	if (--loopUsers_ == 0)
>>>> -		EventLoop::instance()->exit(0);
>>>> +		loop_->exit(0);
>>>>    }
>>>>
>>>>    int CamApp::run()
>>>> @@ -290,7 +275,7 @@ int CamApp::run()
>>>>    	}
>>>>
>>>>    	if (loopUsers_)
>>>> -		loop_.exec();
>>>> +		loop_->exec();
>>>>
>>>>    	/* 6. Stop capture. */
>>>>    	for (const auto &session : sessions) {
>>>> @@ -345,29 +330,36 @@ std::string CamApp::cameraName(const Camera *camera)
>>>>    	return name;
>>>>    }
>>>>
>>>> -namespace {
>>>> -
>>>> -void signalHandler([[maybe_unused]] int signal)
>>>> +int main(int argc, char **argv)
>>>>    {
>>>> -	std::cout << "Exiting" << std::endl;
>>>> -	CamApp::instance()->quit();
>>>> -}
>>>> +	auto sigfd = [] {
>>>> +		sigset_t ss;
>>>>
>>>> -} /* namespace */
>>>> +		sigemptyset(&ss);
>>>> +		sigaddset(&ss, SIGINT);
>>>> +		sigprocmask(SIG_BLOCK, &ss, nullptr);
>>>>
>>>> -int main(int argc, char **argv)
>>>> -{
>>>> -	CamApp app;
>>>> +		return UniqueFD(signalfd(-1, &ss, SFD_CLOEXEC | SFD_NONBLOCK));
>>>> +	}();
>>>
>>> Why a lambda ?
>>>
>>>> +	if (!sigfd.isValid())
>>>> +		return EXIT_FAILURE;
>>>> +
>>>> +	EventLoop loop;
>>>> +	CamApp app(loop);
>>>>    	int ret;
>>>>
>>>> +	loop.addFdEvent(sigfd.get(), EventLoop::Read, [&] {
>>>> +		signalfd_siginfo si;
>>>> +		std::ignore = read(sigfd.get(), &si, sizeof(si));
>>>> +
>>>> +		std::cout << "Exiting" << std::endl;
>>>> +		loop.exit(0);
>>>> +	});
>>>
>>> It could make things simpler to handle this in the CamApp class, you
>>> won't have to change where the EventLoop is instantiated.
>>>
>>>> +
>>>>    	ret = app.init(argc, argv);
>>>>    	if (ret)
>>>>    		return ret == -EINTR ? 0 : EXIT_FAILURE;
>>>>
>>>> -	struct sigaction sa = {};
>>>> -	sa.sa_handler = &signalHandler;
>>>> -	sigaction(SIGINT, &sa, nullptr);
>>>> -
>>>>    	if (app.exec())
>>>>    		return EXIT_FAILURE;
>>>>
>

Patch
diff mbox series

diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp
index fa266eca6..cbc85b59f 100644
--- a/src/apps/cam/main.cpp
+++ b/src/apps/cam/main.cpp
@@ -13,6 +13,8 @@ 
 #include <signal.h>
 #include <string.h>

+#include <sys/signalfd.h>
+
 #include <libcamera/libcamera.h>
 #include <libcamera/property_ids.h>

@@ -27,15 +29,12 @@  using namespace libcamera;
 class CamApp
 {
 public:
-	CamApp();
-
-	static CamApp *instance();
+	CamApp(EventLoop &loop);

 	int init(int argc, char **argv);
 	void cleanup();

 	int exec();
-	void quit();

 private:
 	void cameraAdded(std::shared_ptr<Camera> cam);
@@ -46,26 +45,17 @@  private:

 	static std::string cameraName(const Camera *camera);

-	static CamApp *app_;
 	OptionsParser::Options options_;

 	std::unique_ptr<CameraManager> cm_;

 	std::atomic_uint loopUsers_;
-	EventLoop loop_;
+	EventLoop *loop_ = nullptr;
 };

-CamApp *CamApp::app_ = nullptr;
-
-CamApp::CamApp()
-	: loopUsers_(0)
-{
-	CamApp::app_ = this;
-}
-
-CamApp *CamApp::instance()
+CamApp::CamApp(EventLoop &loop)
+	: loopUsers_(0), loop_(&loop)
 {
-	return CamApp::app_;
 }

 int CamApp::init(int argc, char **argv)
@@ -103,11 +93,6 @@  int CamApp::exec()
 	return ret;
 }

-void CamApp::quit()
-{
-	loop_.exit();
-}
-
 int CamApp::parseOptions(int argc, char *argv[])
 {
 	StreamKeyValueParser streamKeyValue;
@@ -205,7 +190,7 @@  void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
 void CamApp::captureDone()
 {
 	if (--loopUsers_ == 0)
-		EventLoop::instance()->exit(0);
+		loop_->exit(0);
 }

 int CamApp::run()
@@ -290,7 +275,7 @@  int CamApp::run()
 	}

 	if (loopUsers_)
-		loop_.exec();
+		loop_->exec();

 	/* 6. Stop capture. */
 	for (const auto &session : sessions) {
@@ -345,29 +330,36 @@  std::string CamApp::cameraName(const Camera *camera)
 	return name;
 }

-namespace {
-
-void signalHandler([[maybe_unused]] int signal)
+int main(int argc, char **argv)
 {
-	std::cout << "Exiting" << std::endl;
-	CamApp::instance()->quit();
-}
+	auto sigfd = [] {
+		sigset_t ss;

-} /* namespace */
+		sigemptyset(&ss);
+		sigaddset(&ss, SIGINT);
+		sigprocmask(SIG_BLOCK, &ss, nullptr);

-int main(int argc, char **argv)
-{
-	CamApp app;
+		return UniqueFD(signalfd(-1, &ss, SFD_CLOEXEC | SFD_NONBLOCK));
+	}();
+	if (!sigfd.isValid())
+		return EXIT_FAILURE;
+
+	EventLoop loop;
+	CamApp app(loop);
 	int ret;

+	loop.addFdEvent(sigfd.get(), EventLoop::Read, [&] {
+		signalfd_siginfo si;
+		std::ignore = read(sigfd.get(), &si, sizeof(si));
+
+		std::cout << "Exiting" << std::endl;
+		loop.exit(0);
+	});
+
 	ret = app.init(argc, argv);
 	if (ret)
 		return ret == -EINTR ? 0 : EXIT_FAILURE;

-	struct sigaction sa = {};
-	sa.sa_handler = &signalHandler;
-	sigaction(SIGINT, &sa, nullptr);
-
 	if (app.exec())
 		return EXIT_FAILURE;