Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Quagga>
  3. Dev
1 2  View All
point-to-point patch
aschorr at telemetry-investments
Apr 26, 2004, 1:16 PM
Post #1 of 30 (4110 views)
Permalink
Hi,

I am attaching a patch to enhance quagga support for point-to-point
interfaces. Based on my (perhaps incomplete) understanding of
the current code, it is mostly designed to support PtP connections
where the local and peer addresses are borrowed from other interfaces
and specified with a /32 netmask. In the case where one wants to
have a specifically assigned subnet (typically /30) for a PtP link
(not sharing IP addresses with other interfaces), quagga does not
seem to work. Under linux using the iproute2 tools, it is certainly
possible to assign a subnet to a PtP link, and it is optional whether
to specify a peer address in such a case. This patch gets quagga
to support that scenario.

The zebra daemon allows for the possibility of a PtP interface
where the local address is present, but not the peer address.
However, in zebra/zserv.c:zsend_interface_address_add(), the code
translates a NULL destination address pointer into a 0.0.0.0 address.
So the child daemons (e.g. ripd, or ospfd) never receive the
destination address as a NULL pointer, instead they get 0.0.0.0.
So my patch fixes that problem.

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.

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.

Most of the changes to ospfd were to add more debugging statements so
that I could see what was going on. The signicant changes were in
ospf_interface.c to define a new function ospf_if_is_connected,
and then to ospf_spf.c:ospf_nexthop_calculation() to call the new
function instead of ospf_if_is_configured(). And I patched
ospf_lsa.c:lsa_link_ptop_set() to ignore the destination address
if the prefixlen is not 32.

So with this patch, one now has the option of configuring the PtP
interface with a dedicated subnet, just as is possible with Cisco IOS.
In that scenario, one no longer has the strange routes through the
remote router to the local PtP interface.

This patch was developed against quagga-0.96.4 under Linux 2.4.22.
I then forward-ported it to apply against this morning's CVS snapshot.
I have not tested with the CVS code, but it does compile.

I hope somebody finds this patch helpful, and it would be great if it
could be integrated into CVS. I am very interested in any comments you
may have, positive or negative; I know this is very desirable functionality
for our site, and I hope others are interested.

-Andy

Attached Files:

Re: point-to-point patch [ In reply to ]
hasso at estpak
Apr 27, 2004, 5:20 AM
Post #2 of 30 (4098 views)
Permalink
Andrew J. Schorr wrote:
> Hi,
>
> I am attaching a patch to enhance quagga support for point-to-point
> interfaces. Based on my (perhaps incomplete) understanding of
> the current code, it is mostly designed to support PtP connections
> where the local and peer addresses are borrowed from other
> interfaces and specified with a /32 netmask. In the case where one
> wants to have a specifically assigned subnet (typically /30) for a
> PtP link (not sharing IP addresses with other interfaces), quagga
> does not seem to work. Under linux using the iproute2 tools, it is
> certainly possible to assign a subnet to a PtP link, and it is
> optional whether to specify a peer address in such a case. This
> patch gets quagga to support that scenario.

I read only briefly patch and IMHO it makes sense. My understanding
about topic might not be good as well, as I never used Quagga with
PtP interfaces myself. No time to test it though, because I have no
time for Quagga until end of May.

Anyway, any test reports from users having problems and/or using PtP
interfaces are most needed.

Thanks for your work, Andrew.

--
Hasso Tepper
Elion Enterprises Ltd.
WAN administrator
Re: point-to-point patch [ In reply to ]
aschorr at telemetry-investments
Apr 27, 2004, 8:07 AM
Post #3 of 30 (4099 views)
Permalink
On Tue, Apr 27, 2004 at 03:20:41PM +0300, Hasso Tepper wrote:
> I read only briefly patch and IMHO it makes sense. My understanding
> about topic might not be good as well, as I never used Quagga with
> PtP interfaces myself. No time to test it though, because I have no
> time for Quagga until end of May.
>
> Anyway, any test reports from users having problems and/or using PtP
> interfaces are most needed.

Yes, feedback from other testers would be great, particularly for the ospfd
code (which had the most changes).

Also, I have made a couple of minor tweaks (impacting only ripd) to improve
support for /31 netmasks on PtP links (as in RFC 3021); I will post if people
are interested.

Thanks,
Andy

P.S. I realize my patch did not include ChangeLog and NEWS entries, I can
post those as needed...
Re: point-to-point patch [ In reply to ]
paul at clubi
Apr 27, 2004, 8:52 AM
Post #4 of 30 (4102 views)
Permalink
Hi Andrew,

On Mon, 26 Apr 2004, Andrew J. Schorr wrote:

> Hi,
>
> I am attaching a patch to enhance quagga support for point-to-point
> interfaces. Based on my (perhaps incomplete) understanding of the
> current code, it is mostly designed to support PtP connections
> where the local and peer addresses are borrowed from other
> interfaces and specified with a /32 netmask. In the case where one
> wants to have a specifically assigned subnet (typically /30) for a
> PtP link (not sharing IP addresses with other interfaces), quagga
> does not seem to work. Under linux using the iproute2 tools, it is
> certainly possible to assign a subnet to a PtP link, and it is
> optional whether to specify a peer address in such a case. This
> patch gets quagga to support that scenario.
>
> The zebra daemon allows for the possibility of a PtP interface
> where the local address is present, but not the peer address.
> However, in zebra/zserv.c:zsend_interface_address_add(), the code
> translates a NULL destination address pointer into a 0.0.0.0
> address. So the child daemons (e.g. ripd, or ospfd) never receive
> the destination address as a NULL pointer, instead they get
> 0.0.0.0. So my patch fixes that problem.

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.

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

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

> I hope somebody finds this patch helpful, and it would be great if
> it could be integrated into CVS. I am very interested in any
> comments you may have, positive or negative; I know this is very
> desirable functionality for our site, and I hope others are
> interested.

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?

> -Andy

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
The trouble with opportunity is that it always comes disguised as hard work.
-- Herbert V. Prochnow
Re: point-to-point patch [ In reply to ]
aschorr at telemetry-investments
Apr 27, 2004, 9:28 AM
Post #5 of 30 (4101 views)
Permalink
Hi Paul,

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;
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. 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(),
lib/if.c:connected_lookup_address(), ripd/rip_interface.c:if_valid_neighbor(),
and ospfd/ospf_lsa.c:lsa_link_ptop_set().

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

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.
Re: point-to-point patch [ In reply to ]
aschorr at telemetry-investments
Apr 27, 2004, 9:34 AM
Post #6 of 30 (4097 views)
Permalink
On Tue, Apr 27, 2004 at 12:28:08PM -0400, Andrew J. Schorr wrote:
> 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.
>

And actually, there's a 3rd reason: I would personally prefer to have
the daemon crash from dereferencing a NULL pointer than to have it
use a bogus zero IP address in its routing decisions. If it crashes,
we will find the bug much more quickly.

Thanks,
Andy
Re: point-to-point patch [ In reply to ]
paul at clubi
Apr 27, 2004, 10:44 AM
Post #7 of 30 (4106 views)
Permalink
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@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
Earth Destroyed by Solar Flare -- film clips at eleven.
Re: point-to-point patch [ In reply to ]
aschorr at telemetry-investments
Apr 27, 2004, 11:30 AM
Post #8 of 30 (4105 views)
Permalink
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
Re: point-to-point patch [ In reply to ]
paul at clubi
Apr 27, 2004, 2:41 PM
Post #9 of 30 (4107 views)
Permalink
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@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
Corry's Law:
Paper is always strongest at the perforations.
Re: point-to-point patch [ In reply to ]
gdt at ir
Apr 28, 2004, 6:23 AM
Post #10 of 30 (4098 views)
Permalink
I have not studied all this carefully, but another point is that I
would be reluctant to change the daemon/zebra protocol unless it is
really necessary. It seems that adding a flag for whether the peer
addr is there is a protocol change, and since INADDR_ANY is never a
legitimate address to use (as opposed to part of a prefix that is a
default route), that's a perfectly good value for 'unknown peer addr'.

--
Greg Troxel <gdt@ir.bbn.com>
Re: point-to-point patch [ In reply to ]
aschorr at telemetry-investments
Apr 28, 2004, 8:07 AM
Post #11 of 30 (4102 views)
Permalink
Hi Greg,

On Wed, Apr 28, 2004 at 09:23:37AM -0400, Greg Troxel wrote:
> I have not studied all this carefully, but another point is that I
> would be reluctant to change the daemon/zebra protocol unless it is
> really necessary. It seems that adding a flag for whether the peer
> addr is there is a protocol change, and since INADDR_ANY is never a
> legitimate address to use (as opposed to part of a prefix that is a
> default route), that's a perfectly good value for 'unknown peer addr'.

I'm sympathetic to not wanting to change the wire protocol, since I guess
that would allow independent upgrading of various daemons. But there
is an easy fix to that problem:
in lib/zclient.c:zebra_interface_address_add_read(), instead of reading
a flag byte, one can simply test the address after reading it in,
and, if it's all zeroes, just replace it with a NULL pointer. In fact,
that's the way I coded it up the first time, but then I got nervous
about whether INADDR_ANY could ever be a valid destination address.
So I don't think wire protocol compatibility bears on the larger question
of whether to use NULL pointers or zero destination addresses.

When I developed this patch, I based it on the behavior of the existing
code. Currently, the software either checks for a NULL destination pointer
or completely ignores the question of whether a valid destination address
is available and goes ahead and uses it without checking. So I decided
to make the software consistent and always check for a NULL pointer before
using the destination address.

I may be wrong, but I do not believe that there is any existing code that
checks for a zero (INADDR_ANY) destination address. So if you guys would
like to shift to that paradigm, it will actually be a larger patch.
Here's what the new patch would look like:
1. Whereever struct connected's are created, code will have to be added
to create an INADDR_ANY destination address if none was supplied.
2. Whereever the current code tests for a NULL destination address
(for example, zebra/interface.c:connected_dump_vty()), that will
have to be replaced by a test for INADDR_ANY or IN6ADDR_ANY, depending
on the prefix family.
3. Whereever the current code was using the destination address without
checking, it will have to be patched to test for INADDR_ANY or
IN6ADDR_ANY, depending on the context or prefix family.
My current patch only includes the 3rd part, plus the actual coding
of part 3 is simpler because you don't have to worry about the prefix family.

So that's why I think NULL pointers are the right way to go: I think
it's a smaller patch that is more consistent with the existing behavior.
But you guys are in charge, so I defer to your judgement.

Regards,
Andy
Re: point-to-point patch [ In reply to ]
gdt at ir
Apr 28, 2004, 8:44 AM
Post #12 of 30 (4104 views)
Permalink
> I'm sympathetic to not wanting to change the wire protocol, since I guess
> that would allow independent upgrading of various daemons. But there
> is an easy fix to that problem:
> in lib/zclient.c:zebra_interface_address_add_read(), instead of reading
> a flag byte, one can simply test the address after reading it in,
> and, if it's all zeroes, just replace it with a NULL pointer. In fact,
> that's the way I coded it up the first time, but then I got nervous
> about whether INADDR_ANY could ever be a valid destination address.
> So I don't think wire protocol compatibility bears on the larger question
> of whether to use NULL pointers or zero destination addresses.

That's a good point, and my real issue was with the format change, so
that sounds reasonable.

> When I developed this patch, I based it on the behavior of the existing
> code. Currently, the software either checks for a NULL destination pointer
> or completely ignores the question of whether a valid destination address
> is available and goes ahead and uses it without checking. So I decided
> to make the software consistent and always check for a NULL pointer before
> using the destination address.

That sounds good.

> I may be wrong, but I do not believe that there is any existing code that
> checks for a zero (INADDR_ANY) destination address. So if you guys would
> like to shift to that paradigm, it will actually be a larger patch.
> Here's what the new patch would look like:
> 1. Whereever struct connected's are created, code will have to be added
> to create an INADDR_ANY destination address if none was supplied.
> 2. Whereever the current code tests for a NULL destination address
> (for example, zebra/interface.c:connected_dump_vty()), that will
> have to be replaced by a test for INADDR_ANY or IN6ADDR_ANY, depending
> on the prefix family.
> 3. Whereever the current code was using the destination address without
> checking, it will have to be patched to test for INADDR_ANY or
> IN6ADDR_ANY, depending on the context or prefix family.
> My current patch only includes the 3rd part, plus the actual coding
> of part 3 is simpler because you don't have to worry about the prefix family.
>
> So that's why I think NULL pointers are the right way to go: I think
> it's a smaller patch that is more consistent with the existing behavior.

I don't have an issue with NULL pointers; they are semantically
sensible.

I would like to see the rules about null pointer semantics, and use of
structures with it in comments, and probably mentioned in the
ChangeLog as well.

> But you guys are in charge, so I defer to your judgement.

Thanks, but there are four of us, and probably six opinions...
Re: point-to-point patch [ In reply to ]
paul at clubi
Apr 28, 2004, 9:07 AM
Post #13 of 30 (4109 views)
Permalink
On Wed, 28 Apr 2004, Andrew J. Schorr wrote:

> is an easy fix to that problem: in
> lib/zclient.c:zebra_interface_address_add_read(), instead of
> reading a flag byte, one can simply test the address after reading
> it in, and, if it's all zeroes, just replace it with a NULL
> pointer.

Yes, that's what I assumed you were trying to do, not realising you
had added the flag byte.

> In fact, that's the way I coded it up the first time, but then I
> got nervous about whether INADDR_ANY could ever be a valid
> destination address.

Nope. :)

> I may be wrong, but I do not believe that there is any existing code that
> checks for a zero (INADDR_ANY) destination address. So if you guys would
> like to shift to that paradigm, it will actually be a larger patch.

Yep. Though, I suspect it's a thinko in the original code. The intent
was to dereference destination i suspect (though the test for NULL
would have to be on (struct connected *)->destination.s_addr, and 0
isnt NULL either.), hard to know for sure.

> Here's what the new patch would look like:

> 1. Whereever struct connected's are created, code will have to be added
> to create an INADDR_ANY destination address if none was supplied.
> 2. Whereever the current code tests for a NULL destination address
> (for example, zebra/interface.c:connected_dump_vty()), that will
> have to be replaced by a test for INADDR_ANY or IN6ADDR_ANY, depending
> on the prefix family.
> 3. Whereever the current code was using the destination address without
> checking, it will have to be patched to test for INADDR_ANY or
> IN6ADDR_ANY, depending on the context or prefix family.

Right.

> So that's why I think NULL pointers are the right way to go: I
> think it's a smaller patch that is more consistent with the
> existing behavior. But you guys are in charge, so I defer to your
> judgement.

Well, you're the one looking into the problem. My gut feeling though
is that always treating the destination as an address is more
intuitative (with IN?ADDR_ANY meaning 'no destination'), and having
the code treat it as such will aid the next person who comes along
and tries to read the code.

Functionally, either way will work fine and NULL pointer as 'no
destination' will mean less churn, agreed.

Basically, what makes more sense to people? 'no destination' being
IN?ADDR_ANY for the address or having destination be NULL?

The only other thought I'd have on the subject is that if someone
were to try improve locality of struct connected (eg by moving the
prefixes into the struct itself), the IN?ADDR_ANY method would not
break. (apart from changing the method of reference to (struct
prefix)destination in struct connected in the code). (the concept of
'no destination' == IN?ADDR_ANY prefix remains valid).

> Regards,
> Andy

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
A billion here, a billion there -- pretty soon it adds up to real money.
-- Sen. Everett Dirksen, on the U.S. defense budget
Re: point-to-point patch [ In reply to ]
aschorr at telemetry-investments
Apr 28, 2004, 10:42 AM
Post #14 of 30 (4099 views)
Permalink
On Wed, Apr 28, 2004 at 12:45:01PM -0400, Greg Troxel wrote:
> I'm happy to defer to Paul. I find INADDR_ANY to be a sensible way to
> go also. We shouldn't assume the current code is well-though out or
> intrinsically correct, either...

I guess it's pretty obvious that I prefer the NULL pointer approach.

Just to try out the alternative, I made the following test patch
to zebra/interface.c:connected_dump_vty():

--- ./zebra/interface.c.test 2004-03-18 10:40:33.000000000 -0500
+++ ./zebra/interface.c 2004-04-28 13:32:36.000000000 -0400
@@ -413,7 +413,15 @@

/* If there is destination address, print it. */
p = connected->destination;
- if (p)
+ /* only IPV4 and IPV6 address families are currently supported by
+ the prefix_vty_out() display function (since it calls inet_ntop) */
+ if ((p->family == AF_INET) ? (p->u.prefix4.s_addr != INADDR_ANY) :
+#ifdef HAVE_IPV6
+ ((p->family == AF_INET6) && IPV6_ADDR_CMP(&p->u.prefix6,&in6addr_any))
+#else
+ 0
+#endif
+ )
{
if (p->family == AF_INET)
if (ifp->flags & IFF_BROADCAST)

Is that what you guys are proposing? I guess we could package that
test in a macro, but it does raise the question of whether address
families other than AF_INET and AF_INET6 are or will be supported...

-Andy
Re: point-to-point patch [ In reply to ]
gdt at ir
Apr 28, 2004, 10:52 AM
Post #15 of 30 (4093 views)
Permalink
Certainly that could be wrapped in a function, but the example makes a
good point.

I wonder whether there is a useful semantic distinction between:

Conceptually there is no peer with an address (e.g Ethernet)

and

There is a peer, which probably has an address, but we don't know
it.
Re: point-to-point patch [ In reply to ]
paul at clubi
Apr 28, 2004, 11:10 AM
Post #16 of 30 (4100 views)
Permalink
On Wed, 28 Apr 2004, Andrew J. Schorr wrote:

> I guess it's pretty obvious that I prefer the NULL pointer
> approach.

Yes. :)

> Just to try out the alternative, I made the following test patch
> to zebra/interface.c:connected_dump_vty():
>
> --- ./zebra/interface.c.test 2004-03-18 10:40:33.000000000 -0500
> +++ ./zebra/interface.c 2004-04-28 13:32:36.000000000 -0400
> @@ -413,7 +413,15 @@
>
> /* If there is destination address, print it. */
> p = connected->destination;
> - if (p)
> + /* only IPV4 and IPV6 address families are currently supported by
> + the prefix_vty_out() display function (since it calls inet_ntop) */
> + if ((p->family == AF_INET) ? (p->u.prefix4.s_addr != INADDR_ANY) :
> +#ifdef HAVE_IPV6
> + ((p->family == AF_INET6) && IPV6_ADDR_CMP(&p->u.prefix6,&in6addr_any))
> +#else
> + 0
> +#endif
> + )
> {
> if (p->family == AF_INET)
> if (ifp->flags & IFF_BROADCAST)
>
> Is that what you guys are proposing? I guess we could package that
> test in a macro,

A nice wee function, even better.

> but it does raise the question of whether address families other
> than AF_INET and AF_INET6 are or will be supported...

For ospfd, it doesnt matter. For lib/, some handy generic function?
prefix_is_subneted_ptp() or something more appropriate? (add a _ipv4
variant for stuff that deals only with v4, eg ospfd).

> -Andy

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
A vivid and creative mind characterizes you.
Re: point-to-point patch [ In reply to ]
aschorr at telemetry-investments
Apr 29, 2004, 6:35 AM
Post #17 of 30 (4099 views)
Permalink
On Wed, Apr 28, 2004 at 07:10:56PM +0100, Paul Jakma wrote:
> > Is that what you guys are proposing? I guess we could package that
> > test in a macro,
>
> A nice wee function, even better.
>
> > but it does raise the question of whether address families other
> > than AF_INET and AF_INET6 are or will be supported...
>
> For ospfd, it doesnt matter. For lib/, some handy generic function?
> prefix_is_subneted_ptp() or something more appropriate? (add a _ipv4
> variant for stuff that deals only with v4, eg ospfd).

OK, when I get a chance (I hope within the next few days), I will code up
an alternative patch using the INADDR_ANY convention instead of NULL pointers,
and then you can choose which one you prefer.

Regards,
Andy
Re: point-to-point patch [ In reply to ]
aschorr at telemetry-investments
May 3, 2004, 9:46 AM
Post #18 of 30 (4100 views)
Permalink
OK, here's another stab at the point-to-point patch. Note that both of these
patches are compatible with the existing zebra wire protocol.

The first patch gets quagga to work properly with PtP links where a
dedicated subnet (often /30) has been assigned. (I expect it to work
with a /31 link, but I have not tested this.) If a subnet has been assigned,
patched quagga should now work regardless of whether a peer address has been
specified. The basic idea is that the logic for handling a PtP
link where a subnet has been assigned should be more like handling
a regularly broadcast network interface, and the special PtP logic
should be reserved for cases where a peer address with a /32 netmask
has been supplied. This patch fixes a bug in zebra/rt_netlink.c where
the local address was sometimes being supplied as the peer address.
And bugs in various places are fixed where the connected->destination
address was being used without first checking for a NULL pointer.
The lib/zclient.c code is fixed to recognize that zebra encodes
a NULL destination pointer as all zeroes. And I had to tweak
ospfd/ospf_interface.c:ospf_if_is_configured() to get it to work
properly.

I am currently running ospfd with the first patch, and everything seems
to work OK.

At Paul's request, I am also supplying a second patch. This patch
applies after the first one and changes the paradigm for indicating
an unavailable destination address from a NULL pointer to a value
of INADDR_ANY (or in6addr_any for IPV6). To do this involves
ensuring that zebra always supplies a destination prefix when it
creates a struct connected (and populates with zeroes if no valid
value is available), and using a macro to test for a zero value
where the code used to test for a NULL destination pointer.

I have compiled the second patch, but I have not tested it.
I continue to feel that the 2nd patch makes the code less elegant,
marginally slower, and more difficult to maintain (i.e. future
patchers are less likely to understand this paradigm than simply
testing for a NULL pointer). I also feel that a NULL pointer is
preferable because the code will crash if it is mistakenly accessed;
this seems better to me than unwittingly using INADDR_ANY. And I am
also concerned about a few spots in the code where a zero address seems
to have special treatment (search for prefix_ipv4_any in zebra/connected.c
and in bgpd/bgp_nexthop.c).

Feedback is appreciated, I hope somebody else can test this new capability.

Thanks,
Andy

Attached Files:

Re: point-to-point patch [ In reply to ]
hasso at estpak
May 3, 2004, 11:07 AM
Post #19 of 30 (4095 views)
Permalink
Andrew J. Schorr wrote:
> I continue to feel that the 2nd patch makes the code less elegant,
> marginally slower, and more difficult to maintain (i.e. future
> patchers are less likely to understand this paradigm than simply
> testing for a NULL pointer).

Agree.

> I also feel that a NULL pointer is preferable because the code will
> crash if it is mistakenly accessed; this seems better to me than
> unwittingly using INADDR_ANY.

Agree.

Don't take my opinion too seriously, as my knowledge about C is worse
than it should be. It's just my humble opinion. From first patch it
was clear for me what it does from first sight, but I can't say it
about nonulltest.patch.


--
Hasso Tepper
Elion Enterprises Ltd.
WAN administrator
Re: point-to-point patch [ In reply to ]
aschorr at telemetry-investments
May 4, 2004, 9:08 AM
Post #20 of 30 (4098 views)
Permalink
I was just looking at the changelog for 0.96.5, and I noticed that
there have been some problems with ospfd/ospfd.c:ospf_network_match_iface().

I haven't tested this, but wouldn't this make more sense (assuming one
has the CONNECTED_POINTOPOINT_HOST macro from my point-to-point patch)?

int
ospf_network_match_iface(struct connected *co, struct prefix *net)
{
return CONNECTED_POINTOPOINT_HOST(co) ?
IPV4_ADDR_SAME (&(co->destination->u.prefix4), &(net->u.prefix4)) :
prefix_match (net, co->address);
}

-Andy
Re: point-to-point patch [ In reply to ]
paul at clubi
May 5, 2004, 8:42 AM
Post #21 of 30 (4105 views)
Permalink
On Mon, 3 May 2004, Andrew J. Schorr wrote:

> OK, here's another stab at the point-to-point patch. Note that
> both of these patches are compatible with the existing zebra wire
> protocol.

That's good.

> The first patch gets quagga to work properly with PtP links where a
> dedicated subnet (often /30) has been assigned. (I expect it to
> work with a /31 link, but I have not tested this.) If a subnet has
> been assigned, patched quagga should now work regardless of whether
> a peer address has been specified.

Ok, though there is a general expectation that PtP links can be
addressed by their peer address.

> The basic idea is that the logic for handling a PtP link where a
> subnet has been assigned should be more like handling a regularly
> broadcast network interface, and the special PtP logic should be
> reserved for cases where a peer address with a /32 netmask has been
> supplied.

Hmm. ok.

> This patch fixes a bug in zebra/rt_netlink.c where the local
> address was sometimes being supplied as the peer address. And bugs
> in various places are fixed where the connected->destination
> address was being used without first checking for a NULL pointer.

Yep.

> The lib/zclient.c code is fixed to recognize that zebra encodes a
> NULL destination pointer as all zeroes. And I had to tweak
> ospfd/ospf_interface.c:ospf_if_is_configured() to get it to work
> properly.

Ok.

> I am currently running ospfd with the first patch, and everything seems
> to work OK.
>
> At Paul's request, I am also supplying a second patch. This patch
> applies after the first one and changes the paradigm for indicating
> an unavailable destination address from a NULL pointer to a value
> of INADDR_ANY (or in6addr_any for IPV6). To do this involves
> ensuring that zebra always supplies a destination prefix when it
> creates a struct connected (and populates with zeroes if no valid
> value is available), and using a macro to test for a zero value
> where the code used to test for a NULL destination pointer.

Right.


> I have compiled the second patch, but I have not tested it. I
> continue to feel that the 2nd patch makes the code less elegant,
> marginally slower, and more difficult to maintain (i.e. future
> patchers are less likely to understand this paradigm than simply
> testing for a NULL pointer).

I would tend to disagree, INADDR_ANY (or the v6 equivalent) is a
standard way of indicating "no address".

The speed is not a problem. Benchmark it, the overhead would most
likely be locality of the interface and connected lists in general,
fix that and the INADDR_ANY test is 0 overhead too. Further, if you
do fix locality by, eg, moving the address and destination struct
prefixes into the struct connected, there will not be any pointer to
test. The "no address" == "any" model will hold either way.

Maintainance: The INADDR_ANY==no-address model is valid regardless of
whether you are reading from the wire, whether you are using a
pointer to access the struct or whether you have an immediate
reference to it. It is more abstract.

> I also feel that a NULL pointer is preferable because the code will
> crash if it is mistakenly accessed; this seems better to me than
> unwittingly using INADDR_ANY.

What about the converse bug? That dest is allocated, pointer not
NULL, but holds nothing there, ie either all 0 or else random garbage
from the heap.

> And I am also concerned about a few spots in the code where a zero
> address seems to have special treatment (search for prefix_ipv4_any
> in zebra/connected.c

Right, a sanity check to deal with addressless tunnel devices, and
it's testing local address.

> and in bgpd/bgp_nexthop.c).

bgpd assumes that PtP connected networks can be addressed by peer.
This is a problem regardless of which abstraction is used for the
peer/subnet address - but your macro should deal with this (same as
for ospfd, PtP address => (local if subnet assigned, otherwise peer
address).

> Feedback is appreciated, I hope somebody else can test this new capability.

Looks good, if only we could settle the NULL * Vs addr_any argument
;)

> Thanks,
> Andy

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
This sentence contradicts itself -- no actually it doesn't.
-- Douglas Hofstadter
Re: point-to-point patch [ In reply to ]
aschorr at telemetry-investments
May 5, 2004, 12:16 PM
Post #22 of 30 (4103 views)
Permalink
Hi Paul,

On Wed, May 05, 2004 at 04:42:51PM +0100, Paul Jakma wrote:
> Ok, though there is a general expectation that PtP links can be
> addressed by their peer address.

Yes, that is true, but when a subnet has been assigned, there is no
actual need to specify the peer address, and linux is happy to allow
you to run without it specified. I have done this with patched
quagga, and it works fine.

> I would tend to disagree, INADDR_ANY (or the v6 equivalent) is a
> standard way of indicating "no address".
>
> The speed is not a problem. Benchmark it, the overhead would most
> likely be locality of the interface and connected lists in general,
> fix that and the INADDR_ANY test is 0 overhead too. Further, if you
> do fix locality by, eg, moving the address and destination struct
> prefixes into the struct connected, there will not be any pointer to
> test. The "no address" == "any" model will hold either way.
>
> Maintainance: The INADDR_ANY==no-address model is valid regardless of
> whether you are reading from the wire, whether you are using a
> pointer to access the struct or whether you have an immediate
> reference to it. It is more abstract.

I agree that speed is not a meaningful issue here. And I would
agree with you about INADDR_ANY if we knew that connected->destination
was always an IPV4 address. But the actual code says
"struct prefix *destination", and the destination could be IPV4, IPV6,
or some other perhaps yet unknown address family. What I like about
using a NULL pointer (as is currently done in the existing code) is
that it does not require you to special-case the check for a valid
destination based on the address family. This makes for more robust
code.

If you look at the 2nd patch to convert to the INADDR_ANY paradigm,
I think you can see that it is more messy because it has to check
in some cases for IPV4 INADDR_ANY and in others for IPV6 in6addr_any.

> > I also feel that a NULL pointer is preferable because the code will
> > crash if it is mistakenly accessed; this seems better to me than
> > unwittingly using INADDR_ANY.
>
> What about the converse bug? That dest is allocated, pointer not
> NULL, but holds nothing there, ie either all 0 or else random garbage
> from the heap.

I'm afraid there's nothing in the existing code to validate any addresses
anywhere that I've seen. How do we know that the connected->address
doesn't contain random garbage? I think you're introducing a new
standard here. Furthermore, I have added checks to zebra to validate
the initial value of the destination address. To check that it is
propagated correctly throughout the code is an entirely different
problem, and I do not think it has anything to do with the issue being
discussed here.

Thanks for thinking about this,
Andy
Re: point-to-point patch [ In reply to ]
paul at clubi
May 9, 2004, 10:29 AM
Post #23 of 30 (4103 views)
Permalink
On Wed, 5 May 2004, Andrew J. Schorr wrote:

> Yes, that is true, but when a subnet has been assigned, there is no
> actual need to specify the peer address, and linux is happy to
> allow you to run without it specified. I have done this with
> patched quagga, and it works fine.

Right yes, that was a reference to the bgp check you had pointed out,
your patch should fix that one too.

> I agree that speed is not a meaningful issue here. And I would
> agree with you about INADDR_ANY if we knew that
> connected->destination was always an IPV4 address. But the actual
> code says "struct prefix *destination", and the destination could
> be IPV4, IPV6, or some other perhaps yet unknown address family.

right..

> What I like about using a NULL pointer (as is currently done in the
> existing code) is that it does not require you to special-case the
> check for a valid destination based on the address family. This
> makes for more robust code.

Right, but "no destination" will be abstracted away behind some
function. INADDR_ANY and IN6ADDR_ANY/UNSPEC just happen to make sense
for v4 and v6. The users of destination dont need to know that
though, they could use a prefix_is_any() or somesuch.

And any new family is highly likely to have a special 'any' address.
(simply because of convention) which can be used to specify 'no'
address in certain contexts, failing that it will have a special 'no'
address - simply cause there will always be cases when you have to
explicitely define the lack of an address in the context of an
address (ie not lack of information).

The question here essentially is whether the helper function should
be called prefix_is_any or prefix_none. If it should be prefix_none,
then we still conceptually need prefix_is_any. Then the question
arises: What is the difference between them?

Anyway, NULL destination pointer is as good an indication of no
destination as any. And it'll save allocating the prefix. So, as long
as it is kept as any/unspecified address on the wire.

I'll apply your patch soonish. Could you humour me and try test it on
as many daemons as possible please, both with local/remote addressed
PtP links and subnet-assigned. Eg, I dont see any changes to
ospf_lsa.c, how are you describing these subnet-assigned links in the
router-lsa? As broadcast links? (i have my doubts as to whether
that's correct).

> Andy

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
Windows 95 is the most popular virus on the market today.
Re: point-to-point patch [ In reply to ]
aschorr at telemetry-investments
May 10, 2004, 7:36 AM
Post #24 of 30 (4099 views)
Permalink
Hi Paul,

On Sun, May 09, 2004 at 06:29:09PM +0100, Paul Jakma wrote:
> I'll apply your patch soonish. Could you humour me and try test it on
> as many daemons as possible please, both with local/remote addressed
> PtP links and subnet-assigned. Eg, I dont see any changes to
> ospf_lsa.c, how are you describing these subnet-assigned links in the
> router-lsa? As broadcast links? (i have my doubts as to whether
> that's correct).

I just downloaded the latest snapshot. Once I get my patch applied,
I will try to do some testing with ospfd, ripd, and bgpd.

Regarding ospf_lsa.c: my patch makes only a minor change:

--- ospf_lsa.c.ptp 2004-05-03 11:53:03.000000000 -0400
+++ ospf_lsa.c 2004-05-03 11:53:04.000000000 -0400
@@ -519,7 +519,7 @@
links++;
}

- if (oi->connected->destination != NULL)
+ if (CONNECTED_DEST_HOST(oi->connected))
{
/* Option 1:
link_type = LSA_LINK_TYPE_STUB;


Basically, the existing code in lsa_link_ptop_set() was already correctly
handling PtP links with assigned subnets, but that code path was never
exercised (because the destination point was never NULL due to
the decode failure in lib/zclient.c).

The code in lsa_link_ptop_set() corresponds to section 12.4.1.1 of
the OSPFv2 spec, and my patch just gets it to use "Option 2" whenever
a subnet has been assigned.

So I think the LSA should be fine, but I don't have the greatest understanding
of the algorithm; please let me know if you think there's a problem.

Thanks,
Andy
Re: point-to-point patch [ In reply to ]
aschorr at telemetry-investments
May 13, 2004, 10:51 AM
Post #25 of 30 (4108 views)
Permalink
On Mon, May 10, 2004 at 10:36:15AM -0400, Andrew J. Schorr wrote:
> I just downloaded the latest snapshot. Once I get my patch applied,
> I will try to do some testing with ospfd, ripd, and bgpd.

OK, I have done some simple tests of ospfd, bgpd, and ripd using the
quagga-0.96.5-20040510.tar.gz CVS snapshot with my PtP patch. (Note that I am
actually running ospfd in production, so that has gotten much more testing.)

When an explicit subnet is assigned to the PtP interface, everything
works fine for all 3 daemons (although to use RIPv2 multicasts with ripd, one
must explicitly turn on the interface multicast flag; this may be a
bug in ripd, see my separate post on this topic). I tested using
a /30 subnet and using a /31 subnet, and both worked perfectly with
all 3 daemons.

Using the more traditional /32 configuration where the PtP addresses
are borrowed from other interfaces, I found that ospfd and bgpd worked
fine. I was unable, however, to get ripd to work properly in
this configuration, but I had the exact same results from the patched
and unpatched versions. So this does not have anything to do with
my changes. If I tried to use multicast, I was getting errors like
this:

2004/05/13 12:02:23 RIP: Can't setsockopt IP_MULTICAST_IF to fd 10
2004/05/13 12:02:23 RIP: can't send packet : Network is unreachable

So I conclude that either I am configuring ripd incorrectly for this
scenario, or ripd doesn't handle it properly. If anybody can advise
on how to configure ripd to run properly over a PtP /32 link (an example
would be great), I can give it another try. But I'm pretty confident that
my patch is having no effect on these issues.

I am attaching an updated version of the patch that applies cleanly
against the latest CVS snapshot.

Thanks,
Andy

Attached Files:

1 2  View All

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal