Girish Moodalbail wrote:
> On 10/12/09 22:16, Garrett D'Amore wrote:
>> All,
>>
>> I've converted hme to the common MII framework (and hence to
>> Brussels). This fixes at least one bug (it didn't work at forced 10
>> Mbps!), and cleans up a *a lot* of code. hme shrinks by about 2200
>> lines.
>>
>> The webrev is here:
>>
>> http://cr.opensolaris.org/~gdamore/hme-mii
>
> Hello Garrett,
>
> I have looked at only Brussels interfaces and here are my comments:
>
> hme.c:
>
> L123-L128: Since all the 4 private properties are per-interface, the
> comments above those lines are incorrect. They talk about making these
> parameters per-interface (which is already per-interface) and using
> ndd command.
Good catch. I"m just remove the comment.
>
> L130-133: Do we really need these to be global variables? They are
> used only for initiation and so can be represented as macros right.
They could be macros, but making them globals also allows them to be
adjusted via /etc/system. I'm not going to change them for now, because
there simply isn't any compelling need to do so.
>
> L1917: rv should be reset to 0. otherwise when everything is fine we
> will return (ENOTSUP) or else @L1950 return (ENOTSUP) and L1956 return
> (0)
Accept. I guess I didn't try changing these parameters. Need to cover
that with more testing.
>
> L1932: There is a missing 'else' clause.
Accept, good catch.
>
> L72, L90, L127, L269: NIT: no need for ','
Yeah, but C is ok with it either way. I like to leave the last , in
place, so if another member gets added to the end, you don't have to
remember to add one without creating a syntax error. As this is mostly
a stylistic preference, I'm going to leave it alone for now.
Thanks for reviewing this stuff.
- Garrett
>
> ~Girish
