sock/async: add function to retrieve session object of current DTLS event by janosbrodbeck · Pull Request #15755 · RIOT-OS/RIOT

janosbrodbeck · 2021-01-12T18:41:22Z

Contribution description

This PR adds a new function to sock async: sock_dtls_get_event_session() to retrieve the session object within a DTLS event. Currently it is not possible to get this session for every event type (e.g. SOCK_ASYNC_CONN_FIN, SOCK_ASYNC_CONN_RDY). It should work for every possible event type. The issue was described in #15517.

I didn't get it to work properly with just a pointer in the socket structure, since the session needed to be stored somewhere and storing it was not possible within the _event() function of tinydtls. So I placed it right away in the socket.

Not sure whether I placed the new function correctly.

Testing procedure

Implementation in tinyDTLS can be tested with pkg_tinydtls_sock_async

Flash and run on two devices (e.g. native)
Execute dtlss on one of the devices
Execute dtlsc {ip of other node} {some data} on the other device

Issues/PRs references

Fixes #15517

janosbrodbeck · 2021-01-12T18:44:56Z

I have also added a very small doc fix commit. The documentation of sock_dtls_set_cb wrote something about getting something but it just sets stuff. So I've changed Get to Set. But can drop this commit if unwanted or wrong.

pkg/tinydtls/contrib/sock_dtls.c

pkg/tinydtls/include/sock_dtls_types.h

pokgak · 2021-01-14T00:25:49Z

(Is there something like a spoiler or collapse tag?)

You can use the HTML <details></details> tag:

Details

HIDE ME

janosbrodbeck · 2021-01-18T19:27:14Z

Pushed the discussed changes. Now, the event session is only set for SOCK_ASYNC_CONN_READY and SOCK_ASYNC_CONN_FIN. Added this info into the note section of the function documentation. Thought about putting this info into a warning section, but that may be a bit overkill.

Did not adjust the provided test procedure. You can see the expected behaviour in the test. For the SOCK_ASYNC_CONN_RECV event the retrieved session is still the previously set session or empty if it's the first.

janosbrodbeck · 2021-01-18T19:34:05Z

Also adjusted the line length of the function head to comply the 80 character recommendation.

pokgak · 2021-01-19T01:01:10Z

LGTM. Can you also include the patch (only CONN_RECV and CONN_FIN part) as commit to show usage of sock_dtls_get_event_session in the DTLS handler so that it will at least get tested the next time someone run the test?

pkg/tinydtls/include/sock_dtls_types.h

miri64 · 2021-01-19T10:01:06Z

sys/include/net/sock/async.h

+ * @note Only available with @ref SOCK_HAS_ASYNC defined.
+ *       Should only be called within a DTLS event and only for the event
+ *       types @ref SOCK_ASYNC_CONN_RDY and @ref SOCK_ASYNC_CONN_FIN.


Mhhh maybe add a return value, that signals to the caller when the the session is available or not? I know this makes the implementation more complicated than it is now, but sock is an end-user API that is supposed to be easy to use. Could be boolean or an errno-based status code

Totally forgot, sorry. Squashed the undo directly with the related commit.

Regarding the return value. It would be nice to have it...but how? The only idea I have for this currently is to also add (e.g.) a boolean to the socket object and set this boolean when an event occurs. So setting it for the CONN_RDY, CONN_FIN to available and for every other event to not available. Would that be sufficient or do you imagine it differently?

In the specific case of tinydtls you could use the size member of the session_t struct as a signifier if the session is set.

typedef struct { socklen_t size; /**< size of addr */ union { struct sockaddr sa; struct sockaddr_storage st; struct sockaddr_in sin; struct sockaddr_in6 sin6; } addr; uint8_t ifindex; } session_t;

The object needs to zero'd of course. However, I would do this on read and socket init. This way, if you e.g. receive a CONN_RDY and a MSG_RECV in the same callback you don't accidentally remove the session before it can be read.

Done :) Thank you! I've adjusted the test procedure to test that the session is not available for other event types.

janosbrodbeck · 2021-01-21T01:50:33Z

LGTM. Can you also include the patch (only CONN_RECV and CONN_FIN part) as commit to show usage of sock_dtls_get_event_session in the DTLS handler so that it will at least get tested the next time someone run the test?

I've added the usage of the new function to the test. Are you okay with it the way it is now?

tests/pkg_tinydtls_sock_async/dtls-server.c

miri64 · 2021-01-21T12:49:36Z

tests/pkg_tinydtls_sock_async/dtls-server.c

+    sock_dtls_session_t session = { 0 };

+    if (type & SOCK_ASYNC_CONN_RECV) {
        puts("Session handshake received");


Here and in the other event types that are not SOCK_ASYNC_CONN_FIN or SOCK_ASYNC_CONN_RDY (as well in the client), you could also use expect(!sock_dtls_get_event_session(sock, &session)) to make this proper tests ;-).

Done! In the event types SOCK_ASYNC_CONN_FIN and SOCK_ASYNC_CONN_RDY we could also use expect(sock_dtls....) to test it since currently it is only printed and not tested. Would that be too much to also add it there? Would like to keep the session printing, because it is currently the only example usage of this new function.

Would like to keep the session printing, because it is currently the only example usage of this new function.

Yes, as you print output, that provides enough result (more than expect which does not print anything when succeeding)

miri64 · 2021-01-21T17:17:41Z

tests/pkg_tinydtls_sock_async/dtls-client.c

+            sock_dtls_session_get_udp_ep(&session, &ep);
+
+            printf("Session became ready: ");
+            print_ipv6_addr_with_port(ep.addr.ipv6, sizeof(ep.addr), ep.port);


Any reason why you not use sock_udp_ep_fmt() from sys/include/net/sock/util.h here? Seems like re-inventing the wheel.

I just knew the format function when the address is a ipv6_addr_t object but didn't see one for an endpoint. Was not my intention to reinvent, just did not found a proper function. I'll change it. Way nicer to have a proper function for it.

miri64 · 2021-01-21T17:18:26Z

One last comment. Other than that: I tested everything and am fine with most changes (apart from the one mentioned above). You may squash directly.

janosbrodbeck · 2021-01-21T18:20:03Z

Addressed last comment and squashed. Test uses sock_udp_ep_fmt() now for printing.

in DTLS handler

miri64

ACK. Re-tested and everything works as expected. :-)

janosbrodbeck · 2021-01-21T19:22:00Z

Thanks for the help and review @miri64 and @pokgak !

janosbrodbeck requested a review from leandrolanzieri as a code owner January 12, 2021 18:41

janosbrodbeck added Area: network Area: Networking Area: pkg Area: External package ports Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Jan 12, 2021

janosbrodbeck requested review from miri64 and pokgak January 12, 2021 18:45

janosbrodbeck changed the title ~~Pr/dtls/get event session~~ sock/async: add function to retrieve session object of current DTLS event Jan 12, 2021

janosbrodbeck force-pushed the pr/dtls/get_event_session branch 2 times, most recently from e593e51 to 4db4112 Compare January 12, 2021 18:54

pokgak reviewed Jan 13, 2021

View reviewed changes

pkg/tinydtls/contrib/sock_dtls.c Outdated Show resolved Hide resolved

miri64 reviewed Jan 13, 2021

View reviewed changes

pkg/tinydtls/include/sock_dtls_types.h Outdated Show resolved Hide resolved

janosbrodbeck added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jan 14, 2021

janosbrodbeck removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jan 18, 2021

miri64 requested changes Jan 19, 2021

View reviewed changes

janosbrodbeck force-pushed the pr/dtls/get_event_session branch 2 times, most recently from 76167fc to 69b265a Compare January 21, 2021 01:39

janosbrodbeck requested review from MichelRottleuthner, aabadie, fjmolinas and smlng as code owners January 21, 2021 01:39

pokgak approved these changes Jan 21, 2021

View reviewed changes

tests/pkg_tinydtls_sock_async/dtls-server.c Outdated Show resolved Hide resolved

janosbrodbeck force-pushed the pr/dtls/get_event_session branch 2 times, most recently from 2dc3423 to 6cb35f1 Compare January 21, 2021 02:22

miri64 reviewed Jan 21, 2021

View reviewed changes

janosbrodbeck added 3 commits January 21, 2021 18:58

sock/async: introduce function to retrieve session of current DTLS event

80d592b

pkg/tinydtls: implement sock_dtls_get_event_session

04d95d1

sock/async: fix typo in doc of sock_dtls_set_cb

050b268

janosbrodbeck force-pushed the pr/dtls/get_event_session branch from 85e1bea to 7d08c05 Compare January 21, 2021 18:17

janosbrodbeck added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 21, 2021

janosbrodbeck force-pushed the pr/dtls/get_event_session branch from 7d08c05 to 328f0c9 Compare January 21, 2021 18:41

tests/pkg_tinydtls_sock_async: use sock_dtls_get_event_session()

c65b4d3

in DTLS handler

janosbrodbeck force-pushed the pr/dtls/get_event_session branch from 328f0c9 to c65b4d3 Compare January 21, 2021 18:43

miri64 approved these changes Jan 21, 2021

View reviewed changes

miri64 merged commit 879fbc1 into RIOT-OS:master Jan 22, 2021

janosbrodbeck deleted the pr/dtls/get_event_session branch February 8, 2021 16:24

janosbrodbeck mentioned this pull request Feb 19, 2021

pkg/tinydtls: auxiliary data API does not work for async sockets #16054

Open

kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021

Conversation

janosbrodbeck commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

janosbrodbeck commented Jan 12, 2021

Uh oh!

Uh oh!

Uh oh!

pokgak commented Jan 14, 2021

Uh oh!

janosbrodbeck commented Jan 18, 2021

Uh oh!

janosbrodbeck commented Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pokgak commented Jan 19, 2021

Uh oh!

Uh oh!

miri64 Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

janosbrodbeck Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

janosbrodbeck Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janosbrodbeck commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

miri64 Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

janosbrodbeck Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

miri64 Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

miri64 Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

janosbrodbeck Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jan 21, 2021

Uh oh!

janosbrodbeck commented Jan 21, 2021

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

janosbrodbeck commented Jan 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

janosbrodbeck commented Jan 12, 2021 •

edited

Loading

janosbrodbeck commented Jan 18, 2021 •

edited

Loading

janosbrodbeck Jan 19, 2021 •

edited

Loading

janosbrodbeck Jan 21, 2021 •

edited

Loading

janosbrodbeck commented Jan 21, 2021 •

edited

Loading

janosbrodbeck Jan 21, 2021 •

edited

Loading