[Ns-developers] [ns3] socket API

[Tom Henderson]

Mathieu Lacage wrote:
> hi tom,
> 
> comments below,
> 
> On Tue, 2008-05-13 at 16:44 +0000, Tom Henderson wrote:
> 
>> I did this mostly along the lines Mathieu suggested, except for:
>>> * If the size of
>>> * the next packet to return is bigger than maxSize, return 0 and
>>> * set ENOMEM.
>> it seemed to me that ENOMEM is not the right errno response here, so
>> that is an open issue.
> 
> Do you have a better suggestion ? Otherwise, let's just use ENOMEM.

EMSGSIZE?  I think of ENOMEM as typically referring to failed malloc().

>>>
>>> Solution: get rid of the Socket::Send* methods which take raw
>> buffers
>>> and implement them exclusively at the "synchronous" layer.
>> I have not removed these raw buffer calls, although they could be
>> replaced with Packet-like variants such as:
>>
>> replace
>> int Socket::Send (buffer, size); 
>> with
>> int Socket::Send (Create <Packet> (buffer, size));
>>
>> and 
>>
>> uint32_t size = Recv (buffer, len);
>> with something like:
>> uint32_t size = Recv (len)->CopyTo (buffer, len);
>>
>> I would support whatever consensus would emerge on whether to keep or
>> remove these variants, but my sense is that opinions still differ.
> 
> So, what do you suggest we do to make progress on this issue ? Not
> making a decision before merging and leaving your current tree as-is is
> making an implicit decision so, please, let's make this explicit before
> merging. 
> 
> The main argument for not implementing a SocketHelper as was discussed a
> week ago is potential user confusion about slightly-different and
> duplicated API: the _exact same_ argument applies equally to all the
> one-liner overloads of Send in socket.h and is the main reason why I
> originally proposed to remove them.

This is revisiting an old contentious issue (November developers 
meeting) and also seemed to not have universal agreement when raised on 
the list in April.  So I think the default should probably be to keep 
what we have unless discussion results in different consensus now.

We've fixed or are fixing the current API to support the posix API in 
ns-3-simu.  Do people want to do more?

We now have several ways to call Send() and Recv() at our non-posix 
sockets API.  This includes the use of SendTo() and also the use of raw 
buffer based variants to Send() and Recv().

We could:
1) keep all of the overloaded calls in the low-level socket API
2) remove them and say that they belong only at ns-3-simu (process) layer
3) add another "helper API" layer and make the existing low-level API as 
minimalist as possible; that is, move these overloaded Send(), Recv() 
variants to a helper API.

It seems to me that we are not going to agree on which of the above is 
least confusing to future users, but like I said, if a consensus did 
emerge to move off of our previous agreement, any of the above would be 
acceptable to me.  You pointed out that we could get very close (but not 
exact) to the signature of the raw buffer variants using constructs like

int sent = sock->Send (Create <Packet> (buffer, size));
uint32_t size = sock->Recv (len)->CopyTo (buffer, len);

Is this a good enough solution for those who do not like to pass 
ns3::Packet to stream sockets?  Is there interest in moving Send 
(buffer, size) and similar variants to a new helper API?  Or leave as is?

> 
>> 3) How should socket options and ioctls be supported ?
>>
>> I implemented this along the lines of Mathieu's suggestion (define
>> SocketOptions class and aggregate the object to a socket) although I
> 
> I looked at the code and did not find that class: it seems that you have
> implemented the options in the socket subclasses directly.

see the top of sockets.h file

> 
>>  did not go so far as to implement (yet) the UdpSocketOptions or
>> TcpSocketOptions class.  Users who hold a reference to a Ptr<Socket>
>> can call GetObject() to get whichever one they want.  Then, calls to
>> Set..() or Get..() these values indirect through the Socket base class
>> to a setter or getter implemented in the socket implementation class.
>>
>> Only two options are supported for now (just proof of concept),
>> equivalent to SO_SNDBUF and SO_RCVBUF at the socket level.
>>
>> The question arose as to where to put the defaults for these options,
>> since class SocketFactory is abstract base class.  I implemented a
>> class SocketDefaults and made these attribute-based, and aggregated a
>> SocketDefaults object to the Node (in
>> InternetStack::AddInternetStack()).
>>
>> Maybe there is a better implementation strategy that avoids this
>> class, but the above seems workable at least.
> 
> The problem with a single SocketDefaults class per node is that it will
> have to contain all the defaults of all protocols, including the
> protocols developed by users which will kill one of the "features" we
> have been working on for a long time, that is, the ability to add a new
> type of socket/protocol to the system without having to patch anything
> src/node/. So, that solution is really not an option. 

I was thinking that the SocketDefaults class just held "sockets" level 
defaults such as SO_SNDBUF.  The per-protocol defaults could be hosted 
by the respective socket factory objects.  I do not think such an 
approach requires to edit src/node.

For instance, if one implements SctpSocket with SctpSocketOptions, then 
SctpSocketOptions could be aggregated to SctpSocket by the SctpImpl 
factory.  A user holding Ptr<Socket> sock could call 
sock->GetObject<SctpSocketOptions> (); and then could use this class to 
set options on the SctpSocket.  There wouldn't be a need to edit class 
Socket either because the user could get the SctpSocket public API via 
GetObject().  Am I missing something?

> 
> Given all the constraints we have here, I would like to suggest that,
> instead, we do something potentially simpler: for each subclass of
> Socket, define an attribute for each socket option relevant to that
> specific subclass and define its initial value there.   This will allow
> you to:
> 
>   - define any number of options for each socket subclass independentely
> from the other socket subclasses, hence allowing a user to create new
> socket subclasses without having to patch src/node/*

Again, I don't think my suggestion was requiring such a patch.

> 
>   - override on a simulation-wide basis the initial value of each of
> these socket options with the Config::SetDefault function
> 
>   - if you want to allow the user of a socket to write code which sets
> or gets a socket option in the same way across multiple implementations
> of the same kind of socket, all you have to do is use the same attribute
> name and type to allow the following to work consistently:
> socket->SetAttribute ("RcvBufSize", UintegerValue (10));

Yes, but the Config::SetDefault is no longer generic, right?

> 
> The above proposal is not optimal since it will not allow you to specify
> a default value on a per-node basis but, I view this as a much lesser
> evil than the alternatives.
> 

If what I said above was correct (about how it could work) do you still 
view it as evil?

- Tom