Renee,
Thanks for the review. Below is our response to your comments on net80211
ioctl. Please let us know if you have any additional questions.
-------------------------------------------------------------------------------
FEEDBACK ON TECHNICAL DOCUMENTATION/CODE
-------------------------------------------------------------------------------
Reviewer Name: Renee Danson
Document/Module Title: wifi-1018
Document/Module Version/Date: 11/13/06
-------------------------------------------------------------------------------
usr/src/uts/common/io/net80211/net80211_ioctl.c
-------------------------------------------------------------------------------
RD-01 general CodeStd I think cstyle will flag this, but
I'll note it
anyway: there seem to be a lot of places where
the indentation of continuation lines is 8
spaces instead of 4, this should be cleaned up.
| ACCEPT
RD-02 general Func Use of ieee80211_dbg seems inconsistent;
it's
included in some of the wifi_cfg_* functions,
but not in others.
| ACCEPT
RD-03 96 Edit This shouldn't make a difference, but I would
use ic->ic_des_essid as the param rather than
essid, since that's what's actually been set.
| EXPLAIN
| The definition of ic_des_essid & iw_essid_essid are:
| uint8_t ic_des_essid[IEEE80211_NWID_LEN]; /* 32 */
| char wl_essid_essid[34];
| ESSID could be 32 or less than 32 chars. Then when the essid
| is 32 chars, use '%s' with ic_des_essid will cause error. But
| iw_essid_essid is always NULL terminated. So essid(pointer to
| iw_essid_essid) is used instead of ic->ic_des_essid.
|
| To prevent possible memory overflow, Added below code:
| essid[IEEE80211_NWID_LEN] = 0;
RD-04 110, 629 Edit You don't need the /* ARGSUSED */ lint flag
here, as all the args are in fact used.
| ACCEPT
RD-05 176-178 Edit The way the strings are formatted here is
pretty confusing; why not just
ieee80211_dbg(IEEE80211_MSG_CONFIG,
"wifi_cfg_nodename: Set nodename %s %d\n",
ic->ic_nickname, len);
| ACCEPT
RD-06 305-310 Design Given this code, it is possible that
*some* of
320-323 the passed-in keys could be set, while some are
332-335 not (even some valid ones that are in the array
after the invalid one). Seems to me that an
"all or nothing" approach would be better:
either set all valid keys (i.e. continue in
this error case, in case there are other valid
keys) or don't set any if there's an invalid
one in the list.
| ACCEPT
| Changed to continue set the valid keys.
Also, if ieee80211_crypto_setkey() fails, what
happens to the key created by ieee80211_crypto_
newkey()? Is there any cleanup required for
that?
| EXPLAIN
| No need to do cleanup here. ieee80211_crypto_newkey() mainly
| establishes relationship between the specified key and cipher.
| If ieee80211_crypto_setkey() fails, next time when
| ieee80211_crypto_newkey() runs on this key, old cipher attached
| with this key is checked. If different cipher is to be used, then
| old cipher is detached and new cipher is attached.
RD-07 445 Edit Seems cleaner and more consistent to just
declare ow_encryp as in the other functions,
rather than doing this casting here.
| ACCEPT
| Changed as below:
| wl_encryption_t *ow_encryp = (wl_encryption_t *)outp->wldp_buf;
| ...
| *ow_encryp = (ic->ic_flags & IEEE80211_F_PRIVACY) ? 1 : 0;
RD-08 540 Edit Same comment as RD-07; declaring an ow_linkstat
pointer would seem more consistent.
| ACCEPT
| Changed the same as L445
RD-09 562 Edit Another consistency issue: either inp should
be passed to all wifi_cfg_* functions, whether
they use it or not; or it shouldn't be passed
if it's not used. Right now, it isn't passed
to wifi_cfg_linkstatus(), but it is passed to
wifi_cfg_suprates(); neither function uses it.
wifi_cfg_rssi() and wifi_cfg_esslist are other
functions that do not use it.
| ACCEPT & EXPLAIN
| After accepting Kacheong's comments on 'L963, 1030-1032', All
| wifi_cfg_<command> used to process an command will have the
| same arguments as below:
| static int
| wifi_cmd_<command>(struct ieee80211com *ic, mblk_t *mp)
| and all wifi_cfg_<parameter> used to get/set an parameter will
| have the same arguments as below:
| static int
| wifi_cfg_<parameter>(struct ieee80211com *ic, uint32_t cmd, mblk_t
**mp)
RD-10 580 Edit Is WL_HW_ERROR really the appropriate error
for a malloc failure?
| DISCUSS
| Return values defined in <inet/wifi_ioctl.h> are:
| #define WL_SUCCESS 0x0
| #define WL_NOTSUPPORTED EINVAL
| #define WL_LACK_FEATURE ENOTSUP
| #define WL_HW_ERROR EIO
| #define WL_ACCESS_DENIED EACCES
| #define WL_RETURN_BASE 0x7000
| #define WL_READONLY (WL_RETURN_BASE + 0x1)
| #define WL_WRITEONLY (WL_RETURN_BASE + 0x2)
| #define WL_NOAP (WL_RETURN_BASE + 0x3)
| WL_HW_ERROR seems more appropriate than others.
RD-11 677 Cos Extra space between '<' and 'IEEE80211_MODE_MAX'
| ACCEPT
RD-12 733 Func Is ic->ic_bss guaranteed to be non-NULL?
wifi_getrssi() assumes that it is a valid
pointer, so if it's not guaranteed, it should
be checked somewhere before dereferencing on
line 711. And ic might also need to be checked
in wifi_getrssi().
| EXPLAIN
| Both ic and ic_bss must not be NULL. 'ic' is allocated in
| ieee80211_attach() and is associated with every newly created
| node. So ic is non-NULL. 'ic_bss' is allocated in
| ieee80211_media_init() which is called by driver's attach()
| routine and is also guaranteed to be non-NULL.
| Furthermore, this function is called by wifi_getset(), after
| accepting comment RD-16, an ASSERT is added as below:
| ASSERT(ic != NULL && mp != NULL);
RD-13 809 Func The field of the wl_phy_conf union that you
want here is wl_phy_fhss_conf.
| ACCEPT
RD-14 835 CodeStd Cstyle should catch this, but you need a
space
after the '='.
| ACCEPT
RD-15 941 Func I'm not sure on this one: don't you want to
call wifi_loaddefdata() in any case, whether
you had to change the state to INIT or not?
| ACCEPT
| Moved wifi_loaddefdata() out of 'if {}' so that it will be executed
| in any case
RD-16 wifi_cfg_getset Func In this function, and in all the
functions it
calls, we assume that our initial inp and outp
pointers are non-NULL. Should verify that in
here, I would think. Same goes for ic.
| ACCEPT
| Added ASSERT as below:
| ASSERT(ic != NULL && mp != NULL);
-------------------------------------------------------------------------------
-------------------------------------------------------------------------------
Comment type key:
Func comment on functionality
Perf comment on performance
CodeStd comment on coding standards
Design comment on design
Edit editorial comment
Cos cosmetic comment
Com comment on missing comments
Description of feedback:
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)
_______________________________________________
networking-discuss mailing list
[email protected]
