Bug 2423 - Create socket trace for applications
Create socket trace for applications
Status: CONFIRMED
Product: ns-3
Classification: Unclassified
Component: applications
ns-3.25
PC All
: P5 enhancement
Assigned To: George Riley
:
Depends on: 2106
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-25 12:35 EDT by Juan
Modified: 2016-05-30 09:24 EDT (History)
3 users (show)

See Also:


Attachments
add_app_socket_creation_trace (3.09 KB, patch)
2016-05-25 12:35 EDT, Juan
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Juan 2016-05-25 12:35:06 EDT
Created attachment 2455 [details]
add_app_socket_creation_trace

This patch adds the "CreateSocket" trace to the bulksend and onoff applications to be able to control the socket creation and even modify parameters of the socket.
Comment 1 natale.patriciello 2016-05-25 14:27:57 EDT
Comments on the patch:

*) It does not respect the ns-3 coding standard (you miss the space between function name and parenthesis)

Comments on the method:

*) I generally agree on adding this feature to applications. However, in my code I added a method "SetSocket" that supplies an user-created socket to the application, to eliminate the need to connect to a trace source that is used only one time. Comments on this approach ?
Comment 2 Juan 2016-05-26 04:04:20 EDT
Thanks for the comments!. I also thought in adding a setsocket method to the application and I agree for the bulksend and the onoff application there is no much difference. However, other applications migth use more than one socket and in this case the callback approach I think works better. Besides I still think it is sligthly "cleaner" to do it with the callback if you are using helpers to install the applications but that is just because the way I'm used to code. 

Let me know what you think, and I will upload a new patch.
Comment 3 natale.patriciello 2016-05-26 04:35:57 EDT
You're welcome!
Well, I believe that both approach are worth to implement, with SetSocket that clearly works with these two apps (as you said) and the Callback that works for everything. I mean, it's not dangerous the leave the possibility to the user to set his/her own socket in the application (use case: use different TCP implementation on different applications, nsc/ns3/dce).

Another thing is that I prefer to have common properties / traces defined in the common class, to reduce the risk that in the future one developer add the same functionality with a different name. So, CreateSocket trace should (but this is my opinion) go in the base class, with a protected method (called, let's say, NotifySocketCreated()) that derived class must call after the creation of each socket.

Opinions?
Nat
Comment 4 Juan 2016-05-26 05:13:38 EDT
I fully agree with that. Going straight to the implementation I think there are 3 options

a) Modify the Application to add the common functionality
b) Add a new class and use multiple inheritance 
c) Change the applications to inherit only from a new class which will itself inherit from Application

Im reluctant to touch the application class and I think option c) is a little messy so I would go for the b) option. However, as Im quite new to ns3 I have my doubts on how to implement the gettypeid of the applications to include the typeids of both parents (to register the callbacks). I quickly checked the SetParent of the TypeId class and correct me if Im wrong but I think the method only keeps one parent.
Comment 5 Tommaso Pecorella 2016-05-29 16:22:10 EDT
(In reply to Juan from comment #4)
> I fully agree with that. Going straight to the implementation I think there
> are 3 options
> 
> a) Modify the Application to add the common functionality
> b) Add a new class and use multiple inheritance 
> c) Change the applications to inherit only from a new class which will
> itself inherit from Application
> 
> Im reluctant to touch the application class and I think option c) is a
> little messy so I would go for the b) option. However, as Im quite new to
> ns3 I have my doubts on how to implement the gettypeid of the applications
> to include the typeids of both parents (to register the callbacks). I
> quickly checked the SetParent of the TypeId class and correct me if Im wrong
> but I think the method only keeps one parent.

On the opposite, I'd go for option (a), modify the Application class.
The various applications needs to check if the callback has been set tho.
Comment 6 Juan 2016-05-30 04:43:07 EDT
OK! I'll modify the Application class, but I dont get what you say about the applications checking if the callback has been set or not.
Comment 7 natale.patriciello 2016-05-30 05:08:22 EDT
(In reply to Juan from comment #6)
> OK! I'll modify the Application class, but I dont get what you say about the
> applications checking if the callback has been set or not.

if (!m_callback.IsNull())
  m_callback ()

I agree on editing the Application class, adding the CreateSocket trace. Please, update the doxygen documentation and the model documentation accordingly.
Comment 8 Juan 2016-05-30 05:15:11 EDT
Ok, no problem. Another question: Does it make sense to have a SetSocket in the Application class ? I would add it directly to the child classes
Comment 9 natale.patriciello 2016-05-30 05:21:13 EDT
(In reply to Juan from comment #8)
> Ok, no problem. Another question: Does it make sense to have a SetSocket in
> the Application class ? I would add it directly to the child classes

Concentrate on one problem at time .. CreateSocket is enough for one patch :)

we will talk about SetSocket later, since the approach has other problems (pointed out by Tommaso in the other bug)
Comment 10 Juan 2016-05-30 09:14:50 EDT
(In reply to natale.patriciello from comment #7)
> (In reply to Juan from comment #6)
> > OK! I'll modify the Application class, but I dont get what you say about the
> > applications checking if the callback has been set or not.
> 
> if (!m_callback.IsNull())
>   m_callback ()
> 
> I agree on editing the Application class, adding the CreateSocket trace.
> Please, update the doxygen documentation and the model documentation
> accordingly.

Should I use Callback or TracedCallback? I have been looking similar cases and all of them use TracedCallbacks instead (which already checks if the list of callbacks is empty).
Comment 11 natale.patriciello 2016-05-30 09:24:51 EDT
(In reply to Juan from comment #10)
> Should I use Callback or TracedCallback? I have been looking similar cases
> and all of them use TracedCallbacks instead (which already checks if the
> list of callbacks is empty).

Since the number of connected callbacks can vary (from 0 to n) I believe TracedCallback is the right choice.