Hi Erik,
Some comments below.
uts/common/inet/tcp/tcp_kssl.c:
* 208, 214, 298, 304: tcp_send_conn_ind is called without recv attributes,
but from tcp.c:9629 it looks like that is not supported
uts/common/inet/tcp/tcp.c:
* 2751-2753: Only endpoints that use the same lport are on
tcp_bind_hash_port, and it was already determined that the endpoint
being examined is using the requested port. So why is this check needed?
* 4376: Yes :-) (or tcp_input_syn)
* 4706: Seems like bumping tcpAttemptFails would be more appropriate
* 4743: Not needed; tcp_rwnd_set() updates rcvbuf
* 4943: There is code in tcp_conn_request that deals with non-SYN packets
arriving on a listener. Why not feed it in there? It looks like we
could end up triggering the assert on line 10255 otherwise.
* 6758: Change tcp->tcp_connp to connp, since that is used elsewhere
* 6971: s/Alway/Always/
* 9629-30: If there were any credentials, then they would have already been
attached to the t_conn_ind mp in tcp_conn_request. So is it necessary to
do this?
* 9733-9735: Similarly, wouldn't the credentials already be attached to
the t_conn_ind?
* 15240: LSO is being disabled, so the comment should really say that
the STREAM head is being updated to use smaller data blocks, right?
* 20597-20630: It would be nice if these comments could be "pulled up".
Maybe they could now fit on a couple of lines ;-)
* 21407: Looks like the endpoint is not being removed from the bind
hash
uts/common/inet/tcp/tcp_fusion.c:
* 157: The sentence is missing an 'if'
* 488: Comment needs some tweaking
uts/common/inet/tcp/tcp_opt_data.c: No comments
uts/common/inet/tcp.h: No comments
On Wed, Sep 16, 2009 at 12:30:45PM -0700, Erik Nordmark wrote:
>
> With the design review wrapping up (but we are still accepting comments)
> it is time to start the code review.
>
> First let's remind ourselves the purpose of the review. The key purpose is:
> - to find bugs or latent bugs by inspecting the code.
> - to reduce the risk of bugs being introduced in the future by making
> sure that the code and comments are reasonably clear; comments should be
> technically correct and helpful (as opposed to confusing)
>
> For some code reviews I seen in the past there has been stylistic
> comments around number of tabs, 80 column fill, etc.
> I don't view comments relating to white space as consistent with the
> purpose above. The code passes the cstyle checker. However, there might
> be places where the code is inconsistent with the cstyle document (which
> covers a bit more than what the cstyle checker can check). But apart
> from that, there is plenty of code to review for correctness and
> comments to review for clarity so please don't spend time on stylistic
> comments.
>
> There are a few "tags" in comments which will be removed after the code
> review; they serve as tags to point out interesting things for different
> reviewers. We have
> XTX For trusted extensions
> XIPSEC For IPsec interaction
> XTBD General comments, including forward looking statements
>
> Please let us know what parts or aspects of the code you will review, so
> we can track that. For instance, parts like TCP, SCTP, iptun are
> contained in separate files while aspects like TX, IPsec, and routing
> are spread out across the files.
>
> The webrev for usr/src is at
> http://cr.opensolaris.org/~nordmark/refactor-review/
>
> There are some changes to the closed tree (to SDP) which have already
> been reviewed. If anybody wants to look at those they are available
> internally at
> http://jurassic-x4600.sfbay/home/nordmark/hg/refactor-review/usr/closed/webrev/
>
> For those that are internal to Sun and want to use cscope, that is
> available at:
> /net/npt.sfbay/export/refactor/refactor-review/usr/src/uts
> and
> /net/npt.sfbay/export/refactor/refactor-review/usr/src
>
> Other should be able to pull the source from
> ssh://hg.opensolaris.org//hg/ip-refactor/refactor-gate
>
> ip_newroute delenda est,
> Erik
>
>
> _______________________________________________
> networking-discuss mailing list
> [email protected]
Anders
_______________________________________________
networking-discuss mailing list
[email protected]
