TCP endpoint compare refactoring #24407
Conversation
jukkar
force-pushed
the
tcp2-endpoint-cmp
branch
from
156ae03 to
0f4b7b6
Compare
Agreed, the allocation and setting are not needed in the same function. I can look into it. |
In tcp_endpoint_cmd() we allocate and then almost immediately destroy the allocated endpoint. This is quite inefficient so use a endpoint from stack in the compare. Separate endpoint allocation from value setting so the caller can decide how the endpoint union values are set. Signed-off-by: Jukka Rissanen <[email protected]>
It is useful to know which test fails to semaphore timeout so add line number of the failing test to assert print. Signed-off-by: Jukka Rissanen <[email protected]>
jukkar
force-pushed
the
tcp2-endpoint-cmp
branch
from
0f4b7b6 to
3bc67f0
Compare
|
Separated the allocation and setup of endpoint union. |
zephyrbot
added
the
area: Tests
ozhuraki
left a comment
ozhuraki
left a comment
There was a problem hiding this comment.
LGTM.
A few improvements per your judgement.
| } | ||
|
|
||
| ep->sin.sin_port = src ? th->th_sport : th->th_dport; | ||
| static union tcp_endpoint *tcp_endpoint_set(union tcp_endpoint *ep, |
There was a problem hiding this comment.
I would suggest to change the return type to bool. This way it would be easier to do the error handling more clear and intuitive inside this function and in places of its usage.
There was a problem hiding this comment.
I did not want to change the current behaviour too much. So new tcp_endpoint_set() can be put to same place where tcp_endpoint_new() was earlier. Actually we do not seem to handle the out-of-mem case very well in current code either. Hmm, I will send another PR that adds more error checks, will check also would it be better to change the return type.
There was a problem hiding this comment.
Sure, this can be done later. These (handling out or memory and more explicit/clear error handling) were my points.
Alternative approach to consider later is just to embed endpoints into struct tcp and to drop the heap allocations.
There was a problem hiding this comment.
Alternative approach to consider later is just to embed endpoints into struct tcp and to drop the heap allocations.
Yes, that would be better way indeed.
| union tcp_endpoint *ep_new = tcp_endpoint_new(pkt, which); | ||
| bool is_equal = memcmp(ep, ep_new, tcp_endpoint_len(ep->sa.sa_family)) ? | ||
| false : true; | ||
| union tcp_endpoint ep_tmp = { 0 }, *ep_ptr; |
There was a problem hiding this comment.
Perhaps, ep_ptr is extra here, &ep_tmp can be passed right to memcmp().
|
|
||
| conn->dst = tcp_endpoint_new(pkt, TCP_EP_SRC); | ||
| conn->src = tcp_endpoint_new(pkt, TCP_EP_DST); | ||
| conn->dst = tcp_endpoint_set(tcp_endpoint_new(pkt), pkt, TCP_EP_SRC); |
There was a problem hiding this comment.
Per your judgement, consider the first comment (tcp_enpoint_set() -> bool), might make the error/reporting handling more clear.
4 participants
Instead of allocating an endpoint union and then freeing it almost immediately, allocate the ep from stack instead of heap.
This contains also Ravi's SRC/DST rename patch from the TCP2 unit test PR #24346