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

[Jacopo Mondi]

Hi Laurent

On Thu, Sep 05, 2019 at 12:46:36AM +0300, Laurent Pinchart wrote:
> 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.
>

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

> --
> 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/20190905/fd0fcdd6/attachment.sig>