On Thu, 2008-09-04 at 09:46 -0400, Sowmini.Varadhan at Sun.COM wrote:
> The Brussels and Wifi teams need your help with code review of the
> changes for:
>
>
> 6721363 concise 'show-linkprop' default output
> 6744703 wifi property ioctls should try the Brussels path through
> DLD_CONTROL_DEV first.
>
>
> Webrev is at
> http://cr.opensolaris.org/~sowmini/bruwifi/
This is good stuff.
General: This wad doesn't seem to be "hg nits" clean, as it still
contains SCCS keywords.
linkprop.c:
General: Why are the WiFi properties not in the dladm_prop[] array? I
would think that they need to be there. Won't show-linkprop show
nothing as the code stands?
General: How does a program in the ON consolidation set or get a link
property for a WiFi link in a generic way? I don't see it.
1788: no need for braces
2085,2087: these two lines can be combined
libdlwlan.c:
General: Can you explain why all of the calls to i_dladm_wlan_param()
are not dladm_{get|set}_linkprop() or equivalent?
123: This copy doesn't look kosher, but this points to a general flaw in
the design. We're passing around character arrays instead of proper
structures and types. For example, why use buf in dladm_wlan_scan, when
we can use "status" outright? See my next comment:
1222: do_get_linkstatus() should not take "void *" since there's only
one possible correct type for the input. Same with seemingly every
other do_get_*() function. i_dladm_wlan_param() itself takes a void *
as its second argument, so there's no reason to propagate the
type-unsafetiness up to these functions.
net80211_ioctl.c:
General: There is a massive amount of code duplication between the ioctl
paths and the property paths, on the order of ~1000 lines of code. It
seems to me that this can be re-factored to reduce the cost of
maintenance of this code and reduce the probability of their getting out
of sync. I'll hold off on reviewing this file in detail until this is
addressed.
1816: The changes I recently made in here seem to have been undone.
ath_main.c:
1795: You don't need "err". Simply return (ieee80211_getprop());.
iwk2.c:
2561: Same here, return (ieee...);
wpi.c:
2262: Same here, return (ieee...);
-Seb
