[libcamera-devel] [PATCH v2 03/24] libcamera: controls: Avoid exception in ControlList count() and find()

[Laurent Pinchart]

Hi Jacopo,

On Fri, Nov 15, 2019 at 05:00:45PM +0100, Jacopo Mondi wrote:
> On Fri, Nov 08, 2019 at 10:53:48PM +0200, Laurent Pinchart wrote:
> > The ControlList count() and find() methods use at() to lookup the
> > control numerical ID in the idmap_. This causes an exception to be
> > thrown if the ID doesn't exist in the map. Fix it by using the find()
> > method instead.
> 
> Be aware that ControlList::at() (const) still uses at() and could
> potentially raise an exception as well, while I think we clearly
> prescribes that libcamera does not uses them.

There's not much we can do about that, as at() returns a reference, so
there's no way to return an error. It's an error in the caller to use
at() without checking if the item exists first. We could specify this in
the documentation, or maybe remove the at() method completely, forcing
the caller to use find(). What do you think is best ?

> If that's fine please have mine
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/controls.cpp | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 93ad2fc6a276..0c7cd449ad64 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -491,7 +491,11 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const
> >   */
> >  ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
> >  {
> > -	return count(idmap_.at(id));
> > +	auto iter = idmap_.find(id);
> > +	if (iter == idmap_.end())
> > +		return 0;
> > +
> > +	return count(iter->second);
> >  }
> >
> >  /**
> > @@ -502,7 +506,11 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
> >   */
> >  ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)
> >  {
> > -	return find(idmap_.at(id));
> > +	auto iter = idmap_.find(id);
> > +	if (iter == idmap_.end())
> > +		return end();
> > +
> > +	return find(iter->second);
> >  }
> >
> >  /**
> > @@ -513,7 +521,11 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)
> >   */
> >  ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const
> >  {
> > -	return find(idmap_.at(id));
> > +	auto iter = idmap_.find(id);
> > +	if (iter == idmap_.end())
> > +		return end();
> > +
> > +	return find(iter->second);
> >  }
> >
> >  /**

-- 
Regards,

Laurent Pinchart