Sowmini,
Here are comments, sorry for the delay. I focused my review on the GLDv3
kernel components.
usr/src/uts/common/io/dld/dld_drv.c
ND-01 514, 536 miocpullup() could potentially change
mp->b_cont, which would cause the
dipp pointer set before that call
to point to freed memory.
usr/src/uts/common/io/mac/mac.c
ND-02 2965, 2979 The 'extern' keywords should be removed.
2993
ND-03 2971-2974 Should use '{}' for if() block spanning
2985-2988 multiple lines.
ND-04 2994 "maxsdu", "max_sdu", "sdu_max", it would
be nice to have a single way of spelling
this.
usr/src/uts/common/io/strplumb.c
ND-05 74 I'm not clear on why this is needed.
usr/src/uts/common/sys/dld.h
ND-06 285 Shouldn't the size of this array be the
new DLPI_LINKNAME_MAX that you added to
this file? If not then why do we need
these two #define's.
ND-07 282 Missing blank lines before the structure
declaration.
ND-08 291 This could be moved immediately after 281.
ND-09 291 When is this version expected to change?
Why do we need to track it? I looked at the
framework.pdf document in your PSARC
case but it was not documented there either.
usr/src/uts/common/sys/mac.h
ND-10 306 Nit: set_prop_t and get_prop_t
would be easier to read than spropf_t
and gpropf_t, and more consistent with
the callback types.
ND-11 348-349 Do we really need two separate flags for
setting and getting properties? It would
seem reasonable to me to require a driver
to support both set and get operations if
it supports properties.
ND-12 786-787 I would suggest using "get_prop" and "set_prop"
instead of "getprop" and "setprop" to be
consistent with the other MAC client API entry
points. BTW, these two lines should be
moved after line 748, since they are not
part of the driver API.
usr/src/uts/common/io/dld/dld_str.c
usr/src/uts/common/io/dls/dls_vlan.c
ND-00 No comment.
Sowmini.Varadhan at Sun.COM wrote:
> Hi,
>
> We'd like each of you to participate in the code review of the Brussels
> Framework component. This component covers the changes needed
> in the kernel, dladm, libdladm to support the {set/get}-linkprop
> commands via dladm, as well as the changes in the bge driver which
> is the sample "demo" driver for the Framework delivery.
>
> Since we expect to deliver this component chrnologically after Clearview UV,
> and additionally, since most folks are already familiar with the
> changes to be delivered by Clearview-UV, we have generated the webrev
> against the Clearview UV gate.
>
> We've identified reviewers who are working in project areas that
> overlap the code changes we are targeting. However, of course anyone
> else who is interested is welcome to take a look of the webrev, and
> your comments are very much appreciated!
>
> If any of you is unable to review the code, or would like other
> assistance to make the review process easier, please let us know as soon
> as possible. In case you are unable to review the code,
> please also help us identify an alternate reviewer.
>
> The timeout is set as two weeks (Nov 16 2007).
>
> Thanks in advance,
>
> --Sowmini
>
> External webrev:
>
> http://cr.opensolaris.org/~sowmini/webrev.brussels/
>
> Internal webrev:
>
> http://zhadum.east/export/ws/sowmini/cvuv-br/webrev.swan/index.html
>
> Cscope:
>
> /net/zhadum.east/export/ws/sowmini/cvuv-br/usr/src[/uts]
>
--
Nicolas Droux - Solaris Core OS - Sun Microsystems, Inc.
droux at sun.com - http://blogs.sun.com/droux
