[libcamera-devel] [PATCH v2 10/24] libcamera: controls: Catch type mismatch in ControlInfoMap

[Laurent Pinchart]

Hi Kieran,

On Tue, Nov 19, 2019 at 04:19:45PM +0000, Kieran Bingham wrote:
> On 18/11/2019 01:01, Laurent Pinchart wrote:
> > On Fri, Nov 15, 2019 at 05:21:29PM +0100, Jacopo Mondi wrote:
> >> On Fri, Nov 08, 2019 at 10:53:55PM +0200, Laurent Pinchart wrote:
> >>> ControlInfoMap requires the ControlId and ControlRange of each entry to
> >>> have identical types. Check for this and log an error if a mismatch is
> >>> detected.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>> ---
> >>>  src/libcamera/controls.cpp | 13 ++++++++++++-
> >>>  1 file changed, 12 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> >>> index eae0250a92e3..c488b2e4eb3f 100644
> >>> --- a/src/libcamera/controls.cpp
> >>> +++ b/src/libcamera/controls.cpp
> >>> @@ -571,8 +571,19 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const
> >>>  void ControlInfoMap::generateIdmap()
> >>>  {
> >>>  	idmap_.clear();
> >>> -	for (const auto &ctrl : *this)
> >>> +
> >>> +	for (const auto &ctrl : *this) {
> >>> +		if (ctrl.first->type() != ctrl.second.min().type()) {
> >>> +			LOG(Controls, Error)
> >>> +				<< "Control " << utils::hex(ctrl.first->id())
> >>> +				<< " type and range type mismatch";
> >>> +			idmap_.clear();
> >>> +			clear();
> >>> +			return;
> >>> +		}
> >>> +
> >>
> >> Should we just skip the control or actually abort the whole id map
> >> generation ?
> > 
> > As this is a serious error, I think we should abort it completely, in
> > order to show that something went wrong. If you think we should just
> > skip the control, we can discuss that.
> 
> Is this serious enough to be Fatal? (to be really noisy?)
> 
> (Bearing in mind we still need to look at if Fatal messages are
> non-fatal when NDEBUG is defined).

I'm in two minds about this, for this exact reason, we still need to
define how to handle Fatal errors. I think I would prefer avoiding an
abort() here until the code gets tested a bit more.

> >>>  		idmap_[ctrl.first->id()] = ctrl.first;
> >>> +	}
> >>>  }
> >>>
> >>>  /**

-- 
Regards,

Laurent Pinchart