Firstly, *keep no change* for `hash_get()` with NULL
`alloc_func`.
Only focus on cases with non-NULL `alloc_func` of
`hash_get()`.
Since `hash_get()` with non-NULL `alloc_func` parameter
shall not fail, just ignore the returned value of it.
The returned value must not be NULL.
So in this case, remove the unnecessary checking NULL
or not for the returned value and add `void` in front
of it.
Importantly, also *keep no change* for the two cases with
non-NULL `alloc_func` -
1) Use `assert(<returned_data> == <searching_data>)` to
ensure it is a created node, not a found node.
Refer to `isis_vertex_queue_insert()` of isisd, there
are many examples of this case in isid.
2) Use `<returned_data> != <searching_data>` to judge it
is a found node, then free <searching_data>.
Refer to `aspath_intern()` of bgpd, there are many
examples of this case in bgpd.
Here, <returned_data> is the returned value from `hash_get()`,
and <searching_data> is the data, which is to be put into
hash table.
Signed-off-by: anlan_cs <vic.lan@pica8.com>
Don't rely on the OS interface name length definition and use the FRR
definition instead.
Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
Passing NULL for a `%pTVMs` would result in `(null)Ms`, i.e. the `Ms`
flags not eaten up. Change to eat those up, and print `-` instead for
NULL times.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
There's a common pattern of "get VRF context for CLI node" here, which
first got a helper macro in zebra that then permeated into pimd.
Unfortunately the pimd copy wasn't quite adjusted correctly and thus
caused two coverity warnings (CID 1517453, CID 1517454).
Fix the PIM one, and clean up by providing a common base macro in
`lib/vty.h`.
Also rename the macros (add `_VRF`) to make more clear what they do.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
By changing this API call to use a `struct ipaddr`, which encodes the
type of IP address with it. (And rename/remove the `IPV4` from the
command name.)
Also add a comment explaining that this function call is going to be
obsolete in the long run since pimd needs to move to proper MRIB NHT.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
If duplicate value is entered, the whole plist/alist just dropped.
Before:
```
$ grep prefix-list /etc/frr/frr.conf
ip prefix-list test seq 5 permit 1.1.1.1/32
ip prefix-list test seq 10 permit 1.1.1.1/32
$ systemctl restart frr
$ vtysh -c 'show run | include prefix-list'
$
```
After:
```
$ grep prefix-list /etc/frr/frr.conf
ip prefix-list test seq 5 permit 1.1.1.1/32
ip prefix-list test seq 10 permit 1.1.1.1/32
$ systemctl restart frr
$ vtysh -c 'show run | include prefix-list'
ip prefix-list test seq 5 permit 1.1.1.1/32
```
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
End operator is showing:
!
frr version 8.0.1
frr defaults traditional
hostname test.example.com
domainname
domainname should not be printed in this case at all. I do not
see any mechanism in current code that this could happen, but
what do I know? Put some extra stupid insurance in place
to prevent bad config from being generated.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Recent commit e92508a741 changed
the prefix_master->str to a RB tree. This introduced a condition
whnere on shutdown the prefix list was removed from the master list
and then operated on by passing around a name. Which was then used
to lookup the prefix list again when we operated on the code.
This change to a RB Tree first deleted the item from the RB tree
first thus introducing this crash
Crash:
(gdb) bt
index=0x556c07d59650, pentry=0x556c07d29380) at lib/routemap.c:2397
arg=0x7ffdbf84bc60) at lib/hash.c:267
event=RMAP_EVENT_PLIST_DELETED) at lib/routemap.c:2489
Grab the first item on the list, clean it and then remove it.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Just simple helpers to get a scope value, never-forward, and is-SSM for
a given address.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
This has already been a requirement for Solaris, it is still a
requirement for some of the autoconf feature checks to work correctly,
and it will be a requirement for `-fms-extensions`.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
The commands:
router isis 1
mpls-te on
no mpls-te on
mpls-te on
no mpls-te on
!
Will crash
Valgrind gives us this:
==652336== Invalid read of size 8
==652336== at 0x49AB25C: typed_rb_min (typerb.c:495)
==652336== by 0x4943B54: vertices_const_first (link_state.h:424)
==652336== by 0x493DCE4: vertices_first (link_state.h:424)
==652336== by 0x493DADC: ls_ted_del_all (link_state.c:1010)
==652336== by 0x47E77B: isis_instance_mpls_te_destroy (isis_nb_config.c:1871)
==652336== by 0x495BE20: nb_callback_destroy (northbound.c:1131)
==652336== by 0x495B5AC: nb_callback_configuration (northbound.c:1356)
==652336== by 0x4958127: nb_transaction_process (northbound.c:1473)
==652336== by 0x4958275: nb_candidate_commit_apply (northbound.c:906)
==652336== by 0x49585B8: nb_candidate_commit (northbound.c:938)
==652336== by 0x495CE4A: nb_cli_classic_commit (northbound_cli.c:64)
==652336== by 0x495D6C5: nb_cli_apply_changes_internal (northbound_cli.c:250)
==652336== Address 0x6f928e0 is 272 bytes inside a block of size 320 free'd
==652336== at 0x48399AB: free (vg_replace_malloc.c:538)
==652336== by 0x494BA30: qfree (memory.c:141)
==652336== by 0x493D99D: ls_ted_del (link_state.c:997)
==652336== by 0x493DC20: ls_ted_del_all (link_state.c:1018)
==652336== by 0x47E77B: isis_instance_mpls_te_destroy (isis_nb_config.c:1871)
==652336== by 0x495BE20: nb_callback_destroy (northbound.c:1131)
==652336== by 0x495B5AC: nb_callback_configuration (northbound.c:1356)
==652336== by 0x4958127: nb_transaction_process (northbound.c:1473)
==652336== by 0x4958275: nb_candidate_commit_apply (northbound.c:906)
==652336== by 0x49585B8: nb_candidate_commit (northbound.c:938)
==652336== by 0x495CE4A: nb_cli_classic_commit (northbound_cli.c:64)
==652336== by 0x495D6C5: nb_cli_apply_changes_internal (northbound_cli.c:250)
==652336== Block was alloc'd at
==652336== at 0x483AB65: calloc (vg_replace_malloc.c:760)
==652336== by 0x494B6F8: qcalloc (memory.c:116)
==652336== by 0x493D7D2: ls_ted_new (link_state.c:967)
==652336== by 0x47E4DD: isis_instance_mpls_te_create (isis_nb_config.c:1832)
==652336== by 0x495BB29: nb_callback_create (northbound.c:1034)
==652336== by 0x495B547: nb_callback_configuration (northbound.c:1348)
==652336== by 0x4958127: nb_transaction_process (northbound.c:1473)
==652336== by 0x4958275: nb_candidate_commit_apply (northbound.c:906)
==652336== by 0x49585B8: nb_candidate_commit (northbound.c:938)
==652336== by 0x495CE4A: nb_cli_classic_commit (northbound_cli.c:64)
==652336== by 0x495D6C5: nb_cli_apply_changes_internal (northbound_cli.c:250)
==652336== by 0x495D23E: nb_cli_apply_changes (northbound_cli.c:268)
Let's null out the pointer. After this change. Valgrind no longer reports issues
and isisd no longer crashes.
Fixes: #10939
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
These 3 values:
ONE_DAY_SECOND
ONE_WEEK_SECOND
ONE_YEAR_SECOND
Are defined based upon the number of seconds. Unfortunately doing math
on these values say something like:
days = t->tv_sec / ONE_DAY_SECOND;
Once you go over about a day causes the order of operations to cause the multiplication
to get messed up:
204 if (!t)
(gdb) n
207 w = d = h = m = ms = 0;
(gdb) set t->tv_sec = ONE_DAY_SECOND + 30
(gdb) n
208 memset(buf, 0, size);
(gdb)
210 us = t->tv_usec;
(gdb)
211 if (us >= 1000) {
(gdb)
212 ms = us / 1000;
(gdb)
213 us %= 1000;
(gdb)
217 if (ms >= 1000) {
(gdb)
222 if (t->tv_sec > ONE_WEEK_SECOND) {
(gdb)
227 if (t->tv_sec > ONE_DAY_SECOND) {
(gdb)
228 d = t->tv_sec / ONE_DAY_SECOND;
(gdb) n
229 t->tv_sec -= d * ONE_DAY_SECOND;
(gdb) n
232 if (t->tv_sec >= HOUR_IN_SECONDS) {
(gdb) p d
$6 = 2073600
(gdb) p t->tv_sec
$7 = -179158953570
(gdb)
Converting to adding paranthesis around around the ONE_DAY_SECOND causes
the order of operations to work as expected.
Fixes: #10880
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
When using zlog_backtrace I am seeing this:
==66286== Syscall param write(buf) points to uninitialised byte(s)
==66286== at 0x4CDF48A: syscall (in /lib/libc.so.7)
==66286== by 0x4A0D409: ??? (in /usr/local/lib/libunwind.so.8.0.1)
==66286== by 0x4A0D694: ??? (in /usr/local/lib/libunwind.so.8.0.1)
==66286== by 0x4A0E2F4: _ULx86_64_step (in /usr/local/lib/libunwind.so.8.0.1)
==66286== by 0x49662DB: zlog_backtrace (log.c:250)
==66286== by 0x2AFFA6: if_get_mtu (ioctl.c:163)
==66286== by 0x2B2D9D: ifan_read (kernel_socket.c:457)
==66286== by 0x2B2D9D: kernel_read (kernel_socket.c:1406)
==66286== by 0x499F46E: thread_call (thread.c:2002)
==66286== by 0x495D2B7: frr_run (libfrr.c:1196)
==66286== by 0x2B4098: main (main.c:471)
==66286== Address 0x7fc000000 is on thread 1's stack
==66286== in frame #4, created by zlog_backtrace (log.c:239)
==66286==
Let's initialize some data
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
When `terminal monitor` is issued I am seeing this for valgrind on freebsd:
2022/03/24 18:07:45 ZEBRA: [RHJDG-5FNSK][EC 100663304] can't open configuration file [/usr/local/etc/frr/zebra.conf]
==52993== Syscall param sendmsg(sendmsg.msg_control) points to uninitialised byte(s)
==52993== at 0x4CE268A: _sendmsg (in /lib/libc.so.7)
==52993== by 0x4B96245: ??? (in /lib/libthr.so.3)
==52993== by 0x4CDF329: sendmsg (in /lib/libc.so.7)
==52993== by 0x49A9994: vtysh_do_pass_fd (vty.c:2041)
==52993== by 0x49A9994: vtysh_flush (vty.c:2070)
==52993== by 0x499F4CE: thread_call (thread.c:2002)
==52993== by 0x495D317: frr_run (libfrr.c:1196)
==52993== by 0x2B4068: main (main.c:471)
==52993== Address 0x7fc000864 is on thread 1's stack
==52993== in frame #3, created by vtysh_flush (vty.c:2065)
Fix by initializing the memory to `0`
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
The EAD-per-ES route carries ECs for all the ES-EVI RTs. As the number of VNIs
increase all RTs do not fit into a standard BGP UPDATE (4K) so the route needs
to be fragmented.
Each fragment is associated with a separate RD and frag-id -
1. Local ES-per-EAD -
ES route table - {ES-frag-ID, ESI, ET=0xffffffff, VTEP-IP}
global route table - {RD-=ES-frag-RD, ESI, ET=0xffffffff}
2. Remote ES-per-EAD -
VNI route table - {ESI, ET=0xffffffff, VTEP-IP}
global route table - {RD-=ES-frag-RD, ESI, ET=0xffffffff}
Note: The fragment ID is abandoned in the per-VNI routing table. At this
point that is acceptable as we dont expect more than one-ES-per-EAD fragment
to be imported into the per-VNI routing table. But that may need to be
re-worked at a later point.
CLI changes (sample with 4 VNIs per-fragment for experimental pruposes) -
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
root@torm-11:mgmt:~# vtysh -c "show bgp l2vpn evpn es 03:44:38:39:ff:ff:01:00:00:01"
ESI: 03:44:38:39:ff:ff:01:00:00:01
Type: LR
RD: 27.0.0.21:3
Originator-IP: 27.0.0.21
Local ES DF preference: 50000
VNI Count: 10
Remote VNI Count: 10
VRF Count: 3
MACIP EVI Path Count: 33
MACIP Global Path Count: 198
Inconsistent VNI VTEP Count: 0
Inconsistencies: -
Fragments: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
27.0.0.21:3 EVIs: 4
27.0.0.21:13 EVIs: 4
27.0.0.21:22 EVIs: 2
VTEPs:
27.0.0.22 flags: EA df_alg: preference df_pref: 32767
27.0.0.23 flags: EA df_alg: preference df_pref: 32767
root@torm-11:mgmt:~# vtysh -c "show bgp l2vpn evpn es-evi vni 1002 detail"
VNI: 1002 ESI: 03:44:38:39:ff:ff:01:00:00:01
Type: LR
ES fragment RD: 27.0.0.21:13 >>>>>>>>>>>>>>>>>>>>>>>>>
Inconsistencies: -
VTEPs: 27.0.0.22(EV),27.0.0.23(EV)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
PS: The number of EVIs per-fragment has been set to 128 and may need further
tuning.
Ticket: #2632967
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
The wheel data structure is a array of list pointers
but the alloc for it is using the sizeof (struct listnode *)
as the amount to allocate. Even though the (struct listnode *)
and (struct list *) sizes are the same, let's list the correct
values.
Signed-off-by: ron <lyq140hf2006@163.com>
- split NewRpcState object into 2, a Unary and a Streaming variant, which
then allows for the next.
- move all state machine details inside these new state objects
- use a template arg to allow for Streaming state tracking object
creation and deletion w/o requiring this in each specific RPC
hander.
- Code is more rugged by design now.
Thanks to Rafael Zalamena <rzalamena@opensourcerouting.org> for the cleanup
ideas/motivation.
Signed-off-by: Christian Hopps <chopps@labn.net>
Let's clean up the valgrind output even more by calling the protobuf
shutdown function that deallocates all library used memory.
Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
Currently the nexthop tracking code is only sending to the requestor
what it was requested to match against. When the nexthop tracking
code was simplified to not need an import check and a nexthop check
in b8210849b8 for bgpd. It was not
noticed that a longer prefix could match but it would be seen
as a match because FRR was not sending up both the resolved
route prefix and the route FRR was asked to match against.
This code change causes the nexthop tracking code to pass
back up the matched requested route (so that the calling
protocol can figure out which one it is being told about )
as well as the actual prefix that was matched to.
Fixes: #10766
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
sockopt_cork is a no-op function that was cleaned up
in 2017. Since then it's still not being used. At
this point in time there is little point in keeping a
dead function that will not be used because of vagaries
between platforms
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
RB-tree and double-linked-list easily support backwards iteration, and
an use case seems to have popped up. Let's make it accessible.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
The prefix_master->str data structure was a sorted
list of the prefix names. Not that big of a deal
other than insertion and deletion is insanely expensive
when you have a large number of unique prefix-lists.
In my test config file that I discovered this,
I have 587 unique prefix lists spread out acros
~26k lines of prefix-lists. When reading
this config file into FRR the read time goes
from 690 seconds to 650 seconds.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
`json_object_object_add()` adds keys/items to objects/dictionaries.
Useful to have a printfrr based variant for the key there.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
The vtysh live logs don't try to buffer messages when vtysh isn't
reading them fast enough. Either the kernel has space and can accept
messages without delay, or it doesn't and we continue on.
While this is intentional (otherwise slow vtysh could block a routing
daemon), at least give the user an indication if messages were dropped.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
This provides direct raw log output with full metadata directly at
startup regardless of configuration details.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
This was the intent here to begin with, not sure where I managed to
forget this along the way...
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
The timestamps used for the live log are wallclock, not monotonic. Also
some fields were left uninitialized.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
While running singlethreaded, the RCU code is "dormant" and rcu_free is
an immediate operation. This results in the log target loop accessing
free'd memory if a log target removes itself while a message is printed
(which is likely to happen on e.g. error conditions.)
Just use frr_each_safe to avoid this issue.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
- rather than coerce `const char *` to std:string&, just pass the
C ptr, as that's what is used anyway.
fixes#10578
Signed-off-by: Christian Hopps <chopps@labn.net>
Don't let open sockets hang for too long. This will fix an issue where a
improperly coded client (e.g. socat) could exaust the amount of open
file descriptors.
Documentation:
https://grpc.github.io/grpc/cpp/md_doc_keepalive.html
Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
This issue is applicable to other protocols as well.
When user has used route-map, even though the prefixes are falling
under the permit rule, the prefixes were denied and were shown
as inactive route in zebra.
Reason being the parameter which is of type enum was passed to the api
route_map_get_index and was typecasted to uint8_t *.
This problem is visible in case of Big Endian systems because we are
accessing the most significant byte.
'match_ret' field is an enum in the caller and so it is of 4 bytes,
the typecasting it to 1 byte and passing it to the api made
the api to put the value in the most significant byte
which was already zero previously. Therefore the actual value
RMAP_NOMATCH which was 1 never gets reset in this case.
Therefore the api always returns 'RMAP_NOMATCH' and hence
the prefixes are always denied.
Fixes: #9782
Signed-off-by: Mobashshera Rasool <mrasool@vmware.com>
Call `zlog_file_rotate` for command file lines as well otherwise on
`SIGUSR1` the old descriptor will still be used and no new log file will
be created for the rotation.
Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
If a operator issues a series of route-map deletions and
then re-adds, *and* this triggers the hash table to realloc
to grow to a larger size, then subsuquent route-map operations
will be against a corrupted hash table.
Why?
Effectively the route-map code was inserting each
route-map <NAME> into a hash for storage. Upon
deletion there is this concept of delayed processing
so the routemap code sets a bit `to-be-processed`
and marks the route-map for deletion. This is
1 entry in the hash table. Then if the operator
recreates the hash, FRR would add another hash
entry. If another deletion happens then there
now are 2 deletion entries that are indistinguishable
from a hash perspective.
FRR stores the deleted name of the route-map so that
any delayed processing can lookup the name and only process
those peers that are related to that route-map name.
This is good as that if in say BGP, we do not want
to reprocess all the peers that don't use the route-map.
Solution:
The whole purpose of the delay of deletion and the
storage of the route-map is to allow the using protocol
the ability to process the route-map at a later time
while still retaining the route-map name( for more efficient
reprocessing ). The problem exists because we are keeping
multiple copies of deletion events that are indistinguishable
from each other causing hash havoc.
The truth is that we only need to keep 1 copy of the
routemap in the table. If the series of events is:
a) delete ( schedule processing )
b) add ( reschedule processing )
Current code ends up processing the route-map two times
and in this event we really just need to reprocess everything
with the new route-map.
If the series of events is:
a) delete (schedule processing )
b) add (reschedule)
c) delete (reschedule)
d) add (reschedule)
All this really points to is that FRR just needs to keep the last
in the series of maps and ensuring that FRR knows that we need
to continue processing the route-map. So in the creation processing
if the hash has an entry for this map, the routemap code knows that
this is a deletion event. Mark this route-map for later processing
if it was marked so. Also in the lookup function do not return
a map if the map found was deleted.
Fixes: #10708
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
State-only and configuration presence-containers need to be treated
differently when iterating over YANG operational data. Currently the
get_elem() callback is used to know when a state-only p-container
exists or not, and configuration p-containers are assumed to always
exist, which is clearly wrong. Fix this by checking the running
configuration to know whether a rw p-container exists or not.
Signed-off-by: Renato Westphal <renato@opensourcerouting.org>
On FreeBSD I have noticed that subsuquent calls to clock_gettime(..)
can return an after time that is before first calls value.
This in turn is generating CPU_HOG's because the subtraction
is wrapping into very very large numbers:
2022/02/28 20:12:58 SHARP: [PTDQA-70FG5] start: 35.741981000 now: 35.740581000
2022/02/28 20:12:58 SHARP: [XK9YH-ZD8FA][EC 100663313] CPU HOG: task zclient_read (800744240) ran for 0ms (cpu time 18446744073709550ms)
(Please note I added the first line of debug to figure this issue out).
I have been asked to open a FreeBSD bug report and have done so.
In the mean time I think that it is important that FRR does
not generate bogus CPU HOG's on FreeBSD ( especially since
this may or may not be easily fixed and FRR has no control
over what version of the operating system, operators are
going to be running with FRR.
So, add a bit of specialized code that checks to see if
the after time in FreeBSD is before the now time in
thread_consumed_time and do some quick manipulations
to not have this issue.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
This adds the plumbing necessary to yield back a file descriptor to
vtysh. The fd is passed on the command status code bytes through
AF_UNIX SCM_RIGHTS.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Add the ability to inspect the timers and when they will pop
per daemon:
sharpd@eva ~/frr (thread_return_null)> vtysh -c "show thread timers"
Thread timers for zebra:
Showing timers for default
--------------------------
rtadv_timer 00:00:00.520
if_zebra_speed_update 00:00:02.745
if_zebra_speed_update 00:00:02.745
if_zebra_speed_update 00:00:02.745
if_zebra_speed_update 00:00:02.745
if_zebra_speed_update 00:00:02.745
if_zebra_speed_update 00:00:02.745
if_zebra_speed_update 00:00:02.746
if_zebra_speed_update 00:00:02.744
if_zebra_speed_update 00:00:02.745
Showing timers for Zebra dplane thread
--------------------------------------
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Since there are timers that are created based upon doing some
math and we know that unsigned values when doing math and we accidently
subtract a larger number from a smaller number causes the unsigned
number to wrap to very large numbers, let's put in a small catch
in place to see if there are any places in the system that
mistakes are made and FRR is accidently creating a problem
for itself.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
