sock: change IPv4 address type to array by miri64 · Pull Request #5945 · RIOT-OS/RIOT

miri64 · 2016-10-14T09:08:29Z

I really had problems in #5936, #5937, and #5938 to keep byte order. To keep in line with the portability credo of this module I added a helper macro (so we can initialize the IPv4 address statically).

kaspar030 · 2016-10-14T09:39:24Z

I was wondering when this discussion flames up. ;)

I strongly prefer using host byte order at the user API level. The overhead for changing the order as as soon as it is needed is negligible.

miri64 · 2016-10-14T10:09:20Z

I find it weird for IPv4 addresses to be in host-byte order to be honest, but okay then I will change this macro, because I still think its useful for people who don't know about host- or network byteorder ;-)

miri64 · 2016-10-14T10:10:16Z

I find it weird for IPv4 addresses to be in host-byte order to be honest,

(especially since IPv6 addresses are in network byte order)

miri64 · 2016-10-14T10:15:34Z

Mh come to think of it: excectly because of that, I would prefer to make the IPv4 address an array, too... What do you think?

kaspar030 · 2016-10-14T10:25:29Z

Mh come to think of it: excectly because of that, I would prefer to make the IPv4 address an array, too... What do you think?

Yes! I thought about that, too. Let's do that.

miri64 · 2016-10-14T10:35:37Z

Done, but kept the helper macro, for people who like to work with uint32_t still ;-).

miri64 · 2016-10-14T10:36:16Z

Arghs, also needs helper function for dynamic initialization to now...

miri64 · 2016-10-14T10:41:31Z

Done

kaspar030 · 2016-10-14T10:55:09Z

sys/include/net/sock.h

        uint8_t ipv6[16];
 #endif
-        uint32_t ipv4;      /**< IPv4 address mode */
+        uint32_t ipv4;      /**< IPv4 address mode (in network byte order) */


Didn't we want to change this to be an array?

kaspar030 · 2016-10-14T10:58:45Z

sys/include/net/sock.h

+ */
+static inline void sock_init_ipv4(uint8_t *ipv4, uint32_t addr)
+{
+    const uint8_t a[] = SOCK_INIT_IPV4(addr);


This is not gonna work, the macro expects 4 arguments.
I actually think we can get rid of this function.

Users will usually not supply IPv4 addresses as "uint32".
They either get it from other functions (e.g., dns reply), or hardcode it.
In that case

ep = {.addr.ipv4= { a,b,c,d }}

would work as expected if the field is an uint8_t array, right?

Users will usually not supply IPv4 addresses as "uint32".
They either get it from other functions (e.g., dns reply), or hardcode it.
In that case

But a socket wrapper will ;-)

I mean, the output of inet_pton() or any struct in_addr can directly be copied into .addr.ipv4, right?

True... I was more thinking in the direction of e.g.

sock_init_ipv4(out->addr.ipv4, in_addr->sin_addr.s_addr);

But I guess you can do this with memcpy(), too.

miri64 · 2016-10-14T11:01:14Z

Sorry there seems to be some commit fuckup...

miri64 · 2016-10-14T11:05:36Z

Now it's like I intended it to be

miri64 · 2016-10-29T17:47:04Z

@kaspar030 ping?

kaspar030 · 2016-10-29T17:57:00Z

IMHO we don't need the helper macro and that static inline convenience function.

miri64 · 2016-10-29T18:11:01Z

Done.

kaspar030 · 2016-10-29T20:25:59Z

ACK, please squash!

I'll be on holidays for the next ten days, so unfortunately I have to reassign.

miri64 · 2016-10-31T08:42:21Z

Squashed

miri64 · 2016-10-31T09:54:00Z

Fixed a rebase error.

@kaspar030

@kaspar030 ACK'd

haukepetersen

ACK also from my side.

kaspar030 · 2016-11-10T13:35:22Z

I'm just realizing that this complicates some things, e.g.:

sock_udp_ep_t src = { .family = AF_INET, .addr.ipv4=INADDR_ANY}; doesn't work anymore, also if (dst->addr.ipv4 == 0xFFFFFFFF) ...

miri64 · 2016-11-10T14:43:25Z

Well you wanted the macros removed. I was expecting something like that :-/.

kaspar030 · 2016-11-11T15:29:12Z

How about we add a field uint32_t ipv4_u32 to the union?

miri64 · 2016-11-11T15:34:54Z

This leaves the byte-order unclear/inconsistent with the port.

miri64 · 2016-11-11T15:48:13Z

To clarify:

unclear: the type alone would suggest host byte-order, but the .addr.ipv4 (and the way you plan to use it) sets it to network-byte order
inconsistent: uint16_t for ports is in host byte-order, but uint32_t for address should be in network byte-order?

kaspar030 · 2016-11-11T18:43:46Z

Hmm. I don't think that the byte order for an address matters that much, compared to a port number.

I find it absolutely more natural to write port = 25 than ipv4 = 0xc0a80101. The constants we're using (e.g., INADDR_ANY or INADDR_BROADCAST) would have to be (where applicable) in network byte order. And setting an address is probably done either be by string (inet_pton(ep->addr.ipv4, "192.168.1.1", AF_INET)), as result as one of our helper functions or just ep->addr.ipv4={192,168,1,1}, which avoids the byte order matter.

What we gain by introducing an uint32_t version of the IPv4 address is the ability to do 32bit register compares due to 4b alignment, which, apart from being easier to code (and read), is probably way more efficient than having to do memcpys and memcmps.

miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Oct 14, 2016

miri64 assigned kaspar030 Oct 14, 2016

miri64 added this to the Release 2016.10 milestone Oct 14, 2016

kaspar030 previously requested changes Oct 14, 2016

View reviewed changes

miri64 changed the title ~~sock: add helper macro for IPv4 initialization~~ sock: change IPv4 address type to array Oct 29, 2016

kaspar030 assigned haukepetersen and unassigned kaspar030 Oct 29, 2016

miri64 force-pushed the sock/api/ipv4-init-helper branch from 6a7e69d to 334f823 Compare October 31, 2016 08:42

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 31, 2016

miri64 added 2 commits October 31, 2016 10:53

sock: change IPv4 address type to array

9e6b7e8

sock: document byte order for port

6d5fdbf

miri64 force-pushed the sock/api/ipv4-init-helper branch from 334f823 to 6d5fdbf Compare October 31, 2016 09:53

haukepetersen approved these changes Oct 31, 2016

View reviewed changes

miri64 merged commit e7b17dc into RIOT-OS:master Oct 31, 2016

miri64 deleted the sock/api/ipv4-init-helper branch October 31, 2016 11:56

This was referenced Oct 31, 2016

pkg: provide sock_tcp support for lwip #5938

Merged

pkg: provide sock_udp support for lwip #5937

Merged

pkg: provide sock_ip support for lwip #5936

Merged

kaspar030 mentioned this pull request Nov 13, 2016

sys: net: sock: add uint32_t ipv4 address to address union #6114

Merged

Conversation

miri64 commented Oct 14, 2016

Uh oh!

kaspar030 commented Oct 14, 2016

Uh oh!

miri64 commented Oct 14, 2016

Uh oh!

miri64 commented Oct 14, 2016

Uh oh!

miri64 commented Oct 14, 2016

Uh oh!

kaspar030 commented Oct 14, 2016

Uh oh!

miri64 commented Oct 14, 2016

Uh oh!

miri64 commented Oct 14, 2016

Uh oh!

miri64 commented Oct 14, 2016

Uh oh!

kaspar030 Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

kaspar030 Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

miri64 Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

kaspar030 Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

kaspar030 Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

miri64 Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

miri64 commented Oct 14, 2016

Uh oh!

miri64 commented Oct 14, 2016

Uh oh!

miri64 commented Oct 29, 2016

Uh oh!

kaspar030 commented Oct 29, 2016

Uh oh!

miri64 commented Oct 29, 2016

Uh oh!

kaspar030 commented Oct 29, 2016

Uh oh!

miri64 commented Oct 31, 2016

Uh oh!

miri64 commented Oct 31, 2016

Uh oh!

haukepetersen left a comment

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Nov 10, 2016

Uh oh!

miri64 commented Nov 10, 2016

Uh oh!

kaspar030 commented Nov 11, 2016

Uh oh!

miri64 commented Nov 11, 2016

Uh oh!

miri64 commented Nov 11, 2016

Uh oh!

kaspar030 commented Nov 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants