frr icon indicating copy to clipboard operation
frr copied to clipboard

nhrpd: Add topotest for retrying resolution request

Open jmuthiilabn opened this issue 1 year ago • 2 comments

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

jmuthiilabn avatar Oct 15 '24 17:10 jmuthiilabn

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.

donaldsharp avatar Oct 15 '24 20:10 donaldsharp

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 avatar Oct 15 '24 20:10 donaldsharp

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

jmuthiilabn avatar Oct 22 '24 13:10 jmuthiilabn

I don't think these lint errors need to be fixed, but I'd like @donaldsharp or @ton31337 to make certain before pushing

riw777 avatar Oct 22 '24 14:10 riw777

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

jmuthiilabn avatar Oct 28 '24 17:10 jmuthiilabn

I really like to see topotest changes split into their own commit. Can you please do that?

Jafaral avatar Oct 28 '24 18:10 Jafaral

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 avatar Oct 28 '24 18:10 Jafaral

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

jmuthiilabn avatar Oct 28 '24 20:10 jmuthiilabn

Thanks @jmuthiilabn. I did change the title to capture the scope of the changes. Feel free to change as you see fit.

Jafaral avatar Oct 29 '24 17:10 Jafaral

@mergifyio backport dev/10.2

Jafaral avatar Oct 29 '24 17:10 Jafaral

backport dev/10.2

✅ Backports have been created

mergify[bot] avatar Oct 29 '24 17:10 mergify[bot]