[Xorp-hackers] Question on RIB

[Pavlin Radoslavov]

Ben Greear <greearb at candelatech.com> wrote:

> I found the problem:  The Vif reuse logic wasn't quite right:
> (rib.cc)
> 
> @@ -509,14 +524,18 @@
>          new_rib_vif->set_deleted(false);
>          _deleted_vifs.erase(vi);
>          new_rib_vif->copy_in(vif);
> +       _vifs[vifname] = new_rib_vif;
> +       debug_msg("Reused previously deleted vif\n");
>       } else {
>          // Create a new vif

Nice catch!
The fix (slightly modified) is applied to CVS.

> I'm attaching a full patch with more debugging logic for rib
> in case you'd care to apply it.

Adding that amount of debug messages to CVS is probably an overkill
and makes the current code more difficult to read.
For the time being I'd rather leave it out. If we keep seeing more
and more bugs in that part of the code then I might add it to
facilitate the bug chasing.

Thanks,
Pavlin

> Thanks,
> Ben
> 
> -- 
> Ben Greear <greearb at candelatech.com>
> Candela Technologies Inc  http://www.candelatech.com
> 
> Index: rib.cc
> ===================================================================
> RCS file: /cvs/xorp/rib/rib.cc,v
> retrieving revision 1.68
> diff -u -r1.68 rib.cc
> --- rib.cc	3 Oct 2007 00:05:48 -0000	1.68
> +++ rib.cc	26 Oct 2007 22:35:44 -0000
> @@ -495,9 +507,12 @@
>      map<string, RibVif*>::iterator vi;
>      RibVif* new_rib_vif = NULL;
>  
> -    debug_msg("RIB::new_vif: %s\n", vifname.c_str());
> -    if (_vifs.find(vifname) != _vifs.end())
> +    debug_msg("RIB::new_vif: this: %p  name: %s %s\n",
> +	      this, name().c_str(), vifname.c_str());
> +    if (_vifs.find(vifname) != _vifs.end()) {
> +	debug_msg("RIB::new_vif, vif %s already exists.\n", vifname.c_str());
>  	return XORP_ERROR;
> +    }
>  
>      //
>      // If the vif is pending deletion, then reuse it instead
> @@ -509,14 +524,18 @@
>  	new_rib_vif->set_deleted(false);
>  	_deleted_vifs.erase(vi);
>  	new_rib_vif->copy_in(vif);
> +	_vifs[vifname] = new_rib_vif;
> +	debug_msg("Reused previously deleted vif\n");
>      } else {
>  	// Create a new vif
>  	new_rib_vif = new RibVif(this, vif);
>  	_vifs[vifname] = new_rib_vif;
> +	debug_msg("Created new vif\n");
>      }
>      XLOG_ASSERT(new_rib_vif != NULL);
>  
>      if (new_rib_vif->is_underlying_vif_up()) {
> +	debug_msg("Underlying VIF was up\n");
>  	//
>  	// Add the directly connected routes associated with this vif
>  	//
> @@ -535,6 +554,9 @@
>  	    add_connected_route(*new_rib_vif, subnet_addr, addr, peer_addr);
>  	}
>      }
> +    else {
> +	debug_msg("Underlying vif was NOT up, not adding directly connected routes at this time.\n");
> +    }
>  
>      return XORP_OK;
>  }
> @@ -685,6 +707,8 @@
>      if (vi == _vifs.end()) {
>  	XLOG_ERROR("Attempting to add address to non-existant Vif \"%s\"",
>  		   vifname.c_str());
> +	XLOG_ERROR("RIB: this: %p name: %s\n%s\n",
> +		   this, name().c_str(), str().c_str());
>  	return XORP_ERROR;
>      }
>      RibVif* vif = vi->second;
> @@ -1559,6 +1583,23 @@
>  }
>  
>  template <typename A>
> +string
> +RIB<A>::str()
> +{
> +    string rv;
> +    map<string, RibVif*>::iterator iter;
> +
> +    for (iter = _vifs.begin(); iter != _vifs.end(); ++iter) {
> +	rv += iter->first;
> +	rv += ": ";
> +	rv += iter->second->str();
> +	rv += "\n";
> +    }
> +    return rv;
> +}
> +
> +
> +template <typename A>
>  void
>  RIB<A>::print_rib() const
>  {
> Index: rib.hh
> ===================================================================
> RCS file: /cvs/xorp/rib/rib.hh,v
> retrieving revision 1.41
> diff -u -r1.41 rib.hh
> --- rib.hh	3 Oct 2007 00:05:48 -0000	1.41
> +++ rib.hh	26 Oct 2007 22:35:44 -0000
> @@ -486,6 +486,7 @@
>       * Print the RIB structure for debugging
>       */
>      void print_rib() const;
> +    string str();
>  
>      /**
>       * Get RIB name.
> _______________________________________________
> Xorp-hackers mailing list
> Xorp-hackers at icir.org
> http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/xorp-hackers