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.

Sign up for GitHub

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

Jump to bottom

make listening socket creation optional for userspace path manager plugins #297

Merged
merged 4 commits into from
Aug 23, 2024
Merged

make listening socket creation optional for userspace path manager plugins #297

merged 4 commits into from
Aug 23, 2024

Conversation

marco-a-itl
Copy link
Contributor

Proposal to resolve #296

matttbe
matttbe requested changes Aug 7, 2024
View reviewed changes
Copy link
Member

@matttbe matttbe left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! It looks good to me, but I think it will break the API, and it looks like we can avoid that easily, no?

include/mptcpd/path_manager.h Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Aug 7, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 10286346027

Details

  • 6 of 8 (75.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 64.766%
Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/path_manager.c 3 5 60.0%
Totals Coverage Status
Change from base Build 10114597542: -0.01%
Covered Lines: 1397
Relevant Lines: 2157
💛 - Coveralls

@marco-a-itl
Copy link
Contributor Author

Modified the PR so that a new mptcpd_pm_add_addr_no_listener function is defined.
Fixed also mptcp_org_add_addr compile error for out-of-tree kernel.

@marco-a-itl marco-a-itl requested a review from matttbe August 7, 2024 13:28
matttbe
matttbe requested changes Aug 7, 2024
View reviewed changes
Copy link
Member

@matttbe matttbe left a comment

Choose a reason for hiding this comment

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

Thank you! It looks good to me, just two last small requests if that's OK for you.

@mjmartineau / @ossama-othman : because this modifies the public API, do you mind having a quick look please?

src/netlink_pm_upstream.c Outdated Show resolved Hide resolved
lib/path_manager.c Outdated Show resolved Hide resolved
@marco-a-itl
Copy link
Contributor Author

Modified the PR to avoid double negation check.
do_pm_add_addr is just above mptcpd_pm_add_addr*().

@marco-a-itl marco-a-itl requested a review from matttbe August 7, 2024 14:36
matttbe
matttbe approved these changes Aug 7, 2024
View reviewed changes
Copy link
Member

@matttbe matttbe left a comment

Choose a reason for hiding this comment

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

Thank you for the update! It looks good to me, but I prefer to wait for Ossama or Mat's feedback (if they are available).

@matttbe
Copy link
Member

matttbe commented Aug 7, 2024

It looks like the issue with the CI is due to an API breakage on ELL side. I just opened a new bug report as it is not due to these modifications, anybody can fix it: #299

@matttbe
Copy link
Member

matttbe commented Aug 22, 2024

@ossama-othman: if you have the opportunity, do you mind having a quick look because this PR modifies the public API please? :)

@matttbe matttbe requested a review from ossama-othman August 22, 2024 15:37
@ossama-othman
Copy link
Member

@matttbe Sure! I'll take a look today.

@marco-a-itl Thank you very much for the PR!

ossama-othman
ossama-othman approved these changes Aug 22, 2024
View reviewed changes
Copy link
Member

@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.

It looks good to me. Thank you very much!

Perhaps not in this PR, but when releasing a new version of mptcpd the NEWS file could be updated with a description of the API addition, as well bumping the libtool versioning accordingly. From configure.ac:

# ---------------------------------------------------------------
# Libtool Based Library Versioning
# ---------------------------------------------------------------
# Source     changed:  REVISION++
# Interfaces changed:  CURRENT++ REVISION=0
#            added:    CURRENT++ REVISION=0 AGE++
#            removed:  CURRENT++ REVISION=0 AGE=0
LIB_CURRENT=3
LIB_REVISION=0
LIB_AGE=0

@matttbe
Copy link
Member

matttbe commented Aug 23, 2024

It looks good to me. Thank you very much!

Thank you! Just squashed and merged!

Perhaps not in this PR, but when releasing a new version of mptcpd the NEWS file could be updated with a description of the API addition, as well bumping the libtool versioning accordingly.

OK, noted!

From configure.ac:

# ---------------------------------------------------------------
# Libtool Based Library Versioning
# ---------------------------------------------------------------
# Source     changed:  REVISION++
# Interfaces changed:  CURRENT++ REVISION=0
#            added:    CURRENT++ REVISION=0 AGE++
#            removed:  CURRENT++ REVISION=0 AGE=0
LIB_CURRENT=3
LIB_REVISION=0
LIB_AGE=0

So here, because only new things have been added, I would need to update both CURRENT and AGE, to say that this lib supports everything implemented from CURRENT-AGE?

@matttbe matttbe merged commit 334bb9c into multipath-tcp:main Aug 23, 2024
4 of 6 checks passed
matttbe added a commit to matttbe/mptcpd that referenced this pull request Oct 9, 2024
@matttbe
lib: switch to v4.0.1: only added items
94ef796 
The API has been modified: only new items have been added, see multipath-tcp#297 and

It is then required to increase CURRENT and AGE, according to libtool's
documentation.

Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
@matttbe matttbe mentioned this pull request Oct 9, 2024
Merged
matttbe added a commit to matttbe/mptcpd that referenced this pull request Oct 9, 2024
@matttbe
lib: switch to v4.0.1: only added items
b912478 
The API has been modified: only new items have been added, see PR multipath-tcp#297
and multipath-tcp#304.

It is then required to increase CURRENT and AGE, according to libtool's
documentation.

Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
matttbe added a commit that referenced this pull request Oct 26, 2024
@matttbe
lib: switch to v4.0.1: only added items (#306)
05a08bf 
The API has been modified: only new items have been added, see PR #297
and #304.

It is then required to increase CURRENT and AGE, according to libtool's
documentation.

Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ossama-othman ossama-othman ossama-othman approved these changes

@matttbe matttbe matttbe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

make listening socket creation optional for userspace path manager plugins
4 participants
@marco-a-itl @coveralls @matttbe @ossama-othman