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

[Masami Hiramatsu]

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.

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

Thank you,
-- 
Masami Hiramatsu