Bug 271

Summary: PacketSocketFactory should be added in the Node base class, not in InternetStackHelper
Product: ns-3 Reporter: Gustavo J. A. M. Carneiro <gjcarneiro>
Component: networkAssignee: ns-bugs <ns-bugs>
Status: RESOLVED FIXED    
Severity: major    
Priority: P3    
Version: pre-release   
Hardware: All   
OS: All   

Description Gustavo J. A. M. Carneiro 2008-08-05 12:05:34 EDT
Currently the only place where PacketSocketFactory is added to a node is in InternetStackHelper::Install.  This is just plain wrong.  Packet sockets are L2 sockets and are unrelated to "internet".  In fact, if it had anything to do with internet, it would have to be placed in function AddInternetStack, never in here.
Comment 1 Mathieu Lacage 2008-08-05 12:23:06 EDT
PacketSocketFactory is added in PacketSocketHelper if you want to only packet sockets on your node. The reason it is also added to a node from InternetStackHelper is that we wanted to make InternetStackHelper add all the things it could by default.

If you want to add a flag to InternetStackHelper to disable this feature, I would be fine with that. Removing it altogether would be much less desirable because if a user relies on InternetStackHelper to add it, he will figure out that it is not there anymore only at runtime with a less-than-helpful message.

Updating the InternetStackHelper::Install method documentation to describe that the PacketSocket is also aggregated would also be nice.
Comment 2 Gustavo J. A. M. Carneiro 2008-08-05 12:34:17 EDT
(In reply to comment #1)
> PacketSocketFactory is added in PacketSocketHelper if you want to only packet
> sockets on your node.

OK, this is even worse than I thought!

PacketSocketHelper should be added to the class Node.  Every node supports L2 networking, therefore every node should support L2 sockets.  It was a nasty surprise to me to find out PacketSocketFactory was not added by default to Node (especially since it used to be added back in ns-3.11).

> The reason it is also added to a node from
> InternetStackHelper is that we wanted to make InternetStackHelper add all the
> things it could by default.
> 
> If you want to add a flag to InternetStackHelper to disable this feature, I
> would be fine with that. Removing it altogether would be much less desirable
> because if a user relies on InternetStackHelper to add it, he will figure out
> that it is not there anymore only at runtime with a less-than-helpful message.
> 
> Updating the InternetStackHelper::Install method documentation to describe that
> the PacketSocket is also aggregated would also be nice.
> 

I hate helpers, and I always this was going to happen.  Ever since the "helpers" layer was added to ns-3, the "internal" layers started to become increasingly more complex because their complexity is hidden by the helpers.  So, no one really cares about internal complexity anymore :-(

Please lets try to keep the internal classes simple to use, and not rely on helpers so much.
Comment 3 Mathieu Lacage 2008-08-05 12:46:26 EDT
(In reply to comment #2)
> (In reply to comment #1)
> > PacketSocketFactory is added in PacketSocketHelper if you want to only packet
> > sockets on your node.
> 
> OK, this is even worse than I thought!
> 
> PacketSocketHelper should be added to the class Node.  Every node supports L2
> networking, therefore every node should support L2 sockets.  It was a nasty

The rationale for the current behavior is that we have tried to move as much object aggregation as possible to the helper API because we have tried to make helpers modelize object aggregation.

> surprise to me to find out PacketSocketFactory was not added by default to Node
> (especially since it used to be added back in ns-3.11).

We don't care about any kind of compatibility with pre-1.0 releases. It is sad that you were surprised but, clearly, we are not going to change this _again_ to surprise more people who now rely on an official release.

> > The reason it is also added to a node from
> > InternetStackHelper is that we wanted to make InternetStackHelper add all the
> > things it could by default.
> > 
> > If you want to add a flag to InternetStackHelper to disable this feature, I
> > would be fine with that. Removing it altogether would be much less desirable
> > because if a user relies on InternetStackHelper to add it, he will figure out
> > that it is not there anymore only at runtime with a less-than-helpful message.
> > 
> > Updating the InternetStackHelper::Install method documentation to describe that
> > the PacketSocket is also aggregated would also be nice.
> > 
> 
> I hate helpers, and I always this was going to happen.  Ever since the
> "helpers" layer was added to ns-3, the "internal" layers started to become
> increasingly more complex because their complexity is hidden by the helpers. 
> So, no one really cares about internal complexity anymore :-(

I fail to see the kind of internal complexity you are talking about. If you don't want to use the helpers, you have to perform object aggregation yourself. 

> Please lets try to keep the internal classes simple to use, and not rely on
> helpers so much.

Object aggregation has been moved to helpers and it won't move back to internal classes.

Marking this bug as invalid and opening a new one to keep track of the fact that the API documentation is not clear on this subject.
Comment 4 Mathieu Lacage 2008-08-05 12:51:39 EDT
see bug 272
Comment 5 Gustavo J. A. M. Carneiro 2008-08-05 13:22:32 EDT
(In reply to comment #3)
[...]
> We don't care about any kind of compatibility with pre-1.0 releases. It is sad
> that you were surprised but, clearly, we are not going to change this _again_
> to surprise more people who now rely on an official release.

It is not about compatibility, it is about being logical.
[...]
> 
> I fail to see the kind of internal complexity you are talking about. If you
> don't want to use the helpers, you have to perform object aggregation yourself. 

The above sentence is the kind of complexity I am talking about.

If I want to use a UDP socket, I don't need to manually add a UdpSocketFactory.  It is provided in the "internet stack".  Like wise, if I wanted to use a packet socket, it should be provided by the "node stack".  It just turns out that "node stack" == Node.
Comment 6 Mathieu Lacage 2008-08-05 13:58:36 EDT
(In reply to comment #5)
> (In reply to comment #3)
> [...]
> > We don't care about any kind of compatibility with pre-1.0 releases. It is sad
> > that you were surprised but, clearly, we are not going to change this _again_
> > to surprise more people who now rely on an official release.
> 
> It is not about compatibility, it is about being logical.
> [...]

I understand that what currently happens is not what you expected but I explained the rationale for that behavior so, please, don't try to argue for "logic". There is logic. It just happens that you want another one.

> > 
> > I fail to see the kind of internal complexity you are talking about. If you
> > don't want to use the helpers, you have to perform object aggregation yourself. 
> 
> The above sentence is the kind of complexity I am talking about.
> 
> If I want to use a UDP socket, I don't need to manually add a UdpSocketFactory.
>  It is provided in the "internet stack".  Like wise, if I wanted to use a
> packet socket, it should be provided by the "node stack".  It just turns out
> that "node stack" == Node.

I understand what you want and I explained why it is not so right now. Let me repeat: Node == a standalone object defined by src/node.h. Helper == class to aggregate functionality to Node. PacketSocketFactory == functionality which can be aggregated to a node. Hence, PacketSocketHelper == class to aggregate PacketSocketFactory to node.