Re: [PATCH 3/3] staging: wilc1000: Reduce scope for a few variables in mac_ioctl()

Re: [PATCH 3/3] staging: wilc1000: Reduce scope for a few variables in mac_ioctl()

[Lino Sanfilippo]

>  
> -			if (strncasecmp(buff, "RSSI", length) == 0) {
> +			if (strncasecmp(buff, "RSSI", 0) == 0) {
> +				s8 rssi;
> +

Um, please think a second about if it makes any sense at all to compare 
zero chars of two strings.

Lino

Re: [PATCH 3/3] staging: wilc1000: Reduce scope for a few variables in mac_ioctl()

[walter harms]

Am 25.07.2016 21:17, schrieb Lino Sanfilippo:
>>  
>> -			if (strncasecmp(buff, "RSSI", length) == 0) {
>> +			if (strncasecmp(buff, "RSSI", 0) == 0) {
>> +				s8 rssi;
>> +
> 
> Um, please think a second about if it makes any sense at all to compare 
> zero chars of two strings.
> 
> Lino

also:
the switch has only one case and default.

re,
 wh

Re: staging: wilc1000: Reduce scope for a few variables in mac_ioctl()

[SF Markus Elfring]

>> -			if (strncasecmp(buff, "RSSI", length) == 0) {
>> +			if (strncasecmp(buff, "RSSI", 0) == 0) {
>> +				s8 rssi;
>> +
> 
> Um, please think a second about if it makes any sense at all to compare 
> zero chars of two strings.

Under which circumstances should the variable "length" contain an other
value than zero?

How can this open issue be fixed better?

Regards,
Markus

Aw: Re: staging: wilc1000: Reduce scope for a few variables in mac_ioctl()

[Lino Sanfilippo]

> Gesendet: Dienstag, 26. Juli 2016 um 08:25 Uhr
> Von: "SF Markus Elfring" <elfring@users.sourceforge.net>

>
> >> -			if (strncasecmp(buff, "RSSI", length) == 0) {
> >> +			if (strncasecmp(buff, "RSSI", 0) == 0) {
> >> +				s8 rssi;
> >> +
> > 
> > Um, please think a second about if it makes any sense at all to compare 
> > zero chars of two strings.
> 
> Under which circumstances should the variable "length" contain an other
> value than zero?

Which circumstances do "not any sense at all" imply? 

> 
> How can this open issue be fixed better?

The code is not too complicated and I think it is very obvious which value/variable
should be passed instead of 0. I suggest to fix this since it is indeed a bug, instead of 
doing "micro optimizations" - which is the last thing that code in the staging area 
needs (as IIRC you have already been told by others, including the staging maintainer).

Regards,
Lino

Re: staging: wilc1000: Reduce scope for a few variables in mac_ioctl()

[SF Markus Elfring]

> Which circumstances do "not any sense at all" imply? 

Should the expression 'strlen("RSSI")' be passed for the parameter 'length' instead?


> I suggest to fix this since it is indeed a bug,

We can agree that this function implementation was broken for a while there.


> instead of doing "micro optimizations" - which is the last thing that code in the staging area 
> needs (as IIRC you have already been told by others, including the staging maintainer).

The acceptance might grow a bit more for such software fine-tuning
(like refactoring around variable usage).

Regards,
Markus

[PATCH 0/2] staging: wilc1000: Deletion of two unnecessary checks

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>

Further update suggestions were taken into account after a patch was applied
from static source code analysis.

Markus Elfring (2):
  Delete unnecessary checks before two function calls
  One function call less in mac_ioctl() after error detection

 drivers/staging/wilc1000/linux_wlan.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

-- 
2.4.4

[PATCH 0/3] staging: wilc1000: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>

Further update suggestions were taken into account
after a patch was applied from static source code analysis.

Markus Elfring (3):
  Delete an unnecessary check before the function call "release_firmware"
  One function call less in mac_ioctl() after error detection
  Reduce scope for a few variables in mac_ioctl()

 drivers/staging/wilc1000/linux_wlan.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

-- 
2.9.2

[PATCH 3/3] staging: wilc1000: Reduce scope for a few variables in mac_ioctl()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 24 Jul 2016 21:45:37 +0200

Three local variables were used only within a single case branch.

* Thus move the data type definition for "rssi" and "size" into the
  corresponding code block.

* The variable "length" was not modified after its initialisation.
  Thus pass a constant value in the affected function call instead.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/wilc1000/linux_wlan.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 7b1ebcc..173be16 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -1094,8 +1094,6 @@ int wilc_mac_close(struct net_device *ndev)
 static int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
 {
 	u8 *buff = NULL;
-	s8 rssi;
-	u32 size = 0, length = 0;
 	struct wilc_vif *vif;
 	s32 ret = 0;
 	struct wilc *wilc;
@@ -1110,8 +1108,7 @@ static int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
 	case SIOCSIWPRIV:
 	{
 		struct iwreq *wrq = (struct iwreq *)req;
-
-		size = wrq->u.data.length;
+		u32 size = wrq->u.data.length;
 
 		if (size && wrq->u.data.pointer) {
 			buff = memdup_user(wrq->u.data.pointer,
@@ -1119,7 +1116,9 @@ static int mac_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
 			if (IS_ERR(buff))
 				return PTR_ERR(buff);
 
-			if (strncasecmp(buff, "RSSI", length) == 0) {
+			if (strncasecmp(buff, "RSSI", 0) == 0) {
+				s8 rssi;
+
 				ret = wilc_get_rssi(vif, &rssi);
 				netdev_info(ndev, "RSSI :%d\n", rssi);
 
-- 
2.9.2