[libcamera-devel] [PATCH v5 7/8] android: camera_device: Free metadata in error path

[Laurent Pinchart]

Hi Jacopo,

On Thu, Sep 05, 2019 at 12:01:18AM +0200, Jacopo Mondi wrote:
> On Thu, Sep 05, 2019 at 12:46:36AM +0300, Laurent Pinchart wrote:
> > On Wed, Sep 04, 2019 at 11:03:17PM +0200, Jacopo Mondi wrote:
> > > On Wed, Sep 04, 2019 at 08:52:08PM +0300, Laurent Pinchart wrote:
> > >
> > > [snip]
> > >
> > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > > > index 64382bbac76a..9880168a7581 100644
> > > > > --- a/src/android/camera_device.h
> > > > > +++ b/src/android/camera_device.h
> > > > > @@ -19,10 +19,11 @@
> > > > >
> > > > >  #include "message.h"
> > > > >
> > > > > -#define METADATA_ASSERT(_r)		\
> > > > > +#define METADATA_ASSERT(_r, _p)		\
> > > > >  	do {				\
> > > > >  		if (!(_r)) break;	\
> > > > >  		LOG(HAL, Error) << "Error: " << __func__ << ":" << __LINE__; \
> > > > > +		free_camera_metadata((_p));				     \
> > > > >  		return nullptr;		\
> > > > >  	} while(0);
> > > >
> > > > That's really ugly :-(
> > >
> > > The whole METADATA_ASSERT thing is ugly. It has been there since my
> > > early camera hal hacking days, and it stayed there on the assumption
> > > the metadata handling code should be replaced by a proper Metadata
> > > class, but since it has to be changed I agree it should probably be
> > > removed.
> > >
> > > > How about using unique pointers instead ? After allocating the metadata,
> > > > create a unique pointer on the stack
> > > >
> > > > 	std::unique_ptr<camera_metadata_t> deleter(metadata);
> > > >
> > > > And to return in the *normal* path,
> > > >
> > > > 	return deleter.release();
> > > >
> > > > Or, if you prefer,
> > > >
> > > > 	deleter.release();
> > > > 	return metadata;
> > > >
> > > > The two options are equivalent, but the latter may be more readable.
> > >
> > > Gnn, I'm not a fan of those C++ implicities... I would rather goto to
> > > a label where delete the metadata pack and return... Is this to C-ish ?
> >
> > It is, but that part that would be really bad about that would be the
> > goto hidden inside the METADATA_ASSERT() macro.
> 
> Not hidden, and explicit 'goto' replacing the macro.
> 
> Anyway, I'll immediately take it back:
> ../src/android/camera_device.cpp:390:3: error: cannot jump from this goto statement to its label
>                 goto error;
> 
> One thing I realized bothers me is loosing the printout that tells you
> which tag has failed.. true, that code will go away but it will be
> reasonably shaken up a bit as soon as we have controls and properties,
> so it might be worth to keep it to spot errors generated by more
> dynamic control values around.
> 
> A proper helper class would of course take care of that, but for the
> sake of this series, and as long as this code will be used, I would
> rather have that debug information available.
> 
> I can add a LOG line to each if (ret), but really, 4 lines of
> identical error handline code for each
> metadata tag, goto or unique_ptr<> it doesn't matter, bother me a bit.
> 
> Suddenly the ugly macro seems less uglier to me. It might be the late
> time though..

I haven't explained what I meant clearly enough. I think you can keep
the macro for now, but instead of adding free_camera_metadata() to the
macro, use a std::unique_ptr<> as a deleter. This will keep the LOG()
and handle the leak in the request type switch().

-- 
Regards,

Laurent Pinchart