[brussels-dev] Code review request for Brussels Framework.

[Nicolas Droux]

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