gcoap: return Method Not Allowed if appropriate

[nmeum]

If a resource for a path is defined, but the client sent a request with the wrong method, the server responds with the message code 4.05 Method Not Allowed by default now.

See: https://tools.ietf.org/html/rfc7252#section-5.9.2.6

---

Regarding coding style it might make sense to define a macro (or an enum) for the return values of _find_resource.

[kb2ma]

@nmeum, thanks for the PR! I haven't tested yet, but the logic looks sound to me. I recommend macros for the return values, like GCOAP_RESOURCE_FOUND, GCOAP_RESOURCE_NO_PATH, etc. It will make the meaning clearer in _find_resource() and in the caller. You can see similar with the GCOAP_FIND_REQ_... macros. Some of the other _find_... functions return a -1, but that's in the context of a slot number, so not as arbitrary.

I'll get back to you as soon as I get a chance to test the code.

[nmeum]

I recommend macros for the return values, like GCOAP_RESOURCE_FOUND, GCOAP_RESOURCE_NO_PATH, etc.

Thanks for the input. Should I add those macros to gcoap.c since _find_resource is a static function or should they be added to gcoap.h instead?

[kb2ma]

Good thinking -- gcoap.c is a better fit; no need to expose to public interface.

[nmeum]

Updated accordingly, let me know if you have any further suggestions.

[smlng]

IMHO instead of defining/introducing redundant return value you could simply return the appropriate CoAP code, i.e., COAP_CODE_METHOD_NOT_ALLOWED and COAP_CODE_PATH_NOT_FOUND , and use 0 on success.

But it would even be possible to just adapt the current function and use resource == NULL for COAP_CODE_PATH_NOT_FOUND , and listener == NULL for COAP_CODE_METHOD_NOT_ALLOWED. So no need to introduce an explicit error return value, just reuse what's already there.

[nmeum]

I would personally prefer defining an enum type with return values for this kind of use case but that doesn't seem to be the RIOT-way of doing things so I will do whatever @kb2ma prefers.

[kb2ma]

ACK. Tests fine for me on native.

I think the alternative from @smlng to use the COAP... codes or 0 works also, and I would be OK with that solution. It does remove the clutter of the macro definitions at the top of gcoap.c.

On the other hand, I like the present solution because it reads better in the code to use internal resource values separate from external COAP values. It also doesn't add to code size -- it's just labels for numbers.

[smlng]

It also doesn't add to code size -- it's just labels for numbers.

Code size is impacted by changing the functions return value from void to int, that's why it might be considerable to use the information already provided - as suggested in my second solution - to use resource and listener as return values and to check if an error occurred.

But in general, I leave it up to you @kb2ma, as gcoap is your baby 😉 As you already approved I'm fine with that as well.

I'll trigger Murdock to make a check ...

[kb2ma]

Thanks @smlng, it's a group effort and I appreciate the work and support you're providing. I would get confused by use of the listener parameter to understand the result and think the return value is worth it for clarity.

Either way, we're stoked to have @nmeum as a contributor. :-)

[smlng]

I'd like to make some size comparison before merging this one. I'll report back. But generally nice catch @nmeum

[smlng]

made some checks, code size is only increased marginally, i.e. <20B. And I agree having proper return value might be nice.

my objections didn't whole, but I'm still undecided whether to approve

[smlng]

typo in my dismissed review: /whole/hold/

[kb2ma]

Happy to talk through the indecision. :-)

[smlng]

@kb2ma, just for clarification _find_resources is an internal function, so I don't see the usefulness of those extra return values - which are fine and useful for API functions that push errors to the outside aka. user. As said the same functionality can be achieve with the function parameters already present.

Further, I also saw no functional usage of the listener_ptr parameter, while resource_ptr is used as a return value outside the function, listener_ptr is not -> looks to me like it could be removed.

I'd also like to get @haukepetersen opinion here.

[kb2ma]

Good find on the listener parameter. I added the _find_resource() function with the Observe server work, and you can see how it was split off from _handle_req() in the commit. I don't remember my thinking at the time, but as a utility function it might be useful to return the owner of the resource.

I agree that since the listener parameter is unused today, we might as well remove it. We can always add it back in if we need it.

[nmeum]

I agree that since the listener parameter is unused today, we might as well remove it. We can always add it back in if we need it.

Should this be done in a separate pull request or should I do it in this one?

[nmeum]

Ping?

[smlng]

please rebase and resolve conflicts.

[nmeum]

please rebase and resolve conflicts.

Done.

[nmeum]

Is there anything left to do to get this merged?

[miri64]

Let's give this one final rebuild, then I think we can merge as I don't see any objection and this PR was already ACKd ages ago.