I did a short test: a large coap put: time.wasm 566 byte

$coap-client -m put -p 5683 -f time.wasm coap://[fe80::xxxx%tapbr0]/WASM
4.13

coap-client -m put -p 5683 -f time.wasm -b 100 coap://[fe80::xxxx%tapbr0]/WASM

(there is nothing written for success unless verbosity 6 the value is 2.04)

from my POV this works as expected, (prior this patch the transfer without blocks just got stuck)

👍

Style issues addressed (most did indeed fit the 100 char limit).

The TBD Size1 I'm leaving out right now as there are open questions as to what should be indicated there; not sending one is violating a SHOULD of 7252 but all still better than not responding at all, and IMO also better than sending the wrong value.

Are you happy enough with the state of things that one of you can ACK this? [edit: provided a test was done on non-GNRC, see below]

@kfessel, I'm not quite sure reading your comment what you tested. Did you find an example where gcoap-on-lwIP or similar can be run, or just tested the default GNRC gcoap case?

i tested my app (the one i had the issue with that lead me to talk about that in matrix) i tested with native
-> i only tested gnrc

the app takes wasm bytecode with is often more than 100 byte and therefor has that problem but works well with block transfers

this PR is a solution for my case (error response enables me to react and use block transfers no error just silence is bad)

The TBD Size1 I'm leaving out right now as there are open questions as to what should be indicated there; not sending one is violating a SHOULD of 7252 but all still better than not responding at all, and IMO also better than sending the wrong value.

couldn't we just send the limit by buffersize and if the app requires a smaller size it creates its own 4.13 error

What I think 4.13 with Size1 should is to report the payload size the client could use with this request. That means that the message needs to be parsed to the end of options, and then it needs to be determined how much space is left. Doing this is relatively complex, and may need additional error reporting to reliably see if the message was maybe even truncated in the options (in which case ... I don't know if anything sensible can be reported at all). But I'm not even sure that is the right interpretation, whereas not sending a Size1 is bad but at least debuggable.

There are now proper testing descriptions, and I ran them.

A few things worth mentioning happened there, and are expressed in follow-up commits:

• If the message is truncated before the options end, things will still fail. This is because we run the message through the nanocoap coap_parse, and can't (easily and compatibly, at least) tell whether parsing was "successful enough" that we at least get the header populated. So for the time being, requests truncated already in the options will behave as before. This is now documented in the code, but honestly it's not where I expect the trouble to come from.
• The gcoap client had a nasty behavior where rather than checking whether memo->state == GCOAP_MEMO_RESP, it checked for all its known error conditions. I'd say this is OK and call it bad judgement on the application developer side (error checking in enums should always be "accept what's known", not "reject specific errors"), but others may call this an incompatible change.
• Given that we know at response handling time that only the payload was trimmed, the example response handler could be smart and not even complain but just start blockwising (even if the server didn't announce it). I prefer to leave that to later, as it may need meddling with more block-wise code, and isn't essential for getting a gcoap version that doesn't spontaneously break on large messages.

@kfessel, I'm not quite sure reading your comment what you tested. Did you find an example where gcoap-on-lwIP or similar can be run, or just tested the default GNRC gcoap case?

Sidenote: There is a branch by @kb2ma somewhere, that should make that possible.

Thank you I just stumbled upon this as well.
I wrongly assumed the buffer passed to gcoap_req_init() was also used to store the response and wondered why I did not receive anything even though I used a large enough buffer here.

Why can't we use a user supplied buffer here instead of the local _listen_buf?

I saw a message in my email from this PR. It's been a while but my recollection is below.

Sidenote: There is a branch by @kb2ma somewhere, that should make that possible.

See the kb2ma/RIOT/lwip* branches.

Why can't we use a user supplied buffer here instead of the local _listen_buf?

IIRC this is the basic design of gcoap, as a messaging hub running in its own thread and for simplicity. See the wiki page for contrast with nanocoap.

I wrongly assumed the buffer passed to gcoap_req_init() was also used to store the response and wondered why I did not receive anything even though I used a large enough buffer here.

Any incoming message needs to go through gcoap's buffers at one point.

Why can't we use a user supplied buffer here instead of the local _listen_buf?

At the time we're reading into that, we don't even know whether what's coming in is a response or a request, let alone which request this belongs to.

If a response is received through gcoap it is, as it always has been, copied into gcoap's receive buffer, and freed from the socket. When such a truncated response arrives, the tail is gone for good already.

Sure, we could offer all kinds of "yeah but there's a pointer you can follow up on and need to free" stuff, but the beauty of nanocoap and gcoap is that the user doesn't need to gather data from anywhere, and the beauty of CoAP is that this is not needed in practice.

The point of this is to allow the stack to fail gracefully (allowing the protocol to continue rather than deadlocking it) when overly long messages arrive, which generally means to ask for smaller bites of data. An application will pick its buffer in accordance with the messages it expects, so if someone wants to haul 1KiB messages, they can up their buffer.

Mh, so you are basically "misusing" the _buf API for a different than the intended use case, gotcha!

The point of this is to allow the stack to fail gracefully (allowing the protocol to continue rather than deadlocking it) when overly long messages arrive, which generally means to ask for smaller bites of data. An application will pick its buffer in accordance with the messages it expects, so if someone wants to haul 1KiB messages, they can up their buffer.

Mh, but consider this: we only put the header of a packet into the internal buffer and for the rest an application may choose to use:

/* just a sketch! */
static uint8_t payload[SOME_ARRAY_SIZE];
int pos = 0, res;
while ((res = gcoap_get_payload(pkt, &payload[pos], sizeof(payload) - pos) > 0) {
    pos += res;
}

this way, a developer does not need to beat themselves over the head of configuring some module they may or may non know about and can do all the necessary compile-time config in the app. No '"get follow-up or "yeah but there's a pointer you can follow up on and need to free" stuff' required, if we establish that this is the way to get a packet's payload.

Yes. I'm only using _buf because without it I we couldn't even see whether truncation happened, and right here this is all I care about.

consider this

Sure, that's a change we can make. But it's something that needs coordination across nanocoap and gcoap (which I start suspecting is part of what's holding RIOT's CoAP ecosystem back). In the end, this can get us rid of the need for a payload-sized buffer for incoming messages at all (though so far our handlers place their responses where the request came in, so...)

I'm happy to work with this as a proposal, but that'll take several changes and iterations of design, while this here is only about getting the CoAP machinery unstuck when large packages arrive, without any need for applications to alter any behavior. (Unless they specifically checked for whether memo->state indicates some particular form of error rather than for success). Let's do both, but please the good before the better.

Sure, that's a change we can make. But it's something that needs coordination across nanocoap and gcoap (which I start suspecting is part of what's holding RIOT's CoAP ecosystem back). In the end, this can get us rid of the need for a payload-sized buffer for incoming messages at all (though so far our handlers place their responses where the request came in, so...)

Does it oO? If the coap_pkt_t struct needs to be extended for gcoap's sake, it might make sense to instead provide an extended, gcoap-specific type. But granted, I am not as deep into the gcoap/nanocoap-internals (yet), to provide solid feedback on this. Up until now I understood that nanocoap is only used as a message parser and composer for gcoap, but if there is deeper entanglement, maybe some cleanup is indeed required.

nanocoap is used as message parser and composer, but exceeding that, gcoap and nanocoap also share the coap_pkt_t structure as well as the coap_handler_t -- which means that untangling them entails a type change across all user visible CoAP functions.

BTW, Murdock found that I had disregarded the new DTLS code -- that's now "fixed" easily (because if truncation happens to DTLS, tough luck, crypto won't open, and the message can't be acted on sensibly).

nanocoap is used as message parser and composer, but exceeding that, gcoap and nanocoap also share the coap_pkt_t structure as well as the coap_handler_t -- which means that untangling them entails a type change across all user visible CoAP functions.

Ok, but so (as far as I can judge) nothing that couldn't be handled with some good old inheritance and type juggling. In the long run, I think it is a good idea to boilerplate the whole nanocoap underbelly away from the user. But for now, I think we can live with just warning the user about truncated payloads.

Thanksl. Murdock passes too now. @maribu has done some review already, do these still hold? If so, testing is what is missing. For me with the latest commit, tests on native passed as described above for GNRC; LWIP passed after commenting out sys/net/netif/netif.c line 72 (netif_get_name; that's undefined with this setup both in master and this branch and an unrelated yet-to-be-opened issue).

Please let me know if there are any more issues, and whether I can squash. (I'd keep it as the 3 commits, and just let the fixup autosquash).

(netif_get_name; that's undefined with this setup both in master and this branch and an unrelated yet-to-be-opened issue).

See #16741 ;-).

May I rebase this?

Yes, and squash as well, if you like :-).

I'm configured to autosquash ;-)

Rebased, and all described tests still work.

What I think 4.13 with Size1 should is to report the payload size the client could use with this request. That means that the message needs to be parsed to the end of options, and then it needs to be determined how much space is left. Doing this is relatively complex, and may need additional error reporting to reliably see if the message was maybe even truncated in the options (in which case ... I don't know if anything sensible can be reported at all). But I'm not even sure that is the right interpretation, whereas not sending a Size1 is bad but at least debuggable.

@chrysn
the value is already calculated by the nanocoap coap_parse it is pdu->payload_len of the truncated pkg, if that one is 0 there are too many options

I did test my application with this patch, with GNRC and LWIP it works
my application is a gcoap server that take big PUT messages without this patch i get no response, if i do not use blocktransfer, with this patch i get the information -> then I do the transfer again with blocktransfer and it is working.
i will try to do further testing (the example) later

@chrysn: Do you want to add SIZE1 reporting with what coap_parse provides in pdu->payload_len? or just have it as it is?

I tested:

• my own server (takes message bigger than the buffer via put and processes them) with GNRC ✔️ and LWIP ✔️ (in both cases i got an appropriate resonse -> resend with small blocks working ✔️ )
• the gcoap example for the client path with GNRC ✔️ and LWIP ✔️
  (got a warning as described in the PR ✔️ )

I'd prefer to do any Size1 in a follow-up.