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

Paul Jakma paul at clubi.ie
Tue Apr 27 22:41:50 BST 2004


On Tue, 27 Apr 2004, Andrew J. Schorr wrote:

> I guess you may be right, but INADDR_ANY (zero) is also sometimes
> used to represent default routes, isn't it? 

Yes. But a different context though, in the context of routes.

> 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".

Aye, i'm not quite sure what that's about. I'm not sure when you'd 
have interfaces with IN.ADDR_ANY as local address - tunnel devices 
presumably. Anyway, it doesnt affect our case, PtP's with 
/destination/ set to in.addr_any.

> 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.  

Right, and probably it should create a 0.0.0.0/0 otherwise. 

> So the question is whether it is ever possible for
> connected_add_ipv4() to have a NULL 5th argument.

I couldnt say, in any normal conceivable case I'd suspect not, but 
you never know.

> 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).

KKFor netlink the possibilities are i think restricted to the case of 
local address being null.
 
> 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).

Well, /31 would be terminal, not enough space for broadcast network.

> 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.

It is yes. If we could avoid pointer dereferences altogether, code
would be much faster, but we cant. :) - testing for the address is
the more readable, the more maintainable, and for the vast majority 
of PtP links we will have a destination.

> 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...

Because it's adding magic for the sake of saving one pointer
dereference in interface events. If interface event processing were
already highly optimised, ultra-scaleable, i'd consider it - but they
are not, and it's not worth the magic. And if i did consider it worth
it, i'd probably suggest it would be better to work on improving
locality of the address and destination prefixes to the connected
list.
 
> 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.

I might agree with you, but you can just as easily assert when you
encounter bogus state (as early as possible too).

> created a zero destination address. Whether that's allowed or not
> is beyond the scope of this patch. 

It is yes, that's not a problem. I dont see value in creating an
additional distinction that does not currently exist (NULL of dest
versus 0 value.)

> 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...

Yes saw that.

> 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.

It's used for nexthop calculation. Essentially, 
ospf_if_is_configured() must be able to find the correct 
ospf_interface for an address, where an address could well be the 
link_data field of one of our own links from our own router-lsa for 
a given area.

Ie, you need to make sure that you follow 12.4.1.1, LSA PtP link_data
must == local address or ifindex. (local address == NULL is not on
the cards right).

Ie, you can just update ospf_if_is_configured - as long as it
correctly looks up oi's for a given local addresses. (local address
must be unique for active OSPF interfaces, we cant deal with
'unnumbered' interfaces).

> Regards,
> Andy

regards,
-- 
Paul Jakma	paul at clubi.ie	paul at jakma.org	Key ID: 64A2FF6A
	warning: do not ever send email to spam at dishone.st
Fortune:
Corry's Law:
	Paper is always strongest at the perforations.



More information about the Quagga-dev mailing list