Skip to content

Conversation

@haukepetersen
Copy link
Contributor

Contribution description

I currently have a use case, where I need to know the source UDP-endpoint (IPv6 address + port) of for incoming CoAP request. So far, there was no way that I could see to get access this information from within my application.

This PR adds a pointer to the remote sock_udp_ep_t into coap_pkt_t. As a pointer to the pkt is passed to a resources event handler, this allows to access a requests source address and port now.

Caveat: the proposed solution hardcodes sock_udp_ep_t and with this it is only able to work with with UDP. But for now I don't see this as a problem, as gcoap is hardcoded to sock_udp anyway. This is also the reason I only included the udp_remote field for nanocaop in case that gcoap is used. This way it does not pull this hardcoded sock_udp dependency into nanocoap.

Testing procedure

If build tests and examples/gcoap just work as before, nothing is broken by this change.

Issues/PRs references

none

@haukepetersen haukepetersen added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: CoAP Area: Constrained Application Protocol implementations labels Mar 12, 2020
#endif

#ifdef MODULE_GCOAP
#include "net/sock/udp.h"
Copy link
Contributor

@kaspar030 kaspar030 Mar 12, 2020

Choose a reason for hiding this comment

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

Suggested change
#include "net/sock/udp.h"
typedef struct sock_udp_ep_t;

@kaspar030
Copy link
Contributor

Looks good to me. @kb2ma do you see any reason not to expose this?

@kb2ma
Copy link
Member

kb2ma commented Mar 13, 2020

I understand the need. Let me think over the weekend about how this fits in the bigger picture.

@kb2ma
Copy link
Member

kb2ma commented Mar 16, 2020

I agree that we could add the coap_udp_ep_t* remote value as an attribute of coap_pkt_t, and this approach has the advantage of being a simple solution. However, I recommend that we use this need to make gcoap more extensible for the future with the addition of a context struct.

IMO coap_pkt_t is not a great fit for the nature of the data because the remote endpoint is not a part of the CoAP PDU. For client requests the remote is provided directly to gcoap_req_send(), so it's just using up space in that context. In addition, adding the attribute to the PDU means there are now two sources for the remote in gcoap's server side handling in _handle_req() as well as _find_obs_memo().

So similar to #9857, I think it is worthwhile to add a separate struct for server side handling. Let's call it gcoap_recv_ctx_t. This struct could hold the coap_udp_ep_t*, and gives us a place to turn this into a union for different transports and add an attribute to identify the union member. It also future proofs this pathway for passing data to the application. Maybe OSCORE would find it useful. We also could move the current observe_value attribute in here.

The downside to this approach is the limitation of the coap_resp_handler_t function definition to pass this data to the application handler. In this case we could specify a convention for gcoap that the void* context must be cast to a gcoap_recv_ctx_t* by the application. In this case the void* context itself must be moved to an attribute of the gcoap_recv_ctx_t struct.

I don't think this change to coap_resp_handler_t use will be too intrusive, but that depends on how much the context pointer is used at present. We could provide a macro based migration mechanism by defining GCOAP_REQ_HANDLER_CONTEXT_VOID and GCOAP_REQ_HANDLER_CONTEXT_CTX. When VOID, the code would work as it does at present.

I am a proponent of these context structs as a way to accommodate growth in an API. #13637 is now another example from today that can be served in this way. Let me know what you think.

@haukepetersen
Copy link
Contributor Author

Agree. Closing in favor of #13819

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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants