Hi Eric,
Thanks for the detail review, see my response below.
The change webrev is at: http://cr.grommit.com/~zf162725/cr_0413/
Please check.
--
Quaker
this review refers to the webrev at:
http://cr.grommit.com/~zf162725/cr_0410_all/
usr/src/lib/libdladm/common/libdlwlan.c:
892:
you need to set DLADM_WLAN_LINKATTR_WLAN in la_valid
before goto done.
| ACCEPT
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.
1753:
rename to 'do_set_key'
| ACCEPT
2032-2086:
this code is still wrong.
you cannot blindly assume that the dladm_wlan_* structures are
identical to your wl_* ioctl structures.
at the very least, your code must do something like:
dladm_status_t
dladm_wlan_wpa_xxx(const char *link, dladm_wlan_xxx_t *xxx)
{
wl_xxx_t wl_xxx;
/*
* for basic types (e.g. int)
*/
wl_xxx.a = xxx->a;
wl_xxx.b = xxx->b;
/*
* for types that require translation.
*/
switch (xxx->c) {
case DLADM_WLAN_XXX_D1:
wl_xxx.c = WL_XXX_D1;
break;
case DLADM_WLAN_XXX_D2:
wl_xxx.c = WL_XXX_D2;
break;
}
...
return (ioctl_set(link, WL_XXX, &wl_xxx, sizeof (wl_xxx));
}
note that the fields a, b, c in dladm_wlan_xxx_t *must* refer to
either basic types or #defines in libdlwlan.h only.
also, it may be simpler to pass a, b, c as arguments rather than
using a structure.
similar changes are needed for dladm_wlan_wpa_getsr() as well.
(i.e. dladm_wlan_ess_t must not be assumed to be the same size as
struct wpa_ess.
| ACCEPT
| see new webrev.
usr/src/cmd/cmd-inet/usr.lib/wpad/driver_wifi.c:
63: must check la_valid for DLADM_WLAN_LINKATTR_WLAN before
using la_wlan_attr
64: must check wl_attrp->wa_valid for DLADM_WLAN_ATTR_BSSID before
using wl_attrp->wa_bssid.
| ACCEPT
93-94: must check la_valid and wa_valid before using wa_essid.
| ACCEPT
114-116, 119-122: these lines could be moved into dladm_wlan_wpa_setie().
wpa_driver_wifi_set_wpa_ie() would become:
static int
wpa_driver_wifi_set_wpa_ie(const char *ifname,
const char *wpa_ie, uint32_t wpa_ie_len)
{
int ret;
(void) pthread_mutex_lock(&gbuf_1k);
ret = dladm_wlan_wpa_setie(ifname, ie, wpa_ie_len);
(void) pthread_mutex_unlock(&gbuf_1k);
return (ret == DLADM_STATUS_OK ? 0 : -1);
}
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.
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.
144: enabled should be boolean_t
152: setting ret = -1 is useless here because of the assignment at 157.
157: just change dladm_wlan_wpa_setwpa() to:
dladm_wlan_wpa_setwpa(const char *, boolean_t);
this will let you delete 146,155-156.
| ACCEPT
177: dladm_wlan_wpa_delkey() should become:
dladm_wlan_wpa_delkey(const char *, int, unsigned char *);
this will let you delete 166,173-176.
| ACCEPT
217: this function needs to be modified according to my comment
regarding libdlwlan.c:2032-2086 above.
specifically:
268-269,271: these fields must be a DLADM_WLAN_* type, not a
IEEE80211 type.
280: you could remove the wk structure and pass individual arguments to
dladm_wlan_set_wpa_setkey(), whose signature would become
something like:
dladm_wlan_set_wpa_setkey(const char *ifname,
dladm_wpa_cipher_t cipher,
unsigned char *addr, int key_idx, boolean_t set_tx, uint8_t
*seq,
uint32_t seq_len, uint8_t *key, uint32_t key_len);
| ACCEPT
306: change signature to:
dladm_wlan_wpa_disassociate(const char *ifname, dladm_wpa_reason_t
reason);
and delete 304-305.
346: this seems useless.
354: change signature to:
dladm_wlan_wpa_associate(const char *ifname, const char *bssid,
const char *wpa_ie, uint32_t wpa_ie_len);
and delete 349-353.
| 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.
_______________________________________________
networking-discuss mailing list
[EMAIL PROTECTED]
