Client transport error handling - #70
Conversation
|
Can you provide a little bit of context on what some of this is for? I think I get it but it would be useful to have an overview in English. |
| if (received < 2 || _rxBuffer[1] != 0xF6) { | ||
| return ThingSetResult(ThingSetStatusCode::internalServerError); | ||
| } | ||
|
|
||
| // return size having accounted for response code and null | ||
| responseSize -= 2; | ||
| responseSize = (size_t)received - 2; | ||
| *responseBuffer = &_rxBuffer[2]; |
There was a problem hiding this comment.
I feel the first and second of these 2s could perhaps be a const somewhere (and possibly even the third).
|
|
||
| // name needs to be this to make stupid twister check pass | ||
| #define ZCLIENT_SERVER_TEST(test_name, Body) \ | ||
| // variadic so test bodies may contain top-level commas (e.g. template args) |
| if (ctx->filter_id < 0) { | ||
| /* Without this check a full filter table used to be reported as a | ||
| * successful bind, and every subsequent exchange timed out */ | ||
| LOG_ERR("Failed to add RX filter for %x:%x (err %d)", filter.id, filter.mask, | ||
| ctx->filter_id); | ||
| return ISOTP_NO_FREE_FILTER; | ||
| } |
There was a problem hiding this comment.
My main comment with this would be that as it stands, it's now quite hairy: it's very important that one does not forget to increment or decrement the pending_cb count in an early return. I wonder if at the very least a local function might reduce some of the instances where this needs to be done, i.e.:
static int isotp_can_send(const struct device *can_dev, const can_frame *frame, ..., atomic_t *pending_cb)
{
atomic_inc(pending_cb);
int ret = can_send(...);
if (ret) {
atomic_dec(pending_cb);
}
return ret;
}
garethpotter
left a comment
garethpotter
left a comment
There was a problem hiding this comment.
As commented elsewhere, it would be useful to have an overview of what this is trying to do, but I think I get it and there's nothing too controversial from what I can see.
Updated description |
3 participants
This came about because of an edge case testing on BP hardware where the isolated comms were killed whilst a request/response context was still open - managed to recreate somewhat reliably creating a client transport allocated on the stack.
Ultimately, the exec (in this case) call timed out and then the transport goes out of scope, but the driver callback still points to that memory, causing all sorts of issues.
The static orphanage just gives us a place to redirect the callback to with a bunch noops and then die safely.
Claude then reviewed it and saw a couple of other issues that were also fixed as a part of this PR