quaker,
this looks much better than before. my comments below:
Quaker Fang wrote:
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?
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>
remove it if it's not needed. your daemon should able to
call dladm_wlan_wpa_setie() directly.
| ACCEPT & EXPLAIN
| I keep current interface to be easily synced with FreeBSD.
OK
| ACCEPT & EXPLAIN
| I suggest to keep dladm_wlan_wpa_set_mlme(), but follow your comments
to change to
| dladm_wlan_wpa_set_mlme(const char *link, dladm_wlan_op_t op, int
reason, const char *bssid),
| this will be easily synced with FreeBSD, and easily to extended to
support
| AUTH/DEAUTH in future.
OK.
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?
57: this is not a pointer. should declare it as attr.
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));
96-107:
this should be rearranged like 61-71 above.
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.
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.
2142: define a enum (e.g. dladm_wlan_reason_t) and do the translation to
IEEE80211 types as in above.
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?
209: comment should say /* WPA support routines */
eric
_______________________________________________
networking-discuss mailing list
[email protected]
