On Wed, Nov 5, 2025 at 3:01 PM Stefano Brivio
On Wed, 5 Nov 2025 15:24:44 +1100 David Gibson
wrote: On Tue, Nov 04, 2025 at 05:42:33AM +0100, Stefano Brivio wrote:
On Fri, 31 Oct 2025 13:42:40 +0800 Yumei Huang
wrote: [snip] +/** + * tcp_get_rto_params() - Get host kernel RTO parameters + * @c: Execution context +*/ +void tcp_get_rto_params(struct ctx *c) +{ + intmax_t tcp_syn_retries, syn_linear_timeouts; + + tcp_syn_retries = read_file_integer( + TCP_SYN_RETRIES, TCP_SYN_RETRIES_DEFAULT); + syn_linear_timeouts = read_file_integer( + TCP_SYN_LINEAR_TIMEOUTS, TCP_SYN_LINEAR_TIMEOUTS_DEFAULT);
I think this is a bit hard to read. Now:
tcp_syn_retries = read_file_integer(TCP_SYN_RETRIES, TCP_SYN_RETRIES_DEFAULT); syn_linear_timeouts = read_file_integer(TCP_SYN_LINEAR_TIMEOUTS, TCP_SYN_LINEAR_TIMEOUTS_DEFAULT);
would be a bit closer to the coding style we're adopting, but I wonder:
- does read_file_integer() really need to have "integer" in the name?
In a language where integers are called 'int', perhaps read_file_int() is clear enough?
I think the idea is that read_file_integer() can be used for any (signed) integer type (with range checking performed after the call). read_file_int() might suggest it reads exactly an 'int', not anything bigger or smaller.
Oh, I see. It could be read_file_num() then, even if it's slightly less accurate, or it can even remain read_file_integer(). If something like this:
v = read_file_integer(SYN_RETRIES, SYN_RETRIES_DEFAULT); c->tcp.tcp_syn_retries = MIN(v, UINT8_MAX);
v = read_file_integer(SYN_LINEAR_TIMEOUTS, SYN_LINEAR_TIMEOUTS_DEFAULT); c->tcp.syn_linear_timeouts = MIN(v, UINT8_MAX);
fits 80 columns, I'm taking it as a sign that the function name isn't exceedingly long. No particular preference from me.
The longest line just exceeds 80 columns. I can rename the function to read_file_num() if there is no objection from David. Another thing, while I was revising the patches, I noticed the parameter "const struct ctx *c" was removed from tcp_timer_ctl() by commit dd5302dd7bf51 ("tcp, flow: Replace per-connection in_epoll flag with an epollid in flow_common") from Laurent, but we need it to access c->tcp.syn_retries and c->tcp.syn_linear_timeouts. Should we add it back? (Adding Laurent in the loop.) -- Thanks, Yumei Huang