This would make OSCORE integration a lot easier, I'm taking this up for review.

I had to look at how this is used in the proxy PR to understand how this is used.

I'm not convinced this is the right place to put this. It does appear to work, but AFAICT only for some limited patterns of application:

• The default handler

• Adding some condition to the default handler (eg. have one of your gcoap_listener that can only ever produce a match if requests come from, say, a given IP range -- or in virtual hosting, although that a) is niche and b) it'd need another data pointer for the conditional request_matcher function)

• Altering the URI matching rules

  But is iterating over a coap_resource_t array applicable then at all? Wouldn't the entries probably look different?

  One example of this would be LwM2M-style paths (/3/0/22), where the matcher function converts the path to a number, and the resources are stored as native numbers in the resource list. (For that case, it would be nice to hook into _find_resource loop and do the conversion once, rather than on every invocation in the loop).

• Accepting requests into a single entry on some condition (like the proxy case). This could be bent to dispatching to several handlers (say, a reverse proxy), but then you'd be already casting the coap_resource_t array into an array of, say, host names, and it would fall into the above category.

This works, but the latter mode is not immediately obvious from the documentation.

I'd think that the better point to hook into would be to replace the whole _find_resource loop as a whole rather than the function evaluated inside that loop. At that point, resources would become generic (void*) over whatever that matcher would like to iterate over (or resources/resources_len might even just be data1 / data2). Then, the default or filtering case could still interpret their data as a length-tagged resources array, but others (like a reverse proxy) could interpret them as a next-hop lookup table, and yet others (like the forward proxy) would not loop over their resources at all but just treat resources as a handler directly if a Proxy option is present, and defer to the next otherwise.

So this is an improvement because it enables some more cases, but we should at least consider going the next step right away.

while working on the feedback by @chrysn I was wondering whether we should keep two callback functions:

1. a global callback function for _find_resources that is defined outside the context of listeners
2. a second callback function that operates on the listener level (like proposed in this PR)

@chrysn I moved the inner for loop into the callback function of the gcoap listener. _find_resource() only iterates over the list of listeners now. I guess this is a good compromise without introducing huge changes. The proposed changes are now in the latest squash me commit.

This does look better to me. What I still find odd is that a coap_resource_t is set (rather than a handler only), and why the listener is passed back.

I see, though, that the former is is due to observation needing a resource entry (meaning that for the time being it'll be hard to do observation on anything matched with a different method). The latter looks like a relic, do you see any purpose in it? (The listener variable in handle_req is unused).

I did have a deeper look into the observation part, and the fact that this is a resource is only ever used in the debug statements of registration and deregistration. (Otherwise, identity to an arbitrary const pointer is good enough).

I'd prefer to get the generalization away from matching-by-path complete right away (by making gcoap_resource_t merely a thing of the default handler), but do understand if you want to get it done here, as long as that's not entrenching gcoap_resource_t as the way all handlers need to be (which it is not, it's just not pushing it out either).

I did have a deeper look into the observation part, and the fact that this is a resource is only ever used in the debug statements of registration and deregistration. (Otherwise, identity to an arbitrary const pointer is good enough).

hmm, that's true. In addition, it is also used to match against existing observe registrations and to hold the context for the response handler. Generalizing the resource seems possible and is probably also not that complicated. I'd like to focus on that in a separate PR, though.

Conceptually, I'm happy with this (and will go over non-conceptual things like testing, code style and impact next).

I'd still like to indicate in the type documentationof gcoap_listener how it'd be used by other implementations. Would something like

typedef struct gcoap_listener {
    union {
        const coap_resource_t *resources;   /**< First element in the array of
                                             *   resources; must order alphabetically */
        void *data1; /** User data field when request_matcher is not the default handler */
    }
    union {
        size_t resources_len;               /**< Length of array */
        void *data2; /** Additional user data field when request matcher is not the default handler */
    }
    gcoap_link_encoder_t link_encoder;  /**< Writes a link for a resource */
    struct gcoap_listener *next;        /**< Next listener in list */
    /**
    * @brief ptr to a request matcher strategy implementation
    */
   gcoap_request_matcher_t request_matcher;
} gcoap_listener_t;

work for you?

I'd need a more experienced reviewer's help to judge the practical impact of the added field. Can we rely on users to only allocate such structs in ways that guarantee that fields they don't set themselves are zeroed out? (Otherwise a user upgrading without careful reading of the changelog might find themselves call to a garbage address in the request_matcher, possibly even only in production and not in debug builds).

For impact assessment, I've merged in master and built the gcoap example with BOARD=nrf52840dongle DEVHELP=0 GIT_VERSION=xxx (and I'm a bit embarrassed of how long it took me to find that I really need to set the last one).

         text    data     bss     dec
Before:  82728     772   19792  103292
After:   82732     780   19792  103304

The data impact is about what's to be expected (new callbacks for user and default handler), and the text overhead is minimal. Neither IMO warrants the additional code complexity incurred by making this feature optional.

There is some run-time overhead in that coap_get_uri_path is run for each listener rather than once. That only affects the rare requests to .well-known/core and those that 4.04 completely, so I'd put that into acceptable losses. On the long run, I hope to make up for it by replacing the standard matcher with something that doesn't allocate; this PR makes that easy to experiment with.

Follow-up not on the union aliasing: Same goes for the coap_resource_t -- it should be no other code's business than the matcher's to see whether path and/or methods match, and the rest is a (handler, context) pair. The (path, methods) part could be used in any other way just as well. At least in theory, that is -- I vaguely remember having found another reference into the path somewhere else. It might help to document that other matchers may want to cast the resources / resources_len fields to whatever they use internally, but any actual type changes are probably better left to when coap_resource_t can get the same treatment (and that's out of scope for this PR).

Code review:

• The const casting around coap_get_code_detail could be better done by proper constification of that function.
• Is there any reason to have the local const coap_resource_t *resource = NULL rather than passing resource_ptr into the matcher? It's not like anything is checking in later use whether that's even NULL before dereferencing into it, so a matcher not setting this in a successful return is UB no matter whether it's NULL or not.
• _find_resource's return behavior could profit in readability from being constant all the way (given it's not set inbetween).
• It looks like there is a change in behavior when two handlers are added for the same resource but with different method flags (ie. one GET and one POST rather than one that can do either); the old code used to set ret to GCOAP_RESOURCE_WRONG_METHOD and continue there, whereas _request_matcher_default returns right away. Is that intentional, and if so, documented?

Other than the above, the new code does right what the old did.

Testing: This adds no new behavior but only an extension point for which there are no users in the PR, so I consider the existing CoAP testing sufficient.

Suggestion for request_matcher documentation:

Function that picks a suitable request handler from a request. Leaving this NULL selects the default one that picks handlers by matching their Uri-Path to the resources' paths (as per resources/_len field documentation). Alternative handlers may cast the resource/_len fields to fit their needs.

The const casting around coap_get_code_detail could be better done by proper constification of that function.

which constification do you mean here? The return value is an int.

Is there any reason to have the local const coap_resource_t *resource = NULL rather than passing resource_ptr into the matcher? It's not like anything is checking in later use whether that's even NULL before dereferencing into it, so a matcher not setting this in a successful return is UB no matter whether it's NULL or not.

👍 since resource is an output variable, removing the assignment should be fine

_find_resource's return behavior could profit in readability from being constant all the way (given it's not set inbetween).

not sure I fully understand this remark. I constified the pdu parameter for _find_resource(), because it is input only. Does that suffice?

It looks like there is a change in behavior when two handlers are added for the same resource but with different method flags (ie. one GET and one POST rather than one that can do either); the old code used to set ret to GCOAP_RESOURCE_WRONG_METHOD and continue there, whereas _request_matcher_default returns right away. Is that intentional, and if so, documented?

good catch. I restored the old behavior in the latest commit

The const casting around coap_get_code_detail could be better done by proper constification of that function.

The argument pdu to coap_get_code_detail is cast from const coap_pkt_t * to coap_pkt_t *; unless coap_get_code_detail sometimes changes codes it should accept a const right away (and I think a single commit in this patch set to constify that function wouldn't need a separate PR but can easily be done as bycatch, as long as it's not squashed into the other commits at merge time).

_find_resource's return behavior

I was merely referring to the ret variable being initialized at the top, never changed, and then being used in return ret -- could say return GCOAP_RESOURCE_NO_PATH; right away (although that may have changed to actual mutation at when you addressed the last commit, didn't check yet).

1. fixed some minor compile issues that murdock reported
2. added another commit that uses const for the coap_method2flag() function

@chrysn there are two new commits:

1. fixes an error that prevented searching the path in subsequent resources (if no path was found)
2. removed the MISMATCH return code as it is more or less the same as the NO_PATH return code. This is a simplification.

murdock finally agreens (:

@chrysn thanks for this great review!