Re: [PATCH v2] scsi: fcoe: add missed kfree() in an error path

From: "Ewan D. Milne" <emilne@redhat.com>
To: Jing Xiangfeng <jingxiangfeng@huawei.com>,
	hare@suse.de, jejb@linux.ibm.com, martin.petersen@oracle.com,
	robert.w.love@intel.com, Neerav.Parikh@intel.com,
	Markus.Elfring@web.de
Cc: linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] scsi: fcoe: add missed kfree() in an error path
Date: Mon, 13 Jul 2020 13:53:37 -0400	[thread overview]
Message-ID: <d1fa9e60b559b6bf3a37ef5a6aef2bd7bd6e1681.camel@redhat.com> (raw)
In-Reply-To: <20200709120546.38453-1-jingxiangfeng@huawei.com>

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 