Bug 587 - ListRoutingProtocol priority handling buggy
ListRoutingProtocol priority handling buggy
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: routing
ns-3-dev
All All
: P5 blocker
Assigned To: ns-bugs
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-10 04:07 UTC by Mathieu Lacage
Modified: 2009-06-21 00:25 UTC (History)
2 users (show)

See Also:


Attachments
remove stray '-' (558 bytes, patch)
2009-06-10 04:07 UTC, Mathieu Lacage
Details | Diff
patch to fix MacOS crappy gcc (1.47 KB, patch)
2009-06-19 17:06 UTC, Tommaso Pecorella
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mathieu Lacage 2009-06-10 04:07:54 UTC
Created attachment 463 [details]
remove stray '-'

It appears that there is a stray '-' in src/internet-stack/ipv4-list-routing-impl.cc
Comment 1 Mathieu Lacage 2009-06-10 04:08:30 UTC
Note: this changes reference traces for the dynamic global routing test
Comment 2 Mathieu Lacage 2009-06-10 05:46:56 UTC
I should point out that if this is not a bug (which I think I sort of could understand), then, the bug is in 

Ipv4ListRoutingImpl::GetRoutingProtocol:

          priority = (*rprotoIter).first;

should be instead:

          priority = -(*rprotoIter).first;
Comment 3 Mathieu Lacage 2009-06-10 05:48:40 UTC
but, really, it would make sense not to invert the meaning of the priority field and, instead, either use an explicit sorting method which sorts from bigger to smaller or iterate over the list in reverse order with rbegin and rend
Comment 4 Tom Henderson 2009-06-10 10:20:24 UTC
(In reply to comment #0)
> Created an attachment (id=463) [details]
> remove stray '-'
> 
> It appears that there is a stray '-' in
> src/internet-stack/ipv4-list-routing-impl.cc
> 


The above was copied from Ipv4L3Protocol::AddRoutingProtocol, which I tested a while back.

(In reply to comment #2)
> I should point out that if this is not a bug (which I think I sort of could
> understand), then, the bug is in 
> 
> Ipv4ListRoutingImpl::GetRoutingProtocol:
> 
>           priority = (*rprotoIter).first;
> 
> should be instead:
> 
>           priority = -(*rprotoIter).first;
> 

I think the above is the bug.

(In reply to comment #3)
> but, really, it would make sense not to invert the meaning of the priority
> field and, instead, either use an explicit sorting method which sorts from
> bigger to smaller or iterate over the list in reverse order with rbegin and
> rend
> 

OK
Comment 5 Tom Henderson 2009-06-14 00:54:30 UTC
Proposed fix here
http://code.nsnam.org/tomh/ns-3-dev-routing/rev/dfc471e2c2e3
Comment 6 Mathieu Lacage 2009-06-14 02:23:08 UTC
+1
Comment 7 Tom Henderson 2009-06-19 11:08:14 UTC
changeset 8539f55c6b55
Comment 8 Tommaso Pecorella 2009-06-19 14:57:07 UTC
MacOSX compiler isn't keen with the changes made to solve this bug.

../src/internet-stack/ipv4-list-routing.cc: In member function ‘virtual ns3::Ptr<ns3::Ipv4Route> ns3::Ipv4ListRouting::RouteOutput(const ns3::Ipv4Header&, uint32_t, ns3::Socket::SocketErrno&)’:
../src/internet-stack/ipv4-list-routing.cc:80: error: no match for ‘operator!=’ in ‘i != std::list<_Tp, _Alloc>::rend() [with _Tp = std::pair<int16_t, ns3::Ptr<ns3::Ipv4RoutingProtocol> >, _Alloc = std::allocator<std::pair<int16_t, ns3::Ptr<ns3::Ipv4RoutingProtocol> > >]()’
debug/ns3/type-id.h:406: note: candidates are: bool ns3::operator!=(ns3::TypeId, ns3::TypeId)
debug/ns3/address.h:230: note:                 bool ns3::operator!=(const ns3::Address&, const ns3::Address&)
debug/ns3/ipv4-address.h:278: note:                 bool ns3::operator!=(const ns3::Ipv4Address&, const ns3::Ipv4Address&)
debug/ns3/ipv4-address.h:294: note:                 bool ns3::operator!=(const ns3::Ipv4Mask&, const ns3::Ipv4Mask&)
debug/ns3/ipv6-address.h:405: note:                 bool ns3::operator!=(const ns3::Ipv6Address&, const ns3::Ipv6Address&)
debug/ns3/ipv6-address.h:422: note:                 bool ns3::operator!=(const ns3::Ipv6Prefix&, const ns3::Ipv6Prefix&)

I know it's a specific MacOS gcc issue, but still...
Comment 9 Tommaso Pecorella 2009-06-19 17:06:58 UTC
Created attachment 474 [details]
patch to fix MacOS crappy gcc

It seems that MacOS gcc is particularly unhappy to assign an rend() iterator to a const_reverse_iterator in some cases.

The patch just removes the const_ from the [some] of the iterators to make gcc happy again.

Cheers
Comment 10 Tom Henderson 2009-06-19 17:41:33 UTC
(In reply to comment #9)
> Created an attachment (id=474) [details]
> patch to fix MacOS crappy gcc
> 
> It seems that MacOS gcc is particularly unhappy to assign an rend() iterator to
> a const_reverse_iterator in some cases.
> 
> The patch just removes the const_ from the [some] of the iterators to make gcc
> happy again.
> 
> Cheers
> 

Hi Tommaso,
Can you please check whether changeset 5c6e1f086a36 fixes this?

I guess while you were preparing a solution I was testing an alternate one that avoids reverse iterator and just passes a comparison operator to sort().  It passes for me on my max os x gcc-4.2.4.  I see there is a MinGW bug on this too.

Thanks for pointing this out and fixing it too.
Comment 11 Tommaso Pecorella 2009-06-19 19:39:59 UTC
(In reply to comment #10)
> Hi Tommaso,
> Can you please check whether changeset 5c6e1f086a36 fixes this?
> 
> I guess while you were preparing a solution I was testing an alternate one that
> avoids reverse iterator and just passes a comparison operator to sort().  It
> passes for me on my max os x gcc-4.2.4.  I see there is a MinGW bug on this
> too.
> 
> Thanks for pointing this out and fixing it too.


changeset 5c6e1f086a36 fixes it perfectly, and it's way more clean too.

Thank you,

Tommaso