<div dir="ltr"><div dir="ltr">Hi Laurent, thank you for reviewing.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jun 7, 2021 at 3:02 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
Thank you for the patch.<br>
<br>
I'm reviewing users of the new class first, to better assert its API.<br>
I'll reply to patch 01/10 last.<br>
<br>
On Thu, Apr 15, 2021 at 05:38:35PM +0900, Hirokazu Honda wrote:<br>
> EventDispatcherPoll owns the event file descriptor. This manages<br>
> the fd with ScopedFD to avoid leakage.<br>
> <br>
> Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> ---<br>
>  include/libcamera/internal/event_dispatcher_poll.h |  4 +++-<br>
>  src/libcamera/event_dispatcher_poll.cpp            | 12 ++++++------<br>
>  2 files changed, 9 insertions(+), 7 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/internal/event_dispatcher_poll.h b/include/libcamera/internal/event_dispatcher_poll.h<br>
> index 33de051d..b686183d 100644<br>
> --- a/include/libcamera/internal/event_dispatcher_poll.h<br>
> +++ b/include/libcamera/internal/event_dispatcher_poll.h<br>
> @@ -11,6 +11,8 @@<br>
>  #include <map><br>
>  #include <vector><br>
>  <br>
> +#include <libcamera/scoped_file_descriptor.h><br>
> +<br>
>  #include "libcamera/internal/event_dispatcher.h"<br>
>  <br>
>  struct pollfd;<br>
> @@ -48,7 +50,7 @@ private:<br>
>  <br>
>       std::map<int, EventNotifierSetPoll> notifiers_;<br>
>       std::list<Timer *> timers_;<br>
> -     int eventfd_;<br>
> +     ScopedFD eventfd_;<br>
>  <br>
>       bool processingEvents_;<br>
>  };<br>
> diff --git a/src/libcamera/event_dispatcher_poll.cpp b/src/libcamera/event_dispatcher_poll.cpp<br>
> index 456c6def..6b22a8a3 100644<br>
> --- a/src/libcamera/event_dispatcher_poll.cpp<br>
> +++ b/src/libcamera/event_dispatcher_poll.cpp<br>
> @@ -54,14 +54,14 @@ EventDispatcherPoll::EventDispatcherPoll()<br>
>        * Create the event fd. Failures are fatal as we can't implement an<br>
>        * interruptible dispatcher without the fd.<br>
>        */<br>
> -     eventfd_ = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);<br>
> -     if (eventfd_ < 0)<br>
> +     int fd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);<br>
> +     if (fd < 0)<br>
>               LOG(Event, Fatal) << "Unable to create eventfd";<br>
> +     eventfd_ = ScopedFD(fd);<br>
<br>
You could write this<br>
<br>
        eventfd_ = ScopedFD(eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK));<br>
        if (!eventfd_.isValid())<br>
                LOG(Event, Fatal) << "Unable to create eventfd";<br>
<br>
It doesn't matter much.<br>
<br>
>  }<br>
>  <br>
>  EventDispatcherPoll::~EventDispatcherPoll()<br>
>  {<br>
> -     close(eventfd_);<br>
>  }<br>
>  <br>
>  void EventDispatcherPoll::registerEventNotifier(EventNotifier *notifier)<br>
> @@ -154,7 +154,7 @@ void EventDispatcherPoll::processEvents()<br>
>       for (auto notifier : notifiers_)<br>
>               pollfds.push_back({ notifier.first, notifier.second.events(), 0 });<br>
>  <br>
> -     pollfds.push_back({ eventfd_, POLLIN, 0 });<br>
> +     pollfds.push_back({ eventfd_.get(), POLLIN, 0 });<br>
<br>
I wonder if a conversation operator would be useful:<br>
<br>
        operator int() const { return fd_; }<br>
<br>
would allow us to write<br>
<br>
        pollfds.push_back({ eventfd_, POLLIN, 0 });<br>
<br>
It's probably a dangerous idea though.<br>
<br></blockquote><div> </div><div>I would not do so. It is unclear that an integer is passed by that.</div><div>get() should be the unique way of getting the owned file descriptor like unique_ptr.</div><div><br></div><div>Regards,</div><div>-Hiro</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
>  <br>
>       /* Wait for events and process notifiers and timers. */<br>
>       do {<br>
> @@ -176,7 +176,7 @@ void EventDispatcherPoll::processEvents()<br>
>  void EventDispatcherPoll::interrupt()<br>
>  {<br>
>       uint64_t value = 1;<br>
> -     ssize_t ret = write(eventfd_, &value, sizeof(value));<br>
> +     ssize_t ret = write(eventfd_.get(), &value, sizeof(value));<br>
>       if (ret != sizeof(value)) {<br>
>               if (ret < 0)<br>
>                       ret = -errno;<br>
> @@ -230,7 +230,7 @@ void EventDispatcherPoll::processInterrupt(const struct pollfd &pfd)<br>
>               return;<br>
>  <br>
>       uint64_t value;<br>
> -     ssize_t ret = read(eventfd_, &value, sizeof(value));<br>
> +     ssize_t ret = read(eventfd_.get(), &value, sizeof(value));<br>
>       if (ret != sizeof(value)) {<br>
>               if (ret < 0)<br>
>                       ret = -errno;<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>