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