Hi Eric,
Thanks, see my response below.
Eric Cheng wrote:
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?
Since the wificonfig is still not obsolete, and there are a lot of users
of OpenSolaris who are using wificonfig,
I suggest we keep compatibility to avoid confusion.
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>
event loop is okay, but there are also door_up_calls from kernel, which
will cause multiple threads in wpa daemon.
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?
okay.
57: this is not a pointer. should declare it as attr.
okay.
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.
Consider the pthread_mutex protection and wpa_print debug message, seems
it's better
to keep current codes.
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.
The flag doesn't only have two value, it maybe extended to support more
features.
For example, 0 - means disable, 1 - means WPA1, 2 - means WPA2, 3 - mean
WPA1 | WPA2.
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.
Already done, see http://cr.grommit.com/~zf162725/cr_0417/
2142: define a enum (e.g. dladm_wlan_reason_t) and do the translation to
IEEE80211 types as in above.
okay.
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?
okay.
209: comment should say /* WPA support routines */
okay.
Thanks,
--
Quaker
_______________________________________________
networking-discuss mailing list
[email protected]
