frr
frr copied to clipboard
nhrpd: Add topotest for retrying resolution request
Modified nhrp_topo topotest to test for newly added resolution request retry feature
Modified nhrp_connection_authorized() function so that debugging messages don't read into NHRP authentication extension as if it were a null-terminated string - which can lead to printing of non ASCII-compliant bytes.
Moved CISCO_PASS_LENGTH_LEN from nhrp_vty.c to nhrp_protocol.h for easier access to the macro in other files
Looks like frrbot and verify source are finding some issues that need to be cleaned up in the commit. Please take a second and do so.
Can we also rewrite the commit message to first discuss the code change and then the topotest change especially in more detail. When I started looking at the subject line I thought I was going to be looking at topotest changes only and frankly it's a bit confusing
@donaldsharp How's the commit message now? I tried to be a little more descriptive but don't mind changing it again if it's still a little vague.
I don't think these lint errors need to be fixed, but I'd like @donaldsharp or @ton31337 to make certain before pushing
@donaldsharp @ton31337 Do these changes look good to you guys? I tried to fix up the commit message and fix up any of the lint errors. Of course I'll try to be receptive to anything other changes you guys recommend.
I really like to see topotest changes split into their own commit. Can you please do that?
Also, can you change the topic of the PR to reflect the added feature. The main addition is the the feature, the topotest is there just to make sure the new feature works.
@Jafaral Yeah, I can split the commits up for the bug fix and the topotest, no problem. Just to be clear though, this PR is for a topotest for a feature that has already been merged: https://github.com/FRRouting/frr/pull/16788 . The code change mentioned in this PR is just a bug I came across while writing the topotest. It's a bug that was causing issues with the topotest (as in the topotest doesn't run with this bug present) so I figured it makes sense to both fix the bug and add the topotest in the same PR. But the main thing I was going for here was the topotest.
Do you still think I should change it? I'm sure it wouldn't be hard to make the PR focus on the code change if that's what you want.
Thanks @jmuthiilabn. I did change the title to capture the scope of the changes. Feel free to change as you see fit.
@mergifyio backport dev/10.2
backport dev/10.2
✅ Backports have been created
- #17345 nhrpd: fix passphrase handling, add topotest for resolution request (backport #17115) has been created for branch
dev/10.2