Bug 678

Summary: InternetStackHelper::SetRoutingHelper does evil things
Product: ns-3 Reporter: Gustavo J. A. M. Carneiro <gjcarneiro>
Component: helpersAssignee: Tom Henderson <tomh>
Status: RESOLVED FIXED    
Severity: major CC: craigdo, mathieu.lacage
Priority: P1    
Version: ns-3-dev   
Hardware: All   
OS: All   
Attachments: patch to fix

Description Gustavo J. A. M. Carneiro 2009-09-18 06:16:34 EDT 
void 
InternetStackHelper::SetRoutingHelper (const Ipv4RoutingHelper &routing)
{
  m_routing = &routing;
}

It takes the address of an existing object, instead of copying it.  It means I cannot do this:

InternetStackHelper internet;
internet.SetRoutingHelper (OlsrHelper())

The above code will crash (due to a pointer to a temporary stack allocated object), and an inexperienced programmer will have no idea why.
Comment 1 Tom Henderson 2009-09-30 01:43:37 EDT 
The basic problem here is that we seem to need a virtual constructor for a stack-allocated helper.  Traditional ns-3 virtual constructor using CopyObject do not seem to apply if we want to keep these non-Ptr based.

If we want to keep the API to be 
  InternetStackHelper::SetRoutingHelper (const Ipv4RoutingHelper &routing);

and allow the user to pass anonymous temporary objects, then, we could have a solution along these lines:

class Ipv4RoutingHelper
{
public:
  virtual Ipv4RoutingHelper* Copy (void) const = 0;
}

class Ipv4ListRoutingHelper : public Ipv4RoutingHelper
{
public:
  Ipv4ListRoutingHelper ();
  Ipv4ListRoutingHelper (const Ipv4ListRoutingHelper &);
  Ipv4ListRoutingHelper &operator = (const Ipv4ListRoutingHelper &o);
  Ipv4ListRoutingHelper* Copy (void) const 
    { return new Ipv4ListRoutingHelper (*this); }


The problem with this is that it deviates from our memory management convention of not passing memory ownership across APIs using raw pointers.  

Am not seeing another clean alternative at the moment-- other ideas?
Comment 2 Mathieu Lacage 2009-09-30 06:58:31 EDT 
(In reply to comment #1)

> class Ipv4ListRoutingHelper : public Ipv4RoutingHelper
> {
> public:
>   Ipv4ListRoutingHelper ();
>   Ipv4ListRoutingHelper (const Ipv4ListRoutingHelper &);
>   Ipv4ListRoutingHelper &operator = (const Ipv4ListRoutingHelper &o);
>   Ipv4ListRoutingHelper* Copy (void) const 
>     { return new Ipv4ListRoutingHelper (*this); }

> Am not seeing another clean alternative at the moment-- other ideas?

This is exactly what I expected to do here.
Comment 3 Tom Henderson 2009-10-01 01:49:20 EDT 
Created attachment 613 [details]
patch to fix

This patch fixes this bug along the lines of the previous comment, for both IPv4 and IPv6.  ./waf --valgrind --regression is clean.
Comment 4 Mathieu Lacage 2009-10-01 03:34:30 EDT 
Comment on attachment 613 [details]
patch to fix

>+Ipv4GlobalRoutingHelper &
>+Ipv4GlobalRoutingHelper::operator = (const Ipv4GlobalRoutingHelper &o)
>+{
>+  return *this;
>+}

Do you really need this ? Can't you just declare this operator private to disable it ?

Anyway, the patch looks good to me: +1.
Comment 5 Tom Henderson 2009-10-02 01:50:33 EDT 
(In reply to comment #4)
> (From update of attachment 613 [details])
> >+Ipv4GlobalRoutingHelper &
> >+Ipv4GlobalRoutingHelper::operator = (const Ipv4GlobalRoutingHelper &o)
> >+{
> >+  return *this;
> >+}
> 
> Do you really need this ? Can't you just declare this operator private to
> disable it ?
> 
> Anyway, the patch looks good to me: +1.
> 

I don't need them, so I privatized them all.  changeset a63513286aa0.  Thanks for reviewing it.