[quagga-dev 1083] Re: point-to-point patch
Andrew J. Schorr
aschorr at telemetry-investments.com
Tue Apr 27 17:28:08 BST 2004
On Tue, Apr 27, 2004 at 04:52:54PM +0100, Paul Jakma wrote:
> How do you describe a NULL pointer on the wire? Your patch changes
> the destination address field from a statically sized field ==
> sizeof(prefix length of address family) containing 0's to a single 0
> char? I dont see why that's needed.
The relevant code is in zebra/zserv.c:zsend_interface_address_add():
/* Destination. */
p = ifc->destination;
stream_putc (s, 1);
stream_put (s, &p->u.prefix, blen);
stream_putc (s, 0);
So I basically stick a flag byte in front to indicate whether
a destination address will be following. 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.
> > Also, in zebra/rt_netlink.c:netlink_interface_addr(), I had to add
> > a memcmp to ignore the peer address when it's the same as the local
> > address (this is copied from
> > iproute2/ip/ipaddress.c:print_addrinfo(). Without that patch, zebra
> > thinks that the local address is also the peer address in the
> > situation where a peer address was never assigned.
> > Other than that, it's a question of patching the lib and daemon
> > directories to stop assuming that the connected->destination
> > pointer is non-NULL. This involves some minor patches to ripd and
> > bgpd (not tested), and some more significant patches to ospfd.
> That assumption is correct though.
By that "assumption", I take it you are referring to the idea that
connected->destination will never be NULL? While this was true
in the client daemons before my patch, it was never true in
the zebra daemon. 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(),
> > In general, my patches get the code to behave the same as it
> > did before if the connected->destination address is present and the
> > prefixlen is IPV4_MAX_PREFIXLEN (32). If the destination address
> > is missing, or the prefixlen is not 32, then it is assumed that a
> > specific subnet has been dedicated to the PtP link, and the PtP
> > interface is not identified by the peer address.
> Ok, this assumption is fine. You noted the lack of dereference in
> link_ptop_set() in quagga-users 1821, why not just fix that to
> actually compare the contents to INADDR_ANY? As you noted, that
> function otherwise follows the spec.
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.
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.
> You've done a good bit of investigating, nice on. I question though
> whether such wide-ranging changes are needed, is fixing
> checks on co->destination not enough?
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.
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.
More information about the Quagga-dev