| Message ID | 20130905042045.GD15824@redhat.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Dave Jones <davej@redhat.com> Date: Thu, 5 Sep 2013 00:20:45 -0400 > What's the intent here ? This stuff is great, do you have a script that looks for this false indentation pattern? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(Adding Yuchung Cheng and Neal Cardwell as the author and acker of the patch) On Thu, 2013-09-05 at 00:20 -0400, Dave Jones wrote: > What's the intent here ? > > This ? I think the first is most likely. > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index b2f6c74..95544e4 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -2454,10 +2454,11 @@ static int do_tcp_setsockopt(struct sock *sk, int level, > case TCP_THIN_DUPACK: > if (val < 0 || val > 1) > err = -EINVAL; > - else > + else { > tp->thin_dupack = val; > if (tp->thin_dupack) > tcp_disable_early_retrans(tp); > + } > break; > > case TCP_REPAIR: > > Or this ... > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index b2f6c74..187c5a4 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -2456,8 +2456,9 @@ static int do_tcp_setsockopt(struct sock *sk, int level, > err = -EINVAL; > else > tp->thin_dupack = val; > - if (tp->thin_dupack) > - tcp_disable_early_retrans(tp); > + > + if (tp->thin_dupack) > + tcp_disable_early_retrans(tp); > break; > > case TCP_REPAIR: > > > I'll submit the right patch in the right form once I know what was intended. > > The former seems more 'correct' to me, but I'm unsure if that could break something. > > Dave > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 05, 2013 at 12:39:07AM -0400, David Miller wrote: > From: Dave Jones <davej@redhat.com> > Date: Thu, 5 Sep 2013 00:20:45 -0400 > > > What's the intent here ? > > This stuff is great, do you have a script that looks for this false > indentation pattern? Coverity. I'm doing daily builds with it now, in the hope of trying to catch things faster, but there's a *ton* of old stuff in there like this that needs sorting through, because it seems to have strange of notions of what a 'new' bug is. Dave -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-09-05 at 00:43 -0400, Dave Jones wrote: > On Thu, Sep 05, 2013 at 12:39:07AM -0400, David Miller wrote: > > From: Dave Jones <davej@redhat.com> > > Date: Thu, 5 Sep 2013 00:20:45 -0400 > > > > > What's the intent here ? > > > > This stuff is great, do you have a script that looks for this false > > indentation pattern? > > Coverity. Good stuff. > I'm doing daily builds with it now, in the hope of trying > to catch things faster, but there's a *ton* of old stuff in there > like this that needs sorting through, because it seems to have strange > of notions of what a 'new' bug is. I think scripts/coccinelle/misc/ifcol.cocci does something similar. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/05/2013 06:43 AM, Dave Jones wrote: > On Thu, Sep 05, 2013 at 12:39:07AM -0400, David Miller wrote: > > From: Dave Jones <davej@redhat.com> > > Date: Thu, 5 Sep 2013 00:20:45 -0400 > > > > > What's the intent here ? > > > > This stuff is great, do you have a script that looks for this false > > indentation pattern? > > Coverity. I'm doing daily builds with it now, in the hope of trying > to catch things faster, but there's a *ton* of old stuff in there > like this that needs sorting through, because it seems to have strange > of notions of what a 'new' bug is. Right, it seems 'new' is just newly *detected*, not necessarily newly introduced in recent code. > Dave -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 5, 2013 at 12:43 AM, Joe Perches <joe@perches.com> wrote: > (Adding Yuchung Cheng and Neal Cardwell as the > author and acker of the patch) > > On Thu, 2013-09-05 at 00:20 -0400, Dave Jones wrote: >> What's the intent here ? >> >> This ? > > I think the first is most likely. Yes, exactly. The first version makes more sense. We only need to check thin_dupack and potentially disable early retransmit if the setsockopt successfully changes thin_dupack. neal >> >> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >> index b2f6c74..95544e4 100644 >> --- a/net/ipv4/tcp.c >> +++ b/net/ipv4/tcp.c >> @@ -2454,10 +2454,11 @@ static int do_tcp_setsockopt(struct sock *sk, int level, >> case TCP_THIN_DUPACK: >> if (val < 0 || val > 1) >> err = -EINVAL; >> - else >> + else { >> tp->thin_dupack = val; >> if (tp->thin_dupack) >> tcp_disable_early_retrans(tp); >> + } >> break; >> >> case TCP_REPAIR: >> >> Or this ... >> >> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >> index b2f6c74..187c5a4 100644 >> --- a/net/ipv4/tcp.c >> +++ b/net/ipv4/tcp.c >> @@ -2456,8 +2456,9 @@ static int do_tcp_setsockopt(struct sock *sk, int level, >> err = -EINVAL; >> else >> tp->thin_dupack = val; >> - if (tp->thin_dupack) >> - tcp_disable_early_retrans(tp); >> + >> + if (tp->thin_dupack) >> + tcp_disable_early_retrans(tp); >> break; >> >> case TCP_REPAIR: >> >> >> I'll submit the right patch in the right form once I know what was intended. >> >> The former seems more 'correct' to me, but I'm unsure if that could break something. >> >> Dave >> >> -- >> To unsubscribe from this list: send the line "unsubscribe netdev" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 5, 2013 at 10:24 AM, Neal Cardwell <ncardwell@google.com> wrote: > On Thu, Sep 5, 2013 at 12:43 AM, Joe Perches <joe@perches.com> wrote: >> (Adding Yuchung Cheng and Neal Cardwell as the >> author and acker of the patch) >> >> On Thu, 2013-09-05 at 00:20 -0400, Dave Jones wrote: >>> What's the intent here ? >>> >>> This ? >> >> I think the first is most likely. > > Yes, exactly. The first version makes more sense. We only need to > check thin_dupack and potentially disable early retransmit if the > setsockopt successfully changes thin_dupack. ack. first version is the intended one. thanks. > > neal > > >>> >>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >>> index b2f6c74..95544e4 100644 >>> --- a/net/ipv4/tcp.c >>> +++ b/net/ipv4/tcp.c >>> @@ -2454,10 +2454,11 @@ static int do_tcp_setsockopt(struct sock *sk, int level, >>> case TCP_THIN_DUPACK: >>> if (val < 0 || val > 1) >>> err = -EINVAL; >>> - else >>> + else { >>> tp->thin_dupack = val; >>> if (tp->thin_dupack) >>> tcp_disable_early_retrans(tp); >>> + } >>> break; >>> >>> case TCP_REPAIR: >>> >>> Or this ... >>> >>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >>> index b2f6c74..187c5a4 100644 >>> --- a/net/ipv4/tcp.c >>> +++ b/net/ipv4/tcp.c >>> @@ -2456,8 +2456,9 @@ static int do_tcp_setsockopt(struct sock *sk, int level, >>> err = -EINVAL; >>> else >>> tp->thin_dupack = val; >>> - if (tp->thin_dupack) >>> - tcp_disable_early_retrans(tp); >>> + >>> + if (tp->thin_dupack) >>> + tcp_disable_early_retrans(tp); >>> break; >>> >>> case TCP_REPAIR: >>> >>> >>> I'll submit the right patch in the right form once I know what was intended. >>> >>> The former seems more 'correct' to me, but I'm unsure if that could break something. >>> >>> Dave >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe netdev" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> >> >> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index b2f6c74..95544e4 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -2454,10 +2454,11 @@ static int do_tcp_setsockopt(struct sock *sk, int level, case TCP_THIN_DUPACK: if (val < 0 || val > 1) err = -EINVAL; - else + else { tp->thin_dupack = val; if (tp->thin_dupack) tcp_disable_early_retrans(tp); + } break; case TCP_REPAIR: Or this ... diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index b2f6c74..187c5a4 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -2456,8 +2456,9 @@ static int do_tcp_setsockopt(struct sock *sk, int level, err = -EINVAL; else tp->thin_dupack = val; - if (tp->thin_dupack) - tcp_disable_early_retrans(tp); + + if (tp->thin_dupack) + tcp_disable_early_retrans(tp); break; case TCP_REPAIR: