| Message ID | 20210415083843.3399502-2-hiroh@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
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@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@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;
Hi Laurent, thank you for reviewing. On Mon, Jun 7, 2021 at 3:02 AM Laurent Pinchart < laurent.pinchart@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@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@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 >
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); } 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 }); /* 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;
EventDispatcherPoll owns the event file descriptor. This manages the fd with ScopedFD to avoid leakage. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- include/libcamera/internal/event_dispatcher_poll.h | 4 +++- src/libcamera/event_dispatcher_poll.cpp | 12 ++++++------ 2 files changed, 9 insertions(+), 7 deletions(-)