Clean up the zebra's Netlink API #6569
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedCentOS 7 amd64 build: Failed (click for details)Make failed for CentOS 7 amd64 build: CentOS 7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12640/artifact/CI005BUILD/config.status/config.status Debian 10 amd64 build: Failed (click for details)Make failed for Debian 10 amd64 build: Debian 10 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12640/artifact/DEB10BUILD/config.status/config.status Debian 8 amd64 build: Failed (click for details)Make failed for Debian 8 amd64 build: Debian 8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12640/artifact/CI008BLD/config.status/config.status Ubuntu 16.04 amd64 build: Failed (click for details)Make failed for Ubuntu 16.04 amd64 build: Ubuntu 16.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12640/artifact/CI014BUILD/config.status/config.status Ubuntu 18.04 amd64 build: Failed (click for details)Make failed for Ubuntu 18.04 amd64 build: Ubuntu 18.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12640/artifact/U1804AMD64/config.status/config.status Debian 9 amd64 build: Failed (click for details)Make failed for Debian 9 amd64 build: Debian 9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12640/artifact/CI021BUILD/config.status/config.status Ubuntu 20.04 amd64 build: Failed (click for details)Make failed for Ubuntu 20.04 amd64 build: Ubuntu 20.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12640/artifact/U2004AMD64BUILD/config.status/config.status Ubuntu 18.04 ppc64le build: Failed (click for details)Make failed for Ubuntu 18.04 ppc64le build: Ubuntu 18.04 ppc64le build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12640/artifact/U1804PPC64LEBUILD/config.status/config.status Ubuntu 16.04 i386 build: Failed (click for details)Make failed for Ubuntu 16.04 i386 build: Ubuntu 16.04 i386 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12640/artifact/U1604I386/config.status/config.status Fedora 29 amd64 build: Failed (click for details)Make failed for Fedora 29 amd64 build: Fedora 29 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12640/artifact/F29BUILD/config.status/config.status Successful on other platforms/tests
|
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
xThaid
force-pushed
the
dplane_batching_cleanup
branch
from
41b8eed
to
026d200
Compare
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12644/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
mjstapp
reviewed
Jun 12, 2020
zebra/rt_netlink.c
Outdated
| */ | ||
| static void _netlink_route_build_singlepath(const struct prefix *p, | ||
| static bool _netlink_route_build_singlepath(const struct prefix *p, |
There was a problem hiding this comment.
can you say a bit about how this return status would be used? it seems as if there would already have been some data put into whereever 'nlmsg' is pointing - does that ... matter?
There was a problem hiding this comment.
This return status tells us if all attributes appended to the message in this function fits entirely in the buffer. This is used in netlink_route_multipath_msg_encode. And if I understand the second part of the question correctly, this function doesn't encode a separate message, but it adds some attributes to the existing one.
There was a problem hiding this comment.
that's sort of my question: if this returns 'false', it has already ... added some attributes (probably). how would a caller recover?
There was a problem hiding this comment.
I will make sure, but I think it doesn't matter whether the buffer is partially filled or not, because every time we encode a new message we firstly fill it with zeroes.
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 1 on Ubuntu 18.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TP1U1804AMD64-12662/test Topology Tests failed for Topo tests part 1 on Ubuntu 18.04 amd64: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12662/artifact/TP1U1804AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
added 3 commits
June 13, 2020 22:53
* Rename netlink utility functions like addattr to be less ambiguous * Replace rta_attr_* functions with nl_attr_* since they introduced inconsistencies in the code * Add helper functions for adding rtnexthop struct to the Netlink message Signed-off-by: Jakub Urbańczyk <xthaid@gmail.com>
* Move code encoding Netlink messages to separate functions * Add buffer bounds checking while creating Nelink messages Signed-off-by: Jakub Urbańczyk <xthaid@gmail.com>
* Use nl_attr_add32 instead of nl_attr_add where it is possible. * Move common code from build_singlepath() and build_multipath() to separate function. Signed-off-by: Jakub Urbańczyk <xthaid@gmail.com>
xThaid
force-pushed
the
dplane_batching_cleanup
branch
from
551db0b
to
a757997
Compare
|
Rebased onto master. |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12668/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warningsWarnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
mjstapp
approved these changes
Jun 16, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the part of the dataplane batch sending project. I added bounds checking in functions encoding Netlink messages. It shouldn't have any effect on the way the code works now, but this will be more important in the future.
inconsistencies in the code
message