[ovs-dev] conntrack: Remove unnecessary check in process_ftp_ctl_v4

Message ID 1547804297-20969-1-git-send-email-lirongqing@baidu.com
State New
Headers show
Series
  • [ovs-dev] conntrack: Remove unnecessary check in process_ftp_ctl_v4
Related show

Commit Message

Li RongQing Jan. 18, 2019, 9:38 a.m.
It has been assured that both first and second int from ftp
command are not bigger than 255, so their combination(first
int << 8 +second int) must not bigger than 65535

Signed-off-by: Wang Li <wangli39@baidu.com>
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 lib/conntrack.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

0-day Robot Jan. 18, 2019, 10:12 a.m. | #1
Bleep bloop.  Greetings Li RongQing, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Wang Li <wangli39@baidu.com>
Lines checked: 37, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Darrell Ball Feb. 1, 2019, 8:15 a.m. | #2
On Fri, Jan 18, 2019 at 1:49 AM Li RongQing <lirongqing@baidu.com> wrote:

> It has been assured that both first and second int from ftp
> command are not bigger than 255, so their combination(first
> int << 8 +second int) must not bigger than 65535
>
> Signed-off-by: Wang Li <wangli39@baidu.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  lib/conntrack.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 6f6021a97..11a1e05bd 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -2987,12 +2987,7 @@ process_ftp_ctl_v4(struct conntrack *ct,
>          return CT_FTP_CTL_INVALID;
>      }
>
> -    uint16_t port_lo_hs = value;
> -    if (65535 - port_hs < port_lo_hs) {
> -        return CT_FTP_CTL_INVALID;
> -    }
> -
> -    port_hs |= port_lo_hs;
> +    port_hs |= value;
>

This was intentionally done to be documentative and also make it hard to
break;
this code path sees a tiny number of packets.
I am not sure there is much to gain by removing it and adding in lieu of
comments ?


>      ovs_be16 port = htons(port_hs);
>      ovs_be32 conn_ipv4_addr;
>
> --
> 2.16.2
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Li RongQing Feb. 1, 2019, 9:07 a.m. | #3
发件人: Darrell Ball [mailto:dlu998@gmail.com] 
发送时间: 2019年2月1日 16:15
收件人: Li,Rongqing <lirongqing@baidu.com>
抄送: ovs dev <dev@openvswitch.org>
主题: Re: [ovs-dev] [PATCH] conntrack: Remove unnecessary check in process_ftp_ctl_v4


>This was intentionally done to be documentative and also make it hard to break;
>this code path sees a tiny number of packets. 
>I am not sure there is much to gain by removing it and adding in lieu of comments ?
 
 
gain for packets is little, but step by step.

And  it can reduce unnessesary codes, make a newbie to easy study

Thanks

-RongQing
Darrell Ball Feb. 1, 2019, 3:25 p.m. | #4
On Fri, Feb 1, 2019 at 1:07 AM Li,Rongqing <lirongqing@baidu.com> wrote:

>
>
> 发件人: Darrell Ball [mailto:dlu998@gmail.com]
> 发送时间: 2019年2月1日 16:15
> 收件人: Li,Rongqing <lirongqing@baidu.com>
> 抄送: ovs dev <dev@openvswitch.org>
> 主题: Re: [ovs-dev] [PATCH] conntrack: Remove unnecessary check in
> process_ftp_ctl_v4
>
>
> >This was intentionally done to be documentative and also make it hard to
> break;
> >this code path sees a tiny number of packets.
> >I am not sure there is much to gain by removing it and adding in lieu of
> comments ?
>
>
> gain for packets is little, but step by step.
>
> And  it can reduce unnessesary codes, make a newbie to easy study
>

I agree; the useless range check can also be considered misleading, in
retrospect.
Dropping port_lo_hs should not be that confusing.

Can you resend the patch with the missing 'Co-authored-by' tag.
Co-authored-by: Wang Li <wangli39@baidu.com>


> Thanks
>
> -RongQing
>
>
>
Li RongQing Feb. 11, 2019, 2:36 a.m. | #5
发件人: Darrell Ball [mailto:dlu998@gmail.com]
发送时间: 2019年2月1日 23:25
收件人: Li,Rongqing <lirongqing@baidu.com>
抄送: ovs dev <dev@openvswitch.org>
主题: Re: [ovs-dev] [PATCH] conntrack: Remove unnecessary check in process_ftp_ctl_v4



On Fri, Feb 1, 2019 at 1:07 AM Li,Rongqing <lirongqing@baidu.com<mailto:lirongqing@baidu.com>> wrote:


发件人: Darrell Ball [mailto:dlu998@gmail.com<mailto:dlu998@gmail.com>]
发送时间: 2019年2月1日 16:15
收件人: Li,Rongqing <lirongqing@baidu.com<mailto:lirongqing@baidu.com>>
抄送: ovs dev <dev@openvswitch.org<mailto:dev@openvswitch.org>>
主题: Re: [ovs-dev] [PATCH] conntrack: Remove unnecessary check in process_ftp_ctl_v4


>This was intentionally done to be documentative and also make it hard to break;
>this code path sees a tiny number of packets.
>I am not sure there is much to gain by removing it and adding in lieu of comments ?


gain for packets is little, but step by step.

And  it can reduce unnessesary codes, make a newbie to easy study

I agree; the useless range check can also be considered misleading, in retrospect.
Dropping port_lo_hs should not be that confusing.

Can you resend the patch with the missing 'Co-authored-by' tag.
Co-authored-by: Wang Li <wangli39@baidu.com<mailto:wangli39@baidu.com>>



OK ,thanks

-RongQing

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 6f6021a97..11a1e05bd 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -2987,12 +2987,7 @@  process_ftp_ctl_v4(struct conntrack *ct,
         return CT_FTP_CTL_INVALID;
     }
 
-    uint16_t port_lo_hs = value;
-    if (65535 - port_hs < port_lo_hs) {
-        return CT_FTP_CTL_INVALID;
-    }
-
-    port_hs |= port_lo_hs;
+    port_hs |= value;
     ovs_be16 port = htons(port_hs);
     ovs_be32 conn_ipv4_addr;