Hi Sughosh,
2022年1月24日(月) 15:58 Sughosh Ganu sughosh.ganu@linaro.org:
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.
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,