On Fri, Apr 19, 2024 at 4:31 AM Jason Xing <kerneljasonx...@gmail.com> wrote: > > On Fri, Apr 19, 2024 at 7:26 AM Jason Xing <kerneljasonx...@gmail.com> wrote: > > > > > When I said "If you feel the need to put them in a special group, this > > > is fine by me.", > > > this was really about partitioning the existing enum into groups, if > > > you prefer having a group of 'RES reasons' > > > > Are you suggesting copying what we need from enum skb_drop_reason{} to > > enum sk_rst_reason{}? Why not reusing them directly. I have no idea > > what the side effect of cast conversion itself is? > > Sorry that I'm writing this email. I'm worried my statement is not > that clear, so I write one simple snippet which can help me explain > well :) > > Allow me give NO_SOCKET as an example: > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c > index e63a3bf99617..2c9f7364de45 100644 > --- a/net/ipv4/icmp.c > +++ b/net/ipv4/icmp.c > @@ -767,6 +767,7 @@ void __icmp_send(struct sk_buff *skb_in, int type, > int code, __be32 info, > if (!fl4.saddr) > fl4.saddr = htonl(INADDR_DUMMY); > > + trace_icmp_send(skb_in, type, code); > icmp_push_reply(sk, &icmp_param, &fl4, &ipc, &rt); > ende: > ip_rt_put(rt); > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > index 1e650ec71d2f..d5963831280f 100644 > --- a/net/ipv4/tcp_ipv4.c > +++ b/net/ipv4/tcp_ipv4.c > @@ -2160,6 +2160,7 @@ int tcp_v4_rcv(struct sk_buff *skb) > { > struct net *net = dev_net(skb->dev); > enum skb_drop_reason drop_reason; > + enum sk_rst_reason rst_reason; > int sdif = inet_sdif(skb); > int dif = inet_iif(skb); > const struct iphdr *iph; > @@ -2355,7 +2356,8 @@ int tcp_v4_rcv(struct sk_buff *skb) > bad_packet: > __TCP_INC_STATS(net, TCP_MIB_INERRS); > } else { > - tcp_v4_send_reset(NULL, skb); > + rst_reason = RST_REASON_NO_SOCKET; > + tcp_v4_send_reset(NULL, skb, rst_reason); > } > > discard_it: > > As you can see, we need to add a new 'rst_reason' variable which > actually is the same as drop reason. They are the same except for the > enum type... Such rst_reasons/drop_reasons are all over the place. > > Eric, if you have a strong preference, I can do it as you said. > > Well, how about explicitly casting them like this based on the current > series. It looks better and clearer and more helpful to people who is > reading codes to understand: > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > index 461b4d2b7cfe..eb125163d819 100644 > --- a/net/ipv4/tcp_ipv4.c > +++ b/net/ipv4/tcp_ipv4.c > @@ -1936,7 +1936,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb) > return 0; > > reset: > - tcp_v4_send_reset(rsk, skb, (u32)reason); > + tcp_v4_send_reset(rsk, skb, (enum sk_rst_reason)reason); > discard: > kfree_skb_reason(skb, reason); > /* Be careful here. If this function gets more complicated and
It makes no sense to declare an enum sk_rst_reason and then convert it to drop_reason or vice versa. Next thing you know, compiler guys will add a new -Woption that will forbid such conversions. Please add to tcp_v4_send_reset() an skb_drop_reason, not a new enum. skb_drop_reason are simply values that are later converted to strings... So : Do not declare a new enum.