On Fri, 19 Apr 2024 16:00:20 +0800 Jason Xing <kerneljasonx...@gmail.com> wrote:
> If other experts see this thread, please help me. I would appreciate > it. I have strong interests and feel strong responsibility to > implement something like this patch series. It can be very useful!! I'm not a networking expert, but as I'm Cc'd and this is about tracing, I'll jump in to see if I can help. Honestly, reading the thread, it appears that you and Eric are talking past each other. I believe Eric is concerned about losing the value of the enum. Enums are types, and if you typecast them to another type, they lose the previous type, and all the safety that goes with it. Now, I do not really understand the problem trying to be solved here. I understand how TCP works but I never looked into the implementation of MPTCP. You added this: +static inline enum sk_rst_reason convert_mptcp_reason(u32 reason) +{ + return reason += RST_REASON_START; +} And used it for places like this: @@ -309,8 +309,13 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk, return dst; dst_release(dst); - if (!req->syncookie) - tcp_request_sock_ops.send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED); + if (!req->syncookie) { + struct mptcp_ext *mpext = mptcp_get_ext(skb); + enum sk_rst_reason reason; + + reason = convert_mptcp_reason(mpext->reset_reason); + tcp_request_sock_ops.send_reset(sk, skb, reason); + } return NULL; } As I don't know this code or how MPTCP works, I do not understand the above. It use to pass to send_reset() SK_RST_REASON_NOT_SPECIFIED. But now it takes a "reset_reason" calls the "convert_mptcp_reason()" to get back a enum value. If you are mapping the reset_reason to enum sk_rst_reason, why not do it via a real conversion instead of this fragile arithmetic between the two values? static inline enum sk_rst_reason convert_mptcp_reason(u32 reason) { switch(reason) { case 0: return SK_RST_REASON_MPTCP_RST_EUNSPEC; case 1: return SK_RST_REASON_MPTCP_RST_EMPTCP; case 2: return SK_RST_REASON_MPTCP_RST_ERESOURCE; [..] default: return SK_RST_REASON_MAX; // or some other error value ] } I'm not sure if this is any better, but it's not doing any casting and it's easier to understand. It's a simple mapping between the reason and the enum and there's no inherit dependency between the values. Could possibly create enums for the reason numbers and replace the hard coded values with them. That way that helper function is at least doing a real conversion of one type to another. But like I said from the beginning. I don't understand the details here and have not spent the time to dig deeper. I just read the thread and I agree with Eric that the arithmetic conversion of reason to an enum looks fragile at best and buggy at worst. -- Steve