[Ns-developers] About real-world application integration

[Mathieu Lacage]

On Mon, 2009-03-16 at 18:59 +0900, Hajime Tazaki wrote:

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

I see: it's weird that I did not see the ipv6 part of the patch. To make
sure I don't forget anything, would you mind send me a list of files you
have modified and files you have added ?

Anyway, I think you can leave this in for now: the tagging code is going
to undergo changes for the ipv4 rework so, this will probably be solved
during in the ipv4 rework.

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

you can:
  - create a mercurial repo with only the patches you want to send me
and I will pull from this repo, review, and push in mathieu/ns-3-simu

  - or send me raw patches by email (mercurial has a patchbomb
extension:
http://www.selenic.com/mercurial/wiki/index.cgi/PatchbombExtension)


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

If it makes no real difference, then, maybe we can drop this ? I really
have never tried to profile the behavior of the process-manager code so,
I have no idea how it behaves: I would tend to trust you if you did
profile it and if you think it is worth being done :)

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

I meant that I don't really understand what this patch is doing: it
seems that it's really doing the same thing as before, only differently.

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

I honestly have no idea. I usually do all the coding style formatting by
hand. The emacs gnu mode helps but it does not do everything.

Mathieu