[libcamera-devel] [PATCH 07/11] libcamera: Add a poll-based event dispatcher

[Laurent Pinchart]

Hi Niklas,

On Sunday, 6 January 2019 18:25:30 EET Niklas Söderlund wrote:
> On 2019-01-06 04:33:24 +0200, Laurent Pinchart wrote:
> > Provide a poll-based event dispatcher implementation as convenience for
> > applications that don't need a custom event loop. The poll-based
> > dispatcher is automatically instantiated if the application doesn't
> > provide its own dispatcher.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > 
> >  include/libcamera/libcamera.h                 |   1 +
> >  src/libcamera/camera_manager.cpp              |  19 +-
> >  src/libcamera/event_dispatcher_poll.cpp       | 232 ++++++++++++++++++
> >  src/libcamera/include/event_dispatcher_poll.h |  50 ++++
> >  src/libcamera/meson.build                     |   2 +
> >  5 files changed, 299 insertions(+), 5 deletions(-)
> >  create mode 100644 src/libcamera/event_dispatcher_poll.cpp
> >  create mode 100644 src/libcamera/include/event_dispatcher_poll.h

[snip]

> > diff --git a/src/libcamera/event_dispatcher_poll.cpp
> > b/src/libcamera/event_dispatcher_poll.cpp new file mode 100644
> > index 000000000000..2c64c1446772
> > --- /dev/null
> > +++ b/src/libcamera/event_dispatcher_poll.cpp

[snip]

> > +
> > +void EventDispatcherPoll::registerEventNotifier(EventNotifier *notifier)
> > +{
> > +	EventNotifierSetPoll &set = notifiers_[notifier->fd()];
> > +	EventNotifier::Type type = notifier->type();
> > +
> > +	if (set.notifiers[type] && set.notifiers[type] != notifier)
> > +		LOG(Warning) << "Duplicate " << notifierType(type)
> > +			     << " notifiers for fd " << notifier->fd();
> 
> I'm not sure I what I think about this :-) Is it wise to allow the
> notifier to be replaced with just a warning? My gut feeling tells me we
> should warn and ignore the new notifier instead.

I'm not sure either. It indicates a problem in either case, which can result 
in incorrect behaviour. I don't know which option would minimize the impact 
(or if we instead want to maximize the impact so that the issue will be fixed 
earlier).

Should the specific behaviour we pick be documented in the EventNotifier 
class, or be specified as undefined behaviour ?

> At least I think we should rephrase the warning message to reflect what
> action is taken. Either log that the old notifier is replaced or that
> the new one is ignored.

Agreed, I'll fix that.

> > +
> > +	set.notifiers[type] = notifier;
> > +}
> > +
> > +void EventDispatcherPoll::unregisterEventNotifier(EventNotifier
> > *notifier)
> > +{
> > +	auto iter = notifiers_.find(notifier->fd());
> > +	if (iter == notifiers_.end())
> > +		return;
> > +
> > +	EventNotifierSetPoll &set = iter->second;
> > +	EventNotifier::Type type = notifier->type();
> > +
> > +	if (!set.notifiers[type])
> > +		return;
> > +
> > +	if (set.notifiers[type] != notifier) {
> > +		LOG(Warning) << "Duplicate " << notifierType(type)
> > +			     << " notifiers for fd " << notifier->fd();
> 
> I feel the log message should be rephrased, how about
> 
>     "Can't unregister a not register " << notifierType(type) << "
>     notifier for fd " << notifier->fd()

How about

notifierType(type) << " notifier for fd " << notifier->fd() << " not 
registered"

?

> With these small issues addressed
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> 
> > +		return;
> > +	}
> > +
> > +	set.notifiers[type] = nullptr;
> > +	if (!set.notifiers[0] && !set.notifiers[1] && !set.notifiers[2])
> > +		notifiers_.erase(iter);
> > +}

[snip]

-- 
Regards,

Laurent Pinchart