* [rfc] suspicious indentation in do_tcp_setsockopt
@ 2013-09-05 4:20 Dave Jones
2013-09-05 4:39 ` David Miller
2013-09-05 4:43 ` Joe Perches
0 siblings, 2 replies; 8+ messages in thread
From: Dave Jones @ 2013-09-05 4:20 UTC (permalink / raw)
To: netdev
What's the intent here ?
This ?
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [rfc] suspicious indentation in do_tcp_setsockopt
2013-09-05 4:20 [rfc] suspicious indentation in do_tcp_setsockopt Dave Jones
@ 2013-09-05 4:39 ` David Miller
2013-09-05 4:43 ` Dave Jones
2013-09-05 4:43 ` Joe Perches
1 sibling, 1 reply; 8+ messages in thread
From: David Miller @ 2013-09-05 4:39 UTC (permalink / raw)
To: davej; +Cc: netdev
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?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [rfc] suspicious indentation in do_tcp_setsockopt
2013-09-05 4:39 ` David Miller
@ 2013-09-05 4:43 ` Dave Jones
2013-09-05 4:53 ` Joe Perches
2013-09-05 7:42 ` Daniel Borkmann
0 siblings, 2 replies; 8+ messages in thread
From: Dave Jones @ 2013-09-05 4:43 UTC (permalink / raw)
To: David Miller; +Cc: netdev
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [rfc] suspicious indentation in do_tcp_setsockopt
2013-09-05 4:43 ` Dave Jones
@ 2013-09-05 4:53 ` Joe Perches
2013-09-05 7:42 ` Daniel Borkmann
1 sibling, 0 replies; 8+ messages in thread
From: Joe Perches @ 2013-09-05 4:53 UTC (permalink / raw)
To: Dave Jones; +Cc: David Miller, netdev, Julia Lawall
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [rfc] suspicious indentation in do_tcp_setsockopt
2013-09-05 4:43 ` Dave Jones
2013-09-05 4:53 ` Joe Perches
@ 2013-09-05 7:42 ` Daniel Borkmann
1 sibling, 0 replies; 8+ messages in thread
From: Daniel Borkmann @ 2013-09-05 7:42 UTC (permalink / raw)
To: Dave Jones; +Cc: David Miller, netdev
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [rfc] suspicious indentation in do_tcp_setsockopt
2013-09-05 4:20 [rfc] suspicious indentation in do_tcp_setsockopt Dave Jones
2013-09-05 4:39 ` David Miller
@ 2013-09-05 4:43 ` Joe Perches
2013-09-05 17:24 ` Neal Cardwell
1 sibling, 1 reply; 8+ messages in thread
From: Joe Perches @ 2013-09-05 4:43 UTC (permalink / raw)
To: Dave Jones, Yuchung Cheng; +Cc: netdev, Neal Cardwell
(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
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [rfc] suspicious indentation in do_tcp_setsockopt
2013-09-05 4:43 ` Joe Perches
@ 2013-09-05 17:24 ` Neal Cardwell
2013-09-05 17:34 ` Yuchung Cheng
0 siblings, 1 reply; 8+ messages in thread
From: Neal Cardwell @ 2013-09-05 17:24 UTC (permalink / raw)
To: Joe Perches; +Cc: Dave Jones, Yuchung Cheng, Netdev
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
>>
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [rfc] suspicious indentation in do_tcp_setsockopt
2013-09-05 17:24 ` Neal Cardwell
@ 2013-09-05 17:34 ` Yuchung Cheng
0 siblings, 0 replies; 8+ messages in thread
From: Yuchung Cheng @ 2013-09-05 17:34 UTC (permalink / raw)
To: Neal Cardwell; +Cc: Joe Perches, Dave Jones, Netdev
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
>>>
>>
>>
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-09-05 17:34 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-05 4:20 [rfc] suspicious indentation in do_tcp_setsockopt Dave Jones
2013-09-05 4:39 ` David Miller
2013-09-05 4:43 ` Dave Jones
2013-09-05 4:53 ` Joe Perches
2013-09-05 7:42 ` Daniel Borkmann
2013-09-05 4:43 ` Joe Perches
2013-09-05 17:24 ` Neal Cardwell
2013-09-05 17:34 ` Yuchung Cheng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).