Anders Persson wrote:
Hi Erik,
Some comments below.
Thanks for the review.
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
Good catch.
We need to call mblk_setcred() for all the cases when we use
tcp_rcv_enqueue and tcp_kssl_input. (But given your comments below it
isn't needed for the conn_ind case; just for data with trusted extensions.)
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?
Ah - perhaps this is a mismerge from before tcp_bind_hash_port was
introduced. I'll remove it.
* 4376: Yes :-) (or tcp_input_syn)
I actually handles packets other than SYNs - anything where the best
match is a listener will go here. So I'll rename it tcp_input_listener.
* 4706: Seems like bumping tcpAttemptFails would be more appropriate
OK
* 4743: Not needed; tcp_rwnd_set() updates rcvbuf
OK
* 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.
Turns out we can ASSERT for ira_sqp != NULL.
* 6758: Change tcp->tcp_connp to connp, since that is used elsewhere
OK
* 6971: s/Alway/Always/
OK
* 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?
tcp_input_data also calls it; but are you saying that tcp_eager_conn_ind
always has the db_credp set? Sure looks like it.
* 9733-9735: Similarly, wouldn't the credentials already be attached to
the t_conn_ind?
Yes. I was trying to avoid the crhold/crrele manipulation for the socket
case, but the fact that we always pre-allocate the T_CONN_IND and set
db_credp means this isn't easy. So I'll remove this stuff.
* 15240: LSO is being disabled, so the comment should really say that
the STREAM head is being updated to use smaller data blocks, right?
Yes.
* 20597-20630: It would be nice if these comments could be "pulled up".
Maybe they could now fit on a couple of lines ;-)
Will do.
* 21407: Looks like the endpoint is not being removed from the bind
hash
You mean calling tcp_bind_hash_remove? No need to remove from the ipcl
one since we didn't get inserted.
uts/common/inet/tcp/tcp_fusion.c:
* 157: The sentence is missing an 'if'
OK
* 488: Comment needs some tweaking
OK
uts/common/inet/tcp/tcp_opt_data.c: No comments
uts/common/inet/tcp.h: No comments
Thanks again for the review,
Erik
_______________________________________________
networking-discuss mailing list
[email protected]
