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

[Masami Hiramatsu]

Hi Sughosh,

2022年1月24日(月) 15:58 Sughosh Ganu <sughosh.ganu at linaro.org>:
>
> hi Masami,
>
> On Thu, 20 Jan 2022 at 14:13, Masami Hiramatsu
> <masami.hiramatsu at linaro.org> wrote:
> >
> > Hi Sughosh,
> >
> > 2022年1月20日(木) 3:56 Sughosh Ganu <sughosh.ganu at 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.

Of course it is sane. What I was that is easier to introduce mistakes.

If it requires the caller to prepare mdata = NULL as an input,
it easily causes memory leak or other issues, because it seems
not an input but just an output parameter.

My concern is that this method is not a local one, but it is directly
exported via fwu_mdata_opts. That is a problem because this means
other developers can use it. And also, it forces the other backend
driver (like my fwu_mdata_sf.c) to do so.

Thank you,

-- 
Masami Hiramatsu