Skip to content

gnrc_sock: consider all pktsnip for gnrc_neterr reporting #12682

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
Merged
merged 2 commits into from
Nov 12, 2019
Merged

gnrc_sock: consider all pktsnip for gnrc_neterr reporting #12682

merged 2 commits into from
Nov 12, 2019

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Nov 10, 2019 •
edited
Loading

Contribution description

As analyzed in #12678 (comment) there are cases where different reports can be generated for the different snips of the packet send via the sock.

To catch all errors generated by the stack, the sock has to subscribe for all snips of the packet sent. If any of the snips reports an error distinct from GNRC_NETERR_SUCCESS or the previous one, we report that status instead of just the first we receive. This way we are ensured to have the first error reported by the stack for the given packet.

Testing procedure

Merge #12678 into this PR, set ENABLE_DEBUG=1 in sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c (fix for debug messages piggy-backed) and run the test for several scenarios: It should succeed or fail, but with good reason. E.g.

Vanilla (no interfaces) 
ipv6: waiting for incoming message.
main(): This is RIOT! (Version: 2020.01-devel-584-g0d08d-gnrc_sock/fix/consider-all-snips-for-error)
ipv6: GNRC_NETAPI_MSG_TYPE_SND received
ipv6: send unicast
ipv6: no link-layer address or interface for next hop to fe80::2
ipv6: waiting for incoming message.
SUCCESS: error code EHOSTUNREACH (113 == 113)
ipv6: GNRC_NETAPI_MSG_TYPE_SND received
ipv6: send unicast
ipv6: no link-layer address or interface for next hop to 2001:db8::2
ipv6: waiting for incoming message.
SUCCESS: error code ENETUNREACH (101 == 101)
With an IEEE 802.15.4 interface (i.e. using 6Lo-ND) 
[…]
ipv6: waiting for incoming message.
main(): This is RIOT! (Version: 2020.01-devel-584-g0d08d-gnrc_sock/fix/consider-all-snips-for-error)
ipv6: GNRC_NETAPI_MSG_TYPE_SND received
ipv6: send unicast
ipv6: set payload length to 16 (network byteorder 1000)
ipv6: set next header to 17
ipv6: set packet source to fe80::25a:4550:a00:455b
ipv6: write protect up to payload to calculate checksum
ipv6: calculate checksum for upper header.
ipv6: add interface header to packet
ipv6: send unicast over interface 6
ipv6: Sending (src = fe80::25a:4550:a00:455b, dst = fe80::2, next header = 17, length = 16)
ipv6: send to 6LoWPAN instead
ipv6: waiting for incoming message.
FAILURE: gnrc_udp_send() had an unexpected error code: 8
ipv6: GNRC_NETAPI_MSG_TYPE_SND received
ipv6: send unicast
ipv6: no link-layer address or interface for next hop to 2001:db8::2
ipv6: waiting for incoming message.
SUCCESS: error code ENETUNREACH (101 == 101)
[…]

(first send succeeds as the link-local packet is actually send due to address resolution by reverse translation, second fails with -ENETUNREACH, as the non-existing route is looked up as with vanilla)

With an Ethernet interface 
[…]
ipv6: waiting for incoming message.
main(): This is RIOT! (Version: 2020.01-devel-584-g0d08d-gnrc_sock/fix/consider-all-snips-for-error)
[…]
ipv6: GNRC_NETAPI_MSG_TYPE_SND received
ipv6: set payload length to 32 (network byteorder 2000)
ipv6: set next header to 58
ipv6: write protect up to payload to calculate checksum
ipv6: calculate checksum for upper header.
ipv6: send multicast over interface 5
ipv6: Sending (src = fe80::2482:dcff:fe3b:cae6, dst = ff02::1:ff00:2, next header = 58, length = 32)
ipv6: waiting for incoming message.
ipv6: NIB timer event received
ipv6: waiting for incoming message.
ipv6: GNRC_NETAPI_MSG_TYPE_SND received
ipv6: set payload length to 32 (network byteorder 2000)
ipv6: set next header to 58
ipv6: write protect up to payload to calculate checksum
ipv6: calculate checksum for upper header.
ipv6: send multicast over interface 5
ipv6: Sending (src = fe80::2482:dcff:fe3b:cae6, dst = ff02::1:ff00:2, next header = 58, length = 32)
ipv6: waiting for incoming message.
ipv6: NIB timer event received
ipv6: waiting for incoming message.
SUCCESS: error code EHOSTUNREACH (113 == 113)
ipv6: GNRC_NETAPI_MSG_TYPE_SND received
ipv6: send unicast
ipv6: no link-layer address or interface for next hop to 2001:db8::2
ipv6: waiting for incoming message.
SUCCESS: error code ENETUNREACH (101 == 101)
[…]

(node tries to probe for neighbor several times before finally reporting EHOSTUNREACH for the first, for the second ENETUNREACH is again reported, as the non-existing route is looked up as with vanilla

Issues/PRs references

Discovered in #12678.
Fixes #11551.

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 10, 2019
@miri64 miri64 added this to the Release 2020.01 milestone Nov 10, 2019
@miri64 miri64 requested a review from benpicco November 10, 2019 12:50
@miri64 miri64 force-pushed the gnrc_sock/fix/consider-all-snips-for-error branch from 970362d to fc74f2e Compare November 10, 2019 12:51
@miri64 miri64 changed the title Gnrc sock/fix/consider all snips for error gnrc_sock: consider all pktsnip for gnrc_neterr reporting Nov 10, 2019
@miri64 miri64 mentioned this pull request Nov 10, 2019
Merged
@benpicco
Copy link
Contributor

benpicco commented Nov 10, 2019 •
edited
Loading

I'd say this also fixes #11551 (although an update to the documentation of sock_udp_send() is still needed IMHO.)

I can confirm that the errors are now reported properly and not silently dropped.

@miri64
Copy link
Member Author

miri64 commented Nov 10, 2019

although an update to the documentation of sock_udp_send() is still needed IMHO.

What do you have in mind? At least -EHOSTUNREACH is already documented. So just add documentation for -ENETUNREACH? We can't mention gnrc_neterr in that API, as it is stack agnostic.

@benpicco
Copy link
Contributor

We can't mention gnrc_neterr in that API, as it is stack agnostic.

We shouldn't be overly cryptic to our users just for the sake of … not leaking abstractions in documentation?
A

@note If you are using the `gnrc` network stack, full error reporting will only be available using the `gnrc_neterr` module.

would not hurt and avoid surprises.

@miri64
Copy link
Member Author

miri64 commented Nov 10, 2019

We shouldn't be overly cryptic to our users just for the sake of … not leaking abstractions in documentation?
A

@note If you are using the `gnrc` network stack, full error reporting will only be available using the `gnrc_neterr` module.

would not hurt and avoid surprises.

This is as cryptic as not talking about GNRC, if a user does not know about GNRC (sock can also be used on Linux e.g.). How about amending the gnrc_sock documentation with such details and adding a note like "Refer to the implementation's documentation for behavioral optimizations at compile-time"?

@benpicco
Copy link
Contributor

How about amending the gnrc_sock documentation with such details

If that's the right place for it, sure!

and adding a note like "Refer to the implementation's documentation for behavioral optimizations at compile-time"

But please don't be so vague.

Right now it reads

@return The number of bytes sent on success.

And then a list of possible errors that only get reported if gnrc_neterr is used - which is not the default.
I would not be able to guess that 'behavioral optimizations' refers to optional error reporting.

@miri64
Copy link
Member Author

miri64 commented Nov 11, 2019

And then a list of possible errors that only get reported if gnrc_neterr is used - which is not the default.
I would not be able to guess that 'behavioral optimizations' refers to optional error reporting.

Untrue, when using lwip_sock_udp these errors don't require GNRC ;-P

@miri64
Copy link
Member Author

miri64 commented Nov 11, 2019

I would not be able to guess that 'behavioral optimizations' refers to optional error reporting.

But yes, I'll try to be less vague.

benpicco
benpicco approved these changes Nov 11, 2019
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Errors are now properly reported when gnrc_neterr is used.
Code looks alright too.

@miri64
gnrc_sock: consider all pktsnip for gnrc_neterr reporting
8992dce 
As analyzed in RIOT-OS#12678 there are cases where different reports can be
generated for the different snips of the packet send via the `sock`.

To catch all errors generated by the stack, the sock has to subscribe
for all snips of the packet sent. If any of the snips reports an error
distinct from `GNRC_NETERR_SUCCESS` or the previous one, we report that
status instead of just the first we receive. This way we are ensured to
have the first error reported by the stack for the given packet.
@miri64 miri64 force-pushed the gnrc_sock/fix/consider-all-snips-for-error branch from fc74f2e to 8992dce Compare November 11, 2019 15:40
@miri64
Copy link
Member Author

miri64 commented Nov 11, 2019

Added a suppression comment for the cppcheck warning. pkt is initialized at this point or the function has returned before, so the warning is a false positive

@miri64 miri64 merged commit 918a4ac into RIOT-OS:master Nov 12, 2019
@miri64 miri64 deleted the gnrc_sock/fix/consider-all-snips-for-error branch November 12, 2019 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2020.01
Development

Successfully merging this pull request may close these issues.

sock_udp_send() reports success even though the message was never sent
2 participants
@miri64 @benpicco