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

[Laurent Pinchart]

Hi Jacopo,

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.

-- 
Regards,

Laurent Pinchart