Hi, Judy:
I am fine with your responses, just retaining the discuss ones..
-venu
[...]
______________________________________________________________________________
usr/src/uts/common/io/net80211/net80211.c
General comment. Quite a few functions don't have a lot of comments,
some none. Consider adding a block comment giving some details for
those that don't.
| ACCEPT
If it is possible, try break this file functionally into smaller ones.
Else, it will keep growing and may become difficult to maintain.
| ACCEPT
| To keep consistent with FreeBSD file schema, split net80211.c back
| to 5 files as below:
| ieee80211_input.c - process received frames
| ieee80211_output.c - conpose and send out frames
| ieee80211_node.c - node management functions
| ieee80211_proto.c - protocol management, mainly manage state
transition
| ieee80211.c - generic functions
Thanks for doing these!
L622-623 - Not sure what the coding standard says, but I prefer one
L869 defn/line.
L1240-1242,L1384,L1643,L2161,L2445,L2683-2684,2687,L3028,L3310,L3655,
L4015
| DISCUSS
| Changed to be one defn/line.
Thanks.
L1303-1307 - Don't know how often this happens. Is array the most
effective option in such cases?
| DISCUSS
| At least AFAIK, yes.
OK.
L1910 - Should this be an assert, then?
| DISCUSS
| ieee80211_auth_open() can also be called when the device functions
| as an AP. But the feature isn't supported yet now. Instead of an
| assert, I'd like to remove this comment.
OK, that's fine by me.
L2119-2121, - Consider using a macro if this is a recurring thing.
2187-2188,2192
| DISCUSS
| Each infomation element has its own definition and may be included
| in different management frames. But puting these 3 information
| elements together doesn't make any sense. So I'd like to keep
| them seperated. But a Macro is defined for
| (IEEE80211_RATE_MAXSIZE - IEEE80211_RATE_SIZE)
| as below:
|#define IEEE80211_XRATE_SIZE (IEEE80211_RATE_MAXSIZE -
IEEE80211_RATE_SIZE)
| /* size of extended supported rates */
OK.
L3190 - ieee80211_crypto_demic - typo??
| EXPLAIN
| ieee80211_crypto_demic() is to support WPA/WPA2. Currently it's
| not ported. Later it will be added.
In that case, it may be better to take it out of the comment.
L164 - Why not just define it as IEEE80211_FC1_PWR_MGT?
| DISCUSS
| IEEE80211_FC1_PWR_MGT is checked by i_fc[1] ('struct ieee80211_frame'),
| while this one (IEEE80211_NODE_PWR_MGT) is used by in_flags
| (struct ieee80211_node). The comments here doesn't make sense. So
| removed this comment.
OK.
L154-157 - I suppose != 0 isn't required?
| DISCUSS
| This is from previous comments on coding style that if 'x' is a
| numeric variable, then use 'if (x != 0) { ... }'. Here ich_flags
| is of type 'uint16_t'. Then I'd like to keep these unchanged.
OK.
L488 - Should this be static in net80211.c?
L492
| DISCUSS
| In FreeBSD, these functions are called by WiFi drivers. So I'd like
| to keep them global.
OK
[...]
_______________________________________________
networking-discuss mailing list
[email protected]
