| Message ID | 1547804297-20969-1-git-send-email-lirongqing@baidu.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] conntrack: Remove unnecessary check in process_ftp_ctl_v4 | expand |
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
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 >
发件人: 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
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 > > >
发件人: 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
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;