sys/net/ipv6: Simplify type

[maribu]

Contribution description

Make ipv6_addr_t a simple byte array.

Motivation

Using complex types makes interoperability of functions using this type with external code or even with sock_udp_ep_t harder, especially due to the alignment requirements. Using a simple type allows using these functions in the Sock API or in external code (specifically in a pkg I want to update) and thus prevents code duplication and increases maintainability.

Testing procedure

Luckily, @miri64 wrote many unit tests that already helped to fix many bugs this PR previously contained. In addition to automatic tests, testing IPv6 communication would nake sense.

Issues/PRs references

#14737

• Depends on and includes core/byteorder: Add byteorder_bebuftohll() and inverse #14761

[miri64]

I compiled a selection of boards using

BOARD="<board>" BUILD_IN_DOCKER=1 RIOT_CI_BUILD=1 DOCKER_MAKE_ARGS=-j DOCKER_ENVIRONMENT_CMDLINE="-e BINDIRBASE='<ref>-bin'" make -C tests/unittests/ tests-ipv6_addr

For some I needed to remove tests/unittests/Makefile.ci as that file is only for the whole unittests application.

Here are the results:

text	data	bss	dec	BOARD/BINDIRBASE

-1224	154	0	-1070	arduino-mega2560
26168	6304	1363	33835	5d43c5b~1-bin
24944	6458	1363	32765	pr14759-bin

0	0	0	0	iotlab-m3
36264	740	52160	89164	5d43c5b~1-bin
36264	740	52160	89164	pr14759-bin

0	0	0	0	msba2
36264	740	52160	89164	5d43c5b~1-bin
36264	740	52160	89164	pr14759-bin

0	0	0	0	nrf52dk
36264	740	52160	89164	5d43c5b~1-bin
36264	740	52160	89164	pr14759-bin

0	0	0	0	samr21-xpro
36264	740	52160	89164	5d43c5b~1-bin
36264	740	52160	89164	pr14759-bin

-468	304	0	-164	z1
29941	858	1082	31881	5d43c5b~1-bin
29473	1162	1082	31717	pr14759-bin

While for ARM the binary size remains unchanged, there seems to be some weird ROM/RAM trade-off happening for MSP430 and AVR. While the size overall decreases due to big improvements in the ROM usage, the usage in the data segment increases (most likely due to the public constants used by ipv6_addr). Is there maybe a way to lower the size of the data segment again and keep the simplification? Otherwise, I'm uncertain if this should be merged.

[miri64]

I observed similar behavior for esp* and RISC-V but since I do not have the toolchains installed locally, the table was not nicely generated, so I omitted those results xP

[maribu]

Is there maybe a way to lower the size of the data segment again

On AVR the solution would be to use PROGMEM. But it is not that trivial to expose this feature in a platform independent way (so that it still uses the advantage of von-Neumann machines) while also not adding boiler plat code.

I'm not sure why this PR trades RAM for ROM for the MSP430.

Otherwise, I'm uncertain if this should be merged.

The reasoning for this PR is not that it improves ROM/RAM consumption, but that the type can be used more consistently. E.g. sock_udp_ep_t a.k.a. struct _sock_tl_ep did not make use of this type - I assume due to its complexity. Even if the struct _sock_tl_ep would be adapted to use ipv6_addr_t for the IPv6 address without simplifying the type, external code in packages won't be compatible. However, the simple byte array type is something I think every IPv6 representation is compatible with. Thus, functions like ipv6_addr_is_multicast() would become reusable.

[miri64]

E.g. sock_udp_ep_t a.k.a. struct _sock_tl_ep did not make use of this type - I assume due to its complexity

Actually it does not use that type to decrease dependencies. sock was originally designed to also be used in operating systems other than RIOT (including Linux, early versions of nanocoap where able to run on Linux because of that).

[miri64]

The reasoning for this PR is not that it improves ROM/RAM consumption, but that the type can be used more consistently.

I am aware of that. The question I am pondering is, if that re-usability justifies the significant increase in RAM usage.

[miri64]

This test also tests the function ipv6_addr_is_loopback() so just using memcmp() here somewhat nullifies the purpose of the test.

[miri64]

Why not use the already predefined prefix here?

Suggested change

	static const ipv6_addr_t solicited_node_addr = {
	.u8 = {
	0xff, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
	0x00, 0x00, 0x00, 0x01, 0xff, 0x00, 0x00, 0x00
	}
	};
	*out = solicited_node_addr;
	*out = ipv6_addr_solicited_node_prefix;

[miri64]

ipv6_hdr was completely zeroed in l. 166 so it might make more sense to just set the non-zero values here and remove addr_fffe altogether:

Suggested change

	memcpy(&ipv6_hdr->src.u8[8], addr_fffe, sizeof(addr_fffe));
	ipv6_hdr->src.u8[11] = 0xff;
	ipv6_hdr->src.u8[12] = 0xfe;

[miri64]

Same here.

Suggested change

	memcpy(&ipv6_hdr->src.u8[8], addr_fffe, sizeof(addr_fffe));
	ipv6_hdr->src.u8[11] = 0xff;
	ipv6_hdr->src.u8[12] = 0xfe;

[miri64]

And here

Suggested change

	memcpy(&ipv6_hdr->dst.u8[8], addr_fffe, sizeof(addr_fffe));
	ipv6_hdr->dst.u8[11] = 0xff;
	ipv6_hdr->dst.u8[12] = 0xfe;

[miri64]

And here

Suggested change

	memcpy(&ipv6_hdr->dst.u8[8], addr_fffe, sizeof(addr_fffe));
	ipv6_hdr->dst.u8[11] = 0xff;
	ipv6_hdr->dst.u8[12] = 0xfe;

[miri64]

Just from reading the code its not clear where zeros8 comes from, I'd rather compare byte-wise.

[maribu]

So you're saying that this

    if (!memcpy(ipv6_hdr->dest.u8[2], zeros8, 8)) {
        /* ... */
    }

is less readable than this

    int is_all_zero = 1;
    for (size_t i = 2; i < 10; i++) {
        if (ipv6_hdr->dst.u8[2] != 0) {
            is_zero = 0;
            break;
        }
    }
    if (!is_all_zero) {
        /* ... */
    }

?

[miri64]

no, but both your proposals are more obscure than

if ((ipv6_hdr->dest.u8[2] == 0) && (ipv6_hdr->dest.u8[3] == 0) &&
    (ipv6_hdr->dest.u8[4] == 0) && (ipv6_hdr->dest.u8[5] == 0) &&
    (ipv6_hdr->dest.u8[6] == 0) && (ipv6_hdr->dest.u8[7] == 0) &&
    (ipv6_hdr->dest.u8[8] == 0) && (ipv6_hdr->dest.u8[9] == 0)) {

(I think even you reversed the logic in your second proposal ;-))

[miri64]

Same here.

[miri64]

Especially here.

[miri64]

Suggested change

	memcpy(iphc_hdr + inline_pos, ipv6_hdr->dst.u8 + 12, 4);
	memcpy(iphc_hdr + inline_pos, &ipv6_hdr->dst.u8[12], 4);

[miri64]

...

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.