Subject: [Tsvwg] Lots of comments about sctpsocket-02...

From: [email-masked]
Date: Fri, 11 Jan 2002 21:27:35 +0200

Hi all!

	I have been checking the draft and I have many
comments/doubts/typos caught/proposed changes... As I'm not an expert on
these issues, some of them might be completely wrong... Excuse me also
if some have been already commented in the list... Here they are:

	Section 3.1.2, last paragraph: It says that when binding, "One
of those addresses will be the primary address for the association"...
Uhm... Well, it will be the *peers* primary address , not ours... Maybe
it could say "The peer will chose one of those addresses as its primary
address for the association", or maybe nothing at all... This same
comment applies to Section 4.1.2, first paragraph of page 12, and to
Section 4.1.4, third paragraph of page 13.

	- Section 3.2, first paragraph: I don't understand how an
application can use recvmsg() to implicitly open a connection... It does
not carry any peer's address information... so? :-/
	Also, a client application does not usually use the bind() call,
and thus it is not necessary that "all bind() calls are complete".
	When I read it first time I got the impression that the UDP
style sockets could also use connect()... Maybe a "as is the case for
TCP Style sockets" could be added right after the "(i.e., no connect()
calls required".

	- Section 3.3, second paragraph: A typo, "Whenever the user
which want to" should be "Whenever the user which want*s* to".

	- Section 4.1.2, first paragraph of page 12: Another typo,
"equivalant" should be "equivalent".

	- Section 5.1, last paragraph of page 16: "Sections 3.1.3"
should be "Sections 3.1.4".

	- Section 5.2.1: Wouldn't it be interesting to add a new field
to the struct sctp_initmsg containing the cookie life? (maybe two of
them, the "standard" cookie life and the "extended" cookie life, in case
we receive a "Cookie Preservative" parameter).
	Other fields I can think about are: Max Path/Total
retransmissions, initial/Min/Alpha/Beta RTO, Heartbeats allowed,
heartbeat interval, or even if bundling is allowed or not. These values
must be fixed to some default values, and maybe it would be nice to be
able to modify those values since the very beginning. Some of them can
be modified later by calling setsockopt() or sctp_opt_info. However, the
cookie life, the Alpha/Beta values of RTO (would we like to modify
them?), enable/disable bundling, cannot be modified (and to
enable/disable heartbeats we have to go address by address)... Note also
that, while the initial RTO can be modified later, we cannot control its
value during the INIT/COOKIE ECHO retransmissions...
	And what about being able to set the value of the
inbound/outbound buffers?
	Another little issue. I guess that sinit_max_attempsts is
measured in milliseconds, but that's not stated.

	- Section 5.2.2, description of sinfo_stream: "an error
indication is returned", which one?
	The description of sinfo_flags appears out of order.
	In the first paragraph of page 20, "multple" should be
"multiple".
	In the sinfo_timetolive description: "if the message as not been
sent" should be "if the message *has* not been sent".

	- Section 5.3.1.1, beginning of page 23: "provided by this
notificaion" should be "provided by this notification".
	In the description of the sac_state parameter, it should say "16
bits (unsigned integer)".
	In the description of sac_error, is it set to the "Cause Code"
of the Error Cause received? (this also applies to the description of
ssf_error in Section 5.3.1.4)
	Also, it says there that it is a signed 32-bit number, but it is
defined as a uint16_t value. In Section 5.3.1.4 it happens the same with
ssf_error, but just the opposite.
	By the way, the descriptions of the sac/spc/sre/ssf/sse/sai_type
don't include the statement "16 bits (unsigned integer)".
	In that same paragraph "a error condition" should be "an error
condition".

	- Section 5.3.1.2: In the definition of the struct
sctp_paddr_change, I guess that the spc_aaddr field should be spc_addr
instead. At least it is what appears in the example of the appendix.
	In the description of spc_error, how is the relevant error
information expressed?

	- Section 5.3.1.5, in the definition of the struct
sctp_shutdown_event. The sse_flags field appears twice, both in the
definition and in the explanation of the fields.

	- Section 5.3.1.6. Why "ADAPTION" instead of "ADAPTATION"?...
I'm not sure of this. There are many other "adaption"s in the draft.
	In the first paragraph, "sends a Adaption" should be "sends an
Adaption". In any case, I don't understand the paragraph itself. Could
somebody explain me what is the use of this notification? What is an
Adaption Layer Indication parameter? Are them simply INIT/INIT ACK
parameters? In that case, when should they be passed to the application?

	- Section 6.1, page 31, it says "flags  - (described below)",
where?

	- Section 6.2, first line of page 32, "descript" should be
"descriptor".

	- Section 7, second paragraph, "the the".

	- Section 7.1.1, in the definition of the struct sctp_rtoinfo:
Could it be interesting including here srto_alpha and srto_beta?

	- Section 7.1.2, second paragraph starting from the end of the
page 33: "To access or modify these parameters," I guess is a cut n
paste mistake, there is only one parameter, so it would be better "To
access or modify this parameter,".
	In the last paragraph of page 33, "seperate" should be
"separate".

	- Section 7.1.3, second paragraph: "effected" should be
"affected".

	- Section 7.1.5: Then, if the integer is set to 1, the Nagle
algorithm is disabled, right?

	- Section 7.1.8: "An association being idle is defined an
association that has NOT sent or received user data." :-/... Wouldn't it
be better something like "An association is said to be idle for a lapse
of time if it has NOT sent nor received user data during that lapse of
time."? Or something like this...

	- Section 7.1.11: Apart from the Adaption vs. Adaptation issue,
I fail to understand what an Adaption Layer Indication parameter is...
:-0. If it is a parameter, why it is restricted to be 32-bits long?

	- Section 7.2.1, page 37: In the description of sstat_rwnd, is
that value the total peer's receiver window size, or just the unused
space of its receiver buffer? The word "current" leads me to the second
interpretation, but the length of the receiver buffer itself can be
modified, so...

	- Section 8.1: Isn't there any way to specify "all the
addresses"? I mean, a wildcard such as INADDR_ANY, but meaning all of
them, not just one...
	About SCTP_BINDX_ADD_ADDR and SCTP_BINDX_REM_ADDR... It says
"The flags parameter is formed from the bitwise OR of zero or more of
the following...", but later on it says "The two flags are mutually
exclusive"... What am I missing?
	In that same parameter, it says that we can add/remove addresses
from the association... But, if we are binding, which association is
going to be established? Moreover, there isn't any parameter in the
sctp_bindx() call indicating the association... so...

	- Section 8.2: This is not so clear for me... if we branch off
an UDP Style association, does it become a TCP Style one?
	In the description of addrlen it makes a reference to the addr
parameter, but that parameter does not exists...

	- Section 8.3: To keep the same naming conventions thorough the
whole document, I think that the "id" parameter of sctp_getpaddrs could
be "assoc_id".
	For the same reason, in sctp_getladdrs (section 8.5), "sock"
could be modified to be "sd", "id" to be "assoc_id" and "ss" to be
"addrs" (note that the first and last changes are necessary to keep the
consistence with the later description of the parameters).
	Again, in sctp_opt_info, "id" could be changed to be "assoc_id".
	Some other changes that I have just seen. In sendmsg and recvmsg
(Section 3.1.4), "socket" could be changed to be "sd". The same in
shutdown() (Section 4.1.7), getsockname() (Section 4.1.9). In
getsockname(), the definition "socklen_t *len" could be changed to "int
*addrlen (not sure of this). Same for getpeername() (Section 4.1.10)
	Of course, all these changes must be done both in the definition
of the function and in the description of the parameters.

	- Section 8.7: I fail to see why we need another function, and
we can't use getsockopt()... I have the idea that getsockopt is for the
whole association, and sctp_opt_info refers to a single address, and
thus, to keep getsockopt compatible with TCP implementations, you don't
want to modify it so it can carry an address parameter. However, I don't
understand the explanation given in the first paragraph of the section.
	It says later "See 8.5 subsections...", I guess it should be
"See 8.7 subsections..."

	- Section 8.7.1: Uhm... the Heartbeat Interval is a parameter
that affects all the addresses (see page 108 of RFC 2960). So I think
that it shouldn't appear here. I think it should appear as a parameter
in the struct sctp_assocparams defined in Section 7.1.2.
	In the description of spp_assoc_id, "This is filled in the
application", shouldn't it be "This is filled in *by* the application"?
This same sentence appears in Section 8.7.2, in the description of
spinfo_assoc_id...

	- Section 8.7.2: The word "addresses's" appears several times...
Not completely sure of this, but, shouldn't it be "addresses'"?

	- Section 9, first paragraph of page 43: "unprivelged" should be
"unprivileged".

	- Section 12: Well, the [STEVENS] reference is wrong... There is
already a 03 version, published on November 2001...

	- Appendix A, page 45: In the "case SCTP_REMOTE_ERROR", the
field sre_len of the sre variable (struct sctp_remote_error) does not
exist...

	As a final comment, the indentation in the struct definitions is
sometimes done with tabulations, and some other with spaces, so here in
my windows machine they don't coincide... If after such a long list of
changes you still feel like modifying this, its up to you :-)

	Thanks for your time!

	BR Iván Arias Rodríguez

_______________________________________________
tsvwg mailing list
[email-masked]
http://www1.ietf.org/mailman/listinfo/tsvwg