Skip to content

Conversation

maribu
Copy link
Member

@maribu maribu commented Jul 27, 2020

Contribution description

Add variants of sock_<PROTO>_send() and sock_<PROTO>_recv() that allow fetching auxiliary data. As a motivating example, the local address is provided to allow virtual hosting to be implemented on top of the sock API. Additionally, the API for timestamps as auxiliary data is implemented; but the corresponding feature is not yet implemented by any network stack.

These are the functions that were added for the UDP protocol:

ssize_t sock_udp_recv_aux(sock_udp_t *sock, void *data, size_t max_len,
                          uint32_t timeout, sock_udp_ep_t *remote,
                          sock_udp_aux_rx_t *aux);
ssize_t sock_udp_recv_buf_aux(sock_udp_t *sock, void **data, void **buf_ctx,
                              uint32_t timeout, sock_udp_ep_t *remote,
                              sock_udp_aux_rx_t *aux);
ssize_t sock_udp_send_aux(sock_udp_t *sock, const void *data, size_t len,
                          const sock_udp_ep_t *remote, sock_udp_aux_tx_t *aux);

The same was provided for IP and DTLS.

Testing procedure

  • make -C tests/gnrc_sock_udp flash test
  • make -C tests/gnrc_sock_ip flash test
  • LWIP_IPV4=1 LWIP_IPV6=1 make -C tests/lwip_sock_udp flash test
  • LWIP_IPV4=1 LWIP_IPV6=1 make -C tests/lwip_sock_ip flash test

Issues/PRs references

Supersedes: #14402

@miri64
Copy link
Member

miri64 commented Jul 28, 2020

What about sock_dtls and sock_tcp? Regarding sock_tcp, I'm not sure if you aim to have this only work for datagram-based protocols, but sock_dtls is definitely missing from this.

@maribu
Copy link
Member Author

maribu commented Jul 28, 2020

Regarding sock_tcp, I'm not sure if you aim to have this only work for datagram-based protocols

I think that this feature might be useful there, but the API proposed here is likely a poor match. (E.g. we don't expect the destination address to change after accepting the connection.)

but sock_dtls is definitely missing from this.

I agree that this should also be provided that. But I felt this might be a bit more involved there (e.g. should auxiliary data also be provided in the session initialization?).

But how about this: I extend this PR to adapt the DTLS socks in the same manner, and follow up PRs can extend APIs unique to DTLS (like session initialization) if there is a use case for it.

@maribu
Copy link
Member Author

maribu commented Jul 28, 2020

May I squash to fix the following commit message:

- net/sock_udp: Extend IP for auxiliary data
+ net/sock_udp: Extend API for auxiliary data

I tend to forget this, if I don't do such things right away.

@miri64
Copy link
Member

miri64 commented Jul 28, 2020

May I squash to fix the following commit message:

- net/sock_udp: Extend IP for auxiliary data
+ net/sock_udp: Extend API for auxiliary data

I tend to forget this, if I don't do such things right away.

Sure! I usually do git commit --allow-empty --squash=<commit in question> -e and add the new one to the body btw.

@maribu
Copy link
Member Author

maribu commented Jul 28, 2020

Now DTLS is also included. I ended up reusing the aux data types (but typedefed to a new name, to allow revising this decision). That allowed to reuse sock_udp_recv_aux() within sock_dtls_recv_aux() to provide the auxiliary data.

@miri64 miri64 added Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jul 28, 2020
@maribu maribu added the State: waiting for other PR State: The PR requires another PR to be merged first label Jul 31, 2020
@maribu
Copy link
Member Author

maribu commented Jul 31, 2020

I split #14665 out of this, as this is not really related to the target of this PR.

@maribu
Copy link
Member Author

maribu commented Jul 31, 2020

Rebased on master (without squashing)

@maribu maribu removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jul 31, 2020
@maribu
Copy link
Member Author

maribu commented Aug 4, 2020

@miri64 I could split this up into three PRs, one for IP, one for UDP, and one for DTLS. Maybe this would ease review?

maribu added 2 commits August 4, 2020 09:29
Provide address the IP packet / UDP datagram was received on in the auxiliary
data, if module sock_aux_local is used.
@maribu
Copy link
Member Author

maribu commented Aug 4, 2020

Hmm, the implementations are entangled. It might not be such a good idea to split those apart. E.g. both the added parameter to gnrc_sock() requires updating of both the UDP and IP sock API in GNRC anyway...

@fjmolinas fjmolinas requested review from benpicco and bergzand August 4, 2020 08:19
@miri64
Copy link
Member

miri64 commented Aug 4, 2020

Hmm, the implementations are entangled. It might not be such a good idea to split those apart. E.g. both the added parameter to gnrc_sock() requires updating of both the UDP and IP sock API in GNRC anyway...

What you could try is to leave the new functions unimplemented in one PR and then implement them per stack.

@@ -102,6 +102,7 @@ PSEUDOMODULES += shell_hooks
PSEUDOMODULES += slipdev_stdio
PSEUDOMODULES += sock
PSEUDOMODULES += sock_async
PSEUDOMODULES += sock_aux%
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't like using wildcards for pseudo-modules. It is basically just laziness and it confuses the build system mightily when there is an actual module that matches that wildcard as well. It is unlikely that this will happen for sock_aux% and this is not the first offender, so I won't block. However, I would prefer them written out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. (But in #14703)

@maribu
Copy link
Member Author

maribu commented Aug 5, 2020

Split this PR as follows:

(The above list is also the dependency tree.)

@maribu maribu closed this Aug 5, 2020
@maribu maribu requested review from chrysn and kaspar030 September 14, 2020 08:01
@maribu maribu reopened this Sep 14, 2020
@maribu maribu closed this Sep 14, 2020
@maribu
Copy link
Member Author

maribu commented Sep 14, 2020

Sorry, confused this with #14703. This should remain closed in favor of #14703

@maribu maribu deleted the sock-aux branch January 25, 2021 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants