mptcpd
mptcpd copied to clipboard
multipath-tcp
Plugins configuration files
Adds a new plugin operation that allows to read a configuration file. It receives the filename of the configuration file, a parser function and an extra argument that is passed to the parser function.
It also adds a new option to define where the configuration files are located.
The default location is created when make install is executed.
Closes #174.
Pull Request Test Coverage Report for Build 1895191390
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.
- For more information on this, see Tracking coverage changes with pull request builds.
- To avoid this issue with future PRs, see these Recommended CI Configurations.
- For a quick fix, rebase this PR at GitHub. Your next report should be accurate.
Details
- 35 of 60 (58.33%) changed or added relevant lines in 4 files are covered.
- 215 unchanged lines in 4 files lost coverage.
- Overall coverage decreased (-0.05%) to 53.216%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| src/path_manager.c | 0 | 1 | 0.0% |
| lib/plugin.c | 4 | 10 | 40.0% |
| lib/configuration.c | 18 | 26 | 69.23% |
| src/configuration.c | 13 | 23 | 56.52% |
| <!-- | Total: | 35 | 60 |
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| lib/plugin.c | 3 | 66.55% |
| src/configuration.c | 28 | 50.76% |
| src/path_manager.c | 33 | 14.58% |
| lib/network_monitor.c | 151 | 44.04% |
| <!-- | Total: | 215 |
| Totals | |
|---|---|
| Change from base Build 1764368853: | -0.05% |
| Covered Lines: | 1084 |
| Relevant Lines: | 2037 |
💛 - Coveralls
Several of the tests in the GitHub build are also failing with these changes in places (see the "Checks" tab).
Yeah, I noticed.
Since I didn't know if you would agree with some of the changes I though it would be better to send the patch before actually finishing and cleaning it.
Although it did not appear during the checks, test-cxx-build actually fails with ell >= 0.45.
In ell version 0.45 DEFINE_CLEANUP_FUNC was added, and since ell/settings.h is included in the mptcpd/types.h header, the compilation of test-cxx-build results in the following error:
In file included from /usr/include/ell/settings.h:28,
from ../include/mptcpd/types.h:15,
from ../include/mptcpd/path_manager.h:14,
from test-cxx-build.cpp:16:
/usr/include/ell/settings.h: In function ‘void l_settings_free_cleanup(void*)’:
/usr/include/ell/settings.h:41:1: error: invalid conversion from ‘void*’ to ‘l_settings*’ [-fpermissive]
41 | DEFINE_CLEANUP_FUNC(l_settings_free);
| ^~~~~~~~~~~~~~~~~~~
| |
| void*
In file included from ../include/mptcpd/types.h:15,
from ../include/mptcpd/path_manager.h:14,
from test-cxx-build.cpp:16:
/usr/include/ell/settings.h:40:41: note: initializing argument 1 of ‘void l_settings_free(l_settings*)’
40 | void l_settings_free(struct l_settings *settings);
| ~~~~~~~~~~~~~~~~~~~^~~~~~~~
I'm gonna try to get a workaround in order for the check to be successful.
Which compiler are you using? test-cxx-build successfully builds and runs for me with gcc 11.2 and ELL 0.45 and the current ELL master branch (0.49+).
That's very interesting.
I use the same, gcc 11.2. I tested it on my Arch linux, in one system with Archlabs and also in a Fedora vm, all with gcc 11.2 and ell 0.48 and test-cxx-build failed to build on all of them with the same error.
Strange. I tested on both Ubuntu 21.10 and Fedora with gcc 11.2 and the latest ELL with no problem. I'll keep digging.
Are you configuring mptcpd in a specific way or just running configure without any command line options or variable tweaks?
Are you configuring mptcpd in a specific way or just running
configurewithout any command line options or variable tweaks?
Just ./configure.
Would it be possible to bump the version of ell to one >= 0.45 of the automated builds to check if the error occurs?
I bumped the version of ell of the automated build in one of my forks and got the same error (check https://github.com/MPTCP-Lab/mptcpd/tree/plugins_confs_test2 which contains the patch without the workaround and with ell == 0.48).
Although the workaround works (check https://github.com/MPTCP-Lab/mptcpd/tree/plugins_confs_test which has the workaround and ell == 0.48), I don't know if there is any problem with the way the workaround was done, for example with the licenses, since it is a copy of the ell/cleanup.h with void * replaced by struct l_settings *.
I'm still having a hard time reproducing the C++ build error you're running into. For example, the build against the ELL master branch I added in PR #222 works fine. I'll try with the code coverage build like you did.
The error only occurs with this patch, since it adds #include <ell/settings.h> to mptcpd/types.h.
With the master version the error does not occur.
Okay, I see.
Do <ell/settings.h> and <ell/cleanup.h> need to be included in <mptcpd/types.h>? Generally, we try to avoid directly or indirectly including ELL headers in the <mptcpd/*.h> public headers. Furthermore, it looks like a forward declaration of struct l_settings could be used in <mptcpd/types.h> in this patch instead of including <ell/settings.h> and <ell/cleanup.h>. That would prevent exposing potentially problematic ELL headers to C++ compilers.
I'll probably modify the test-cxx-build.c source to include all of the <mptcpd/*.h> public headers instead of the current subset to make sure they can all be parsed by a C++ compiler, too.
Do <ell/settings.h> and <ell/cleanup.h> need to be included in <mptcpd/types.h>? Generally, we try to avoid directly or indirectly including ELL headers in the <mptcpd/*.h> public headers.
I have the habit of just including where the structs/types in use are defined. The inclusion of <ell/cleanup.h was to make sure it used the workaround.
Furthermore, it looks like a forward declaration of struct l_settings could be used in <mptcpd/types.h> in this patch instead of including <ell/settings.h> and <ell/cleanup.h>.
You are right. I never thought of that.
I already reverted the workaround and just declared the struct l_settings inside of <mptcpd/types.h> instead of including <ell/settings.h>.