<div dir="ltr">Hi Jouni,<div><br></div><div>Thanks for review.</div><div>I will submit a v2 where we wont be exposing interface type outside drivers i.e. driver_nl80211.</div><div>Also I will take care of formatting comments.</div><div><br></div><div>Thanks,</div><div>Avinash</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Nov 15, 2014 at 2:47 PM, Jouni Malinen <span dir="ltr"><<a href="mailto:j@w1.fi" target="_blank">j@w1.fi</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Thu, Nov 06, 2014 at 12:44:14PM +0530, Avinash Patil wrote:<br>
> This patch adds dbus getter method for nl80211 iftype.<br>
> This is required by certain applications which intend to start<br>
> AP operations only if current interface type is AP.<br>
> Getter method for capabilities cannot be used for this purpose as<br>
> this enumarates all the supported interface types.<br>
><br>
> Patch also adds notification handlers for interface type change<br>
> events.<br>
<br>
</span>This has significant whitespace damage in the inline patch.. Could you<br>
please resend with those fixed? For example:<br>
<br>
checking file src/drivers/driver_nl80211.c<br>
patch: **** malformed patch at line 196: *priv, u8 *bssid)<br>
<br>
> diff --git a/src/common/defs.h b/src/common/defs.h<br>
<span class="">> @@ -297,6 +297,24 @@ enum wpa_ctrl_req_type {<br>
> NUM_WPA_CTRL_REQS<br>
> };<br>
><br>
> +enum wpa_nl80211_iftype {<br>
<br>
</span>nl80211 should not really show up in any way in defs.h. In addition, it<br>
is a bit strange to see interface types defined here, i.e., I would have<br>
expected to see this in src/drivers/driver.h.<br>
<br>
> + WPA_IFTYPE_UNSPECIFIED,<br>
..<br>
<span class=""><br>
> + /* keep last */<br>
> + WPA_NL80211_IFTYPES,<br>
> + WPA_IFTYPE_MAX = WPA_NL80211_IFTYPES - 1<br>
<br>
</span>What would these be used for? There was nothing using these in the<br>
patch..<br>
<br>
<br>
> diff --git a/src/drivers/driver.h b/src/drivers/driver.h<br>
<span class="">> @@ -1427,6 +1427,18 @@ struct wpa_driver_ops {<br>
</span><span class="">> /**<br>
> + * get_iftype - Get the current NL80211 iftype<br>
<br>
</span>driver.h is supposed to define a driver independent interface. This<br>
should not be documented as being specific to nl80211.<br>
<span class=""><br>
> + int (*get_iftype)(void *priv, u8 *iftype);<br>
<br>
</span>u8 * ??<br>
<br>
Shouldn't this be that enum wpa_nl80211_iftype (or well, renamed to<br>
something not nl80211 specific) and the enum defined in this file?<br>
<span class=""><br>
> +static int wpa_driver_nl80211_get_nl80211_iftype(void *priv, u8 *iftype)<br>
> +{<br>
> + struct i802_bss *bss = priv;<br>
> + if (!bss)<br>
> + return -1;<br>
> +<br>
> + *iftype = (u8)nl80211_get_ifmode(bss);<br>
> +<br>
> + return 0;<br>
> +}<br>
<br>
</span>This would need to map driver interface (nl80211) specific values to<br>
generic values defined in driver.h. That said, I'm not sure I really<br>
understand why we would expose every nl80211 iftype to external programs<br>
through wpa_supplicant. What is the real need for that information? Is<br>
it just something like "is this interface configured in AP mode"? I'd<br>
assume something much simpler would cover that need..<br>
<br>
> + if(wpa_drv_get_iftype(wpa_s, &iftype)) {<br>
<br>
"if (wpa_drv..."<br>
<br>
> + char_iftype = wpa_supplicant_iftype_txt(iftype);<br>
<br>
"char_iftype = wpa_..."<br>
<span class=""><br>
> diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c<br>
> index 6761c1a..3a075c8 100644<br>
> --- a/wpa_supplicant/events.c<br>
> +++ b/wpa_supplicant/events.c<br>
> @@ -182,9 +182,12 @@ void wpa_supplicant_stop_countermeasures(void<br>
> *eloop_ctx, void *sock_ctx)<br>
> void wpa_supplicant_mark_disassoc(struct wpa_supplicant *wpa_s)<br>
> {<br>
> int bssid_changed;<br>
> -<br>
> + u8 old_iftype = 0, new_iftype = 0;<br>
> wnm_bss_keep_alive_deinit(wpa_s);<br>
<br>
</span>Should not move that empty line in that way.. And why use u8 for<br>
something that seemed to get an enum defined for it?<br>
<span class=""><br>
> + if(wpa_drv_get_iftype(wpa_s, &new_iftype)) {<br>
> + wpa_dbg(wpa_s, MSG_DEBUG, "Failed to get new iftype!");<br>
> + }<br>
> + if (new_iftype != old_iftype)<br>
> + wpas_notify_interface_type_changed(wpa_s->global);<br>
<br>
</span>Why would interface type be reported to change if wpa_drv_get_iftype()<br>
failed?<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Jouni Malinen PGP id EFC895FA<br>
</font></span></blockquote></div><br></div>