We had various forms of min/max macros across multiple daemons
all of which duplicated what we have in compiler.h. Convert
everyone to use the `correct` ones
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Currently, we automatically delete an inactive VRF when its last
interface is deleted. This code introduces a couple of crashes because
of the following problems:
- vrf_delete is called before calling if_del hook, so daemons may try to
dereference an ifp->vrf pointer which is freed
- in if_terminate, we continue to use the VRF in the loop condition
after the last interface is deleted
This check is needed only when the interface is deleted by the user,
because if the interface is deleted by the system, VRF must still exist
in the system. Move the check to appropriate places to fix crashes.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
There are two APIs to control the expected number of hops for a BFD
session – `bfd_sess_set_mininum_ttl` and `bfd_sess_set_hop_count`.
The former is very confusing, as it takes an expected TTL in the
BFD packet which is actually a protocol internal value. The latter is
simple and straightforward – it takes an expected number of hops, which
is always 1 for single-hop and >1 for multi-hop.
As the former API is not used anywhere, just remove it to avoid any
confusion.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
... need to ignore TLS sections, their address is effectively
meaningless but can overlap other sections we actually need to access.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
`assert.h` -> `xref.h` -> `typesafe.h` -> `assert.h`
Might be possible to do this more cleanly some way, but that way is not
obvious, so here's the "simple & dumb" approach.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Looks much prettier if `libunwind` is available, but works with glibc or
libexecinfo's `backtrace()` too.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
... its only purpose was to serve as a footgun, and all such uses have
been eliminated now.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
The `struct thread **ref` that the thread code takes is written to and
needs to stay valid over the lifetime of a thread. This does not hold
up if thread pointers are directly put in a `vector` since adding items
to a `vector` may reallocate the entire array. The thread code would
then write to a now-invalid `ref`, potentially corrupting entirely
unrelated data.
This should be extremely rare to trigger in practice since we only use
one c-ares channel, which will likely only ever use one fd, so the
vector is never resized. That said, c-ares using only one fd is just
plain fragile luck.
Either way, fix this by creating a resolver_fd tracking struct, and
clean up the code while we're at it.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
These are just used to iterate over active vty sessions, a vector is a
weird choice there.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
These had no remaining users for a while now. The logging backend has
its own list of receivers.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Constify some BFD library function parameters to signalize they are
not going to get modified.
Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
If the VRF is not enabled, if_terminate deletes the VRF after the last
interface is removed from it. Therefore daemons crash on the subsequent
call to vrf_delete. We should call vrf_delete only for enabled VRFs.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
When the netns is deleted, we should always clear the vrf->ns_ctxt
pointer. Currently, it is not cleared when there are interfaces in the
netns at the time of deletion.
If the netns is re-created, zebra crashes because it tries to use the
stale pointer.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
The script entries were being stored in a hash lookup with
the script name a pre-defined array of characters. The hash
lookup is succeeding since it is auto-installed at script
start time irrelevant if there is a handler function.
Modify the code so that if the scriptname is an empty
string "\0" just return a NULL so that zebra does
not attempt to actually load up the script
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
ls_node_same, ls_attributes_same and ls_prefix_same are not producing expected
result due to a wrong usage of memcmp. In addition, if respective structures
are not initialized with 0, there is a risk that the comparison failed.
This patch correct usage of memcmp and expand comparison to each invidual
parameters of the respective structure for safer result.
Signed-off-by: Olivier Dugeon <olivier.dugeon@orange.com>
This function doesn't work correctly with netns VRF backend as the same
index may be used in multiple netns simultaneously. So let's hide it
from the public API to reduce temptation to use it instead of writing
the correct code.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
The fact that the interface name is used in some nexthop config doesn't
mean that the interface is configured.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
```
exit1-debian-9(config-route-map)# match ip route-source prefix-list ?
<cr>
PREFIXLIST_NAME IP prefix-list name
p1 p2
```
Signed-off-by: Donatas Abraitis <donatas.abraitis@gmail.com>
zclient_send_localsid is called by various routing protocol daemons. To set the
srv6 endpoint function. Fix a hard-coded error in the initial implementation.
Before this PR, the srv6 function will be registered to zebra as a BGP route
even if isisd executes zclient_send_localsid.
Signed-off-by: Hiroki Shirokura <hiroki.shirokura@linecorp.com>
When writing the config from the NB-converted daemon, we must not rely
on the operational data. This commit changes the output of the interface
configuration to use only config data. As the code is the same for all
daemons, move it to the lib and remove all the duplicated code.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Do not return pointer to the newly created thread from various thread_add
functions. This should prevent developers from storing a thread pointer
into some variable without letting the lib know that the pointer is
stored. When the lib doesn't know that the pointer is stored, it doesn't
prevent rescheduling and it can lead to hard to find bugs. If someone
wants to store the pointer, they should pass a double pointer as the last
argument.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
The function thread_is_scheduled allows us to know if
the particular thread is scheduled for execution or not.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
This removes a giant `switch { }` block from lib/zclient.c and
harmonizes all zclient callback function types to be the same (some had
a subset of the args, some had a void return, now they all have
ZAPI_CALLBACK_ARGS and int return.)
Apart from getting rid of the giant switch, this is a minor security
benefit since the function pointers are now in a `const` array, so they
can't be overwritten by e.g. heap overflows for code execution anymore.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
*_anywhere(item) returns whether an item is on _any_ container. Only
available for unsorted containers for now.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Using a non-NULL sentinel allows distinguishing between "end of list"
and "item not on any list". It's a compare either way, just the value
is different.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
This provides a "is this item on this list" check, which may or may not
be faster than using *_find() for the same purpose. (If the container
has no faster way of doing it, it falls back to using *_find().)
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Some of the typesafe containers didn't null out their innards of items
after an item was deleted or popped off the container. This is both a
bit unsafe as well as hinders the upcoming _member() from working
efficiently.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
It allows FRR to read the interface config even when the necessary VRFs
are not yet created and interfaces are in "wrong" VRFs. Currently, such
config is rejected.
For VRF-lite backend, we don't care at all about the VRF of the inactive
interface. When the interface is created in the OS and becomes active,
we always use its actual VRF instead of the configured one. So there's
no need to reject the config.
For netns backend, we may have multiple interfaces with the same name in
different VRFs. So we care about the VRF of inactive interfaces. And we
must allow to preconfigure the interface in a VRF even before it is
moved to the corresponding netns. From now on, we allow to create
multiple configs for the same interface name in different VRFs and
the necessary config is applied once the OS interface is moved to the
corresponding netns.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
When something is used only from zebra and part of its description is
"should be called from zebra only" then it belongs to zebra, not lib.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
... to speed up vector_empty_slot() among other things.
Behavior should be 100% identical to previous.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
This function doesn't work correctly with netns VRF backend as the same
ifname may be used in multiple netns simultaneously. So let's hide it
from the public API to reduce temptation to use it instead of writing
the correct code.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
The fact that the interface name is used in some nexthop config doesn't
mean that the interface is configured.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
```
exit1-debian-9# show ip route 172.16.16.1/32
Routing entry for 172.16.16.1/32
Known via "bgp", distance 20, metric 0, best
Last update 00:00:28 ago
* 192.168.0.2, via eth1, weight 1
AS-Path : 65003
Communities : first 65001:2 65001:3
Large-Communities: 65001:1:1 65001:1:2 65001:1:3
Selection reason : First path received
```
Signed-off-by: Donatas Abraitis <donatas.abraitis@gmail.com>
To ensure this, add a const modifier to functions' arguments. Would be
great do this initially and avoid this large code change, but better
late than never.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
The switch statement over `enum zebra_link_type` had a default
and FRR was missing a few of the pre-defined types we cared about.
Remove the default statement and add the missing values.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
There's no more difference between number-named and word-named access-lists.
This commit removes separate arguments for number-named ACLs from CLI.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
We should never pass pointers to local variables to thread_add_* family.
When an event is executed, the library writes into this pointer, which
means it writes into some random memory on a stack.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
The current code passes an address of a local variable to `thread_add_read`
which stores it into `thread->ref` by the lib. The next time the thread
callback is executed, the lib stores NULL into the `thread->ref` which
means it writes into some random memory on the stack.
To fix this, we should pass a pointer to the vector entry to the lib.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
`yang_dnode_get` will `assert` if no YANG node/model exist, so lets test for
its existence first before trying to access it.
This `assert` is only acceptable for internal FRR usage otherwise we
might miss typos or unmatching YANG models nodes/leaves. For gRPC usage
we should let users attempt to use non existing models without
`assert`ing.
Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
Since there's very few locations where the `frr-format` actually prints
false positive warnings, consensus seems to be to just work around the
false positives even if the code is correct.
In fact, there is only one pattern of false positives currently, in
`bfdd/dplane.c` which does `vty_out("%"PRIu64, (uint64_t)be64toh(...))`.
The workaround/fix for this is a replacement `be64toh` whose type is
always `uint64_t` regardless of what OS we're on, making the cast
unnecessary.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Pass down the safi for when we need address
resolution. At this point in time we are
hard coding the safi to SAFI_UNICAST.
Future commits will take advantage of this.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
These are no longer really needed. The client just needs
to call nexthop resolution instead.
So let's remove the zapi types.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
After `<daemon_name> is not running` message vtysh does not return
error. For example if you disable ospf in `/etc/frr/daemons` and run
`vtysh -c configure -c "router ospf"` it prints the message to stderr,
but returns 0.
This commit will make vtysh return error when not in interractive mode.
But if you run commands from vtysh, you will still be able to enter
views and exit them if daemon is not running.
Signed-off-by: Yaroslav Fedoriachenko <yar.fed99@gmail.com>
Instead of sorting each command one-by-one using listnode_add_sort, add
them to the list without sorting and then sort the list only once.
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
frrmod_load() attempts to dlopen() several possible paths
(constructed from its basename argument) until one succeeds.
Each dlopen() attempt may fail for a different reason, and
the important one might not be the last one. Example:
dlopen(a/foo): file not found
dlopen(b/foo): symbol "bar" missing
dlopen(c/foo): file not found
Previous code reported only the most recent error. Now frrmod_load()
describes each dlopen() failure.
Signed-off-by: G. Paul Ziemba <paulz@labn.net>
