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

Correct mptcpd internal initialization order issues. #171

Merged
merged 16 commits into from
Nov 2, 2021

Conversation

ossama-othman
Copy link
Member

Mptcpd synchronizes its internal state with the kernel at process start, and then continues with initialization such as plugin load and trigger the "pm_ready" callback. However, the kernel sync operation completes asynchronously meaning the kernel isn't actually "ready" when the plugins are loaded and "pm_ready" callback is triggered.

Introduce a new "complete" function parameter to mptcpd path manager command functions that complete asynchronously, e.g. dump_addrs, that is called when the operation completes. The "complete" function differs from user provided callbacks that are called when asynchronous results are available in that it is called regardless of whether or not asynchronous results are available. Furthermore, they are only called once at the very end of the asynchronous call, whereas those called upon availability of results may be called multiple times for a single asynchronous call, such as the case for dump_addrs.

Leverage the new "complete" function parameter to force correct initialization order of internal mptcpd path manager components. In particular, the dump_addrs call used to sync kernel/mptcpd state now completes before continuing with things like mptcpd plugin load. This allows internal mptcpd state to be stable and consistent prior to plugin load, and before the "pm_ready" callback is triggered.

MPTCP path management generic netlink event handlers are also now registered after plugins are loaded since there is no point handling events without the plugins.

@ossama-othman ossama-othman added bug Something isn't working enhancement New feature or request labels Oct 29, 2021
@ossama-othman ossama-othman added this to the Q4-2021 milestone Oct 29, 2021
@ossama-othman ossama-othman self-assigned this Oct 29, 2021
@coveralls
Copy link

coveralls commented Oct 29, 2021

Pull Request Test Coverage Report for Build 1400154986

Warning: 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

  • 35 of 46 (76.09%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 48.899%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/path_manager.c 17 28 60.71%
Totals Coverage Status
Change from base Build 1356760512: 0.1%
Covered Lines: 977
Relevant Lines: 1998

💛 - Coveralls

Copy link
Member Author

@ossama-othman ossama-othman left a comment

Choose a reason for hiding this comment

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

I'd like to get a complete function parameter to the mptcpd_kpm_get_limits() function as well since it also completes asynchronously. Another pull request with that change might be more suitable.

*
* @param[in] limits Array of MPTCP resource type/limit
* pairs. @c NULL on error.
* @param[in] len Length of the @a limits array. Zero
* on error.
* @param[in,out] callback_data Data provided by the caller of
* @c mptcpd_pm_get_limits().
* @c mptcpd_kpm_get_limits().
*/
typedef void (*mptcpd_pm_get_limits_cb)(
struct mptcpd_limit const *limits,
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll probably rename this typedef to mptcpd_kpm_get_limits_cb_t in another pull request for the sake of consistency.

tests/test-commands.c Outdated Show resolved Hide resolved
Copy link
Member

@mjmartineau mjmartineau left a comment

Choose a reason for hiding this comment

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

A few minor questions & suggestions

include/mptcpd/types.h Outdated Show resolved Hide resolved
include/mptcpd/types.h Outdated Show resolved Hide resolved
include/mptcpd/private/path_manager.h Outdated Show resolved Hide resolved
@ossama-othman ossama-othman marked this pull request as ready for review November 2, 2021 17:50
Copy link
Member

@mjmartineau mjmartineau left a comment

Choose a reason for hiding this comment

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

I'm seeing a sudo test-commands failure, but it's the same as the main branch.

Ossama Othman added 16 commits November 2, 2021 13:42
Introduce a new mptcpd_complete_func_t typedef to be used as the
paremeter type for functions to be called when a mptcpd asynchronous
call completes.  That case is not the same as when asynchronous
results are available since results are not always available, but
completion always occurs.

Also introduce mptcpd_kpm_complete_func_t alias of
mptcpd_complete_func_t to be used specifically for mptcpd_{k}pm
related asynchronous functions, such as mptcpd_kpm_dump_addrs().

Lastly, rename mptcpd_pm_get_addr_cb to mptcpd_kpm_get_addr_cb_t, and
deprecate the former.
Leverage the newly added changes that allow the user to pass a
function to be alled on completion of the mptcpd asynchronous path
management operations, e.g. dump_addrs, to force correct
initialization order of internal mptcpd path manager components.  In
particular, the dump_addrs call used to sync kernel/mptcpd state now
completes before continuing with things like mptcpd plugin load.  This
allows internal mptcpd state to be stable and consistent prior plugin
load, and before the "pm_ready" callback is triggered.

MPTCP path management generic netlink event handlers are also now
registered after plugins are loaded since there is no point handling
events without the plugins.
@ossama-othman ossama-othman merged commit 6e72ddb into multipath-tcp:master Nov 2, 2021
@ossama-othman ossama-othman deleted the async-complete-func branch November 2, 2021 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants