sys/net/sock: provide ipv{4, 6}_addr_t in union

[benpicco]

Contribution description

This allows to statically initialize a e.g. sock_udp_ep_t with a prefix and IID.

Testing procedure

    const sock_udp_ep_t remote = {
        .family = AF_INET6,
        .port = CONFIG_DATA_ENDPOINT_PORT,
        .netif = SOCK_ADDR_ANY_NETIF,
        .addr = {
            .ipv6_addr = {
                .u64[0] = byteorder_htonll(CONFIG_DATA_ENDPOINT_PREFIX),
                .u64[1] = byteorder_htonll(CONFIG_DATA_ENDPOINT_IID),
            }
        }
    };

works now

Issues/PRs references

[maribu]

Can we consider the members of some sock_<foo>_ep_t as implementation details not to be touched by application developers? So that we can change it in breaking ways?

If so, IMO we could just drop all other members of the union and only keep ipv6_addr_t and ipv4_addr_t. After all, those are unions as well provided access to various representations of the address.

If not, I suggest to mark all other members of the union as deprecated, so that we can phase them out eventually.

[benpicco]

Can we consider the members of some sock_<foo>_ep_t as implementation details not to be touched by application developers?

Certainly not, it's not uncommon to statically initialize a e.g. sock_udp_ep_t and for that we need the struct members.
The reason for this PR is to make more static initialization possible.

[chrysn]

But then still, deprecating the others sounds like a good thing to do at this point. (Currently that'd just be through documentation; #18565 is still blocked on doxygen issues).

[miri64]

I think @kaspar030 has his opinion on this. ;-)

[miri64]

The reason why we did not include ipv{4,6}_addr_t was to keep the sock API RIOT-independent (which was something @kaspar030 fought for hard). By introducing this RIOT dependency we loose this (and thus potentially testing capabilities). But, since the sock API moved somewhat away from that design, I am actually not 100% sure, if this independence is still upheld.

[miri64]

https://github.com/kaspar030/sock

[kaspar030]

The reason why we did not include ipv{4,6}_addr_t was to keep the sock API RIOT-independent (which was something @kaspar030 fought for hard).

IMO, keeping it that way also keeps it a bit sane. The address types in sys/net/ipvX/addr.h are quite complex.

[chrysn]

I see that point about sock being standalone (and ad "what other than RIOT": a portable application can take RIOT's sock API and implement it on the other platforms it wants to be portable to). But maybe we can treat things the other way, move the address types into socket (say, <net/sock/addresses.h>) and then use these sock addresses in the rest of RIOT?

[benpicco]

IMO, keeping it that way also keeps it a bit sane. The address types in sys/net/ipvX/addr.h are quite complex.

That argument is pretty odd, on the one hand we have a ipv6_addr_t type but then we don't use it because it's too complex?

[chrysn]

I'd prefer having a type that does the type punning right once-and-for-all (with type names indicating that this is not a machine-encoded uint32_t but a 32-bit integer encoded in network byte order, if treating an IP address as a 32bit int ever makes sense in any than opaque aligned-4-byte-blob form) than having places like this where again an IP address is split up into its different possible representations.

[maribu]

I agree that ipv6_addr_t is completely insane. It currently looks (fully expended) like this:

typedef union {
    uint8_t u8[16];
    union __attribute__((packed)) {
        uint16_t u16;
        uint8_t u8[2];
    } u16[8];
    union __attribute__((packed)) {
        uint32_t u32;
        uint8_t u8[4];
        uint16_t u16[2];
        union __attribute__((packed)) {
            uint16_t u16;
            uint8_t u8[2];
        } b16[2];
    } u32[4];
    union __attribute__((packed)) {
        uint64_t u64;
        uint8_t u8[8];
        uint16_t u16[4];
        uint32_t u32[2];
        union __attribute__((packed)) {
            uint16_t u16;
            uint8_t u8[2];
        } b16[4];
        union __attribute__((packed)) {
            uint32_t u32;
            uint8_t u8[4];
            uint16_t u16[2];
            union __attribute__((packed)) {
                uint16_t u16;
                uint8_t u8[2];
            } b16[2];
        } b32[2];
    } u64[2];
} ipv6_addr_t;

But rather than running away from it screaming (which also is my first instinct), we should just fix it. And then we can actually use it.

[maribu]

If we would have CI time stats, we could actually track down how much CI time it will shave of to fix ipv6_addr_t.

[kaspar030]

So,

typedef struct {
  uint8_t bytes[IPV6_ADDR_LEN];
} ipv6_addr_t;

?

• not using plain array type so it can be returned from functions
• not using any union so it doesn't turn into the complexity mess we already have, and no unnecessary alignment restrictions
• name of the single field needs bikeshedding

[kaspar030]

this is how it looks in linux:

   33 struct in6_addr {                                                                                                                            
   34 ›   union {                                                                                                                                  
   35 ›   ›   __u8›   ›   u6_addr8[16];                                                                                                            
   36 #if __UAPI_DEF_IN6_ADDR_ALT                                                                                                                  
   37 ›   ›   __be16› ›   u6_addr16[8];                                                                                                            
   38 ›   ›   __be32› ›   u6_addr32[4];                                                                                                            
   39 #endif                                                                                                                                       
   40 ›   } in6_u;                                                                                                                                 
   41 #define s6_addr››   ›   in6_u.u6_addr8                                                                                                       
   42 #if __UAPI_DEF_IN6_ADDR_ALT                                                                                                                  
   43 #define s6_addr16›  ›   in6_u.u6_addr16                                                                                                      
   44 #define s6_addr32›  ›   in6_u.u6_addr32                                                                                                      
   45 #endif                                                                                                                                       
   46 };                                                                                                                                           

[chrysn]

Addresses are lugged around often enough that alignment can make sense (but that can be done with _Alignas just as well). Other than that: yes, let's ditch any needless union variants, and let's let the compilers do their jobs. (I have no clue what it does in detail, but https://godbolt.org/z/oTzfh8Wze shows that a memcpy of a full address is done efficiently, and apparently more efficiently (don't understand the difference, but the compiler obviously deems it worthy) when there are alignment constraints).

But I don't want to slow this down by arguing about alignment: Both an aligned and an unaligned simple ipv6_addr_t are improvements over the current complex thing.