On (02/08/08 12:11), Garrett D'Amore wrote:
> Now actual review feedback.
>
> 1) It looks like the cmn_err() I was talking about in my last post is
> already there. WIth the additions in mac.c 1037-1054, you can remove
> 1055-1066, I believe.
>
> 2) mac.c: lines 2918-2919 are superfluous.
Ok, accepted.
> 3) mac_ndd.c: Since '?' is normally used only by humans, maybe this would
> be a good time to issue a warning inline? Alternatively, just replacing
> the '?' with a message to see dladm(1M) may be appropriate. (But due to
> i18n concerns, maybe the better way to do this is in ndd(1M) itself, using
> a special errno to indicate that the driver is a Brussels driver?)
Actually, I would not be surprised if there were scripts out there
that did something like
var =`ndd -get $DRV \?|grep pause`
for example, given the variations that we see in the ndd tunable
names, so I think we should retain the support for "ndd -get \?".
I do, however end up printing a warning:
# ndd -get /dev/bge1 \?
Feb 8 16:30:51 whitestar2-9.East.Sun.COM mac: WARNING: The ndd
commands are obsolete and may be removed in a future release of
Solaris. Use dladm(1M) to manage driver tunables
:
>
> 4) mac_ndd.c: line 247: Nit: furthermore, this should probably be strncmp
> or somesuch. A property name "could" start with '?'.
Ok, accepted.
> 5) mac_ndd.c: line 255: I don't think you need a check for reading
> properties.
>
> 6) mac_ndd.c: line 391: But you do need it here. Although its not
> ip_config but net_config.
Ok.. I'll be cleaning up all the secpolicy stuff next week, so I'll
make sure I get to these..
> 7) mac_ndd.c: ~line 295 "value" and "newvalue" are confusing. Maybe use
> "valueptr" to indicate its a pointer? And then s/newvalue/value/.
Ok, I'll use vp and newvalue.
> 8) mac_ndd.c: line 322-323: this test (and the relationship to this loop,
> and the loop above it) are somewhat confusing. Maybe just nest the for in
> an if {} block? Or use a goto in the above loop, putting a label
> "prop_found" at 339? (then you could remove the prop_found boolean
> variable, and the associated tests
I first used goto's and then abandoned it (Djikstra's warnings
hit my conscience :-)). I guess I'll put them back.
> 9) should the mapping code include tests for "-" substituted with "_" ?
> You called these outliers, but I'm not so sure.
(See response for #12)
> 10) mac_ndd.c: mac_mii_prop implies these are only mii properties. Maybe
> "mac_public_prop" is a better choice?
but public props include things like mtu and flowctrl which are not
traditionally supported by ndd (ndd seems to typically cover mii). How
about mac_ndd_public_prop?
> 11) dld.h: no change obvious
right.. will have to unedit this out at some point before putback...
> 12) The more and more I think about it, the more I dislike the
> mac_ndd_handle separation. These should just be Brussels properties.
> Period. Having a separate mechanism for drivers to expose different sets
> of properties via NDD versus Brussels seems to add complexity and
> confusion, and I see no merit in it. That does mean that non-public
> properties are only ever interpreted as a string, but that's how NDD works
> today. What do you think?
there's several factors here to consider
- outliers can be very colorful, going beyond just a swap between - and _.
For example, here's a table that Raymond Li put together at one point,
when we were evaluating the variations in flow control:
ieee802.3(5) bge e1000g nge
adv_asmpause_cap adv_asym_pause_cap adv_asym_pause_cap adv_asym_pause_cap
asmpause_cap asym_pause_cap asym_pause_cap asym_pause_cap
lp_asmpause_cap lp_asym_pause_cap lp_asym_pause_cap lp_asym_pause_cap
link_rx_pause link_rx_pause -- link_rx_pause
link_tx_pause link_tx_pause -- link_tx_pause
link_pause -- -- --
link_asmpause -- -- --
So it would be hard to anticipate every variant in the code. One approach
would be to just toss support for these out, but I'm afraid that
there are drivers like ce that have a profuse set of tunables which
are actively used by customers, so we can't just drop legacy support
for these.
- converting outliers to private props: since we decided that private props
should begin with _ (see discussion at
http://mail.opensolaris.org/pipermail/brussels-dev/2007-August/000388.html),
we can't have a 1-1 mapping between ndd outliers and private props.
Besides, for each property, I think that the amount of code that
we'd delete out of the <drv>_ndd.c would equal the amount of code
that we'd end up adding to the drivers private-property implementation,
so it would not really help with code cleanup.
I see your point about the yuck-factor here, that comes from
having an additional way to provide tunables.. let me think about this
some more, and see if there's some better solution here.
--Sowmini
