[PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used

[PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used

[Justin P. Mattock]

Resend version 3

The below patch fixes a warning message generated by GCC:
  CC [M]  drivers/net/wireless/hostap/hostap_ioctl.o
drivers/net/wireless/hostap/hostap_ioctl.c: In function 'prism2_request_scan':
drivers/net/wireless/hostap/hostap_ioctl.c:1666:6: warning: variable 'ret' set but not used

Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>

---
 drivers/net/wireless/hostap/hostap_ioctl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/hostap/hostap_ioctl.c b/drivers/net/wireless/hostap/hostap_ioctl.c
index a85e43a..2cf6532 100644
--- a/drivers/net/wireless/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/hostap/hostap_ioctl.c
@@ -1689,7 +1689,7 @@ static int prism2_request_scan(struct net_device *dev)
 	if (local->func->set_rid(dev, HFA384X_RID_SCANREQUEST, &scan_req,
 				 sizeof(scan_req))) {
 		printk(KERN_DEBUG "SCANREQUEST failed\n");
-		ret = -EINVAL;
+		return ret;
 	}
 
 	if (!local->host_roaming)
-- 
1.7.1.rc1.21.gf3bd6

Re: [PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used

[Julian Calaby]

[-- Attachment #1: Type: text/plain, Size: 1581 bytes --]

On Tue, Aug 3, 2010 at 11:46, Justin P. Mattock <justinmattock@gmail.com> wrote:
> Resend version 3
>
> The below patch fixes a warning message generated by GCC:
>  CC [M]  drivers/net/wireless/hostap/hostap_ioctl.o
> drivers/net/wireless/hostap/hostap_ioctl.c: In function 'prism2_request_scan':
> drivers/net/wireless/hostap/hostap_ioctl.c:1666:6: warning: variable 'ret' set but not used
>
> Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
>
> ---
>  drivers/net/wireless/hostap/hostap_ioctl.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/hostap/hostap_ioctl.c b/drivers/net/wireless/hostap/hostap_ioctl.c
> index a85e43a..2cf6532 100644
> --- a/drivers/net/wireless/hostap/hostap_ioctl.c
> +++ b/drivers/net/wireless/hostap/hostap_ioctl.c
> @@ -1689,7 +1689,7 @@ static int prism2_request_scan(struct net_device *dev)
>        if (local->func->set_rid(dev, HFA384X_RID_SCANREQUEST, &scan_req,
>                                 sizeof(scan_req))) {
>                printk(KERN_DEBUG "SCANREQUEST failed\n");
> -               ret = -EINVAL;
> +               return ret;
>        }
>
>        if (!local->host_roaming)

NACK: This doesn't actually fix the error mentioned.

Attached is a more correct patch.

Apologies for the attachment, I'm building the patch by hand at work
and only have gMail to send it with.

This should apply to wireless-testing.

Thanks,

-- 

Julian Calaby

Email: julian.calaby@gmail.com
.Plan: http://sites.google.com/site/juliancalaby/

[-- Attachment #2: fix-ret-not-used-hostap.diff --]
[-- Type: text/x-patch, Size: 633 bytes --]

Hostap: Fix "'ret' set but not used" warning message from GCC in hostap

 CC [M]  drivers/net/wireless/hostap/hostap_ioctl.o
drivers/net/wireless/hostap/hostap_ioctl.c: In function 'prism2_request_scan':
drivers/net/wireless/hostap/hostap_ioctl.c:1666:6: warning: variable 'ret' set but not used

Signed-off-by: Julian Calaby <julian.calaby@gmail.com>

---

--- a/drivers/net/wireless/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/hostap/hostap_ioctl.c
@@ -1696,7 +1696,7 @@
 		hostap_set_word(dev, HFA384X_RID_CNFROAMINGMODE,
 				HFA384X_ROAMING_FIRMWARE);
 
-	return 0;
+	return ret;
 }
 
 #else /* !PRISM2_NO_STATION_MODES */

Re: [PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used

[Justin P. Mattock]

On 08/02/2010 06:58 PM, Julian Calaby wrote:
> On Tue, Aug 3, 2010 at 11:46, Justin P. Mattock<justinmattock@gmail.com>  wrote:
>> Resend version 3
>>
>> The below patch fixes a warning message generated by GCC:
>>   CC [M]  drivers/net/wireless/hostap/hostap_ioctl.o
>> drivers/net/wireless/hostap/hostap_ioctl.c: In function 'prism2_request_scan':
>> drivers/net/wireless/hostap/hostap_ioctl.c:1666:6: warning: variable 'ret' set but not used
>>
>> Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>
>>
>> ---
>>   drivers/net/wireless/hostap/hostap_ioctl.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/wireless/hostap/hostap_ioctl.c b/drivers/net/wireless/hostap/hostap_ioctl.c
>> index a85e43a..2cf6532 100644
>> --- a/drivers/net/wireless/hostap/hostap_ioctl.c
>> +++ b/drivers/net/wireless/hostap/hostap_ioctl.c
>> @@ -1689,7 +1689,7 @@ static int prism2_request_scan(struct net_device *dev)
>>         if (local->func->set_rid(dev, HFA384X_RID_SCANREQUEST,&scan_req,
>>                                  sizeof(scan_req))) {
>>                 printk(KERN_DEBUG "SCANREQUEST failed\n");
>> -               ret = -EINVAL;
>> +               return ret;
>>         }
>>
>>         if (!local->host_roaming)
>
> NACK: This doesn't actually fix the error mentioned.
>
> Attached is a more correct patch.
>
> Apologies for the attachment, I'm building the patch by hand at work
> and only have gMail to send it with.
>
> This should apply to wireless-testing.
>
> Thanks,
>

wait im lost.. looking at your patch looks the same(number wise) except 
I see different parts of code in there:

hostap_set_word(dev, HFA384X_RID_CNFROAMINGMODE,
  				HFA384X_ROAMING_FIRMWARE);

-	return 0;
+	return ret;



mine is:

printk(KERN_DEBUG "SCANREQUEST failed\n");
-		ret = -EINVAL;
+		return ret;


The tree Im using is Linus's tree.

any ideas?

Justin P. Mattock

Re: [PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used

[Julian Calaby]

On Tue, Aug 3, 2010 at 12:23, Justin P. Mattock <justinmattock@gmail.com> wrote:
> wait im lost.. looking at your patch looks the same(number wise) except I
> see different parts of code in there:
>
> hostap_set_word(dev, HFA384X_RID_CNFROAMINGMODE,
>                                HFA384X_ROAMING_FIRMWARE);
>
> -       return 0;
> +       return ret;
>
>
>
> mine is:
>
> printk(KERN_DEBUG "SCANREQUEST failed\n");
> -               ret = -EINVAL;
> +               return ret;
>
>
> The tree Im using is Linus's tree.

I'm using wireless-testing, and the line you're changing is #1692, I'm
changing #1699.

Thanks,

-- 

Julian Calaby

Email: julian.calaby@gmail.com
.Plan: http://sites.google.com/site/juliancalaby/

Re: [PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used

[Justin P. Mattock]

On 08/02/2010 07:31 PM, Julian Calaby wrote:
> On Tue, Aug 3, 2010 at 12:23, Justin P. Mattock<justinmattock@gmail.com>  wrote:
>> wait im lost.. looking at your patch looks the same(number wise) except I
>> see different parts of code in there:
>>
>> hostap_set_word(dev, HFA384X_RID_CNFROAMINGMODE,
>>                                 HFA384X_ROAMING_FIRMWARE);
>>
>> -       return 0;
>> +       return ret;
>>
>>
>>
>> mine is:
>>
>> printk(KERN_DEBUG "SCANREQUEST failed\n");
>> -               ret = -EINVAL;
>> +               return ret;
>>
>>
>> The tree Im using is Linus's tree.
>
> I'm using wireless-testing, and the line you're changing is #1692, I'm
> changing #1699.
>
> Thanks,
>


ahh.. so there's two return ret's that need to be added then..
(if so then go ahead and take my patch and add it to yours.. I'm off to 
other things)

cheers,

Justin P. Mattock

Re: [PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used

[Julian Calaby]

On Tue, Aug 3, 2010 at 12:49, Justin P. Mattock <justinmattock@gmail.com> wrote:
> On 08/02/2010 07:31 PM, Julian Calaby wrote:
>>
>> On Tue, Aug 3, 2010 at 12:23, Justin P. Mattock<justinmattock@gmail.com>
>>  wrote:
>>>
>>> wait im lost.. looking at your patch looks the same(number wise) except I
>>> see different parts of code in there:
>>>
>>> hostap_set_word(dev, HFA384X_RID_CNFROAMINGMODE,
>>>                                HFA384X_ROAMING_FIRMWARE);
>>>
>>> -       return 0;
>>> +       return ret;
>>>
>>>
>>>
>>> mine is:
>>>
>>> printk(KERN_DEBUG "SCANREQUEST failed\n");
>>> -               ret = -EINVAL;
>>> +               return ret;
>>>
>>>
>>> The tree Im using is Linus's tree.
>>
>> I'm using wireless-testing, and the line you're changing is #1692, I'm
>> changing #1699.
>>
>> Thanks,
>>
>
>
> ahh.. so there's two return ret's that need to be added then..
> (if so then go ahead and take my patch and add it to yours.. I'm off to
> other things)

No.

The function works like this:

1. set up scan_req
2. if local->host_roaming is not set, set host roaming on the hardware
3. set scan_req on the hardware
4. if local->host_roaming is not set, set the hardware to do firmware roaming
5. If set_req could not be set on the hardware, return -EINVAL,
otherwise return 0.

The significant bits are steps 2 and 4. These must *both* be executed,
regardless of whether setting scan_req on the hardware succeeds or
not.

As such, the return value is stored in the ret variable, which is
initialised to 0 initially, then set to -EINVAL if setting scan_req on
the hardware fails. Hence, step 5 can be performed by returning ret.

As such, all of your patches are wrong as they are either returning
the wrong value when setting scan_req on the hardware fails, or
failing to execute step 4. The *only* correct place to return a value
is once the roaming status of the hardware has been set properly -
i.e. at the end of the function.

I hope this has cleared up your confusion.

Thanks,

-- 

Julian Calaby

Email: julian.calaby@gmail.com
.Plan: http://sites.google.com/site/juliancalaby/

Re: [PATCH v3]hostap:hostap_ioctl.c Fix variable 'ret' set but not used

[Justin P. Mattock]

On 08/02/2010 08:31 PM, Julian Calaby wrote:
> On Tue, Aug 3, 2010 at 12:49, Justin P. Mattock<justinmattock@gmail.com>  wrote:
>> On 08/02/2010 07:31 PM, Julian Calaby wrote:
>>>
>>> On Tue, Aug 3, 2010 at 12:23, Justin P. Mattock<justinmattock@gmail.com>
>>>   wrote:
>>>>
>>>> wait im lost.. looking at your patch looks the same(number wise) except I
>>>> see different parts of code in there:
>>>>
>>>> hostap_set_word(dev, HFA384X_RID_CNFROAMINGMODE,
>>>>                                 HFA384X_ROAMING_FIRMWARE);
>>>>
>>>> -       return 0;
>>>> +       return ret;
>>>>
>>>>
>>>>
>>>> mine is:
>>>>
>>>> printk(KERN_DEBUG "SCANREQUEST failed\n");
>>>> -               ret = -EINVAL;
>>>> +               return ret;
>>>>
>>>>
>>>> The tree Im using is Linus's tree.
>>>
>>> I'm using wireless-testing, and the line you're changing is #1692, I'm
>>> changing #1699.
>>>
>>> Thanks,
>>>
>>
>>
>> ahh.. so there's two return ret's that need to be added then..
>> (if so then go ahead and take my patch and add it to yours.. I'm off to
>> other things)
>
> No.
>
> The function works like this:
>
> 1. set up scan_req
> 2. if local->host_roaming is not set, set host roaming on the hardware
> 3. set scan_req on the hardware
> 4. if local->host_roaming is not set, set the hardware to do firmware roaming
> 5. If set_req could not be set on the hardware, return -EINVAL,
> otherwise return 0.
>
> The significant bits are steps 2 and 4. These must *both* be executed,
> regardless of whether setting scan_req on the hardware succeeds or
> not.
>
> As such, the return value is stored in the ret variable, which is
> initialised to 0 initially, then set to -EINVAL if setting scan_req on
> the hardware fails. Hence, step 5 can be performed by returning ret.
>
> As such, all of your patches are wrong as they are either returning
> the wrong value when setting scan_req on the hardware fails, or
> failing to execute step 4. The *only* correct place to return a value
> is once the roaming status of the hardware has been set properly -
> i.e. at the end of the function.
>
> I hope this has cleared up your confusion.
>
> Thanks,
>


cleared it up perfectly... didnt think that using ret down more would 
work..(was stuck a few lines up)  but it does.. just tested your version 
out and no warning messages..

cheers,

Justin P. Mattock