On (09/12/08 12:37), Sebastien Roy wrote:
>
> 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?
show-linkprop output will remain the same as it does today, i.e.,
it will show all properties listed in the prop_table[], using the
DLD_CONTROL_DEV as first choice for this purpose.
The dladm_prop[] array is there to help the i_dladm_{get, set}_public_prop
functions figure out the buffer size needed. The wireless drivers frequently
use var length buffers, and that equivalent information is implemented
in the get/set function itself.
> 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.
this workspace does not introduce any new properties. Thus the existing
methods will be supported as they are today.
>
> 1788: no need for braces
Ok.
> 2085,2087: these two lines can be combined
Ok.
> libdlwlan.c:
>
> General: Can you explain why all of the calls to i_dladm_wlan_param()
> are not dladm_{get|set}_linkprop() or equivalent?
The wifi parameter retrieval occurs in multiple contexts other than
the {set,get}_linkprop one. For example,
do_connect -> do_set_key, do_set_channel etc.
In order for all these calls to invoke dladm _*_linkprop, one would have
to setup entries for all these "properties" in the prop_table[].
I don't know why these properties were not included in the initial
list, but also, we have the need to support legacy ioctl paths,
until all drivers are converted.
> 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.
I'm taking a look at this.. will get back shortly.
> 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.
I'll talk to the driver team and get back to you..
> 1816: The changes I recently made in here seem to have been undone.
Accept. Seems to be a merge error. will fix.
