| Message ID | 20200709120546.38453-1-jingxiangfeng@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] scsi: fcoe: add missed kfree() in an error path | expand |
See below. On Thu, 2020-07-09 at 20:05 +0800, Jing Xiangfeng wrote: > fcoe_fdmi_info() misses to call kfree() in an error path. > Add a label 'free_fdmi' and jump to it. > > Fixes: f07d46bbc9ba ("fcoe: Fix smatch warning in fcoe_fdmi_info > function") > Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com> > --- > drivers/scsi/fcoe/fcoe.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c > index 25dae9f0b205..a63057a03772 100644 > --- a/drivers/scsi/fcoe/fcoe.c > +++ b/drivers/scsi/fcoe/fcoe.c > @@ -830,7 +830,7 @@ static void fcoe_fdmi_info(struct fc_lport > *lport, struct net_device *netdev) > if (rc) { > printk(KERN_INFO "fcoe: Failed to retrieve FDMI > " > "information from netdev.\n"); > - return; > + goto free_fdmi; > } > > snprintf(fc_host_serial_number(lport->host), > @@ -868,6 +868,7 @@ static void fcoe_fdmi_info(struct fc_lport > *lport, struct net_device *netdev) > > /* Enable FDMI lport states */ > lport->fdmi_enabled = 1; > +free_fdmi: > kfree(fdmi); > } else { > lport->fdmi_enabled = 0; Normally I would like to see goto labels for error paths outside conditionals and at the end of the function. In this case it would seem to be cleaner to put an else { } clause in the if (rc) above around the snprintf() calls. -Ewan
On 2020/7/14 1:53, Ewan D. Milne wrote: > See below. > > On Thu, 2020-07-09 at 20:05 +0800, Jing Xiangfeng wrote: >> fcoe_fdmi_info() misses to call kfree() in an error path. >> Add a label 'free_fdmi' and jump to it. >> >> Fixes: f07d46bbc9ba ("fcoe: Fix smatch warning in fcoe_fdmi_info >> function") >> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com> >> --- >> drivers/scsi/fcoe/fcoe.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c >> index 25dae9f0b205..a63057a03772 100644 >> --- a/drivers/scsi/fcoe/fcoe.c >> +++ b/drivers/scsi/fcoe/fcoe.c >> @@ -830,7 +830,7 @@ static void fcoe_fdmi_info(struct fc_lport >> *lport, struct net_device *netdev) >> if (rc) { >> printk(KERN_INFO "fcoe: Failed to retrieve FDMI >> " >> "information from netdev.\n"); >> - return; >> + goto free_fdmi; >> } >> >> snprintf(fc_host_serial_number(lport->host), >> @@ -868,6 +868,7 @@ static void fcoe_fdmi_info(struct fc_lport >> *lport, struct net_device *netdev) >> >> /* Enable FDMI lport states */ >> lport->fdmi_enabled = 1; >> +free_fdmi: >> kfree(fdmi); >> } else { >> lport->fdmi_enabled = 0; > > Normally I would like to see goto labels for error paths outside > conditionals and at the end of the function. I agree. In this case it would > seem to be cleaner to put an else { } clause in the if (rc) above > around the snprintf() calls. It is ok with me. v1 is also a simpler way. +Hannes, Which is preferable? Thanks > > -Ewan > > . >
diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c index 25dae9f0b205..a63057a03772 100644 --- a/drivers/scsi/fcoe/fcoe.c +++ b/drivers/scsi/fcoe/fcoe.c @@ -830,7 +830,7 @@ static void fcoe_fdmi_info(struct fc_lport *lport, struct net_device *netdev) if (rc) { printk(KERN_INFO "fcoe: Failed to retrieve FDMI " "information from netdev.\n"); - return; + goto free_fdmi; } snprintf(fc_host_serial_number(lport->host), @@ -868,6 +868,7 @@ static void fcoe_fdmi_info(struct fc_lport *lport, struct net_device *netdev) /* Enable FDMI lport states */ lport->fdmi_enabled = 1; +free_fdmi: kfree(fdmi); } else { lport->fdmi_enabled = 0;
fcoe_fdmi_info() misses to call kfree() in an error path. Add a label 'free_fdmi' and jump to it. Fixes: f07d46bbc9ba ("fcoe: Fix smatch warning in fcoe_fdmi_info function") Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com> --- drivers/scsi/fcoe/fcoe.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)