[quagga-dev 1087] Re: point-to-point patch

Andrew J. Schorr aschorr at telemetry-investments.com
Tue Apr 27 19:30:39 BST 2004


On Tue, Apr 27, 2004 at 06:44:34PM +0100, Paul Jakma wrote:
> Ok. I dont see the need to distinguish between destination == 
> INADDR_ANY (or IN6ADDR_ANY for other daemons) and NULL though, 
> IN(6)?ADDR_ANY for destination would seem sufficient to imply "no 
> destination".

I guess you may be right, but INADDR_ANY (zero) is also sometimes used
to represent default routes, isn't it?   Also,
zebra/connected.c:connected_up_ipv4() includes the following comment:

  /* In case of connected address is 0.0.0.0/0 we treat it tunnel
     address. */

So it seems to me that INADDR_ANY has meanings in some situations that might
be different than "address not available".


> > While this was true in the client daemons before my patch, it was
> > never true in the zebra daemon.
> 
> Right, that'd potentially be a problem so. AFAICT, connected.c does 
> create a prefix for destination, regardless. interface.c does not.

I'm not so sure about that.  In zebra/connected.c:connected_add_ipv4(),
the destination prefix is created only if the 5th argument to the
function ("broad") is non-NULL.  So the question is whether
it is ever possible for connected_add_ipv4() to have a NULL 5th argument.
Certainly if you accept my patch to zebra/rt_netlink.c, then this
can happen.  Even without my patch, there's no assurance in the calling 
logic that the "broad" argument won't be NULL.   And the logic
in if_ioctl.c also holds out the possibility of a NULL 5th argument
(if neither IFF_POINTOPOINT nor IFF_BROADCAST were set, which I recognize
is extremely unlikely, but perhaps not impossible).


> > Furthermore, the code in the library and in the daemons was
> > inconsistent: sometimes it tested for a non-NULL pointer, and
> > sometimes it went ahead and used it without checking.  For example,
> > you can see cases where the pointer was checked in
> > lib/if.c:if_lookup_address(), lib/if.c:connected_lookup_address(),
> > ripd/rip_interface.c:if_valid_neighbor(), and
> > ospfd/ospf_lsa.c:lsa_link_ptop_set().
> 
> For the daemons it doesnt quite matter. For zebra, from a glance, it 
> appears only zebra created addresses can have this problem (and zebra 
> can not (properly) create PtP addresses, so..).

That's true, although an ethernet interface using a /31 netmask might
also not have a destination address (I recognize that such a case
is extremely unlikely).

> 
> > For 2 reasons:
> > 
> >   1. I think that it's more elegant to have a consistent approach throughout
> >      the code: the destination pointer may or may not be present.  Why would
> >      we want the zebra and lib code to have to check for NULL destination
> >      pointers, but have the protocol daemons check for a non-zero value?
> >      This hybrid approach certainly does not work for library functions
> >      that can be called from both zebra and the protocol daemons.
> 
> Agreed. However, I think (struct connected *)->destination->s_addr ==
> INADDR_ANY should suffice.

I guess that testing against INADDR_ANY should probably work for IPV4,
but what would one do for IPV6?  IPV6_ADDR_SAME(destination,&in6addr_any)?
That's certainly a much more heavyweight test than checking for a NULL
pointer.

In fact, checking for a NULL pointer is both more elegant and faster.  
So why would you resist the patch?  I've already done the work...

Furthermore, I still think it would be better to crash dereferencing
a NULL pointer than to use a zero value by mistake.  So I think
a NULL pointer would help enforce code correctness.

> Right, but it adds yet another assumption, when the original 
> assumption will suffice. Eg, if we add the NULL case, what then would 
> would !NULL && destination == INADDR_ANY imply?

Well, honestly, there wasn't really an original assumption at all,
the code was entirely ignoring this issue.  If destination is non-NULL
but equals zero, then you know that something (the kernel or the
zebra interface creation) explicitly created a zero destination address.
Whether that's allowed or not is beyond the scope of this patch.  If you'd
like, we can add some additional checks to the zebra daemon to make
sure that this doesn't happen.

Please note that my patch already adds checks to
zebra/connected.c:connected_add_ipv4() to warn about destination addresses
that don't make sense.  If desired, we can have it take action in
those cases...

> > P.S. The actual patch is not as large as it looks, since much of it
> > involves adding debug messages to ospfd.  If you would like, I
> > could resubmit it without those enhanced debugging messages.
> 
> Yes please. You're well along the right track, and the patch will
> look mostly the same, though, why the new ospf_if_is_connected()  
> function, I'd rather not add more duplicated code - there's enough 
> already.

I will try to do this when I get a chance.

The new ospf_if_is_connected() behaves differently than the old
ospf_if_is_configured() for PtP interfaces.  It may be possible
simply to replace the old ospf_if_is_configured() function with the
new one, but I wasn't certain of this.  I was kind of hoping that
somebody who understands the OSPF algorithm better than I do would
take a look at this.  But I can perhaps try making that change
and see what happens.

Regards,
Andy



More information about the Quagga-dev mailing list