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

gnrc_tcp: Prepare for sock integration. #13026

Merged
merged 1 commit into from
Feb 26, 2020
Merged

gnrc_tcp: Prepare for sock integration. #13026

merged 1 commit into from
Feb 26, 2020

Conversation

brummer-simon
Copy link
Member

This PR changes required for a later sock integration.

This PR introduces a dedicated struct called Endpoint storing all Information required to specify one end of a TCP connection. Additionally all IP-Address parsing/Interface ID handling is moved out of the core TCP functions into the initialization function of an Endpoint. Beware, this PR contains API changes for gnrc_tcp.

@brummer-simon brummer-simon requested a review from miri64 January 4, 2020 11:43
@brummer-simon brummer-simon force-pushed the gnrc_tcp-prepare_for_sock_integration branch from 1cb01fe to 65db70d Compare January 4, 2020 13:38
@benpicco benpicco added Area: network Area: Networking Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Jan 4, 2020
miri64
miri64 reviewed Jan 6, 2020
View reviewed changes
@miri64 miri64 changed the title Prepare for sock integration. gnrc_tcp: Prepare for sock integration. Jan 9, 2020
miri64
miri64 requested changes Jan 9, 2020
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Except for a naming problem (see above) and a parameter problem (resolved), this PR mostly only has documentation issues.

@brummer-simon
Copy link
Member Author

The Documentation related Issues should be solved. The Naming / Semantics of the Endpoint construction function are still open.

@brummer-simon brummer-simon force-pushed the gnrc_tcp-prepare_for_sock_integration branch from f1c049a to 99d6ff0 Compare January 24, 2020 13:31
@miri64
Copy link
Member

miri64 commented Jan 28, 2020

The Naming / Semantics of the Endpoint construction function are still open.

Yepp, still waiting for a comment of yours there ;-).

@brummer-simon
Copy link
Member Author

The Naming / Semantics of the Endpoint construction function are still open.

Yepp, still waiting for a comment of yours there ;-).

I know, I'll address this as soon as I find some spare time.

@brummer-simon
Copy link
Member Author

brummer-simon commented Feb 2, 2020 •
edited
Loading

@miri64 - The remaining task is done.

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 5, 2020
@benpicco
Copy link
Contributor

benpicco commented Feb 5, 2020

Murdock is not happy yet.

@brummer-simon
Copy link
Member Author

@benpicco - Murdock is not happy because the PR is not squashed. This PR not squashed because it is easier to review the latest changes this way. As soon as @miri64 is happy with the changes, I'll squash the above change into a single PR what can be built and merged ;)

@benpicco
Copy link
Contributor

benpicco commented Feb 5, 2020 •
edited
Loading

Murdock is not happy because the PR is not squashed

Nope, that's not the sole reason for it's unhappiness.
It's the empty unions that make it sad.

@brummer-simon
Copy link
Member Author

@benpicco - Ah I see. I'll fix that right away.

@brummer-simon brummer-simon force-pushed the gnrc_tcp-prepare_for_sock_integration branch 2 times, most recently from 6e052d7 to a823b0a Compare February 6, 2020 08:52
@brummer-simon
Copy link
Member Author

@miri64 - Ping

@brummer-simon brummer-simon requested a review from miri64 February 16, 2020 17:56
@brummer-simon
Copy link
Member Author

@miri64 - Ping Ping

@miri64
Copy link
Member

miri64 commented Feb 21, 2020

Sorry, this somehow fell under the rug. I have another look!

miri64
miri64 previously requested changes Feb 21, 2020
View reviewed changes
@brummer-simon
Copy link
Member Author

@miri64 - Are the remaining issues resolved by now or is there something left todo?

@miri64 miri64 added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines labels Feb 25, 2020
@miri64 miri64 added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Feb 25, 2020
@miri64 miri64 dismissed their stale review February 25, 2020 19:19

There are some vera++ warnings, but you may ignore them (the 80 char limit anyway, see #13400). I successfully tested on native but still like to test on samr21-xpro tomorrow. Other than that, you may squash

@miri64 miri64 self-requested a review February 25, 2020 19:19
miri64
miri64 previously approved these changes Feb 26, 2020
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Tests succeed also on samr21-xpro

@miri64 miri64 added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Feb 26, 2020
@miri64 miri64 dismissed their stale review February 26, 2020 11:16

Let's resolve #13026 (comment) first.

@miri64 miri64 removed the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Feb 26, 2020
miri64
miri64 approved these changes Feb 26, 2020
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Minor remaining nit. You may squash that immediately.

sys/include/net/gnrc/tcp.h Outdated
uint8_t ipv6[sizeof(ipv6_addr_t)]; /**< IPv6 address storage */
} addr; /**< IP address storage */
#else
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#else
#endif

This way it is better extensible ;-) (of course the #endif below needs to be removed)

@brummer-simon brummer-simon force-pushed the gnrc_tcp-prepare_for_sock_integration branch from 9b8a755 to 10872d9 Compare February 26, 2020 20:24
@miri64 miri64 added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Feb 26, 2020
@miri64
Copy link
Member

miri64 commented Feb 26, 2020

Feel free to hit the friendly green button, once its their. Not sure I will look at Github again 'til next Tuesday ;-)

@benpicco
Copy link
Contributor

I'm afraid @brummer-simon has no button-clicking powers yet.
I'll do the deed - enjoy your vacation! 🧙‍♀️

@benpicco benpicco merged commit 244c7da into RIOT-OS:master Feb 26, 2020
@brummer-simon
Copy link
Member Author

Enjoy your vacation and thanks for the review.

@brummer-simon brummer-simon deleted the gnrc_tcp-prepare_for_sock_integration branch February 27, 2020 02:49
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

Assignees
No one assigned
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2020.04
Development

Successfully merging this pull request may close these issues.
4 participants
@brummer-simon @miri64 @benpicco @leandrolanzieri