On (11/21/07 15:53), Nicolas Droux wrote:
> Sowmini,
>
> Here are comments, sorry for the delay. I focused my review on the GLDv3
> kernel components.
>
Nicolas,
many thanks for taking the time to review this for us! Here are
the responses:
> 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.
good catch- Accept.
>
> usr/src/uts/common/io/mac/mac.c
>
> ND-02 2965, 2979 The 'extern' keywords should be removed.
> 2993
Accept.
> ND-03 2971-2974 Should use '{}' for if() block spanning
> 2985-2988 multiple lines.
Accept.
> ND-04 2994 "maxsdu", "max_sdu", "sdu_max", it would
> be nice to have a single way of spelling
> this.
Accept.
> usr/src/uts/common/io/strplumb.c
>
> ND-05 74 I'm not clear on why this is needed.
There were some circular dependancies between dld.h and mac.h which
created problems when we tried to define the dld_ioc_prop_val_t related
structures. As it turned out, dld.h does not have to include mac.h,
so in the process of straightening this out, we had to fix some related
files to explicitly include mac.h where 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.
Correct; accept. BTW, the two #defines were also needed as part of
the cleanup described in response to ND-05.
>
> ND-07 282 Missing blank lines before the structure
> declaration.
Accept.
> ND-08 291 This could be moved immediately after 281.
Accept.
> 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.
the version number management is described at the top of page 9 of
http://opensolaris.org/os/project/brussels/files/brussels.pdf
Although we don't foresee any seismic changes to the structure,
the version number is a place-holder for major structure redefinitions
that may potentially occur down the road.
> 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.
Accept
> 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.
Probably not, and I encounted this question when I was prototyping
the ndd compat handling myself, but given that every other callback
has its own flag, I was hesitant to overload one flag for 2 callbacks.
>
> 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.
Accept.
> usr/src/uts/common/io/dld/dld_str.c
> usr/src/uts/common/io/dls/dls_vlan.c
>
> ND-00 No comment.
Thanks much for the review!
--Sowmini
