Sebastien Roy wrote:
Garrett D'Amore wrote:
Per the request of PSARC 2007/298, I've updated the web rev for nemo
so that the mac_ether module gets to append "100 Mbps, full duplex"
(or whatever) to the logged messages.
This meant a small addition to the mac_ether module, and to the MAC
type plugin. (To create a new "link_details" function.)
I'm not too worried about the driver specific bits that have already
been reviewed by driver owners, but I would like someone to
review/re-review the bits in nemo. Specifically, mac.c, mac.h,
mac_impl.h,and mac_ether.h are of interest.
The updated webrev is located at
http://cr.opensolaris.org/~gdamore/nemo-stats-log/
This is good. I took a look at mac*.*, and only have a couple of nits:
General:
* The MAC-Type plugin operations were ARC'ed as Consolidation Private
as part of 2006/248, and that case's materials contain documentation
on all of the entrypoints. I'd suggest adding mtops_link_details as a
Consolidation Private interface to your 2007/298 case, and providing
an updated nemo-mactype-plugins.txt spec (contained in the materials
directory of 2006/248) as part of that case.
Agreed.
mac_ether.c
* This file no longer looks lint-clean (294).
Accept. Nice catch. Apparently lint only complains about unused return
values when performing the global crosschecks, not during its first
pass. :-( Something to keep in mind going forward.
mac.c:
* 174: 64 characters doesn't seems like very many for a syslog message.
Accept. I'm changing it to 200 for now. To be honest I picked that
number from a nether region... :-)
* 189,220: cstyle, no need for brackets here.
Accept. I'm not sure that this is required per cstyle. I usually
prefer to have braces even when not needed, to avoid problems when the
conditional actions grow (and someone forgets to add the braces
later.) But in this case they're not strictly necessary and their
absence is unlikely to pose a future a problem, so I've removed them.
mac.h:
* 459: the comment mentions mtops_link_notify() but the plugin
operation is mtops_link_details()
Accept. Oops. Artifact of my inability to make up my mind during the
first part of this. I've adjusted the comment slightly as a result of
your bringing my attention to it. It now reads:
/*
* mtops_link_details() is an optional callback that provides
* extended information about the link state. Its primary purpose
* is to provide type-specific support for syslog contents on
* link up events. If no implementation is provided, then a default
* implementation will be used.
*/
Thanks for doing the review. Now I just need to get the revised case
through PSARC next week, then I can do the putback. ;-)
-- Garrett
_______________________________________________
networking-discuss mailing list
[email protected]
