On (01/30/08 13:20), Sebastien Roy wrote: > Looks good. Only a couple of comments: >
Thanks for the prompt review! > usr/src/lib/libdladm/common/linkprop.c > > 192: The alignment of one of the columns is off. > > 204: The description is not quite accurate. The term "frame" is specific > to Ethernet, and the MTU property isn't the "default", but the "current" > link MTU. "current link MTU" would be more accurate IMO. consider these fixed > 1182-1229: The checks for DL_ETHER make the code inherently not scalable. > I think the only two cases are DL_WIFI, and not DL_WIFI. In other words, > WiFi is the only special case, and every other media type is the norm. I > believe you've started to codify this at line 311 by making the property > applicable to DATALINK_ANY_MEDIATYPE, but stopped there. Correct. Actually one of the items in the Brussels roadmap is to also provide support in DL_WIFI for mc_setprop/mc_getprop (this came up in PSARC review- see ged-04 in http://opensolaris.org/os/community/arc/caselog/2007/429/20070912-2007-429-inception/ so the plan is to work on the wifi drivers, as part of driver conversion, at which point the DL_WIFI specific code will be merged in as a public property. > A related question is, how does DL_IB treat speed? not sure.. I'll check and find out. > > 1189,1209,1268: The caller of these functions already knew the media type > so that it could compare it against pd_dmedia. It's inefficient to go the > door call a second time to get the same information that we already had a > moment ago. A better approach might be to simply pass in the media type as > an argument to these functions. But the caller in this case has media type DATALINK_ANY_MEDIATYPE. so we can't avoid doing another call to ladm_datalink_id2info (unless we change the semantics of pd_set). > usr/src/uts/common/io/bge/bge_main2.c > > 1274: cstyle: err is not a boolean. ok, will fix. --Sowmini
