Patchwork Android: Fix bgscan-start / bgscan-stop call

login
register
mail settings
Submitter Dmitry Shmidt
Date Nov. 29, 2011, 1:07 a.m.
Message ID <20111129010938.E3CF93835E@ushik.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/128191/
State Accepted
Headers show

Comments

Dmitry Shmidt - Nov. 29, 2011, 1:07 a.m.
Signed-off-by: Dmitry Shmidt <dimitrysh@google.com>
---
 wpa_supplicant/ctrl_iface.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
Jouni Malinen - Nov. 29, 2011, 11:03 a.m.
On Mon, Nov 28, 2011 at 05:07:26PM -0800, Dmitry Shmidt wrote:
> diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c
>  	i = 0;
> +	ssid = wpa_s->conf->ssid;
>  	while (ssid) {

Thanks! Applied this part.

> @@ -3377,10 +3378,12 @@ static int wpa_supplicant_driver_cmd(struct wpa_supplicant *wpa_s, char *cmd,
>  
>  #ifdef ANDROID
>  	if (os_strcasecmp(cmd, "BGSCAN-START") == 0)
> -		return pno_start(wpa_s);
> +		ret = pno_start(wpa_s);
> +
> +	else if (os_strcasecmp(cmd, "BGSCAN-STOP") == 0)
> +		ret = pno_stop(wpa_s);
>  
> -	if (os_strcasecmp(cmd, "BGSCAN-STOP") == 0)
> -		return pno_stop(wpa_s);
> +	else
>  #endif /* ANDROID */
>  
>  	ret = wpa_drv_driver_cmd(wpa_s, cmd, buf, buflen);

This does not look correct. The reason for the special handlers for
BGSCAN-START/STOP here are for the explicit purpose of avoiding that
wpa_drv_driver_cmd() call and as such, those returns are there for
purpose. pno_start/stop end up calling
wpa_drv_sched_scan/wpa_drv_stop_sched_scan and the driver wrappers can
use those to implement whatever is needed for this. In the android
branch, driver_wext.c converts this to what driver_cmd() does in
wpa_supplicant_8.git (and the private wpa_supplicant_8_lib) and
driver_nl80211.c uses nl80211 sched_scan commands if the driver supports
them and if not, does what wpa_supplicant_8.git tree is doing with the
private ioctl.
Dmitry Shmidt - Nov. 29, 2011, 6:26 p.m.
On Tue, Nov 29, 2011 at 3:03 AM, Jouni Malinen <j@w1.fi> wrote:
> On Mon, Nov 28, 2011 at 05:07:26PM -0800, Dmitry Shmidt wrote:
>> diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c
>>       i = 0;
>> +     ssid = wpa_s->conf->ssid;
>>       while (ssid) {
>
> Thanks! Applied this part.
>
>> @@ -3377,10 +3378,12 @@ static int wpa_supplicant_driver_cmd(struct wpa_supplicant *wpa_s, char *cmd,
>>
>>  #ifdef ANDROID
>>       if (os_strcasecmp(cmd, "BGSCAN-START") == 0)
>> -             return pno_start(wpa_s);
>> +             ret = pno_start(wpa_s);
>> +
>> +     else if (os_strcasecmp(cmd, "BGSCAN-STOP") == 0)
>> +             ret = pno_stop(wpa_s);
>>
>> -     if (os_strcasecmp(cmd, "BGSCAN-STOP") == 0)
>> -             return pno_stop(wpa_s);
>> +     else
>>  #endif /* ANDROID */
>>
>>       ret = wpa_drv_driver_cmd(wpa_s, cmd, buf, buflen);
>
> This does not look correct. The reason for the special handlers for
> BGSCAN-START/STOP here are for the explicit purpose of avoiding that
> wpa_drv_driver_cmd() call and as such, those returns are there for
> purpose. pno_start/stop end up calling
> wpa_drv_sched_scan/wpa_drv_stop_sched_scan and the driver wrappers can
> use those to implement whatever is needed for this. In the android
> branch, driver_wext.c converts this to what driver_cmd() does in
> wpa_supplicant_8.git (and the private wpa_supplicant_8_lib) and
> driver_nl80211.c uses nl80211 sched_scan commands if the driver supports
> them and if not, does what wpa_supplicant_8.git tree is doing with the
> private ioctl.

The point here to return proper status in the return buffer. ("Ok" in
this case or "Fail").
Thanks,

Dmitry

>
> --
> Jouni Malinen                                            PGP id EFC895FA
> _______________________________________________
> HostAP mailing list
> HostAP@lists.shmoo.com
> http://lists.shmoo.com/mailman/listinfo/hostap
>
Jouni Malinen - Nov. 29, 2011, 7:48 p.m.
On Tue, Nov 29, 2011 at 10:26:27AM -0800, Dmitry Shmidt wrote:
> On Tue, Nov 29, 2011 at 3:03 AM, Jouni Malinen <j@w1.fi> wrote:
> > On Mon, Nov 28, 2011 at 05:07:26PM -0800, Dmitry Shmidt wrote:
> >> @@ -3377,10 +3378,12 @@ static int wpa_supplicant_driver_cmd(struct wpa_supplicant *wpa_s, char *cmd,
> >>
> >>  #ifdef ANDROID
> >>       if (os_strcasecmp(cmd, "BGSCAN-START") == 0)
> >> -             return pno_start(wpa_s);
> >> +             ret = pno_start(wpa_s);
> >> +
> >> +     else if (os_strcasecmp(cmd, "BGSCAN-STOP") == 0)
> >> +             ret = pno_stop(wpa_s);
> >>
> >> -     if (os_strcasecmp(cmd, "BGSCAN-STOP") == 0)
> >> -             return pno_stop(wpa_s);
> >> +     else
> >>  #endif /* ANDROID */
> >>
> >>       ret = wpa_drv_driver_cmd(wpa_s, cmd, buf, buflen);

> The point here to return proper status in the return buffer. ("Ok" in
> this case or "Fail").

Ah. Sorry, obviously I cannot read anymore.. ;-) I missed the last
"else" there going across #endif and the context in the patch ended just
before the key functionality. I applied this now with the ifdef block
removed so that this is easier to read.

Patch

diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c
index 8a6d00c..38538c9 100644
--- a/wpa_supplicant/ctrl_iface.c
+++ b/wpa_supplicant/ctrl_iface.c
@@ -85,6 +85,7 @@  static int pno_start(struct wpa_supplicant *wpa_s)
 	if (params.filter_ssids == NULL)
 		return -1;
 	i = 0;
+	ssid = wpa_s->conf->ssid;
 	while (ssid) {
 		if (!ssid->disabled) {
 			params.ssids[i].ssid = ssid->ssid;
@@ -3377,10 +3378,12 @@  static int wpa_supplicant_driver_cmd(struct wpa_supplicant *wpa_s, char *cmd,
 
 #ifdef ANDROID
 	if (os_strcasecmp(cmd, "BGSCAN-START") == 0)
-		return pno_start(wpa_s);
+		ret = pno_start(wpa_s);
+
+	else if (os_strcasecmp(cmd, "BGSCAN-STOP") == 0)
+		ret = pno_stop(wpa_s);
 
-	if (os_strcasecmp(cmd, "BGSCAN-STOP") == 0)
-		return pno_stop(wpa_s);
+	else
 #endif /* ANDROID */
 
 	ret = wpa_drv_driver_cmd(wpa_s, cmd, buf, buflen);