nanocoap: define and use coap_request_ctx_t for request handlers

[benpicco]

Contribution description

Currently a resource handler has no information about the resource it handles.
This leads to awkward work-around such as specifying the resource path in the user context again.

To avoid this, introduce a coap_resource_ctx_t struct that contains the user context and the resource path, that can be easily extended if more context is needed in the future.

Testing procedure

All CoAP applications should still work.
Since the user context was usually NULL, most will not even notice a change.

Issues/PRs references

#14397 (comment)
Can provide an alternative to #16827 (and also move tl_type out of coap_pkt_t into coap_resource_ctx_t)

depends on !11

[fabian18]

The idea to present the resource path in a handler of a resource would only occur to me if there existed generic handlers, but usually a handler is written for a dedicated resource, so the path cast in stone.

const coap_resource_t coap_resources[] = {
    COAP_WELL_KNOWN_CORE_DEFAULT_HANDLER,
    { "/riot/board", COAP_GET, _riot_board_handler, NULL },

    /* this line adds the whole "/suit"-subtree */
    SUIT_COAP_SUBTREE,
};

Within _riot_board_handler we know that the path is "/riot/board".

Edit: Ok, but redefining a const char *path = "/my/resource" within _my_resource_handler() would theoretically duplicate the "" string, so it makes sense though.
Edit: Identical const char * are not duplicated, by the compiler.

I would keep it more general.

typedef struct {
    const coap_resource_t *resource;
} coap_request_ctx_t;

typedef ssize_t (*coap_handler_t)(coap_pkt_t *pkt, uint8_t *buf, size_t len,
                                  coap_request_ctx_t *context);

The name makes it possible to extend this struct with resource unrelated members, like endpoint addresses, as you have written in "Issues/PRs references".
And in the opposite direction, I would provide a coap_response_ctx_t which is constructed before memo->resp_handler() is called. But this has not to be done in this PR, I think.

[benpicco]

The idea to present the resource path in a handler of a resource would only occur to me if there existed generic handlers, but usually a handler is written for a dedicated resource, so the path cast in stone.

The use case was the CoAP fileserver.
There you have a generic handler for e.g. /vfs that is then also going to be called for /vfs/path/to/file.

The fileserver is configured to serve a certain directory, e.g. /nvm0/srv

You can re-assemble the request path from the packet, but without knowing the resource there is no way to tell if you should now try to serve /nvm0/srv/vfs/path/to/file or /nvm0/srv/path/to/file.

I would keep it more general.

typedef struct {
    const coap_resource_t *resource;
} coap_request_ctx_t;

I thought that might generate a bit too much noise, but if there is a use case for the other fields of coap_resource_t this is of course pretty straightforward (and we can add helper functions for const char *coap_request_get_resource(const coap_resource_t *resource) to spare the user from typing out all the nested struct details.)

[fabian18]

I am not a big fan of get_member() functions. But they allow us to hide the details of coap_resource_ctx_t from the user, using just a forward declaration. Which is good to not cause API changes. Just having coap_request_get_resource() sufficient instead of ... get_path(), ...get_methods() and so on.

[fabian18]

You forgot to update some functions:

RIOT/examples/cord_ep/main.c

Lines 78 to 82 in 5643b52

	static const coap_resource_t _resources[] = {
	{ "/node/info", COAP_GET, _handler_info, NULL },
	{ "/sense/hum", COAP_GET, _handler_dummy, NULL },
	{ "/sense/temp", COAP_GET, _handler_dummy, NULL }
	};

RIOT/examples/cord_epsim/main.c

Lines 60 to 64 in 5643b52

	static const coap_resource_t resources[] = {
	{ "/riot/bar", COAP_GET, handler_text, "foo" },
	{ "/riot/foo", COAP_GET, handler_text, "bar" },
	{ "/riot/info", COAP_GET, handler_info, NULL }
	};

RIOT/examples/nanocoap_server/coap_handler.c

Lines 166 to 173 in 5643b52

	const coap_resource_t coap_resources[] = {
	COAP_WELL_KNOWN_CORE_DEFAULT_HANDLER,
	{ "/echo/", COAP_GET | COAP_MATCH_SUBTREE, _echo_handler, NULL },
	{ "/riot/board", COAP_GET, _riot_board_handler, NULL },
	{ "/riot/value", COAP_GET | COAP_PUT | COAP_POST, _riot_value_handler, NULL },
	{ "/riot/ver", COAP_GET, _riot_block2_handler, NULL },
	{ "/sha256", COAP_POST, _sha256_handler, NULL },
	};

RIOT/sys/net/application_layer/gcoap/forward_proxy.c

Lines 34 to 35 in 5643b52

	static ssize_t _forward_proxy_handler(coap_pkt_t* pdu, uint8_t *buf,
	size_t len, void *ctx);

RIOT/tests/pkg_edhoc_c/responder.c

Lines 102 to 105 in 5643b52

	const coap_resource_t coap_resources[] = {
	COAP_WELL_KNOWN_CORE_DEFAULT_HANDLER,
	{ "/.well-known/edhoc", COAP_POST, _edhoc_handler, NULL },
	};

RIOT/tests/riotboot_flashwrite/coap_handler.c

Lines 76 to 79 in 5643b52

	const coap_resource_t coap_resources[] = {
	COAP_WELL_KNOWN_CORE_DEFAULT_HANDLER,
	{ "/flashwrite", COAP_POST, _flashwrite_handler, &_writer },
	};

And if we decide to not expose the definition of coap_request_ctx_t to the user (which I would be in favor of), make use of the member access functions. But this would at latest be catched by the CI.

[fabian18]

You may also squash if you like.

[leandrolanzieri]

Shouldn't we have the order of the members swapped? Otherwise this would break applications that cast the user context, right?

[fabian18]

Applications cannot cast coap_request_ctx_t * to whatever they expect. But as of now, they can cast the void *context member of that struct to whatever they expect. But hiding the definition of coap_request_ctx_t and providing coap_request_get_context() would be better IMO.

[leandrolanzieri]

Applications cannot cast coap_request_ctx_t * to whatever they expect

Yes, true. My bad. But then I'm worried this is a big API change for applications out there.

[fabian18]

I think it is reasonable and beneficial. Users who are registering a function prototype with a void *context in their static const coap_resource_t _resources[] resource array are going to face a compilation error. And then they should look at the documentation and should be able to update the prototype.

[chrysn]

But then I'm worried this is a big API change for applications out there.

There are pending use cases for an API change on this (like knowing which client requested this, esp. w/rt security). This change (that adds an extensible pointer) should be the one we need to compatibly add the others later.

[fabian18]

Yup, this is good from my side. You may squash and on green Murdock I will give the first ACK.

[benpicco]

@chrysn can this still make it into the next release?

[chrysn]

On 11 July 2022 12:57:36 CEST, benpicco ***@***.***> wrote: ***@***.*** can this still make it into the next release? Yes. Let's not drag this out needlessly, it's early enough to still spot and fix any fallout.

[benpicco]

@chrysn so want to throw in a 2nd ACK? 😉

[benpicco]

rebased to include #16705

[chrysn]

The API change is warranted, and allows future evolution of the data provided to the handler without further API breaks.

Thanks, seconded.