Garrett D'Amore wrote:
> I'm looking for folks to review my changes to hme. I *cannot* commit
> these changes if I don't get reviewers!
>
> The webrev is at http://cr.opensolaris.org/~gdamore/nemo-hme
More functionality with thousands of lines of code removed. Nice. :-)
usr/src/uts/sparc/hme/Makefile
* Why does hme depend on ip? It doesn't seem like it should.
usr/src/uts/sun/io/hme.c
* 61-93: There's no point in assigning each member of the enumeration
with the value it would have by default.
* 2583-2589: I don't understand the need for the cascading chain of if
statements here. This isn't much of an improvement over the previous
code, and the indentation is still not cstyle compliant. It could be
simplified to:
if ((GET_ROM8(&(hmep->hme_romp[i])) & 0xff) == 'P' &&
(GET_ROM8(&(hmep->hme_romp[i+1])) & 0xff) == 'C' &&
(GET_ROM8(&(hmep->hme_romp[i+2])) & 0xff) == 'I' &&
(GET_ROM8(&(hmep->hme_romp[i+3])) & 0xff) == 'R') {
vpd_base =
(int)((GET_ROM8(&(hmep->hme_romp[i+8])) & 0xff) |
(GET_ROM8(&(hmep->hme_romp[i+9])) & 0xff) << 8);
break; /* VPD pointer found */
}
* 2762: This line of code is quite twisted. You treat the a non boolean
expression as a boolean expression in order to explicitly return a
boolean value to a boolean variable. Why not just:
doinit = ((hme->hme_flags & HMESTARTED) != 0);
Which actually is a boolean expression.
* 3146: cstyle: no need for brackets.
* 3177, 3179, 3221, 3244: cstyle: none of the expressions evaluate to a
boolean. Change to: "if (<expr> != NULL)" ...
* 3709: Is there a need for these macros? Why not have this code be
unconditionally enabled at compile time?
* 3724-3726: The locking accomplishes nothing here.
* 3802: cstyle: need blank line between locals and code.
* 4204: return a boolean_t
* 4227: please fix this comment with the new return semantics. It looks
like this returns B_TRUE if the mblk was consumed (regardless of whether
it was successfully transmitted), or B_FALSE if it wasn't.
* 4732: Who is this question for?
* 5263,5264: It would be easier to read if these were down at 5434.
* 5871: To answer the XXX comment you added: No, you don't. The details
behind that answer lie in i_dls_link_subchain(), which calls
DLS_PREPARE_PKT() on every packet. This uses the DLS_STRIP_PADDING()
macro to accomplish the same thing as this now redundant code in hme.
The code here and at 6043 can thus be removed.
* 6091-6100: cstyle: need brackets around the conditional expressions.
-Seb
_______________________________________________
networking-discuss mailing list
[email protected]
