Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ripngd: add ability to match by ipv6 access/prefix list in route-maps #9200

Closed
wants to merge 1 commit into from

Conversation

idryzhov
Copy link
Contributor

Closes #8141.

Signed-off-by: Igor Ryzhov [email protected]

Copy link

@polychaeta polychaeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution to FRR!

Click for style suggestions

To apply these suggestions:

curl -s https://gist.githubusercontent.com/polychaeta/4e336dc43bb8a25ba354b5a01211c8dd/raw/12172e9e82fbc04c38606315727937978e1a73eb/cr_9200_1627411135.diff | git apply

diff --git a/ripngd/ripng_routemap.c b/ripngd/ripng_routemap.c
index 8e287a135..a3a551e06 100644
--- a/ripngd/ripng_routemap.c
+++ b/ripngd/ripng_routemap.c
@@ -152,11 +152,8 @@ static void route_match_ipv6_address_free(void *rule)
 }
 
 static const struct route_map_rule_cmd route_match_ipv6_address_cmd = {
-	"ipv6 address",
-	route_match_ipv6_address,
-	route_match_ipv6_address_compile,
-	route_match_ipv6_address_free
-};
+	"ipv6 address", route_match_ipv6_address,
+	route_match_ipv6_address_compile, route_match_ipv6_address_free};
 
 /* match ipv6 address prefix-list PREFIX_LIST */
 
@@ -184,12 +181,11 @@ static void route_match_ipv6_address_prefix_list_free(void *rule)
 }
 
 static const struct route_map_rule_cmd
-		route_match_ipv6_address_prefix_list_cmd = {
-	"ipv6 address prefix-list",
-	route_match_ipv6_address_prefix_list,
-	route_match_ipv6_address_prefix_list_compile,
-	route_match_ipv6_address_prefix_list_free
-};
+	route_match_ipv6_address_prefix_list_cmd = {
+		"ipv6 address prefix-list",
+		route_match_ipv6_address_prefix_list,
+		route_match_ipv6_address_prefix_list_compile,
+		route_match_ipv6_address_prefix_list_free};
 
 /* `match tag TAG' */
 /* Match function return 1 if match is success else return zero. */

If you are a new contributor to FRR, please see our contributing guidelines.

After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.

@donaldsharp
Copy link
Member

doc and topotest?

@LabN-CI
Copy link
Collaborator

LabN-CI commented Jul 27, 2021

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/9200 e765984
Date 07/27/2021
Start 16:37:35
Finish 17:03:11
Run-Time 25:36
Total 1813
Pass 1813
Fail 0
Valgrind-Errors
Valgrind-Loss
Details vncregress-2021-07-27-16:37:35.txt
Log autoscript-2021-07-27-16:38:56.log.bz2
Memory 483 482 425

For details, please contact louberger

@NetDEF-CI
Copy link
Collaborator

NetDEF-CI commented Jul 27, 2021

Continuous Integration Result: FAILED

Continuous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20583/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

Topotests Ubuntu 18.04 arm8 part 8: Failed (click for details) Topotests Ubuntu 18.04 arm8 part 8: No useful log found
Successful on other platforms/tests
  • Ubuntu 18.04 deb pkg check
  • Ubuntu 20.04 deb pkg check
  • Topotests Ubuntu 18.04 i386 part 7
  • Topotests Ubuntu 18.04 i386 part 2
  • Topotests debian 10 amd64 part 1
  • Debian 9 deb pkg check
  • Addresssanitizer topotests part 4
  • Topotests Ubuntu 18.04 amd64 part 1
  • Topotests debian 10 amd64 part 6
  • Topotests Ubuntu 18.04 amd64 part 2
  • Debian 10 deb pkg check
  • CentOS 7 rpm pkg check
  • Topotests Ubuntu 18.04 amd64 part 8
  • Topotests Ubuntu 18.04 arm8 part 7
  • Topotests Ubuntu 18.04 arm8 part 2
  • Addresssanitizer topotests part 2
  • Topotests Ubuntu 18.04 i386 part 3
  • Topotests Ubuntu 18.04 arm8 part 3
  • Topotests debian 10 amd64 part 5
  • Topotests Ubuntu 18.04 i386 part 8
  • Addresssanitizer topotests part 7
  • Topotests debian 10 amd64 part 0
  • Addresssanitizer topotests part 9
  • Topotests debian 10 amd64 part 3
  • Addresssanitizer topotests part 5
  • Ubuntu 16.04 deb pkg check
  • Topotests Ubuntu 18.04 i386 part 6
  • Topotests Ubuntu 18.04 arm8 part 0
  • Topotests Ubuntu 18.04 amd64 part 6
  • Topotests debian 10 amd64 part 2
  • Topotests Ubuntu 18.04 i386 part 1
  • Topotests Ubuntu 18.04 arm8 part 5
  • IPv4 ldp protocol on Ubuntu 18.04
  • Topotests Ubuntu 18.04 amd64 part 4
  • Addresssanitizer topotests part 0
  • Topotests debian 10 amd64 part 9
  • Topotests debian 10 amd64 part 4
  • Topotests Ubuntu 18.04 i386 part 0
  • Topotests Ubuntu 18.04 i386 part 5
  • Static analyzer (clang)
  • Addresssanitizer topotests part 3
  • Topotests Ubuntu 18.04 amd64 part 0
  • Addresssanitizer topotests part 1
  • Topotests Ubuntu 18.04 i386 part 4
  • IPv6 protocols on Ubuntu 18.04
  • Topotests Ubuntu 18.04 i386 part 9
  • Topotests Ubuntu 18.04 amd64 part 3
  • IPv4 protocols on Ubuntu 18.04
  • Topotests Ubuntu 18.04 arm8 part 6
  • Topotests Ubuntu 18.04 arm8 part 1
  • Addresssanitizer topotests part 8
  • Topotests Ubuntu 18.04 arm8 part 9
  • Topotests Ubuntu 18.04 arm8 part 4
  • Topotests Ubuntu 18.04 amd64 part 7
  • Topotests debian 10 amd64 part 8
  • Topotests Ubuntu 18.04 amd64 part 9
  • Topotests debian 10 amd64 part 7
  • Addresssanitizer topotests part 6
  • Fedora 29 rpm pkg check
  • Topotests Ubuntu 18.04 amd64 part 5

@idryzhov
Copy link
Contributor Author

doc and topotest?

This is just a quick improvement that fully reuses the code from other daemons – no really new-to-FRR code here. I found this shortcoming during my issue cleanup and decided to fix it very quickly to close the issue.
Currently, there are no topotests and no documentation for RIPng route-map usage, and I don't really have time right now to introduce new topotests/doc sections.
I initially marked it as a draft because it was not tested, but now it is tested internally by our QA team and is ready to go from our side. I hope we can just merge it as-is.

@idryzhov idryzhov marked this pull request as ready for review July 28, 2021 12:36
@idryzhov
Copy link
Contributor Author

ci:rerun

@donaldsharp
Copy link
Member

Nack from me until doc and topotests

@donaldsharp
Copy link
Member

to clarify ->

I am repeatedly hammered by development and users on both code quality and documentation. This is the only tool in my chest to try to combat both of these. I know it sucks having to do this work. I don't want to either but it's what we must do for this project in order for it too succeed

@idryzhov
Copy link
Contributor Author

@donaldsharp here is an example of a very similar change you merged several months ago without requiring any docs and topotests – #8206.
This change really added some new daemon-specific code while my change is a straight copy-paste of well-tested code from another daemon.
RIPng is really unmaintained and I assume not very popular, so I definitely won't spend time adding new docs and topotests for it. In my opinion, we can make an exception in this particular case because this is a very simple feature and people ask for it. If you're not going to change your mind, I'll just close the PR and the issue (it's already in our to-do list).

@donaldsharp
Copy link
Member

Then I should have been called on it.

@idryzhov
Copy link
Contributor Author

Ok, closing.

@NetDEF-CI
Copy link
Collaborator

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20595/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

route_match_ipv6_address_prefix_list missing in ripng_routemap.c
5 participants