Bug 171 - code bug in function (olsr)AgentImpl::MprComputation() about using std::vector::erase().
code bug in function (olsr)AgentImpl::MprComputation() about using std::vecto...
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: routing
pre-release
All Windows
: P3 minor
Assigned To: ns-bugs
: bug
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-15 08:27 EDT by Liu Jian
Modified: 2008-07-01 13:32 EDT (History)
2 users (show)

See Also:


Attachments
modify code about std::vector::erase() using in AgentImpl::MprComputation() (687 bytes, patch)
2008-04-15 08:34 EDT, Liu Jian
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Liu Jian 2008-04-15 08:27:24 EDT
there were code bug in function AgentImpl::MprComputation() about using std::vector.erase(). (In Version ns-3-dev)
when i run the simple-point-to-point-olsr.cc, exceptions tracked.
 
Commonly, std::vetor::erase() used like this example:
/*****************************************************/
vector<int>   arr;
arr.push_back(8);
for(vector<int>::iterator it=.begin(); it!=arr.end(); )
    {
        if(* it == 8)
        {
            it = arr.erase(it);
        }
        else
        {
            ++it;
        }
    }
/*****************************************************/
Also a patch file attached below.
Comment 1 Liu Jian 2008-04-15 08:34:47 EDT
Created attachment 129 [details]
modify code about std::vector::erase() using in AgentImpl::MprComputation()
Comment 2 Mathieu Lacage 2008-04-15 11:22:16 EDT
adding gustavo to CC list to get feedback.
Comment 3 Gustavo J. A. M. Carneiro 2008-04-15 11:44:33 EDT
First of all, please submit the patch as unified diff (diff -upd); I can't read this patch.  Moreover, from the little I can read from the patch it looks like the patch is reversed?...

Then I must confess I did not write most of the code, simply ported it from UM-OLSR (NS-2) to NS-3, so it is possible a bug or two prevailed...

Incidentally, I did find some code rather strange, even added one or two FIXME comments.  For example:

void
OlsrState::EraseTwoHopNeighborTuples (const Ipv4Address &neighborMainAddr,
                                      const Ipv4Address &twoHopNeighborAddr)
{
  for (TwoHopNeighborSet::iterator it = m_twoHopNeighborSet.begin ();
       it != m_twoHopNeighborSet.end (); it++)
    {
      if (it->neighborMainAddr == neighborMainAddr
          && it->twoHopNeighborAddr == twoHopNeighborAddr)
        {
          it = m_twoHopNeighborSet.erase (it);
          it--; // FIXME: is this correct in the case 'it' pointed to the first element?
          m_modified = true;
        }
    }
}

But, since the code appeared to work, even when simulating with hundreds or thousands of nodes, when in doubt I chose to let it be.  If you say it is giving segfaults then I'm all for fixing the code, as soon as you provided a proper patch.  Thanks in advance.
Comment 4 Liu Jian 2008-04-15 20:57:09 EDT
(In reply to comment #3)
> First of all, please submit the patch as unified diff (diff -upd); I can't read
> this patch.  Moreover, from the little I can read from the patch it looks like
> the patch is reversed?...
sorry about that.
> Then I must confess I did not write most of the code, simply ported it from
> UM-OLSR (NS-2) to NS-3, so it is possible a bug or two prevailed...
> Incidentally, I did find some code rather strange, even added one or two FIXME
> comments.  For example:
> void
> OlsrState::EraseTwoHopNeighborTuples (const Ipv4Address &neighborMainAddr,
>                                       const Ipv4Address &twoHopNeighborAddr)
> {
>   for (TwoHopNeighborSet::iterator it = m_twoHopNeighborSet.begin ();
>        it != m_twoHopNeighborSet.end (); it++)
>     {
>       if (it->neighborMainAddr == neighborMainAddr
>           && it->twoHopNeighborAddr == twoHopNeighborAddr)
>         {
>           it = m_twoHopNeighborSet.erase (it);
>           it--; // FIXME: is this correct in the case 'it' pointed to the first
> element?

here when the vector.size()== 1, 'it' still point to the first emlemnt, then "it--" will give exception.
when i tracked ns-3-dev sample code, i does run abnormally.
>           m_modified = true;
>         }
>     }
> }


> But, since the code appeared to work, even when simulating with hundreds or
> thousands of nodes, when in doubt I chose to let it be.  If you say it is
> giving segfaults then I'm all for fixing the code, as soon as you provided a
> proper patch.  Thanks in advance.

Comment 5 Gustavo J. A. M. Carneiro 2008-04-16 05:25:18 EDT
-> http://code.nsnam.org/ns-3-dev/rev/5d4ff983595b