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

Message ID 20191108205409.18845-11-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Control serialization and IPA C API
Related show

Commit Message

Laurent Pinchart Nov. 8, 2019, 8:53 p.m. UTC
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@ideasonboard.com>
---
 src/libcamera/controls.cpp | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi Nov. 15, 2019, 4:21 p.m. UTC | #1
Hi LAurent

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@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 ?

>  		idmap_[ctrl.first->id()] = ctrl.first;
> +	}
>  }
>
>  /**
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Nov. 18, 2019, 1:01 a.m. UTC | #2
Hi Jacopo,

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@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.

> >  		idmap_[ctrl.first->id()] = ctrl.first;
> > +	}
> >  }
> >
> >  /**
Niklas Söderlund Nov. 18, 2019, 5:16 p.m. UTC | #3
Hi Laurent,

Thanks for your work.

On 2019-11-08 22:53:55 +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@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  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;
> +		}
> +
>  		idmap_[ctrl.first->id()] = ctrl.first;
> +	}
>  }
>  
>  /**
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Nov. 18, 2019, 10:55 p.m. UTC | #4
Hi Laurent,

On Mon, Nov 18, 2019 at 03:01:33AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> 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@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.
>

No, it's fine.. If this happens there has been a problem during
development, is not something a user could trigger.. In this case it's
better to fail loudly

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> > >  		idmap_[ctrl.first->id()] = ctrl.first;
> > > +	}
> > >  }
> > >
> > >  /**
>
> --
> Regards,
>
> Laurent Pinchart
Kieran Bingham Nov. 19, 2019, 4:19 p.m. UTC | #5
Hi Laurent,

On 18/11/2019 01:01, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> 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@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).

--
Kieran

> 
>>>  		idmap_[ctrl.first->id()] = ctrl.first;
>>> +	}
>>>  }
>>>
>>>  /**
>
Laurent Pinchart Nov. 19, 2019, 10:36 p.m. UTC | #6
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@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;
> >>> +	}
> >>>  }
> >>>
> >>>  /**

Patch

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;
+		}
+
 		idmap_[ctrl.first->id()] = ctrl.first;
+	}
 }
 
 /**