Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gcoap: add remote sock_udp_ep_t to coap_request_ctx_t #18519

Merged
merged 5 commits into from
Sep 12, 2022

Conversation

benpicco
Copy link
Contributor

Contribution description

It can be useful to know over which interface a CoAP request was received.
(e.g. we have a device with two SLIP + power interfaces, both can be turned off and on individually. If a board registers with the central control board, we want to know to which interface it is connected so we know where to cut power.)

Testing procedure

Issues/PRs references

@github-actions github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System labels Aug 26, 2022
@benpicco benpicco requested review from chrysn and fabian18 August 26, 2022 17:30
sys/include/net/nanocoap.h Outdated Show resolved Hide resolved
@benpicco benpicco force-pushed the coap_request_ctx_get_remote branch 2 times, most recently from 3190d8b to 628edcc Compare September 2, 2022 11:40
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 2, 2022
@benpicco
Copy link
Contributor Author

benpicco commented Sep 2, 2022

This allows us to remove some warts introduced by the GCoAP forward proxy - and since the coap_request_ctx_t struct is completely internal, this isn't even an API change 😄

sys/include/net/nanocoap.h Outdated Show resolved Hide resolved
sys/net/application_layer/gcoap/forward_proxy.c Outdated Show resolved Hide resolved
@benpicco benpicco force-pushed the coap_request_ctx_get_remote branch from acfce49 to 388c1c8 Compare September 7, 2022 10:45
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Sep 8, 2022
sys/include/net/nanocoap.h Outdated Show resolved Hide resolved
Copy link
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

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

If you do not intend to add local address information in this PR and murdock becomes green, I think this is ready to be merged.

Just some testing:

diff --git a/sys/net/application_layer/gcoap/fileserver.c b/sys/net/application_layer/gcoap/fileserver.c
index eaf14a252b..35c742816d 100644
--- a/sys/net/application_layer/gcoap/fileserver.c
+++ b/sys/net/application_layer/gcoap/fileserver.c
@@ -25,10 +25,12 @@
 #include "checksum/fletcher32.h"
 #include "net/gcoap/fileserver.h"
 #include "net/gcoap.h"
+#include "net/nanocoap.h"
+#include "net/sock/util.h"
 #include "vfs.h"
 #include "vfs_util.h"
 
-#define ENABLE_DEBUG 0
+#define ENABLE_DEBUG 1
 #include "debug.h"
 
 /** Maximum length of an expressible path, including the trailing 0 character. */
@@ -514,6 +516,16 @@ ssize_t gcoap_fileserver_handler(coap_pkt_t *pdu, uint8_t *buf, size_t len,
     const char *resource = coap_request_ctx_get_path(ctx);
     struct requestdata request = {0};
 
+    if (ENABLE_DEBUG) {
+        if (GCOAP_SOCKET_TYPE_UDP == coap_request_ctx_get_tl_type(ctx)) {
+            const sock_udp_ep_t *ep = coap_request_ctx_get_remote_udp(ctx);
+            char addr_str[] = "0000:0000:0000:0000:0000:0000:0000:0000";
+            uint16_t port;
+            sock_udp_ep_fmt(ep, addr_str, &port);
+            DEBUG("gcoap_fileserver: got a request from [%s]:%u\n", addr_str, port);
+        }
+    }
+
     /** Index in request.namebuf. Must not point at the last entry as that will be
      * zeroed to get a 0-terminated string. */
     size_t namelength = 0;
2022-09-11 14:36:46,131 # gcoap_fileserver: got a request from [fe80::3478:9157:9fd5:81c3%6]:34428

@benpicco benpicco force-pushed the coap_request_ctx_get_remote branch from cbdfdc8 to 0ba9a50 Compare September 12, 2022 15:51
@benpicco
Copy link
Contributor Author

If you do not intend to add local address information in this PR and murdock becomes green, I think this is ready to be merged.

I think this PR does enough already. We can add local address information in a follow-up.

@benpicco benpicco force-pushed the coap_request_ctx_get_remote branch from 0ba9a50 to 5070ca9 Compare September 12, 2022 15:53
Copy link
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

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

ACK

@benpicco benpicco merged commit b117171 into RIOT-OS:master Sep 12, 2022
@benpicco
Copy link
Contributor Author

Thank you for the review!

@benpicco benpicco deleted the coap_request_ctx_get_remote branch September 12, 2022 18:30
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants