Recently had a case where I was attempting to debug a nexthop tracking
issue across multiple bgp vrf's and since the setup vrf's in it with
overlapping address ranges, it became real fun real fast to track
vrf data associated. Add a bit of code to allow us to figure out
what vrf we are in when we print out debug messages.
Look through the rest of the code and find debugs where we are
not using bgp->name_pretty and switch it over.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Generally available hook for plugging application-specific
code in for bgp peer change events.
This hook (peer_status_changed) replaces the previous, more
specific 'peer_established' hook with a more general-purpose one.
Also, 'bgp_dump_state' is now registered under this hook.
Signed-off-by: Marton Kun-Szabo <martonk@amazon.com>
The correct cast for these is (unsigned char), because "char" could be
signed and thus have some negative value. isalpha & co. expect an int
arg that is positive, i.e. 0-255. So we need to cast to (unsigned char)
when calling any of these.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
The MRT dump code is already hooked in at the right places to write out
packets; the BMP code needs exactly the same access so let's make this
a hook.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
The bgp_info data is stored as a void pointer in `struct bgp_node`.
Abstract retrieval of this data and setting of this data
into functions so that in the future we can move around
what is stored in bgp_node.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Do a straight conversion of `struct bgp_info` to `struct bgp_path_info`.
This commit will setup the rename of variables as well.
This is being done because `struct bgp_info` is not descriptive
of what this data actually is. It is path information for routes
that we keep to build the actual routes nexthops plus some extra
information.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
The following types are nonstandard:
- u_char
- u_short
- u_int
- u_long
- u_int8_t
- u_int16_t
- u_int32_t
Replace them with the C99 standard types:
- uint8_t
- unsigned short
- unsigned int
- unsigned long
- uint8_t
- uint16_t
- uint32_t
Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>
Signed-off-by: Daniel Walton <dwalton@cumulusnetworks.com>
peer->ifindex was only used in two places but it was never populated so
neither of them worked as they should. 'struct peer' also has a 'struct
interface' pointer which we can use to get the ifindex.
This reverts commit c14777c6bf.
clang 5 is not widely available enough for people to indent with. This
is particularly problematic when rebasing/adjusting branches.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Signed-off-by: Daniel Walton <dwalton@cumulusnetworks.com>
This allows frr-reload.py (or anything else that scripts via vtysh)
to know if the vtysh command worked or hit an error.
The FSF's address changed, and we had a mixture of comment styles for
the GPL file header. (The style with * at the beginning won out with
580 to 141 in existing files.)
Note: I've intentionally left intact other "variations" of the copyright
header, e.g. whether it says "Zebra", "Quagga", "FRR", or nothing.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Pass pointer to pointer instead of assigning by return value. See
previous commit message.
To ensure that the behavior stays functionally correct, any assignments
with the result of a thread_add* function have been transformed to set
the pointer to null before passing it. These can be removed wherever the
pointer is known to already be null.
Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>
The way thread.c is written, a caller who wishes to be able to cancel a
thread or avoid scheduling it twice must keep a reference to the thread.
Typically this is done with a long lived pointer whose value is checked
for null in order to know if the thread is currently scheduled. The
check-and-schedule idiom is so common that several wrapper macros in
thread.h existed solely to provide it.
This patch removes those macros and adds a new parameter to all
thread_add_* functions which is a pointer to the struct thread * to
store the result of a scheduling call. If the value passed is non-null,
the thread will only be scheduled if the value is null. This helps with
consistency.
A Coccinelle spatch has been used to transform code of the form:
if (t == NULL)
t = thread_add_* (...)
to the form
thread_add_* (..., &t)
The THREAD_ON macros have also been transformed to the underlying
thread.c calls.
Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>
thread.c fails to build properly on systems that do
not have a CLOCK_MONOTONIC. Therefore there is
no need for bgp to have knowledge of it.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Additionally:
* Add [ip] to a couple bgp show commands
* Quick refactor of a couple ISIS commands
* Quick refactor of a couple OSPF6 commands
Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>
* Correct dump type was not showing for routes-mrt &
updates-et
* Could not unconfigure most of them
Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>
I've been working on a small patch to correct an issue in the BGP MRT
table dump code. It's a quick'n'easy fix initially, and I'd appreciate
any feedback on making it better :)
Issue:
When the BGP table dump code runs, it generates the peer_index_table.
This walks the list of peers, and dumps out their IP, ASN, address
family, etc. It also sets the peer index number in the peer struct.
Then the code walks the RIB, and for each prefix, writes out RIB
entries, that refer to the peer index number.
However, when it finds prefixes that are locally originated, the
associated peer is the 'self' peer, which wasn't in the list of peers,
never gets an index number assigned, but because it is calloc'd, the
index number is set to 0.
End result: locally-originated routes are associated with whichever peer
happens to be first in the list of remote peers in the index table :)
Example (from one of our route collectors) - these are two of our
originated prefixes (bgpdump output):
TABLE_DUMP2|1457568002|B|12.0.1.63|7018|84.205.80.0/24||IGP|193.0.4.28|0|0||NAG|64512
10.255.255.255|
TABLE_DUMP2|1457568006|B|12.0.1.63|7018|2001:7fb:ff00::/48||IGP|::|0|0||NAG||
The prefixes are announced by us (note it has an empty AS PATH (the
field after the prefix)) but also looks like it was received from AS7018
(12.0.1.63). In fact, the AS7018 peer just happens to be the first peer
in the index table.
Fix:
The simplest fix (which is also the method adopted by both OpenBGPd and
the BIRD mrtdump branch) is to create an empty placeholder 'peer' at the
start of the peer index table, for all the routes which are locally
originated to refer to.
I've attached a patch for this.
Here's a resulting bgpdump output after the patch:
TABLE_DUMP2|1458828539|B|0.0.0.0|0|93.175.150.0/24||IGP|0.0.0.0|0|0||NAG||
Now it is more obvious that the prefix is locally originated.
There are more complicated potential ways of fixing it
1) skip the local routes when dumping the RIB. This leads to questions
about what an MRT table dump *should* contain :)
2) include the 'self' peer in the list of peers used to generate the
index table.
etc etc.
But I'm quite happy with my 'create a fake peer, and associate local
routes with it' method :)
Your thoughts and feedback are welcome!
Regards,
Colin Petrie
Systems Engineer
RIPE NCC RIS Project
Tested-by: NetDEF CI System <cisystem@netdef.org>
lib/zebra.h has FILTER_X #define's. These do not belong there.
Put them in lib/filter.h where they belong.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
(cherry picked from commit 0490729cc033a3483fc6b0ed45085ee249cac779)
Now if the number of entries for some prefix is too large, multiple TABLE_DUMP_V2 records are created.
In the previous version in such situation bgpd crashed with SIGABRT.
Allow the bgp dump functionality to handle the Extended Time format
as specified in RFC 6396.
Fixes a segmentation fault with multiple dump rules as well.
Signed-off-by: Alexis Fasquel <alexis@pch.net>
Acked-by: Donald Sharp <sharpd@cumulusnetworks.com>
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Ticket: CM-7861
Reviewed by: CCR-3651
Testing: See bug
bgp is using both bm->master and master pointers interchangebly
for thread manipulation. Since they are the same thing consolidate
to one pointer.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Ticket: CM-7177
Reviewed-by: CCR-3396
Testing: See bug
This code change does several small things:
(A) Fix a couple detected memory leaks
(B) Fix all malloc operations to use the correct XMALLOC operation in bgpd and parts of lib
(C) Adds a few new memory types to make it easier to detect issues
This patch implements the 'update-groups' functionality in BGP. This is a
function that can significantly improve BGP performance for Update generation
and resultant network convergence. BGP Updates are formed for "groups" of
peers and then replicated and sent out to each peer rather than being formed
for each peer. Thus major BGP operations related to outbound policy
application, adj-out maintenance and actual Update packet formation
are optimized.
BGP update-groups dynamically groups peers together based on configuration
as well as run-time criteria. Thus, it is more flexible than update-formation
based on peer-groups, which relies on operator configuration.
[Note that peer-group based update formation has been introduced into BGP by
Cumulus but is currently intended only for specific releases.]
From 11098af65b2b8f9535484703e7f40330a71cbae4 Mon Sep 17 00:00:00 2001
Subject: [PATCH] updgrp commits
Quagga sources have inherited a slew of Page Feed (^L, \xC) characters
from ancient history. Among other things, these break patchwork's
XML-RPC API because \xC is not a valid character in XML documents.
Nuke them from high orbit.
Patches can be adapted simply by:
sed -e 's%^L%%' -i filename.patch
(you can type page feeds in some environments with Ctrl-V Ctrl-L)
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
This reverts commit b07458a055.
On second thought, the right way to do this is with rename(), not by
introducing a lock that can potentially even stall bgpd.
Reported-by: Christian Franke <chris@opensourcerouting.org>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Older versions of Quagga/Zebra would output a value in MRT table
dump files for "uptime" aka "ORIGINATED" that was a WALL clock
value. Given that uptime is now internally a bgp_clock MONOTONIC
value, the output in the MRT files is showing up as monotonic.
Note: time of MRT dump is still recorded correctly as a
time() based value, so we haven't lost that value.
Proposal is to correct the uptime output on the vty and in the
MRT files to again display something more akin to WALL time.
* bgp_dump.c: (bgp_dump_routes_func) add conditional correction
* bgp_route.c: (route_vty_out_detail) make correction conditional, move
variable declaration to beginning of the function
I've spent the last several weeks working on stability fixes to bgpd.
These patches fix all of the numerous crashes, assertion failures, memory
leaks and memory stomping I could find. Valgrind was used extensively.
Added new function bgp_exit() to help catch problems. If "debug bgp" is
configured and bgpd exits with status of 0, statistics on remaining
lib/memory.c allocations are printed to stderr. It is my hope that other
developers will use this to stay on top of memory issues.
Example questionable exit:
bgpd: memstats: Current memory utilization in module LIB:
bgpd: memstats: Link List : 6
bgpd: memstats: Link Node : 5
bgpd: memstats: Hash : 8
bgpd: memstats: Hash Bucket : 2
bgpd: memstats: Hash Index : 8
bgpd: memstats: Work queue : 3
bgpd: memstats: Work queue item : 2
bgpd: memstats: Work queue name string : 3
bgpd: memstats: Current memory utilization in module BGP:
bgpd: memstats: BGP instance : 1
bgpd: memstats: BGP peer : 1
bgpd: memstats: BGP peer hostname : 1
bgpd: memstats: BGP attribute : 1
bgpd: memstats: BGP extra attributes : 1
bgpd: memstats: BGP aspath : 1
bgpd: memstats: BGP aspath str : 1
bgpd: memstats: BGP table : 24
bgpd: memstats: BGP node : 1
bgpd: memstats: BGP route : 1
bgpd: memstats: BGP synchronise : 8
bgpd: memstats: BGP Process queue : 1
bgpd: memstats: BGP node clear queue : 1
bgpd: memstats: NOTE: If configuration exists, utilization may be expected.
Example clean exit:
bgpd: memstats: No remaining tracked memory utilization.
This patch fixes bug #397: "Invalid free in bgp_announce_check()".
This patch fixes bug #492: "SIGBUS in bgpd/bgp_route.c:
bgp_clear_route_node()".
My apologies for not separating out these changes into individual patches.
The complexity of doing so boggled what is left of my brain. I hope this
is all still useful to the community.
This code has been production tested, in non-route-server-client mode, on
a linux 32-bit box and a 64-bit box.
Release/reset functions, used by bgp_exit(), added to:
bgpd/bgp_attr.c,h
bgpd/bgp_community.c,h
bgpd/bgp_dump.c,h
bgpd/bgp_ecommunity.c,h
bgpd/bgp_filter.c,h
bgpd/bgp_nexthop.c,h
bgpd/bgp_route.c,h
lib/routemap.c,h
File by file analysis:
* bgpd/bgp_aspath.c: Prevent re-use of ashash after it is released.
* bgpd/bgp_attr.c: #if removed uncalled cluster_dup().
* bgpd/bgp_clist.c,h: Allow community_list_terminate() to be called from
bgp_exit().
* bgpd/bgp_filter.c: Fix aslist->name use without allocation check, and
also fix memory leak.
* bgpd/bgp_main.c: Created bgp_exit() exit routine. This function frees
allocations made as part of bgpd initialization and, to some extent,
configuration. If "debug bgp" is configured, memory stats are printed
as described above.
* bgpd/bgp_nexthop.c: zclient_new() already allocates stream for
ibuf/obuf, so bgp_scan_init() shouldn't do it too. Also, made it so
zlookup is global so bgp_exit() can use it.
* bgpd/bgp_packet.c: bgp_capability_msg_parse() call to bgp_clear_route()
adjusted to use new BGP_CLEAR_ROUTE_NORMAL flag.
* bgpd/bgp_route.h: Correct reference counter "lock" to be signed.
bgp_clear_route() now accepts a bgp_clear_route_type of either
BGP_CLEAR_ROUTE_NORMAL or BGP_CLEAR_ROUTE_MY_RSCLIENT.
* bgpd/bgp_route.c:
- bgp_process_rsclient(): attr was being zero'ed and then
bgp_attr_extra_free() was being called with it, even though it was
never filled with valid data.
- bgp_process_rsclient(): Make sure rsclient->group is not NULL before
use.
- bgp_processq_del(): Add call to bgp_table_unlock().
- bgp_process(): Add call to bgp_table_lock().
- bgp_update_rsclient(): memset clearing of new_attr not needed since
declarationw with "= { 0 }" does it. memset was already commented
out.
- bgp_update_rsclient(): Fix screwed up misleading indentation.
- bgp_withdraw_rsclient(): Fix screwed up misleading indentation.
- bgp_clear_route_node(): Support BGP_CLEAR_ROUTE_MY_RSCLIENT.
- bgp_clear_node_queue_del(): Add call to bgp_table_unlock() and also
free struct bgp_clear_node_queue used for work item.
- bgp_clear_node_complete(): Do peer_unlock() after BGP_EVENT_ADD() in
case peer is released by peer_unlock() call.
- bgp_clear_route_table(): Support BGP_CLEAR_ROUTE_MY_RSCLIENT. Use
struct bgp_clear_node_queue to supply data to worker. Add call to
bgp_table_lock().
- bgp_clear_route(): Add support for BGP_CLEAR_ROUTE_NORMAL or
BGP_CLEAR_ROUTE_MY_RSCLIENT.
- bgp_clear_route_all(): Use BGP_CLEAR_ROUTE_NORMAL.
Bug 397 fixes:
- bgp_default_originate()
- bgp_announce_table()
* bgpd/bgp_table.h:
- struct bgp_table: Added reference count. Changed type of owner to be
"struct peer *" rather than "void *".
- struct bgp_node: Correct reference counter "lock" to be signed.
* bgpd/bgp_table.c:
- Added bgp_table reference counting.
- bgp_table_free(): Fixed cleanup code. Call peer_unlock() on owner if
set.
- bgp_unlock_node(): Added assertion.
- bgp_node_get(): Added call to bgp_lock_node() to code path that it was
missing from.
* bgpd/bgp_vty.c:
- peer_rsclient_set_vty(): Call peer_lock() as part of peer assignment
to owner. Handle failure gracefully.
- peer_rsclient_unset_vty(): Add call to bgp_clear_route() with
BGP_CLEAR_ROUTE_MY_RSCLIENT purpose.
* bgpd/bgp_zebra.c: Made it so zclient is global so bgp_exit() can use it.
* bgpd/bgpd.c:
- peer_lock(): Allow to be called when status is "Deleted".
- peer_deactivate(): Supply BGP_CLEAR_ROUTE_NORMAL purpose to
bgp_clear_route() call.
- peer_delete(): Common variable listnode pn. Fix bug in which rsclient
was only dealt with if not part of a peer group. Call
bgp_clear_route() for rsclient, if appropriate, and do so with
BGP_CLEAR_ROUTE_MY_RSCLIENT purpose.
- peer_group_get(): Use XSTRDUP() instead of strdup() for conf->host.
- peer_group_bind(): Call bgp_clear_route() for rsclient, and do so with
BGP_CLEAR_ROUTE_MY_RSCLIENT purpose.
- bgp_create(): Use XSTRDUP() instead of strdup() for peer_self->host.
- bgp_delete(): Delete peers before groups, rather than after. And then
rather than deleting rsclients, verify that there are none at this
point.
- bgp_unlock(): Add assertion.
- bgp_free(): Call bgp_table_finish() rather than doing XFREE() itself.
* lib/command.c,h: Compiler warning fixes. Add cmd_terminate(). Fixed
massive leak in install_element() in which cmd_make_descvec() was being
called more than once for the same cmd->strvec/string/doc.
* lib/log.c: Make closezlog() check fp before calling fclose().
* lib/memory.c: Catch when alloc count goes negative by using signed
counts. Correct #endif comment. Add log_memstats_stderr().
* lib/memory.h: Add log_memstats_stderr().
* lib/thread.c: thread->funcname was being accessed in thread_call() after
it had been freed. Rearranged things so that thread_call() frees
funcname. Also made it so thread_master_free() cleans up cpu_record.
* lib/vty.c,h: Use global command_cr. Add vty_terminate().
* lib/zclient.c,h: Re-enable zclient_free().
