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

[Jacopo Mondi]

Hi Laurent,

On Wed, Sep 04, 2019 at 08:52:08PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,

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


>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190904/c6d0d420/attachment.sig>