Re: [RFC PATCH v3 2/9] FWU: Add FWU metadata access functions for GPT partitioned block devices

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Mon, Jan 24, 2022 at 12:28:24PM +0530, Sughosh Ganu wrote:
> hi Masami,
> 
> On Thu, 20 Jan 2022 at 14:13, Masami Hiramatsu
> <masami.hiramatsu@linaro.org> wrote:
> >
> > Hi Sughosh,
> >
> > 2022年1月20日(木) 3:56 Sughosh Ganu <sughosh.ganu@linaro.org>:
> >
> > > +static int fwu_gpt_update_mdata(struct fwu_mdata *mdata)
> > > +{
> > > +       int ret;
> > > +       struct blk_desc *desc;
> > > +       u16 primary_mpart = 0, secondary_mpart = 0;
> > > +
> >
> > I think this update_mdata() method (or fwu_update_mdata() wrapper)
> > should always update mdata::crc32. calculate crc32 at each call site is
> > inefficient and easy to introduce bugs.
> 
> Okay. Will do.
> 
> >
> > > +       ret = fwu_plat_get_blk_desc(&desc);
> > > +       if (ret < 0) {
> > > +               log_err("Block device not found\n");
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       ret = gpt_get_mdata_partitions(desc, &primary_mpart,
> > > +                                         &secondary_mpart);
> > > +
> > > +       if (ret < 0) {
> > > +               log_err("Error getting the FWU metadata partitions\n");
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       /* First write the primary partition*/
> > > +       ret = gpt_write_mdata_partition(desc, mdata, primary_mpart);
> > > +       if (ret < 0) {
> > > +               log_err("Updating primary FWU metadata partition failed\n");
> > > +               return ret;
> > > +       }
> > > +
> > > +       /* And now the replica */
> > > +       ret = gpt_write_mdata_partition(desc, mdata, secondary_mpart);
> > > +       if (ret < 0) {
> > > +               log_err("Updating secondary FWU metadata partition failed\n");
> > > +               return ret;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int gpt_get_mdata(struct fwu_mdata **mdata)
> > > +{
> > > +       int ret;
> > > +       struct blk_desc *desc;
> > > +       u16 primary_mpart = 0, secondary_mpart = 0;
> > > +
> > > +       ret = fwu_plat_get_blk_desc(&desc);
> > > +       if (ret < 0) {
> > > +               log_err("Block device not found\n");
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       ret = gpt_get_mdata_partitions(desc, &primary_mpart,
> > > +                                      &secondary_mpart);
> > > +
> > > +       if (ret < 0) {
> > > +               log_err("Error getting the FWU metadata partitions\n");
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       *mdata = malloc(sizeof(struct fwu_mdata));
> > > +       if (!*mdata) {
> > > +               log_err("Unable to allocate memory for reading FWU metadata\n");
> > > +               return -ENOMEM;
> > > +       }
> > > +
> > > +       ret = gpt_read_mdata(desc, *mdata, primary_mpart);
> > > +       if (ret < 0) {
> > > +               log_err("Failed to read the FWU metadata from the device\n");
> >
> > Also, please release mdata inside the gpt_get_mdata() itself.
> >
> > I think it is not a good design to ask caller to free mdata if get_mdata()
> > operation is failed because mdata may or may not allocated in error case.
> >
> > In success case, user must free it because it is allocated (user accessed it),
> > and in error case, user can ignore it because it should not be allocated.
> > This is simpler mind model and less memory leak chance.
> 
> I think this is confusing. It would be better to be consistent and
> have the caller free up the allocated/not allocated memory. If you
> check, the mdata pointer is being initialised to NULL in every
> instance. Calling free with a NULL pointer is a valid case, which the
> free call handles. There are multiple places in u-boot where the
> caller is freeing up the allocated memory. So i think this should not
> be an issue.

The simple rule should be that, if the function returns 0 (successfully),
the area will be allocated. If not (i.e. returns an error), the area
will not be allocated.

This will be a much more common behavior.

-Takahiro Akashi

> -sughosh
> 
> >
> > Thank you,
> > --
> > Masami Hiramatsu