[PATCH] supplicant: add dbus getter method for nl80211 iftype

[Avinash Patil]

Hi Jouni,

Thanks for review.
I will submit a v2 where we wont be exposing interface type outside drivers
i.e. driver_nl80211.
Also I will take care of formatting comments.

Thanks,
Avinash

On Sat, Nov 15, 2014 at 2:47 PM, Jouni Malinen <j at w1.fi> wrote:

> On Thu, Nov 06, 2014 at 12:44:14PM +0530, Avinash Patil wrote:
> > This patch adds dbus getter method for nl80211 iftype.
> > This is required by certain applications which intend to start
> > AP operations only if current interface type is AP.
> > Getter method for capabilities cannot be used for this purpose as
> > this enumarates all the supported interface types.
> >
> > Patch also adds notification handlers for interface type change
> > events.
>
> This has significant whitespace damage in the inline patch.. Could you
> please resend with those fixed? For example:
>
> checking file src/drivers/driver_nl80211.c
> patch: **** malformed patch at line 196: *priv, u8 *bssid)
>
> > diff --git a/src/common/defs.h b/src/common/defs.h
> > @@ -297,6 +297,24 @@ enum wpa_ctrl_req_type {
> >         NUM_WPA_CTRL_REQS
> >  };
> >
> > +enum wpa_nl80211_iftype {
>
> nl80211 should not really show up in any way in defs.h. In addition, it
> is a bit strange to see interface types defined here, i.e., I would have
> expected to see this in src/drivers/driver.h.
>
> > +       WPA_IFTYPE_UNSPECIFIED,
> ..
>
> > +       /* keep last */
> > +       WPA_NL80211_IFTYPES,
> > +       WPA_IFTYPE_MAX = WPA_NL80211_IFTYPES - 1
>
> What would these be used for? There was nothing using these in the
> patch..
>
>
> > diff --git a/src/drivers/driver.h b/src/drivers/driver.h
> > @@ -1427,6 +1427,18 @@ struct wpa_driver_ops {
> >         /**
> > +        * get_iftype - Get the current NL80211 iftype
>
> driver.h is supposed to define a driver independent interface. This
> should not be documented as being specific to nl80211.
>
> > +       int (*get_iftype)(void *priv, u8 *iftype);
>
> u8 * ??
>
> Shouldn't this be that enum wpa_nl80211_iftype (or well, renamed to
> something not nl80211 specific) and the enum defined in this file?
>
> > +static int wpa_driver_nl80211_get_nl80211_iftype(void *priv, u8 *iftype)
> > +{
> > +       struct i802_bss *bss = priv;
> > +       if (!bss)
> > +               return -1;
> > +
> > +       *iftype = (u8)nl80211_get_ifmode(bss);
> > +
> > +       return 0;
> > +}
>
> This would need to map driver interface (nl80211) specific values to
> generic values defined in driver.h. That said, I'm not sure I really
> understand why we would expose every nl80211 iftype to external programs
> through wpa_supplicant. What is the real need for that information? Is
> it just something like "is this interface configured in AP mode"? I'd
> assume something much simpler would cover that need..
>
> > +       if(wpa_drv_get_iftype(wpa_s, &iftype)) {
>
> "if (wpa_drv..."
>
> > +       char_iftype =  wpa_supplicant_iftype_txt(iftype);
>
> "char_iftype = wpa_..."
>
> > diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c
> > index 6761c1a..3a075c8 100644
> > --- a/wpa_supplicant/events.c
> > +++ b/wpa_supplicant/events.c
> > @@ -182,9 +182,12 @@ void wpa_supplicant_stop_countermeasures(void
> > *eloop_ctx, void *sock_ctx)
> >  void wpa_supplicant_mark_disassoc(struct wpa_supplicant *wpa_s)
> >  {
> >         int bssid_changed;
> > -
> > +       u8 old_iftype = 0, new_iftype = 0;
> >         wnm_bss_keep_alive_deinit(wpa_s);
>
> Should not move that empty line in that way.. And why use u8 for
> something that seemed to get an enum defined for it?
>
> > +       if(wpa_drv_get_iftype(wpa_s, &new_iftype)) {
> > +               wpa_dbg(wpa_s, MSG_DEBUG, "Failed to get new iftype!");
> > +       }
> > +       if (new_iftype != old_iftype)
> > +               wpas_notify_interface_type_changed(wpa_s->global);
>
> Why would interface type be reported to change if wpa_drv_get_iftype()
> failed?
>
> --
> Jouni Malinen                                            PGP id EFC895FA
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.shmoo.com/pipermail/hostap/attachments/20141204/988dd7bf/attachment.htm>