[erlang-patches] SCTP improvements

[Raimo Niskanen]

On Thu, Dec 03, 2009 at 11:44:18AM -0800, Simon Cornish wrote:
> This branch contains one fix, to correctly type sctp_assoc_id as
> signed, not unsigned.
> And one enhancement to implement a non-blocking gen_sctp:connect
> 
> git fetch git://github.com/dotsimon/otp.git sctp_connect_enhancement
> 
> The fix for the sctp_assoc_id type is correct for all platforms but
> only noticeable when using the SCTP NKE under OSX.
> 
> The non-blocking connect enhancement offers a way to avoid an
> race-condition noted in the existing code. More detail (and some
> spelling mistakes!) can be found in the commit message:
> http://github.com/dotsimon/otp/commit/bb9d9a9e813242275e868457c4b30f40f13d65e8

I have now looked at the patches and there are some problems:



http://github.com/erlang/otp/commit/92f3ceea0fa51df308f6f2689ff82857b6442c30
Implement a non-blocking gen_sctp:connect

lib/kernel/src/inet_sctp.erl: 74/82
  Comparision Timeout /= 0 should have been Timeout =/= 0
  just to be certain...

  To compare Timeout =/= 0 is unfortunately also wrong since
  time may have elapsed since inet:start_timer/1 was
  called and inet:timeout/1 may return 0 when the original
  Timeout actually was nonzero.

And, furthermore; having gen_sctp:connect/4,5 to return a
different type for some value (i.e 0) of an argument
(i.e Timeout) is considered ugly. And introducing such
a change in a service release is out of the question.

Therefore; can you rewrite your patch to introduce a new
function gen_sctp:connect_nowait/4 that does exactly
what your gen_sctp:connect(S, Add, Port, Opts, 0) did?

There will be some code duplication, but you can maybe
also avoid creating a timer alltogether and perhaps
use a special value (e.g 0) to inet_sctp:connect/5, 
last argument, to not have to duplicate everything.
inet_sctp is a non-public interface so it can be altered.
Doing it this way also avoids fixing inet6_sctp.erl.



http://github.com/erlang/otp/commit/2666b68aab2167266f49b462e3ed63e6f2045709
Correctly type sctp_assoc_id as signed, not unsigned

Your patch seems to fix a bug when the returned
assoc_id from gen_sctp is later rejected by gen_sctp.

The patch would work, but I would rather change
what inet_drv returns. Something like this:
erts/emulator/drivers/common/inet_drv.c:
 #   define GET_ASSOC_ID		get_int32
 #   define ASSOC_ID_LEN		4
-#   define LOAD_ASSOC_ID	LOAD_INT
-#   define LOAD_ASSOC_ID_CNT	LOAD_INT_CNT
+#   define LOAD_ASSOC_ID	LOAD_UINT
+#   define LOAD_ASSOC_ID_CNT	LOAD_UINT_CNT
 #   define SCTP_ANC_BUFF_SIZE   INET_DEF_BUFFER/2 /* XXX: not very good... */
 #endif

Motivation:

The Sockets API Extensions for SCTP draft tries very
hard not to specify any signedness for sctp_assoc_t,
as you say the only defined value is 0, and that in
fact is the only clue I find that is an integral type.

You could if so inclined interpret it to be a struct
and assume you should memset(, 0, ) to it to make it zero.

Therefore it was decided to use unsigned integer in
the Erlang representation of it, and hence the check
in prim_inet for X band 16#ffffffff =:= X. The bug
is that inet_drv converts it into a signed integer
when it is delivered to Erlang (much thanks to the
name confusion get_int32 returning an unsigned...)

I think using an unsigned type when it is apparent that
signedness is unimportant is the right thing to do...

Can you rework that patch too, please?


> 
> /Simon
> 
> ________________________________________________________________
> erlang-patches mailing list. See http://www.erlang.org/faq.html
> erlang-patches (at) erlang.org

-- 

/ Raimo Niskanen, Erlang/OTP, Ericsson AB