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_sock: provide port for sock_ip and sock_udp #5772

Merged
merged 4 commits into from
Oct 27, 2016
Merged

gnrc_sock: provide port for sock_ip and sock_udp #5772

merged 4 commits into from
Oct 27, 2016

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Aug 26, 2016 •
edited by cgundogan
Loading

This PR provides the port for sock_ip and sock_udp (see #5758) to GNRC + tests.

Depends on #5526 (mbox support for GNRC), #5749 (fix to UDP so the tests are able to run), and #5758 (the sock header files).

@miri64 miri64 added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first labels Aug 26, 2016
@miri64
Copy link
Member Author

miri64 commented Aug 26, 2016

@brummer-simon this PR might also be of interest for you to give you an idea for how to port sock_tcp using mbox ;-).

@miri64 miri64 mentioned this pull request Aug 26, 2016
Merged
@miri64
Copy link
Member Author

miri64 commented Aug 26, 2016

Rebased due to changes in #5526.

@miri64 miri64 added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Aug 26, 2016
This was referenced Aug 30, 2016
Closed
Closed
Closed
Merged
@kaspar030
Copy link
Contributor

sock is merged, pls rebase

@immesys immesys mentioned this pull request Sep 29, 2016
Merged
@miri64 miri64 added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Oct 4, 2016
@miri64
Copy link
Member Author

miri64 commented Oct 4, 2016

Can we merge the dependencies first please? Also, while working on lwIP I realized I need to adapt the tests a little ;-).

@miri64
Copy link
Member Author

miri64 commented Oct 4, 2016

Rebased to current dependencies and master.

@miri64 miri64 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Oct 4, 2016
@miri64
Copy link
Member Author

miri64 commented Oct 4, 2016

Applied the changes I spoke about too.

@miri64 miri64 force-pushed the gnrc/feat/sock branch 3 times, most recently from 58c2df3 to 779ff21 Compare October 4, 2016 16:36
@miri64
Copy link
Member Author

miri64 commented Oct 4, 2016

Rebased to current master, since #5884 was merged.

@miri64 miri64 mentioned this pull request Oct 5, 2016
Merged
@miri64
Copy link
Member Author

miri64 commented Oct 6, 2016

While working on #5802 I realized that their raw sock equivalent require the IP header to be provided. This seems to be the behavior with POSIX sockets, too (see e.g. this tutorial). I did not implement it like that in gnrc_sock_ip, because the API was conceptualized under a different premise (the header is build by the API). However that would make the implementation of the socket wrapper much more complicated. So how do we handle this?

@miri64
Copy link
Member Author

miri64 commented Oct 6, 2016

Nvm... lwIP isn't consistent in this itself... and so is the information about raw sockets themselves..

@miri64
Copy link
Member Author

miri64 commented Oct 12, 2016

Needs some small adjustments due to #5929 being merged.

@haukepetersen
Copy link
Contributor

From my side everything looks in order. Tests succeed and I successfully tested the sock_udp implementation under native. ACK once squashed and Murdock is happy.

@miri64
Copy link
Member Author

miri64 commented Oct 25, 2016

Squashed

@miri64 miri64 added this to the Release 2016.10 milestone Oct 25, 2016
@miri64 miri64 removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable State: waiting for other PR State: The PR requires another PR to be merged first labels Oct 25, 2016
@miri64
Copy link
Member Author

miri64 commented Oct 25, 2016

Rebased to current master.

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 25, 2016
@miri64
Copy link
Member Author

miri64 commented Oct 25, 2016

Ooops, didn't adapt it to the pseudo-module change in #5526. Done now.

@cgundogan
Copy link
Member

failed to download the tiny-asn1 pkg, retriggering murdock

@cgundogan cgundogan added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 26, 2016
@cgundogan
Copy link
Member

Murdock still fails @miri64

@miri64
Copy link
Member Author

miri64 commented Oct 26, 2016

Fixed, rebased and squashed.

@miri64
Copy link
Member Author

miri64 commented Oct 26, 2016

Okay, cppcheck is really acting weird on this lines... first it complains about the "redundant" check (dereferencing sock after I checked if it is NULL in an || connection, which thanks to lazy evaluation should be fine), when I suppress it it complains that there is a NULL pointer dereference at that line (no it isn't because lazy evaluation) and when I write it out for it ((sock == NULL) || ((sock != NULL) && (sock->foobar == ...))) it is complaining that the additional check for (sock != NULL) isn't necessary, since (sock == NULL) || (sock->foobar == ...) is equivalent (which is what was there in the first place...) so I just suppressed everything on this line. Since my tests are covering this line and they are not crashing I guess we are fine and cppcheck just is weird. @cgundogan could you re-ACK please? The changes are in https://github.com/RIOT-OS/RIOT/pull/5772/files#diff-71ddba2942574ad9a8d73c8de31b2230R188 and https://github.com/RIOT-OS/RIOT/pull/5772/files#diff-97c7bd9404c4bc328009313d1665dfe5R150 respectively.

@miri64
Copy link
Member Author

miri64 commented Oct 26, 2016

Murdock is finally happy!

@haukepetersen
Copy link
Contributor

I am good with the changes (so re-ACK) and Murdock is happy -> go

@miri64 Congrats, this is big step towards usability of GNRC!

@haukepetersen haukepetersen merged commit f6413ee into RIOT-OS:master Oct 27, 2016
@miri64 miri64 deleted the gnrc/feat/sock branch October 27, 2016 09:51
@miri64 miri64 mentioned this pull request Sep 7, 2020
Merged
benpicco
benpicco reviewed Nov 6, 2022
View reviewed changes
sys/net/gnrc/sock/udp/gnrc_sock_udp.c
Comment on lines +147 to +156
if ((sock->remote.family != AF_UNSPEC) && /* check remote end-point if set */
((sock->remote.port != byteorder_ntohs(hdr->src_port)) ||
/* We only have IPv6 for now, so just comparing the whole end point
* should suffice */
((memcmp(&sock->remote.addr, &ipv6_addr_unspecified,
sizeof(ipv6_addr_t)) != 0) &&
(memcmp(&sock->remote.addr, &tmp.addr, sizeof(ipv6_addr_t)) != 0)))) {
gnrc_pktbuf_release(pkt);
return -EPROTO;
}
Copy link
Contributor

@benpicco benpicco Nov 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why is this condition needed?
This enforces that if the socket was previously used for sending, received packets must come from the same address.

But there is nothing that says a socket must be bound to the remote address, but there is even a test for this.

Why is this there? This makes it impossible to use a socket to send to a multicast address and receive unicast responses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keestux keestux keestux left review comments

@benpicco benpicco benpicco left review comments

@haukepetersen haukepetersen haukepetersen approved these changes

Assignees

@kaspar030 kaspar030

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 Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2016.10
Development

Successfully merging this pull request may close these issues.
6 participants
@miri64 @kaspar030 @haukepetersen @cgundogan @keestux @benpicco