Messages in this thread | | | Date | Wed, 30 Nov 2022 05:18:20 -0800 | From | Breno Leitao <> | Subject | Re: [PATCH RESEND net-next] tcp: socket-specific version of WARN_ON_ONCE() |
| |
On Tue, Nov 29, 2022 at 09:16:16PM +0000, Iwashima, Kuniyuki wrote: > > On Nov 29, 2022, at 21:48, Breno Leitao <leitao@debian.org> wrote: > >> On Tue, Nov 29, 2022 at 10:00:55AM +0900, Kuniyuki Iwashima wrote:
<snip>
> >>> +void tcp_sock_warn(const struct tcp_sock *tp) > >>> +{ > >>> + const struct sock *sk = (const struct sock *)tp; > >>> + struct inet_sock *inet = inet_sk(sk); > >>> + struct inet_connection_sock *icsk = inet_csk(sk); > >>> + > >>> + WARN_ON(1); > >>> + > >>> + if (!tp) > >> > >> Is this needed ? > > > > We are de-referencing tp/sk in the lines below, so, I think it is safe to > > check if they are not NULL before the de-refencing it. > > tp->snd_cwnd is accessed just after this WARN, > so I thought there were no cases where tp is NULL.
Oh, important to say that we want to re-use this macro on other places as well. This initial usage (on tcp_snd_cwnd_set()) is just for the initial patch. I see value replacing some WARN_ON_*() by TCP_SOCK_WARN_ON_ONCE() in other parts of the code, so, this check is to protect this warning when TCP_SOCK_WARN_ON_ONCE() is called from different places.
Anyway, I definitely can remove the check here, but, we might want to re-add it later, as we replace some WARN_ON_* by TCP_SOCK_WARN_ON_*();
> I think this additional if could confuse future readers and > want to make sure if there is such a case.
How come checking if a pointer is valid before de-refencing it could confuse readers?
Thank you for the review!
| |