gnrc_sock: consider all pktsnip for gnrc_neterr reporting

[miri64]

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.

[benpicco]

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]

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]

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]

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]

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]

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]

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]

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

[miri64]

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