-
Notifications
You must be signed in to change notification settings - Fork 39
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
Support only one kernel at run-time, #156
Support only one kernel at run-time, #156
Conversation
Feedback by way of git commits: https://github.com/mjmartineau/mptcpd/commits/user-pm-cmds |
@mjmartineau, I'm splitting the new user space PM command support into a pull request separate from this one. I think this one is self-contained enough. |
Switched to draft. Some additional changes are still pending for this pull request. |
Refactor the multipath-tcp.org kernel path management commands implementations so that they may be leveraged by the corresponding and pending upstream kernel user space commands.
Allow the user to choose build-time support for either the upstream or multipath-tcp.org kernel, e.g.: ./configure --with-kernel=multipath-tcp.org Valid kernel options are "upstream" or "multipath-tcp.org", with the former being the default. Mptcpd will no longer support both kernels at run-time.
The MPTCP path management generic netlink API commands between the upstream and multipath-tcp.org kernels will not be source compatible. Move the refactored command implementation back netlink_pm_mptcp_org.c. This reverts commit dc92d3b.
The common netlink PM command implementation in mptcpd will not work both the upstream and multipath-tcp.org kernels since their netlink path management APIs are not the same. Move kernel-specific code to their respective netlink_pm_{mptcp_org,upstream}.c source file.
Don't bother running the mptcpd unit tests for the multipath-tcp.org kernel build since the GitHub Ubuntu environment doesn't run it.
Refactor upstream and multipath-tcp.org <linux/mptcp.h> header detection checks to a new `m4/mptcpd_kernel.m4' Autoconf macro file.
The HAVE_LINUX_MPTCP_H_UPSTREAM_EVENTS preprocessor symbol is no longer used. Drop references to it.
f63f44a
to
b25666f
Compare
Pull Request Test Coverage Report for Build 1670502137
💛 - Coveralls |
9faaf06
to
af3236d
Compare
@@ -84,6 +86,8 @@ enum { | |||
MPTCP_PM_CMD_SET_LIMITS, | |||
MPTCP_PM_CMD_GET_LIMITS, | |||
MPTCP_PM_CMD_SET_FLAGS, | |||
MPTCP_PM_CMD_ANNOUNCE, | |||
MPTCP_PM_CMD_REMOVE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know there's more to the new API than this. More to come in a subsequent pull request.
@mjmartineau I don't have any other changes pending for this pull request. A subsequent pull request will replace the stubs for the new upstream netlink PM API in src/netlink_pm_upstream.c. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok to me. I did build and run the tests with the upstream kernel configuration, but only configured and built the multipath-tcp.org version (no self tests attempted). Would it be helpful for me to dust off my multipath-tcp.org VM and run the tests there, or have you covered that already?
@@ -280,14 +274,14 @@ LIBS="$LIBS $ELL_LIBS" | |||
dnl l_genl_msg_get_extended_error() was introduced in ELL v0.31. | |||
AC_CHECK_FUNC([l_genl_msg_get_extended_error], | |||
[AC_DEFINE([HAVE_L_GENL_MSG_GET_EXTENDED_ERROR], | |||
[], | |||
[ELL has l_genl_msg_get_extended_error()])]) | |||
[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tab to space conversion? Wasn't sure if it was intentional, but looks like everything has now been normalized to spaces, so that seems fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spaces were intended. It seems there were left over tabs in this file from previous commits that I missed.
Only if you have time. Since I've been focusing on the upstream kernel, I haven't cranked up my multipath-tcp.org kernel VM in a while, so I'll give it a try. Thanks! |
I'll commit any potential changes or fixes for the multipath-tcp.org kernel in a separate pull request. |
Pull Request Test Coverage Report for Build 1670502137Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 1669548099Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Support only one MPTCP capable kernel at run-time, either the upstream or multipath-tcp.org kernel but not both. This simplifies the kernel support in mptcpd, and further prepares mptcpd to support the upcoming user space MPTCP path management commands in the upstream kernel.