[Ns-developers] About real-world application integration

[Hajime Tazaki]

Hi,

Thanks for your valuable comments.
#even though my source is nasty, a lot of hard coding...

Please find my comments in line.

At Mon, 16 Mar 2009 09:48:51 +0100,
Mathieu Lacage wrote:

>> code, and I will do a more thorough review of your code sometime next
>> week (I am supposed to be away from email this week :).
>
>So, I did look at your code :) A couple of random comments:
>
>1) you checked in a couple of emacs-generated
>files: /scratch/process-zebra.cc~, scratch/process-zebra.py~

Thanks.
Already fixed and pushed at my repo.

>2) I see a couple of patches to the ipv6 code: it would be nice if you
>could contribute these back to sebastien separately.

Okay.
I'll try to.

>3) In src/node/node.cc, what is the use of this extra tag ? Where is it
>used ?
>
>+  SocketRecvIfTag tag;
>+  tag.SetRecvIf (device->GetIfIndex());
>+  packet->AddTag (tag);
>+

This tag is used in unix-datagram-socket.cc to emulate
"IPV6_RECVPKTINFO" sockopt.

+  if(msg->msg_controllen > 0){
+    Cmsg cmsg = Cmsg (msg);
+    if (IsRecvPktInfo ())
+      {
+        struct in6_pktinfo pkt;
+      
+        SocketRecvIfTag tag;
+        bool found;
+        found = packet->FindFirstMatchingTag (tag);
+        NS_ASSERT (found);
+
+        pkt.ipi6_ifindex = tag.GetRecvIf ();
+        memcpy(&pkt.ipi6_addr, msg->msg_name, sizeof(pkt.ipi6_addr));
+        cmsg.Add (SOL_IPV6, IPV6_PKTINFO, sizeof (struct in6_pktinfo), (const uint8_t*)&pkt);
+      }
+    cmsg.Finish ();
+  }

Modifying in node.cc isn't good idea, but I can't find any
other suitable place to do it. Do you have any good idea?

>4) I see a couple of patches to src/process-manager/* which could be
>merged in ns-3-simu right now, so, it would be nice if you could try to
>split them from the rest to try to minimize the number of differences
>between your tree and ns-3-simu. For example, the following looks like a
>good idea and I would be happy to take patches to add them to
>http://code.nsnam.org/mathieu/ns-3-simu

Okay, I'll try it later.
Do you mind if I'll send patches for this modification?
Or using bugzilla is better?


>diff -r 89718962b257 -r 6066282751f1 src/process-manager/alloc.cc
>--- a/src/process-manager/alloc.cc      Fri Feb 27 23:57:07 2009 +0900
>+++ b/src/process-manager/alloc.cc      Wed Mar 11 15:03:07 2009 +0900
>@@ -54,6 +54,7 @@
>   : m_defaultMmapSize (1<<15)
> {
>   NS_LOG_FUNCTION (this);
>+  memset(m_buckets, 0, sizeof(m_buckets));
> }
> KingsleyAlloc::~KingsleyAlloc ()
> {

I can make a patch.

>this too:
>diff -r cc696412ba7e -r 9bd77de2052c
>src/process-manager/unix-datagram-socket-fd.cc
>--- a/src/process-manager/unix-datagram-socket-fd.cc    Fri Feb 27
>23:58:01 2009 +0900
>+++ b/src/process-manager/unix-datagram-socket-fd.cc    Fri Mar 06
>13:25:32 2009 +0900
>@@ -181,7 +181,7 @@
>   Thread *current = Current ();
>   NS_LOG_FUNCTION (this << current);
>   NS_ASSERT (current != 0);
>-  uint32_t old_ifindex;
>+  uint32_t old_ifindex = 0;

I can make a patch, too.

>as well as:
>
>-  process->alloc = new StupidAlloc ();
>-  //  process->alloc = new KingsleyAlloc ();
>+  process->alloc = new KingsleyAlloc ();
>
>and, I assume that this made a bit difference for you:
>
>-  std::vector<std::pair<int,Ptr<UnixFd> > > openFiles;
>+  __gnu_cxx::hash_map<int, Ptr<UnixFd> > openFiles;
>
>but, oh, look at src/internet-stack/sgi-hashmap.h: would you mind move
>it to src/core and use this one instead ?

I don't mind, I just wanna use hash_map, but I wasn't sure
how I can do it, so just trid to do same as
src/internet-cache/arp-cache.h.

BTW, using hash_map comes from oprofile output to improve
performance, but it makes not so big differences.

>The following looks a bit gratuituous:
>
>diff -r 89718962b257 -r 6066282751f1
>src/process-manager/process-manager.cc
>--- a/src/process-manager/process-manager.cc    Fri Feb 27 23:57:07 2009
>+0900
>+++ b/src/process-manager/process-manager.cc    Wed Mar 11 15:03:07 2009
>+0900
>@@ -109,12 +109,12 @@
> {
>   NS_LOG_FUNCTION (this);
>   GarbageCollectDeadProcessesAndThreads ();
>-  for (std::vector<struct Process *>::iterator i = m_processes.begin
>(); i != m_processes.end (); ++i)
>+  for (std::vector<struct Process *>::iterator i = m_processes.begin
>(); i != m_processes.end ();)
>     {
>       struct Process *process = *i;
>       DeleteProcess (process);
>+      i = m_processes.erase (i);
>     }
>-  m_processes.clear ();

I can make a patch, too.

>This would be nice rewritten with the coding style in mind:
>
>+  if(current->process->openFiles.find(fd) !=
>current->process->openFiles.end())
>+    return fd;
>
>i.e.:
>if ()
>  {
>    // do stuff
>  }

Okay, 
I read http://www.nsnam.org/codingstyle.html right now.

Is "indent -gnu {all files}" good enough?

regards,
hajime