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

Paul Jakma paul at clubi.ie
Tue Apr 27 18:44:34 BST 2004


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

> The relevant code is in zebra/zserv.c:zsend_interface_address_add():
> 
>   /* Destination. */
>   p = ifc->destination;
>   if (p)
>     {
>       stream_putc (s, 1);
>       stream_put (s, &p->u.prefix, blen);
>     }
>   else
>     stream_putc (s, 0);
> 
> So I basically stick a flag byte in front to indicate whether a
> destination address will be following.

Ah yes. missed that.

> The unpacking code in
> lib/zclient.c:zebra_interface_address_add_read() reads the flag
> byte first to decide whether the destination address is following
> or should be instead set to NULL.

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".
  
> By that "assumption", I take it you are referring to the idea that
> connected->destination will never be NULL?  

Correct, it's always filled in by zclient_read_interface_add.

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

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

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

>   2. I was not 100% certain that there will never be a situation where the
>      destination address is zero, so I thought it was important to
>      distinguish between the two cases.

I dont think there's a need for this.
 
> Most of the changes simply check for whether the destination
> pointer is present and the netmask is /32 before using it.  If we
> stick with a zero value in the destination address, we will still
> have to patch all the same spots, it's just a question of whether
> we test for a NULL pointer or for a 0 value.  I think that being
> consistent throughout the code base will reduce the likelihood of
> errors in the future as other people hack on the code.

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?

> Thanks,
> Andy

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

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:
Earth Destroyed by Solar Flare -- film clips at eleven.



More information about the Quagga-dev mailing list