[libcamera-devel] [RFC PATCH 02/10] libcamera: EventDispatcherPoll: Manage fd by ScopedFD

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

I'm reviewing users of the new class first, to better assert its API.
I'll reply to patch 01/10 last.

On Thu, Apr 15, 2021 at 05:38:35PM +0900, Hirokazu Honda wrote:
> EventDispatcherPoll owns the event file descriptor. This manages
> the fd with ScopedFD to avoid leakage.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/internal/event_dispatcher_poll.h |  4 +++-
>  src/libcamera/event_dispatcher_poll.cpp            | 12 ++++++------
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/internal/event_dispatcher_poll.h b/include/libcamera/internal/event_dispatcher_poll.h
> index 33de051d..b686183d 100644
> --- a/include/libcamera/internal/event_dispatcher_poll.h
> +++ b/include/libcamera/internal/event_dispatcher_poll.h
> @@ -11,6 +11,8 @@
>  #include <map>
>  #include <vector>
>  
> +#include <libcamera/scoped_file_descriptor.h>
> +
>  #include "libcamera/internal/event_dispatcher.h"
>  
>  struct pollfd;
> @@ -48,7 +50,7 @@ private:
>  
>  	std::map<int, EventNotifierSetPoll> notifiers_;
>  	std::list<Timer *> timers_;
> -	int eventfd_;
> +	ScopedFD eventfd_;
>  
>  	bool processingEvents_;
>  };
> diff --git a/src/libcamera/event_dispatcher_poll.cpp b/src/libcamera/event_dispatcher_poll.cpp
> index 456c6def..6b22a8a3 100644
> --- a/src/libcamera/event_dispatcher_poll.cpp
> +++ b/src/libcamera/event_dispatcher_poll.cpp
> @@ -54,14 +54,14 @@ EventDispatcherPoll::EventDispatcherPoll()
>  	 * Create the event fd. Failures are fatal as we can't implement an
>  	 * interruptible dispatcher without the fd.
>  	 */
> -	eventfd_ = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);
> -	if (eventfd_ < 0)
> +	int fd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);
> +	if (fd < 0)
>  		LOG(Event, Fatal) << "Unable to create eventfd";
> +	eventfd_ = ScopedFD(fd);

You could write this

	eventfd_ = ScopedFD(eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK));
	if (!eventfd_.isValid())
 		LOG(Event, Fatal) << "Unable to create eventfd";

It doesn't matter much.

>  }
>  
>  EventDispatcherPoll::~EventDispatcherPoll()
>  {
> -	close(eventfd_);
>  }
>  
>  void EventDispatcherPoll::registerEventNotifier(EventNotifier *notifier)
> @@ -154,7 +154,7 @@ void EventDispatcherPoll::processEvents()
>  	for (auto notifier : notifiers_)
>  		pollfds.push_back({ notifier.first, notifier.second.events(), 0 });
>  
> -	pollfds.push_back({ eventfd_, POLLIN, 0 });
> +	pollfds.push_back({ eventfd_.get(), POLLIN, 0 });

I wonder if a conversation operator would be useful:

	operator int() const { return fd_; }

would allow us to write

	pollfds.push_back({ eventfd_, POLLIN, 0 });

It's probably a dangerous idea though.

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

>  
>  	/* Wait for events and process notifiers and timers. */
>  	do {
> @@ -176,7 +176,7 @@ void EventDispatcherPoll::processEvents()
>  void EventDispatcherPoll::interrupt()
>  {
>  	uint64_t value = 1;
> -	ssize_t ret = write(eventfd_, &value, sizeof(value));
> +	ssize_t ret = write(eventfd_.get(), &value, sizeof(value));
>  	if (ret != sizeof(value)) {
>  		if (ret < 0)
>  			ret = -errno;
> @@ -230,7 +230,7 @@ void EventDispatcherPoll::processInterrupt(const struct pollfd &pfd)
>  		return;
>  
>  	uint64_t value;
> -	ssize_t ret = read(eventfd_, &value, sizeof(value));
> +	ssize_t ret = read(eventfd_.get(), &value, sizeof(value));
>  	if (ret != sizeof(value)) {
>  		if (ret < 0)
>  			ret = -errno;

-- 
Regards,

Laurent Pinchart