Quaker,
the latest changes look fine.
I have no further comments.
eric
Quaker Fang wrote:
Hi Eric,
Thanks for the review, please see my response below.
The webrev against onnv is at: http://cr.grommit.com/~zf162725/cr_0417/
--
Quaker
>
> 906:
> why not add a new WL_ENC_WPA type?
>
> | DISCUSS
> | Considering the wificonfig, and wificonfig on OpenSolaris, and many
users
> | on OpenSolaris who are using wificonfig, if add this type, will cause
> | incompitable problem between wificonfig and drivers, since wificonfig
> | desn't know what is WL_ENC_WPA.
>
now that dladm with wifi is available, I'd think that more people would
be using it instead of wificonfig. also,
since these ioctls are private interfaces, I'd prefer not to be
constrained by them. I believe we've introduced
binary-incompatible changes when dladm-wifi was putback, so why can't we
do that now?
| ACCEPT
>
> btw, why do you need the gbuf_1k mutex?
>
> | DISCUSS
> | I am not sure libdladm is MT-safe or not, if so, I'll remove the
mutex.
>
libdladm is not mt-safe. but from looking at your daemon event loop
code, I don't see a need for thread safety.
I suggest you remove the lock, all the pthread calls, and the #include
<pthread.h>
| ACCEPT
| removed.
more comments on the latest code:
usr/src/cmd/cmd-inet/usr.lib/wpad/driver_wifi.c:
76: (ret == DLADM_STATUS_OK? 0 : -1);
can you use a macro for this, and fix it in other places as well?
| ACCEPT
| WPA_STATUS defined.
57: this is not a pointer. should declare it as attr.
| ACCEPT
61-71:
better for this to look like:
ret = dladm_get_link_attr(ifname, &attr);
if (ret != DLADM_STATUS_OK)
return (WPA_STATUS(ret));
wl_attrp = &attr.la_wlan_attr;
if ((attrp.la_valid & DLADM_WLAN_LINKATTR_WLAN) == 0 ||
(wl_attrp->wa_valid & DLADM_WLAN_ATTR_BSSID) == 0)
return (WPA_STATUS(DLADM_STATUS_FAILED));
(void) memcpy(bssid, wl_attrp->wa_bssid.wb_bytes, DLADM_WLAN_BSSID_LEN);
return (WPA_STATUS(ret));
| ACCEPT
96-107:
this should be rearranged like 61-71 above.
| ACCEPT
156:
can dladm_wlan_wpa_set_wpa() take a boolean_t instead of a uint32_t?
if you do this, 146,155 can be removed.
| ACCEPT
usr/src/lib/libdladm/common/libdlwlan.c:
2074: this should be len = sizeof (wl_wpa_ie_t) + wpa_ie_len;
is there a maximum length for wpa_ie_len? if so, check for it
before malloc().
2119-2125: ik_type, ik_flags are not in the libdlwlan
namespace so these assignments cannot be made directly.
you need to translate these to the kernel types, i.e.:
switch (cipher) {
case DLADM_WLAN_CIPHER_WEP:
wk.ik_type = IEEE80211_CIPHER_WEP;
case DLADM_WLAN_CIPHER_TKIP:
wk.ik_type = IEEE80211_CIPHER_TKIP;
....
}
do the same for ik_flags.
| ACCEPT
2142: define a enum (e.g. dladm_wlan_reason_t) and do the translation to
IEEE80211 types as in above.
| ACCEPT
| As we discussed offline, add the type dladm_wlan_reason_t, but not need
| to translate.
usr/src/lib/libdladm/common/libdlwlan.h:
73-79: better to add a prefix to each field. e.g. we_bssid, we_ssid....
76: why is there a blank line here?
| ACCEPT
209: comment should say /* WPA support routines */
| ACCEPT
_______________________________________________
networking-discuss mailing list
[email protected]
_______________________________________________
networking-discuss mailing list
[email protected]
