[PATCH v4 01/11] FWU: Add FWU metadata structure and driver for accessing metadata

[Masami Hiramatsu]

Hi Sughosh,

2022年2月8日(火) 19:24 Sughosh Ganu <sughosh.ganu at linaro.org>:
>
> hi Masami,
>
> On Tue, 8 Feb 2022 at 15:04, Masami Hiramatsu
> <masami.hiramatsu at linaro.org> wrote:
> >
> > Hi Sughosh,
> >
> > Thanks for updating the series. I have some comment on this patch.
> >
> > 2022年2月8日(火) 3:21 Sughosh Ganu <sughosh.ganu at linaro.org>:
> > [snip]
> > > +
> > > +/**
> > > + * fwu_get_active_index() - Get active_index from the FWU metadata
> > > + * @active_idx: active_index value to be read
> > > + *
> > > + * Read the active_index field from the FWU metadata and place it in
> > > + * the variable pointed to be the function argument.
> > > + *
> > > + * Return: 0 if OK, -ve on error
> > > + *
> > > + */
> > > +int fwu_get_active_index(u32 *active_idx)
> > > +{
> > > +       int ret;
> > > +       struct fwu_mdata *mdata = NULL;
> > > +
> > > +       ret = fwu_get_mdata(&mdata);
> > > +       if (ret < 0) {
> > > +               log_err("Unable to get valid FWU metadata\n");
> > > +               goto out;
> >
> > Again, as we discussed previous series, please don't return unused
> > allocated memory if the function returns an error.
> > That is something like putting a burden on the callers. They always
> > needs to initialize the pointer before call and free it even if the
> > function is failed.
>
> I would like to keep the convention consistent. The function that
> declares the pointer will also be responsible for free'ing it up. I
> find the alternative to be more confusing, where in some instances the
> callee frees up the memory, while in some cases it is the caller that
> frees it up. If there is no stated convention in u-boot which forbids
> this style of memory handling, I would like to keep this as is. I
> would say that this makes the implementation easier for the callee,
> since it is the responsibility of the caller to free up the memory.

Hmm, ... I'm not convinced yet. Please give me the last chance to argue.
I think the pointer is the pointer, that is not the resource itself.

Please see `man asprintf` for example.
Usually, if the function, which allocates any resource including
memory, returns an error, the resource pointer is undefined.
Of course, there is no need to unallocate the memory for
undefined address.
That is I think the standard way to handle the resource
allocation.

And actually, the callee implementation isn't simplified. In my
case, it forces me to re-initialize the pointer to NULL if an error
occurs. additional 1-line is needed :-) (maybe I need more
comment lines to explain why this NULL setting is required)

BTW, I attached a patch to change this code. You can see
the gpt_get_mdata() is refined into 2 gpt_get_mdata_part()
and many gotos are removed.
This shows how both of callee and caller can be simplified
with the convention which I suggested.

Thank you,

-- 
Masami Hiramatsu
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-FWU-Free-metadata-copy-if-gpt_get_mdata-failed.patch
Type: text/x-patch
Size: 5099 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20220208/f2e3f233/attachment.bin>