gnrc: Use existing utility function to extract IPv6 header #10071
Conversation
miri64
added
Type: enhancement
There was a problem hiding this comment.
I'm not sure all those checks are actually necessary, so it might be worth checking with the GNRC architectural decisions whether a GNRC_NETTYPE_IPV6 marked snip can actually have its data NULL and a size of < 40, but without that checked, I'd err on the side of caution.
It can. A "user" could just allocate a packet snip with that type and set data and type to whatever and then send it to the IPv6 thread via NETAPI_RCV. There are no restriction given to that.
Impact-wise, this might add a few bytes of text as gnrc_ipv6_get_header is so far not used in the rest of the code; if that's a concern, we might consider making it inline-able (how's -flto coming?).
It could also, as in the original check, go into an assert(). Since as I pointed already out that this check might only be caused by user error anyway, this might be the better way to go for most if not all of the checks in gnrc_ipv6_get_header().
| @@ -197,7 +197,7 @@ ipv6_hdr_t *gnrc_ipv6_get_header(gnrc_pktsnip_t *pkt) | |||
| { | |||
| ipv6_hdr_t *hdr = NULL; | |||
| gnrc_pktsnip_t *tmp = gnrc_pktsnip_search_type(pkt, GNRC_NETTYPE_IPV6); | |||
| if ((tmp != NULL) && (tmp->data != NULL) && ipv6_hdr_is(tmp->data)) { | |||
| if ((tmp != NULL) && (tmp->data != NULL) && ipv6_hdr_is(tmp->data) && tmp->size >= sizeof (ipv6_hdr_t)) { | |||
There was a problem hiding this comment.
As mentioned in the top post of this review: I think it is a better idea to put this into an assert(). That needs to be then documented as a pre-condition for this function of course and would be an API change. But this way we can safe some bytes and as far as I can tell this function is only used within this PR and the unittests (though you wrote something about GNRC sock [which for me is this I wasn't quite able to parse in your OP).
There was a problem hiding this comment.
Updated; all but tmp != NULL is now in asserts. (I'll happily change it to assert that there's an IPv6 header, but that felt like a legitimate result as opposed to the other checks).
My GNRC sock referece was about the change in gnrc_sock.c -- that originally does not use the gnrc_ipv6_get_header function but duplicates its functionality in gnrc_sock_recv, this patch makes it the first in-code user of gnrc_ipv6_get_header.
There was a problem hiding this comment.
My GNRC sock referece was about the change in gnrc_sock.c -- that originally does not use the gnrc_ipv6_get_header function but duplicates its functionality in gnrc_sock_recv, this patch makes it the first in-code user of gnrc_ipv6_get_header.
I somehow was convinced that this PR edited gnrc_ipv6.c only...
| @@ -197,7 +197,7 @@ ipv6_hdr_t *gnrc_ipv6_get_header(gnrc_pktsnip_t *pkt) | |||
| { | |||
| ipv6_hdr_t *hdr = NULL; | |||
| gnrc_pktsnip_t *tmp = gnrc_pktsnip_search_type(pkt, GNRC_NETTYPE_IPV6); | |||
| if ((tmp != NULL) && (tmp->data != NULL) && ipv6_hdr_is(tmp->data)) { | |||
| if ((tmp != NULL) && (tmp->data != NULL) && ipv6_hdr_is(tmp->data) && tmp->size >= sizeof (ipv6_hdr_t)) { | |||
There was a problem hiding this comment.
(please also stick to coding conventions and put the expression into parenthesis and consider the line length)
chrysn
force-pushed
the
deduplicate-ipv6-address-extraction
branch
2 times, most recently
from
f471d9e to
8efef36
Compare
|
Updated again as I had forgotten to update the commit message accordingly. I hope I got the sizeof code style right now; at least uncrustify does not complain. Thanks for the review. |
gnrc_sock_recv used to duplicate functionality of gnrc_ipv6_get_header, but additionally checked whether the IPv6 snip is large enough. All checks are now included in gnrc_ipv6_get_header, but as most of them stem from programming / user errors, they were moved into asserts; this constitutes an API change.
chrysn
force-pushed
the
deduplicate-ipv6-address-extraction
branch
from
8efef36 to
f07308b
Compare
miri64
added
the
CI: ready for build
miri64
added
the
Process: API change
Different checks were applied in the two places, and got unioned into the utility function.
Contribution description
This change to GNRC socks is mainly motivated by seeing that the different places (gnrc_sock_recv and the utility function gnrc_ipv6_get_header) used different checks when pulling a ipv6_hdr out of a pktsnip. The new version adds a check in public function (replacing a hard-coded constant with a sizeof).
I'm not sure all those checks are actually necessary, so it might be worth checking with the GNRC architectural decisions whether a GNRC_NETTYPE_IPV6 marked snip can actually have its data NULL and a size of < 40, but without that checked, I'd err on the side of caution.
Impact-wise, this might add a few bytes of text as gnrc_ipv6_get_header is so far not used in the rest of the code; if that's a concern, we might consider making it inline-able (how's -flto coming?).
Testing procedure
Tested on native with a custom application that has both direct netapi threads and a gcoap server running. I did not try to produce situations in which any of the additional checks are triggered (see above: I'm not sure they're all necessary)