On Fri, Apr 13, 2007 at 12:08:24PM +0800, Quaker Fang wrote: > 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. >
can you generate the full webrev against onnv as well? thanks eric > -- > 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]
