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.hirama...@linaro.org> wrote: > > > > Hi Sughosh, > > > > 2022年1月20日(木) 3:56 Sughosh Ganu <sughosh.g...@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