[MLton] INetSock problems, hton/ntoh usage

[Matthew Fluet]

Vesa Karvonen wrote:
> As has been reported earlier
> 
>  http://mlton.org/pipermail/mlton/2007-March/029675.html
>  http://mlton.org/pipermail/mlton/2006-December/029465.html
> 
> the net code of MLton has been broken for some time.  Trying to get my
> smlbot running, I took a look at the code.
> 
> The port number of a socket is a 16-bit integer (the sin_port field of
> struct sockaddr_in).  OTOH, the current net code in MLton represents port
> numbers as C_Int.t values which usually aren't 16-bit integers.

That's a very good observation.  IIRC, when reworking the Basis Library 
implementation to use more C types, I handled the NetServDB structure 
before the Socket and INetSock structures.

The s_port field of struct servent is specified as int, so (internally) 
a port in the NetServDB structure is represented by C_Int.t

I probably just assumed that ports in other structures would also be 
specified as int.  However, I do see that the in_port_t type (which is 
the type of the sin_port field of struct sockaddr_in) is specified as 
being equal to uint16_t.

 > So,
> inside the Net structure of MLton, the wrong hton/ntoh conversion (namely
> htonl/ntohl) gets chosen.  AFAICT, the conversion should be done on the
> lower 16-bits only.  Below is patch that should fix the problem.  Note
> that I changed names from ntoh -> ntohs and hton -> htons to make it more
> apparent that the functions only work on the lower 16-bits.

I think the better solution would be to use Word16.word for ports in the 
INetSock structure (and C-side functions in INetSock.c).  It probably 
isn't worth creating a whole C_InPort structure, when we know it should 
be equivalent to Word16.  My general approach has been to follow the C 
types as closely as possible, since it makes the C-side code very simple.

The Basis Library doesn't specify the behavior of INetSock.toAddr (ia, 
i) when the port i (of ML type int) is too large to be represented by 
the C-side representation of a port.  I think we currently would raise 
the Overflow exception, which seems reasonable.