Hi Eric,
Thanks for the review. Below is my response to your comments. Please
let me know if you have any additional questions.
The change webrev is at: http://cr.grommit.com/~zf162725/cr_0403/
--
Quaker
=====================================================
REVIEWER: Eric.Cheng
WEBREV: http://cr.grommit.com/~zf162725/cr_0308/
FILES: Misc
NOTES: Description of feedbacks:
ACCEPT Request accepted
REJECT Request rejected
EXPLAIN Explanation given
DISCUSS Request requires further discussion to resolve
DEFER Request deferred (e.g. because work is out-of-scope)
=====================================================
| EXPLAIN
| Since libwladm has been restructure (PSARC/2007/140),
| the lib APIs names have been changed to be consistent, and the new webrev
| will be against the merged libdlwlan.[ch].
the following comments refer to the webrev at:
http://cr.grommit.com/~zf162725/cr_0308/
usr/src/cmd/dladm/dladm.c
2070:
you are using this for parsing wpa keys so it shouldn't be
called parse_wep_keys() anymore. it's better to call it
parse_wlan_keys(). also, wladm_wep_key_t should be renamed to
wladm_key_t.
| ACCEPT
| Since libwladm has been restructure (PSARC/2007/140),
| wladm_wep_key_t rename to dladm_wlan_key_t
| parse_wep_keys() rename to parse_wlan_keys()
238:
change this to:
" [-s wep|wpa]\n"
| ACCEPT
2217-2227:
these lines can be replaced with:
die("key required for security mode '%s'",
wladm_secmode2str(&attr.secmode, buf));
| ACCEPT
3073:
change string to:
"valid values are: wep, wpa"
| ACCEPT
3186:
change "wep" to "unknown"
| ACCEPT
usr/src/lib/libwladm/common/libwladm.h:
159-163:
please check if these #defines are valid for wpa or not.
if so, remove the 'WEP' from the names.
if not, you'll likely need a new wladm_wpa_key_t.
| ACCEPT
| It's valid for wpa, so
| replace DLADM_WLAN_MAX_WEPKEY_LEN (13) with DLADM_WLAN_MAX_KEY_LEN (64)
| rename DLADM_WLAN_MAX_WEPKEYNAME_LEN to DLADM_WLAN_MAX_KEYNAME_LEN
| others needn't to be changed.
usr/src/lib/libwladm/common/libwladm.c:
45-47:
which functions need these #includes?
| EXPLAIN
| <libscf.h> is need by service APIs, others are removed.
2355:
you should move this function to 2239 so you won't have to
declare it at 72.
| ACCEPT
522:
shouldn't this be in wpa.h instead?
(btw, wpa.h is not in the webrev)
| EXPLAIN
| IEEE80211_C_WPA is defined in <sys/net80211.h>
| Like "#define IEEE80211_RATE 0x7f", put the definition there to
avoid
| including the <sys/net80211.h>
564-570,596-597:
can these be moved outside of do_connect(), maybe to 653?
if you do this, you can remove the link argument to do_connect().
| EXPLAIN
| Since wpa daemon need to get essid from driver,
| so the wpa_instance_create must follows the do_set_essid().
| I will add a comment there.
2027:
delete this comment.
| ACCEPT
2289,2294-2295:
username seems to be always 'root'.
you should use getpid() and getpwuid() to get the current username.
| ACCEPT
the following comments refer to the webrev at:
http://cr.grommit.com/~zf162725/cr_0322/
usr/src/lib/libwladm/common/libwladm.h:
195-203:
which one of these interfaces are wpa specific?
the wpa specific interfaces should be renamed like so:
wladm_wpa_get_sr(), wladm_wpa_set_ie(),...
| ACCEPT
| changed to dladm_wlan_wpa_*()
wladm_cmd_scan(), wladm_get_bssid(), wladm_get_essid()
are not really necessary because they seem to be implementable
by extending the existing interfaces.
for wladm_cmd_scan(), you can just change wladm_scan() to
check if func is NULL and only do the scan if it is.
| EXPLAIN
| wladm_cmd_scan() just sends WL_SCAN command to driver,
| and return immediately, don't wait to get the scan results;
| It is different from the wladm_scan().
| To be clear, I rename wladm_cmd_scan to dladm_wlan_scan_only().
for get_bssid,essid, you should just use wladm_get_link_attr()
and extend the wladm_link_attr_t with the fields la_bssid and
la_essid. these represent the local bssid/essid. corresponding
WLADM_LINK_ATTR_ESSID/BSSID flags need to be added as well.
| EXPLAIN
| The result of wladm_get_link_attr() is only available when link is
| CONNECTED. If not connected, it will return nothing.
| Wpa service need to get ESSID, BSSID when doing a connection/key
| negotiation, so need to keep these two APIs.
also, I don't like the exposure of the wl_* data structures to
clients of libwladm. for each one of these structures, you
should create a similar one called wladm_xxx; and in your new
wladm routines, you translate these types to your wl_xxx types.
the reason for doing this is to keep libwladm clients insulated
from ioctl interface changes.
| ACCEPT
| use void * replacing spsecific data structure.
once you've done the above, the <net/wpa.h> include can be removed.
| ACCEPT
uts/common/net/wpa.h:
this file is not in your webrevs yet.
| Sorry, the file is in the part 2 (kernel) review webrev,
| missed in this webrev.
I noticed there are lots of #defines here prefixed with IEEE80211.
are these supposed to be part of net80211.h?
| EXPLAIN
| These #defines are specific to WPA protocol, and will be used by
| wpa daemon. I prefer to isulate the wpa daemon from net80211*.h.
117,144,174:
these structure names seem to be inconsistent with the typedef'ed name.
e.g. struct ieee80211req_key should be struct wl_key instead.
| ACCEPT
_______________________________________________
networking-discuss mailing list
[email protected]
