On Wed, 2006-06-07 at 00:02 -0700, Nicolas Droux wrote:
> Seb,
>
> This looks really good, I have only a few minor comments.
Thank you very much for the review:
>
> * usr/src/lib/libdladm/common/libdladm.c
>
> ND-001 215 You need to be able to identify the aggregation from
> which
> you want to retrieve the defined VLANs. With these changes
> it seems that it will always look for "aggr0".
ACCEPT
This was caught during testing, and has been fixed.
>
> * usr/src/lib/liblaadm/common/liblaadm.c
>
> ND-002 60 Should update the notes on file syntax if it has
> changed.
ACCEPT
>
> ND-003 413 Isn't this going to cause a failure if the slash
> separator
> is not present?
REJECT
No, because in that case the first token is the entire string, which is
what is returned by strtok_r() (this differs from the strtok()
behavior). I re-tested this to be sure, and it (liblaadm) behaves as
expected when reading both formats (bge0 or bge0/0, for example) from
the /etc/aggregation.conf file.
>
> * usr/src/pkgdefs/SUNWckr/prototype_com
>
> ND-004 90 mismerge?
ACCEPT
Yes, this has been fixed.
>
> * usr/src/pkgdefs/SUNWckr/prototype_i386
>
> ND-005 89 mismerge?
> 197
> 264
> 371
ACCEPT
Yes, this has also been fixed.
>
> * usr/src/pkgdefs/SUNWckr/prototype_sparc
>
> ND-006 96 mismerge?
> 205
ACCEPT
Yes, this has been fixed.
>
> * usr/src/uts/common/io/aggr/aggr_port.c
>
> ND-007 172-174 Use {} for multi-line for() block
ACCEPT
>
> * usr/src/uts/common/io/mac/mac.c
>
> ND-008 180 Looks like a gratuitous change
ACCEPT
I'm not sure why this was changed, it will be reverted.
> ND-009 1043 Typo: s/zefo/zero
ACCEPT
Even though I think there should be a word "zefo", I'll make it
"zero". ;-)
>
> * usr/src/uts/common/sys/aggr_impl.h
>
> ND-010 134 Could use another bit of the bitmask above
ACCEPT
Thank you!
-Seb
_______________________________________________
networking-discuss mailing list
[email protected]
