Hi Sughosh,
2022年2月8日(火) 19:24 Sughosh Ganu <sughosh.g...@linaro.org>:
>
> hi Masami,
>
> On Tue, 8 Feb 2022 at 15:04, Masami Hiramatsu
> <masami.hirama...@linaro.org> wrote:
> >
> > Hi Sughosh,
> >
> > Thanks for updating the series. I have some comment on this patch.
> >
> > 2022年2月8日(火) 3:21 Sughosh Ganu <sughosh.g...@linaro.org>:
> > [snip]
> > > +
> > > +/**
> > > + * fwu_get_active_index() - Get active_index from the FWU metadata
> > > + * @active_idx: active_index value to be read
> > > + *
> > > + * Read the active_index field from the FWU metadata and place it in
> > > + * the variable pointed to be the function argument.
> > > + *
> > > + * Return: 0 if OK, -ve on error
> > > + *
> > > + */
> > > +int fwu_get_active_index(u32 *active_idx)
> > > +{
> > > + int ret;
> > > + struct fwu_mdata *mdata = NULL;
> > > +
> > > + ret = fwu_get_mdata(&mdata);
> > > + if (ret < 0) {
> > > + log_err("Unable to get valid FWU metadata\n");
> > > + goto out;
> >
> > Again, as we discussed previous series, please don't return unused
> > allocated memory if the function returns an error.
> > That is something like putting a burden on the callers. They always
> > needs to initialize the pointer before call and free it even if the
> > function is failed.
>
> I would like to keep the convention consistent. The function that
> declares the pointer will also be responsible for free'ing it up. I
> find the alternative to be more confusing, where in some instances the
> callee frees up the memory, while in some cases it is the caller that
> frees it up. If there is no stated convention in u-boot which forbids
> this style of memory handling, I would like to keep this as is. I
> would say that this makes the implementation easier for the callee,
> since it is the responsibility of the caller to free up the memory.
Hmm, ... I'm not convinced yet. Please give me the last chance to argue.
I think the pointer is the pointer, that is not the resource itself.
Please see `man asprintf` for example.
Usually, if the function, which allocates any resource including
memory, returns an error, the resource pointer is undefined.
Of course, there is no need to unallocate the memory for
undefined address.
That is I think the standard way to handle the resource
allocation.
And actually, the callee implementation isn't simplified. In my
case, it forces me to re-initialize the pointer to NULL if an error
occurs. additional 1-line is needed :-) (maybe I need more
comment lines to explain why this NULL setting is required)
BTW, I attached a patch to change this code. You can see
the gpt_get_mdata() is refined into 2 gpt_get_mdata_part()
and many gotos are removed.
This shows how both of callee and caller can be simplified
with the convention which I suggested.
Thank you,
--
Masami Hiramatsu
From 695b4276283654bcf0440889cf214b887174345d Mon Sep 17 00:00:00 2001
From: Masami Hiramatsu <masami.hirama...@linaro.org>
Date: Thu, 20 Jan 2022 15:41:49 +0900
Subject: [PATCH] FWU: Free metadata copy if gpt_get_mdata() failed
It is better if a function which returns an error then release
all allocated memory resources. This simplifies the mind model
and less chance to forgot to free memory and double free.
Signed-off-by: Masami Hiramatsu <masami.hirama...@linaro.org>
---
drivers/fwu-mdata/fwu-mdata-uclass.c | 24 +++++--------
drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 49 +++++++++++++++------------
2 files changed, 36 insertions(+), 37 deletions(-)
diff --git a/drivers/fwu-mdata/fwu-mdata-uclass.c b/drivers/fwu-mdata/fwu-mdata-uclass.c
index fad44b25d1..512ce157e9 100644
--- a/drivers/fwu-mdata/fwu-mdata-uclass.c
+++ b/drivers/fwu-mdata/fwu-mdata-uclass.c
@@ -79,12 +79,12 @@ int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part)
int fwu_get_active_index(u32 *active_idx)
{
int ret;
- struct fwu_mdata *mdata = NULL;
+ struct fwu_mdata *mdata;
ret = fwu_get_mdata(&mdata);
if (ret < 0) {
log_err("Unable to get valid FWU metadata\n");
- goto out;
+ return ret;
}
/*
@@ -96,8 +96,6 @@ int fwu_get_active_index(u32 *active_idx)
log_err("Active index value read is incorrect\n");
ret = -EINVAL;
}
-
-out:
free(mdata);
return ret;
@@ -115,17 +113,17 @@ out:
int fwu_update_active_index(u32 active_idx)
{
int ret;
- struct fwu_mdata *mdata = NULL;
+ struct fwu_mdata *mdata;
if (active_idx > CONFIG_FWU_NUM_BANKS - 1) {
log_err("Active index value to be updated is incorrect\n");
- return -1;
+ return -EINVAL;
}
ret = fwu_get_mdata(&mdata);
if (ret < 0) {
log_err("Unable to get valid FWU metadata\n");
- goto out;
+ return ret;
}
/*
@@ -144,8 +142,6 @@ int fwu_update_active_index(u32 active_idx)
log_err("Failed to update FWU metadata partitions\n");
ret = -EIO;
}
-
-out:
free(mdata);
return ret;
@@ -225,12 +221,12 @@ int fwu_revert_boot_index(void)
{
int ret;
u32 cur_active_index;
- struct fwu_mdata *mdata = NULL;
+ struct fwu_mdata *mdata;
ret = fwu_get_mdata(&mdata);
if (ret < 0) {
log_err("Unable to get valid FWU metadata\n");
- goto out;
+ return ret;
}
/*
@@ -250,8 +246,6 @@ int fwu_revert_boot_index(void)
log_err("Failed to update FWU metadata partitions\n");
ret = -EIO;
}
-
-out:
free(mdata);
return ret;
@@ -277,14 +271,14 @@ static int fwu_set_clear_image_accept(efi_guid_t *img_type_id,
{
int ret, i;
u32 nimages;
- struct fwu_mdata *mdata = NULL;
+ struct fwu_mdata *mdata;
struct fwu_image_entry *img_entry;
struct fwu_image_bank_info *img_bank_info;
ret = fwu_get_mdata(&mdata);
if (ret < 0) {
log_err("Unable to get valid FWU metadata\n");
- goto out;
+ return ret;
}
nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK;
diff --git a/drivers/fwu-mdata/fwu_mdata_gpt_blk.c b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
index 9170c3f6af..a32195db2e 100644
--- a/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
+++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c
@@ -177,18 +177,9 @@ static int fwu_gpt_update_mdata(struct udevice * dev, struct fwu_mdata *mdata)
return 0;
}
-static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata **mdata)
+static int gpt_get_mdata_part(struct blk_desc *desc, struct fwu_mdata **mdata, u16 part)
{
int ret;
- u16 primary_mpart = 0, secondary_mpart = 0;
-
- 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) {
@@ -196,28 +187,42 @@ static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata **mdata)
return -ENOMEM;
}
- ret = gpt_read_mdata(desc, *mdata, primary_mpart);
+ ret = gpt_read_mdata(desc, *mdata, part);
if (ret < 0) {
log_err("Failed to read the FWU metadata from the device\n");
- return -EIO;
+ ret = -EIO;
+ } else {
+ ret = fwu_verify_mdata(*mdata, 1);
+ if (!ret)
+ return 0;
}
+ free(*mdata);
+
+ return ret;
+}
- ret = fwu_verify_mdata(*mdata, 1);
+static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata **mdata)
+{
+ int ret;
+ u16 primary_mpart = 0, secondary_mpart = 0;
+
+ ret = gpt_get_mdata_partitions(desc, &primary_mpart,
+ &secondary_mpart);
+
+ if (ret < 0) {
+ log_err("Error getting the FWU metadata partitions\n");
+ return -ENODEV;
+ }
+
+ ret = gpt_get_mdata_part(desc, mdata, primary_mpart);
if (!ret)
return 0;
/*
- * Verification of the primary FWU metadata copy failed.
+ * Reading of the primary FWU metadata copy failed.
* Try to read the replica.
*/
- memset(*mdata, 0, sizeof(struct fwu_mdata));
- ret = gpt_read_mdata(desc, *mdata, secondary_mpart);
- if (ret < 0) {
- log_err("Failed to read the FWU metadata from the device\n");
- return -EIO;
- }
-
- ret = fwu_verify_mdata(*mdata, 0);
+ ret = gpt_get_mdata_part(desc, mdata, secondary_mpart);
if (!ret)
return 0;
--
2.25.1
