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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Nov 18, 2019 at 11:51:21PM +0100, Jacopo Mondi wrote:
> On Mon, Nov 18, 2019 at 02:44:54AM +0200, Laurent Pinchart wrote:
> > 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
> 
> s/ControlList/ControlInfoMap/
> 
> > > > 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 ?
> 
> I don't see users of ControlInfoMap::at() if not the the
> ControlInfoMap class itself. I would be fine in dropping it. There are
> usaged of at() on maps in many places on std::map instances though, so
> you don't want to remove it for consitency, that's fine as well.

I think there's at least one user in the tests, as compilation failed
when I dropped the method.

Would you like to submit a patch, or do you think this is overkill ?

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