[PATCH] scsi: bfa: bfad_attr.c: Optimization of the Code

[PATCH] scsi: bfa: bfad_attr.c: Optimization of the Code

[Rickard Strandqvist]

Minimized the use of snprintf()
And removed a variable that was only used for the temporary storage.

This was partly found using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/scsi/bfa/bfad_attr.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/bfa/bfad_attr.c b/drivers/scsi/bfa/bfad_attr.c
index 40be670..5f5917a 100644
--- a/drivers/scsi/bfa/bfad_attr.c
+++ b/drivers/scsi/bfa/bfad_attr.c
@@ -839,12 +839,10 @@ bfad_im_symbolic_name_show(struct device *dev, struct device_attribute *attr,
 			(struct bfad_im_port_s *) shost->hostdata[0];
 	struct bfad_s *bfad = im_port->bfad;
 	struct bfa_lport_attr_s port_attr;
-	char symname[BFA_SYMNAME_MAXLEN];
 
 	bfa_fcs_lport_get_attr(&bfad->bfa_fcs.fabric.bport, &port_attr);
-	strncpy(symname, port_attr.port_cfg.sym_name.symname,
-			BFA_SYMNAME_MAXLEN);
-	return snprintf(buf, PAGE_SIZE, "%s\n", symname);
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			port_attr.port_cfg.sym_name.symname);
 }
 
 static ssize_t
@@ -865,7 +863,9 @@ static ssize_t
 bfad_im_drv_version_show(struct device *dev, struct device_attribute *attr,
 				char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%s\n", BFAD_DRIVER_VERSION);
+	strncpy(buf, BFAD_DRIVER_VERSION  "\n" , PAGE_SIZE);
+	buf[PAGE_SIZE - 1] = '\0';
+	return strlen(buf);
 }
 
 static ssize_t
@@ -913,7 +913,9 @@ static ssize_t
 bfad_im_drv_name_show(struct device *dev, struct device_attribute *attr,
 				char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%s\n", BFAD_DRIVER_NAME);
+	strncpy(buf, BFAD_DRIVER_NAME  "\n" , PAGE_SIZE);
+	buf[PAGE_SIZE - 1] = '\0';
+	return strlen(buf);
 }
 
 static ssize_t
-- 
1.7.10.4

Re: [PATCH] scsi: bfa: bfad_attr.c: Optimization of the Code

[Bart Van Assche]

On 06/04/14 20:08, Rickard Strandqvist wrote:
> Minimized the use of snprintf()
> And removed a variable that was only used for the temporary storage.
> 
> This was partly found using a static code analysis program called cppcheck.
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  drivers/scsi/bfa/bfad_attr.c |   14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/bfa/bfad_attr.c b/drivers/scsi/bfa/bfad_attr.c
> index 40be670..5f5917a 100644
> --- a/drivers/scsi/bfa/bfad_attr.c
> +++ b/drivers/scsi/bfa/bfad_attr.c
> @@ -839,12 +839,10 @@ bfad_im_symbolic_name_show(struct device *dev, struct device_attribute *attr,
>  			(struct bfad_im_port_s *) shost->hostdata[0];
>  	struct bfad_s *bfad = im_port->bfad;
>  	struct bfa_lport_attr_s port_attr;
> -	char symname[BFA_SYMNAME_MAXLEN];
>  
>  	bfa_fcs_lport_get_attr(&bfad->bfa_fcs.fabric.bport, &port_attr);
> -	strncpy(symname, port_attr.port_cfg.sym_name.symname,
> -			BFA_SYMNAME_MAXLEN);
> -	return snprintf(buf, PAGE_SIZE, "%s\n", symname);
> +	return snprintf(buf, PAGE_SIZE, "%s\n",
> +			port_attr.port_cfg.sym_name.symname);
>  }
>  
>  static ssize_t
> @@ -865,7 +863,9 @@ static ssize_t
>  bfad_im_drv_version_show(struct device *dev, struct device_attribute *attr,
>  				char *buf)
>  {
> -	return snprintf(buf, PAGE_SIZE, "%s\n", BFAD_DRIVER_VERSION);
> +	strncpy(buf, BFAD_DRIVER_VERSION  "\n" , PAGE_SIZE);
> +	buf[PAGE_SIZE - 1] = '\0';
> +	return strlen(buf);
>  }
>  
>  static ssize_t
> @@ -913,7 +913,9 @@ static ssize_t
>  bfad_im_drv_name_show(struct device *dev, struct device_attribute *attr,
>  				char *buf)
>  {
> -	return snprintf(buf, PAGE_SIZE, "%s\n", BFAD_DRIVER_NAME);
> +	strncpy(buf, BFAD_DRIVER_NAME  "\n" , PAGE_SIZE);
> +	buf[PAGE_SIZE - 1] = '\0';
> +	return strlen(buf);
>  }

This is ugly. Please use sprintf(buf, "%.*s\n", PAGE_SIZE - 1, str)
instead of strncpy() + strlen().

Bart.

Re: [PATCH] scsi: bfa: bfad_attr.c: Optimization of the Code

[Bart Van Assche]

On 06/05/14 08:55, Bart Van Assche wrote:
> On 06/04/14 20:08, Rickard Strandqvist wrote:
> This is ugly. Please use sprintf(buf, "%.*s\n", PAGE_SIZE - 1, str)
> instead of strncpy() + strlen().

(replying to my own e-mail)

The above should of course have read "sprintf(buf, "%.*s\n", PAGE_SIZE -
2, str)" to avoid that the terminating '\0' triggers a buffer overflow.

Bart.

Re: [PATCH] scsi: bfa: bfad_attr.c: Optimization of the Code

[Rickard Strandqvist]

Hi

Several have remarked in the other patch for strncpy I posted. That
there is a strlcpy that works exactly as one would like to strncpy was
done :)
And the return value is like for snprintf, but quite a lot faster!

So I submit patches based with it instead, and did a couple more
exchanges of snprintf to strlcpy.



Best regards
Rickard Strandqvist


2014-06-05 9:10 GMT+02:00 Bart Van Assche <bvanassche@acm.org>:
> On 06/05/14 08:55, Bart Van Assche wrote:
>> On 06/04/14 20:08, Rickard Strandqvist wrote:
>> This is ugly. Please use sprintf(buf, "%.*s\n", PAGE_SIZE - 1, str)
>> instead of strncpy() + strlen().
>
> (replying to my own e-mail)
>
> The above should of course have read "sprintf(buf, "%.*s\n", PAGE_SIZE -
> 2, str)" to avoid that the terminating '\0' triggers a buffer overflow.
>
> Bart.

[PATCH] scsi: bfa: bfad_attr.c: Optimization of the code

[Rickard Strandqvist]

Minimized the use of snprintf()
And removed a variable that was only used for the temporary storage.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/scsi/bfa/bfad_attr.c |  114 ++++++++++++++++++++++++------------------
 1 file changed, 66 insertions(+), 48 deletions(-)

diff --git a/drivers/scsi/bfa/bfad_attr.c b/drivers/scsi/bfa/bfad_attr.c
index 40be670..3fe75ff 100644
--- a/drivers/scsi/bfa/bfad_attr.c
+++ b/drivers/scsi/bfa/bfad_attr.c
@@ -751,69 +751,89 @@ bfad_im_model_desc_show(struct device *dev, struct device_attribute *attr,
 	bfa_get_adapter_model(&bfad->bfa, model);
 	nports = bfa_get_nports(&bfad->bfa);
 	if (!strcmp(model, "Brocade-425"))
-		snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-			"Brocade 4Gbps PCIe dual port FC HBA");
+		strlcpy(model_descr,
+				"Brocade 4Gbps PCIe dual port FC HBA\n",
+				BFA_ADAPTER_MODEL_DESCR_LEN);
 	else if (!strcmp(model, "Brocade-825"))
-		snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-			"Brocade 8Gbps PCIe dual port FC HBA");
+		strlcpy(model_descr,
+				"Brocade 8Gbps PCIe dual port FC HBA\n",
+				BFA_ADAPTER_MODEL_DESCR_LEN);
 	else if (!strcmp(model, "Brocade-42B"))
-		snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-			"Brocade 4Gbps PCIe dual port FC HBA for HP");
+		strlcpy(model_descr,
+				"Brocade 4Gbps PCIe dual port FC HBA for HP\n",
+				BFA_ADAPTER_MODEL_DESCR_LEN);
 	else if (!strcmp(model, "Brocade-82B"))
-		snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-			"Brocade 8Gbps PCIe dual port FC HBA for HP");
+		strlcpy(model_descr,
+				"Brocade 8Gbps PCIe dual port FC HBA for HP\n",
+				BFA_ADAPTER_MODEL_DESCR_LEN);
 	else if (!strcmp(model, "Brocade-1010"))
-		snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-			"Brocade 10Gbps single port CNA");
+		strlcpy(model_descr,
+				"Brocade 10Gbps single port CNA\n",
+				BFA_ADAPTER_MODEL_DESCR_LEN);
 	else if (!strcmp(model, "Brocade-1020"))
-		snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-			"Brocade 10Gbps dual port CNA");
+		strlcpy(model_descr,
+				"Brocade 10Gbps dual port CNA\n",
+				BFA_ADAPTER_MODEL_DESCR_LEN);
 	else if (!strcmp(model, "Brocade-1007"))
-		snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-			"Brocade 10Gbps CNA for IBM Blade Center");
+		strlcpy(model_descr,
+				"Brocade 10Gbps CNA for IBM Blade Center\n",
+				BFA_ADAPTER_MODEL_DESCR_LEN);
 	else if (!strcmp(model, "Brocade-415"))
-		snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-			"Brocade 4Gbps PCIe single port FC HBA");
+		strlcpy(model_descr,
+				"Brocade 4Gbps PCIe single port FC HBA\n",
+				BFA_ADAPTER_MODEL_DESCR_LEN);
 	else if (!strcmp(model, "Brocade-815"))
-		snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-			"Brocade 8Gbps PCIe single port FC HBA");
+		strlcpy(model_descr,
+				"Brocade 8Gbps PCIe single port FC HBA\n",
+				BFA_ADAPTER_MODEL_DESCR_LEN);
 	else if (!strcmp(model, "Brocade-41B"))
-		snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-			"Brocade 4Gbps PCIe single port FC HBA for HP");
+		strlcpy(model_descr,
+				"Brocade 4Gbps PCIe single port FC HBA for HP\n",
+				BFA_ADAPTER_MODEL_DESCR_LEN);
 	else if (!strcmp(model, "Brocade-81B"))
-		snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-			"Brocade 8Gbps PCIe single port FC HBA for HP");
+		strlcpy(model_descr,
+				"Brocade 8Gbps PCIe single port FC HBA for HP\n",
+				BFA_ADAPTER_MODEL_DESCR_LEN);
 	else if (!strcmp(model, "Brocade-804"))
-		snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-			"Brocade 8Gbps FC HBA for HP Bladesystem C-class");
+		strlcpy(model_descr,
+				"Brocade 8Gbps FC HBA for HP Bladesystem C-class\n",
+				BFA_ADAPTER_MODEL_DESCR_LEN);
 	else if (!strcmp(model, "Brocade-1741"))
-		snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-			"Brocade 10Gbps CNA for Dell M-Series Blade Servers");
+		strlcpy(model_descr,
+				"Brocade 10Gbps CNA for Dell M-Series Blade Servers\n",
+				BFA_ADAPTER_MODEL_DESCR_LEN);
 	else if (strstr(model, "Brocade-1860")) {
 		if (nports == 1 && bfa_ioc_is_cna(&bfad->bfa.ioc))
-			snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-				"Brocade 10Gbps single port CNA");
+			strlcpy(model_descr,
+					"Brocade 10Gbps single port CNA\n",
+					BFA_ADAPTER_MODEL_DESCR_LEN);
 		else if (nports == 1 && !bfa_ioc_is_cna(&bfad->bfa.ioc))
-			snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-				"Brocade 16Gbps PCIe single port FC HBA");
+			strlcpy(model_descr,
+					"Brocade 16Gbps PCIe single port FC HBA\n",
+					BFA_ADAPTER_MODEL_DESCR_LEN);
 		else if (nports == 2 && bfa_ioc_is_cna(&bfad->bfa.ioc))
-			snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-				"Brocade 10Gbps dual port CNA");
+			strlcpy(model_descr,
+					"Brocade 10Gbps dual port CNA\n",
+					BFA_ADAPTER_MODEL_DESCR_LEN);
 		else if (nports == 2 && !bfa_ioc_is_cna(&bfad->bfa.ioc))
-			snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-				"Brocade 16Gbps PCIe dual port FC HBA");
+			strlcpy(model_descr,
+					"Brocade 16Gbps PCIe dual port FC HBA\n",
+					BFA_ADAPTER_MODEL_DESCR_LEN);
 	} else if (!strcmp(model, "Brocade-1867")) {
 		if (nports == 1 && !bfa_ioc_is_cna(&bfad->bfa.ioc))
-			snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-				"Brocade 16Gbps PCIe single port FC HBA for IBM");
+			strlcpy(model_descr,
+					"Brocade 16Gbps PCIe single port FC HBA for IBM\n",
+					BFA_ADAPTER_MODEL_DESCR_LEN);
 		else if (nports == 2 && !bfa_ioc_is_cna(&bfad->bfa.ioc))
-			snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-				"Brocade 16Gbps PCIe dual port FC HBA for IBM");
+			strlcpy(model_descr,
+					"Brocade 16Gbps PCIe dual port FC HBA for IBM\n",
+					BFA_ADAPTER_MODEL_DESCR_LEN);
 	} else
-		snprintf(model_descr, BFA_ADAPTER_MODEL_DESCR_LEN,
-			"Invalid Model");
+		strlcpy(model_descr,
+				"Invalid Model\n",
+				BFA_ADAPTER_MODEL_DESCR_LEN);
 
-	return snprintf(buf, PAGE_SIZE, "%s\n", model_descr);
+	return strlcpy(buf, model_descr, PAGE_SIZE);
 }
 
 static ssize_t
@@ -839,12 +859,10 @@ bfad_im_symbolic_name_show(struct device *dev, struct device_attribute *attr,
 			(struct bfad_im_port_s *) shost->hostdata[0];
 	struct bfad_s *bfad = im_port->bfad;
 	struct bfa_lport_attr_s port_attr;
-	char symname[BFA_SYMNAME_MAXLEN];
 
 	bfa_fcs_lport_get_attr(&bfad->bfa_fcs.fabric.bport, &port_attr);
-	strncpy(symname, port_attr.port_cfg.sym_name.symname,
-			BFA_SYMNAME_MAXLEN);
-	return snprintf(buf, PAGE_SIZE, "%s\n", symname);
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			port_attr.port_cfg.sym_name.symname);
 }
 
 static ssize_t
@@ -865,7 +883,7 @@ static ssize_t
 bfad_im_drv_version_show(struct device *dev, struct device_attribute *attr,
 				char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%s\n", BFAD_DRIVER_VERSION);
+	return strlcpy(buf, BFAD_DRIVER_VERSION  "\n", PAGE_SIZE);
 }
 
 static ssize_t
@@ -913,7 +931,7 @@ static ssize_t
 bfad_im_drv_name_show(struct device *dev, struct device_attribute *attr,
 				char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%s\n", BFAD_DRIVER_NAME);
+	return strlcpy(buf, BFAD_DRIVER_NAME  "\n", PAGE_SIZE);
 }
 
 static ssize_t
@@ -932,7 +950,7 @@ bfad_im_num_of_discovered_ports_show(struct device *dev,
 	rports = kzalloc(sizeof(struct bfa_rport_qualifier_s) * nrports,
 			 GFP_ATOMIC);
 	if (rports == NULL)
-		return snprintf(buf, PAGE_SIZE, "Failed\n");
+		return strlcpy(buf, "Failed\n", PAGE_SIZE);
 
 	spin_lock_irqsave(&bfad->bfad_lock, flags);
 	bfa_fcs_lport_get_rport_quals(port->fcs_port, rports, &nrports);
-- 
1.7.10.4