net/gcoap: add user ptr to response handler functions

[haukepetersen]

Contribution description

This PR changes the gcoap API slightly to allow passing user defined pointers to response handler functions (gcoap_resp_handler_t). This is analog to the user context pointers that were added to the resource handler functions in nanocoap a while ago (coap_resource_t).

Adding these user context pointers is quite useful, when handling concurrent requests of a similar type (i.e. registration to multiple CoRE RD's at the same time in my case).

I actually can't remember if this change has been discussed before, or if this change does even already exist. A quick search through the open PRs and issue did not yield any results, so I guess not?!

Testing procedure

So far, this is rather a request for comments, so all changes to the code are untested...

Issues/PRs references

none

[kb2ma]

I can see the value in a context for a request, and passing the context via the request would be simpler than looking up the context in the handler. The example that comes to my mind is the state associated with a guaranteed delivery requirement. In other words, even a Confirmable request can fail. What then?

I have only inspected the code so far, but it looks good. I will test as soon as I get the chance.

My only regret is that this change will break all existing request code, but at least the fix is pretty mechanical.

[kb2ma]

To future-proof the change, we can include the request memo in the callback rather than the context itself.

typedef void (*gcoap_resp_handler_t)(unsigned req_state, coap_pkt_t* pdu,
                                     gcoap_request_memo_t *memo);

With this declaration, we can make more parameters available in the future via the memo without requiring that users change existing code. I replaced the remote endpoint parameter because I'm not sure how widely it is used, and of course the endpoint is available in the request memo.

We could specify in the documentation that the user can count on only specific attributes -- remote_ep and context.

[haukepetersen]

In general, using the memo as parameter sounds sensible. I am not sure about dropping the remote parameter however, since the remote parameter in the callback and the remote_ep field in the memo struct are not necessarily the same to my understanding -> request to multicast address vs reply from a unicast address.

But then I just noticed, that I might have a large gap in understanding when it comes to CoAP and multicast traffic. I did never read into the group communication documents and it seems that I have misunderstood the RFC so far - it seems to me that the scenario above is not allowed in the first place?!

In that case dropping the remote parameter in favor of the memo parameter sounds good to me. But we keep the change of adding a context parameter to the send function, right?

[kb2ma]

I totally blanked that the original purpose of the remote host in the handler was for the multicast case. I have essentially no multicast experience, so I read up on 7252 sec. 8 as well as 7390. What did you mean specifically about your gap in understanding from the groupcomm documents relative to the need for the remote parameter in the handler?

As I read those documents, particularly 7390, sec 2.5, I agree it makes sense to retain the remote reference in the handler. For example, the client might make a multicast request for which it expects individual responses, like "Are you functioning nominally?" And now the client will know which servers responded.

So, how about this declaration for the response handler? I re-added the remote server pointer and made the request memo const.

typedef void (*gcoap_resp_handler_t)(unsigned req_state, coap_pkt_t* pdu,
              sock_udp_ep_t *remote, const gcoap_request_memo_t *memo);

As I mentioned earlier in this thread, please document which memo attributes the implementer can count on. Let's limit the exposure of the internals.

And yes, I completely agree that the context pointer must be included in gcoap_req_send2() and gcoap_req_send(), in order to place it in the request memo.

[haukepetersen]

What did you mean specifically about your gap in understanding from the groupcomm documents relative to the need for the remote parameter in the handler?

Mainly that I simply never opened/read RFC7390 :-)

I am still thinking about the best solution for selecting paramters to the resp_handler callback, my current thoughts are: I don't quite like the idea to pass the memo struct, and then telling the user which fields are ok to use, and which should not be used. This seems not to be the cleanest/most resilient approach to API design. Further having this mixture of passing some memo fields directly (-> state) while others can be accessed indirectly (e.g. context, remote_ep).

So let's see what our options can be (at least what I can think of right now):

• go back to pass the context parameter instead of memo: not very future prove and not flexible (e.g. user may not access the original remote_ep parameter) -> IMO not a good idea
• live with the fact that users have to make sure to read the doc any only access/use the allowed fields in the memo struct -> might indeed be a sensible trade off between flexibility and API robustness
• we could do a simple cosmetic restructuring of the memo struct: move 'non-public' fields into something like an internal' (e.g. memo->internal.PRIVATE_FIELD`) sub-struct -> overkill?

On top when passing the memo field, we might at least think about dropping the req_state paramter then, right?

[haukepetersen]

just to get a feel: I just pushed a commit that drops the req_state parameter in favor of passing a memo pointer. I think I tend to like it for its cleanliness.

[kb2ma]

Thanks for summarizing the options for the handler. I like your latest resolution, like below.

typedef void (*gcoap_resp_handler_t)(const gcoap_request_memo_t *memo,
                                     coap_pkt_t* pdu,
                                     const sock_udp_ep_t *remote);

I was in favor of keeping the req_state parameter in the declaration because it was like a return value for the handler. The user definitely needs to inspect this parameter first. However, your change basically keeps this order of parameters. So now, the user must inspect memo->state instead, which is just a small change. It's nice with this declaration that we also get access to the context parameter at no extra cost.

I am not concerned about including the memo in the declaration. The memo as a whole really is a context object, qualifying the response PDU. Regarding the need to specify which fields in the PDU that the user should access:

• We mark the struct 'const', so the user knows not to mess with it.
• We have a similar use model for the coap_pkt_t in a request handler. The user must access the payload pointer and length via the coap_pkt_t when building the response.
• It's worth it not to break applications in the future if we need to retain/pass another parameter.

I also agree that it's overkill to add some 'private' marking to the struct, at least for now. Maybe later when the APIs are stable.

So, please document and update examples, and I'll test it out. When you update documentation, remember the Handling the response section in the module documentation.

[haukepetersen]

I fully agree, and looking at the function today I also still like the solution in passing the memo as first parameter. I will adapt the PR accordingly later today.

[haukepetersen]

Adapted this PR according to the results of our discussion:

• the resp_handler() callback now passes a const pointer to the memo struct as first parameter
• the memo holds an additional user defined context field
• the gcoap_req_send() and gcoap_req_send2() functions take an additional void *context parameter

[kb2ma]

Sorry to take so long to get back on this one, @haukepetersen. Let's get this merged! The code looks basically sound and matches our discussion above.

I think the first step is to rebase and update cord_ep's use of gcoap_req_send2() and gcoap_resp_handler_t callback. We also need some instructions on how to test.

[haukepetersen]

no problem, I am not less responsible for delays in this PR...

I will try to do the requested, but can't promise anything before next Wednesday... Will try my best.

[kb2ma]

As we discussed, I rebased and updated the cord_ep* examples in my gcoap/req_user_context branch. Please rebase your opt_gcoap_contextptrtoresphandler branch so I can create a PR against it.

Also, would you please confirm that the cord_epsim example fails with the default RD address of ff02::1. I use a tap interface between aiocoap-rd on my Linux workstation and the cord_epsim example for native. aiocoap complains with:

WARNING:coap-server:Dropping pktinfo from ancdata because it indicates a multicast address

and eventually the cord_epsim example errors in the CLI with:

[cord_epsim] error: unable to send registration

The example works when I compile with RD_ADDR set to the link local unicast address.

[haukepetersen]

Also, would you please confirm that the cord_epsim example fails with the default RD address of ff02::1

Confirmed! Even if aiocoap would not reject the request, the example would fail with a multi-cast address since gcoap is not able to handle group communication (yet). I don't remember the exact location in the code, but somewhere when receiving, we check if the dst addr of the outgoing request is equal to the src addr of the response (as stated in the RFC) -> and this will of course fail for multicast messages...

I will change the epsim example in a separate PR, so that at least the is no broken default...

[haukepetersen]

also rebased.

[kb2ma]

the example would fail with a multi-cast address since gcoap is not able to handle group communication (yet)

Right, and this PR provides the data for the client to deal with a response from a multicast request. I see a folllow-on PR on the horizon. :-)

[edited for clarity]

[haukepetersen]

Fixed the issue for now in #10464

But the group communication stuff would indeed be nice to implement at some point :-)

[kb2ma]

In the description you say:

Adding these user context pointers is quite useful, when handling concurrent requests of a similar type (i.e. registration to multiple CoRE RD's at the same time in my case)

I'm starting to understand CoRE RD. What exactly would you want to retrieve from the context?

[kb2ma]

Now that the response handler has access to the coap_request_memo_t, we may want to streamline the implementation of cord_ep in a follow on PR. Presently cord_ep has separate handlers for on_update and on_remove. However, the coap_request_memo_t has access to the request header. So, a single handler could read the request code from the header instead of hard coding the code in the separate functions.

[haukepetersen]

Hej @kb2ma: lost this PR get out of view once more :-)

I rebased it and adapted it to the latest changes in gcoap. Would you mind to have another look?! Thanks!

[kb2ma]

Thanks for rebasing this PR back to life. I will look again after the release. It will be a good time to add infrastructure.

[haukepetersen]

sounds good to me!

[chrysn]

Having this would help applications using OSCORE (their response handler is basically "decrypt and verify with these nonces, then pass on to the actual response handler").

Is this PR in an overall state where we expect no further obstacles? If so, I'd make this branch a prerequisite for the OSCORE demos and build on it (reviewing the usability as I go), otherwise I'll need to work around (and can still review, but from a more theoretical perspective).

[haukepetersen]

go ahead! I hope its just the final review/verification that this PR is waiting for :-)

[chrysn]

Works very well for my OSCORE demo use case. (In its current example, the caller allocates a struct that contains the request ID necessary for correlation, and a mutex used to tell whoever allocated that struct that he may return thus freeing the struct).

[haukepetersen]

addressed issues and rebased. Let me know when you want me to squash.

[kb2ma]

Looks good. Tested all the clients. Please squash!

[haukepetersen]

squashed