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

[Hirokazu Honda]

Hi Laurent, thank you for reviewing.

On Mon, Jun 7, 2021 at 3:02 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> 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.
>
>
I would not do so. It is unclear that an integer is passed by that.
get() should be the unique way of getting the owned file descriptor like
unique_ptr.

Regards,
-Hiro

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210607/fe95b533/attachment-0001.htm>