Hi, Erik:
I looked at a few SCTP files and here are some comments. Will look at the
rest of the SCTP changes get back by Mon.
thanks,
-venu
http://cr.opensolaris.org/~nordmark/refactor-review/
usr/src/cmd/mdb/common/modules/sctp/sctp.c
L581: is crb_recvdstaddr this really used?
L710: Yes.
L957-970 : connp instead of sctp->sctp_connp?
usr/src/uts/common/inet/sctp/sctp_impl.h
L900-903 (old code) : could removing this cause any overflow/deadlock
issues etc. i.e in a loopback case could this thread send the packet
and when it gets to the receiving SCTP can it continue (assuming
sctp_recvq is null) and then get back with the response.. handshake?
could this exchange lead to stack overflow? Also, when we call
sctp_output from sctp_sendmsg we now has sctp_running set, when the
loopback thread (in the above scenario) comes back we'd probably
queue the packet in the receive queue and ask the task queue to
process it, but will then wait in sctp_process_recvq since sctp_output
has still not returned to WAKE_SCTP, is that so? Did we check loopback
tests? Sorry if I am missing something obvious.
usr/src/uts/common/inet/sctp/sctp_asconf.h
usr/src/uts/common/inet/sctp/sctp_addr.h
No comments
usr/src/uts/common/inet/sctp/sctp.c
L217-219: Whatis is the reason for this assert here (i.e. in
sctp_create_eager)
L1056: why is this notyet?
L1070-1072: When sending a packet out we use info. from the
appropriate faddr, right? So, this should be handled correctly?
L1154,1253: What is this comment for?
usr/src/uts/common/inet/sctp/sctp_addr.c
usr/src/uts/common/inet/sctp/sctp_asconf.c
usr/src/uts/common/inet/sctp/sctp_bind.c
No comments
usr/src/uts/common/inet/sctp/sctp_common.c
L648: We just heard from the fp (i.e. it is alive), so what could
result in fp->state being unreach?
L948-951: (related to comments L1070-1072 in sctp.c) should this
be w.r.t the fp we are using to send the packet?
usr/src/uts/common/inet/sctp/sctp_conn.c
L241: will we add an assert here before putback?
L580: I think so.
On Wed, 16 Sep 2009, 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]
_______________________________________________
networking-discuss mailing list
[email protected]
