[Ns-developers] [ns3] socket API

[Mathieu Lacage]

(sorry for the late reply) 

On Tue, 2008-05-13 at 22:45 -0700, Tom Henderson wrote:

> > Do you have a better suggestion ? Otherwise, let's just use ENOMEM.
> 
> EMSGSIZE?  I think of ENOMEM as typically referring to failed malloc().

good for me.

> >> 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.

Since then, a number of issues have been raised, most notably the fact
that we were wrongly trying to make our users believe that we were
providing a posix socket API while we are not. It is based on these new
discussions that I proposed that removal so, arguing that you would like
to keep the current state based on discussions which predate these new
arguments would be misleading. 

A better way to deal with this issue would be to revisit the previous
decision and rationale based on the new discussions we have had recently
about actual user confusion. Since I don't remember anything (I honestly
don't) about the november discussion you allude to, it might be helpful
to dig these and post them here: it might be that there is a showstopper
which I forgot about. 

Another alternative is for you to state an explicit decision about this
and ignore the pointless bickering we are into. I would be fine with
this but I want to make progress and I don't want to see decisions being
made implicitely.

> >> 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

I see.


> >> 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.

The issue here is that I can imagine sockets for which the "TxBufSize"
option makes zero sense (for example, a netlink socket) so, I don't
think you will be able to come up with a useful set of generic options
which apply to everyone to stick in there.

> 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?

You are right.

> > 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.

I see this now.

> 
> > 
> >   - 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?

Not really. If you really think that RcvBufSize is a generic attribute,
you could conceivably move it to the Socket base class and make it
indirect through a pair of private virtual methods:

class Socket
{
public:
  static TypeId GetTypeId (void) {
    static TypeTid tid = TypeId ("ns3::Socket")
      ...
      .AddAttribute ("RcvBufSize", "help text",
                     UintegerValue (10),
                     MakeUintegerAccessor (&Socket::GetRcvBufSize,
                                           &Socket::SetRcvBufSize)
                     ...)
      ;
    return tid;
  }
private:
  virtual void SetRcvBufSize (uint32_t size) = 0;
  virtual uint32_t GetRcvBufSize (void) = 0;
}

> > 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?

Given your comments above, no, I do not see this as evil anymore
(although I _really_ feel uncomfortable about these "generic" socket
options stored in the class SocketOptions).

I know that we discussed this at length last friday in my office but the
more I look at this, the less I am convinced that we need the extra
complexity (both from an implementation and from a user point of view)
provided by these aggregated socket option classes. It seems just
simpler to go with an attribute-based approach and I have a hard time
convincing myself that the extra features provided by the aggregate
classes are worth the extra pain. 

To summarize, I tend to favor giving up a single feature (namely, the
ability to specify per-node defaults) for a net decrease in terms of
implementation and user-visible complexity. i.e., users don't have to
call GetObject before calling SetAttribute or GetAttribute to set or get
an option.

regards,
Mathieu