[ovs-dev] [PATCH] ovn: remove the dead code.

[nickcooper-zhangtonghao]

I apologize for this mistake.

> On Jul 25, 2016, at 2:40 AM, Ryan Moats <rmoats at us.ibm.com> wrote:
> 
> Ben Pfaff <blp at ovn.org> wrote on 07/24/2016 12:56:29 PM:
> 
> > From: Ben Pfaff <blp at ovn.org>
> > To: Ryan Moats/Omaha/IBM at IBMUS
> > Cc: Numan Siddique <nusiddiq at redhat.com>, ovs dev 
> > <dev at openvswitch.org>, nickcooper-zhangtonghao <nickcooper-
> > zhangtonghao at opencloud.tech>
> > Date: 07/24/2016 12:56 PM
> > Subject: Re: [ovs-dev] [PATCH] ovn: remove the dead code.
> > 
> > On Sun, Jul 24, 2016 at 12:04:43PM -0500, Ryan Moats wrote:
> > > Numan Siddique <nusiddiq at redhat.com> wrote on 07/24/2016 11:21:07 AM:
> > > 
> > > > From: Numan Siddique <nusiddiq at redhat.com>
> > > > To: Ryan Moats/Omaha/IBM at IBMUS
> > > > Cc: nickcooper-zhangtonghao <nickcooper-
> > > > zhangtonghao at opencloud.tech>, ovs dev <dev at openvswitch.org>
> > > > Date: 07/24/2016 11:21 AM
> > > > Subject: Re: [ovs-dev] [PATCH] ovn: remove the dead code.
> > > >
> > > > On Sun, Jul 24, 2016 at 6:26 AM, Ryan Moats <rmoats at us.ibm.com> wrote:
> > > > "dev" <dev-bounces at openvswitch.org> wrote on 07/21/2016 05:36:13 AM:
> > > >
> > > > > From: nickcooper-zhangtonghao <nickcooper-zhangtonghao at opencloud.tech>
> > > > > To: dev at openvswitch.org
> > > > > Cc: nickcooper-zhangtonghao <nickcooper-zhangtonghao at opencloud.tech>
> > > > > Date: 07/21/2016 05:36 AM
> > > > > Subject: [ovs-dev] [PATCH] ovn: remove the dead code.
> > > > > Sent by: "dev" <dev-bounces at openvswitch.org>
> > > > >
> > > > > It is not necessary to assign 0 to ip_csum.
> > > > >
> > > > > Signed-off-by: nickcooper-zhangtonghao <nickcooper-
> > > > > zhangtonghao at opencloud.tech>
> > > > > ---
> > > > >  ovn/controller/pinctrl.c | 1 -
> > > > >  1 file changed, 1 deletion(-)
> > > > >
> > > > > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> > > > > index 7893872..b35576a 100644
> > > > > --- a/ovn/controller/pinctrl.c
> > > > > +++ b/ovn/controller/pinctrl.c
> > > > > @@ -349,7 +349,6 @@ pinctrl_handle_put_dhcp_opts(
> > > > >      struct ip_header *out_ip = dp_packet_l3(&pkt_out);
> > > > >      out_ip->ip_tot_len = htons(pkt_out.l4_ofs - pkt_out.l3_ofs +
> > > > > new_l4_size);
> > > > >      udp->udp_csum = 0;
> > > > > -    out_ip->ip_csum = 0;
> > > >
> > > > ​This is not a dead code. It is required. The reason it is required
> > > > is because the DHCPv4 response packet is constructed by copying the
> > > > DHCPv4 request packet and then adding the dhcp options. So the
> > > > ip_csum needs to be set to 0 before calling "csum", otherwise,
> > > > csum() will calculate wrong checksum. because of which the VM will
> > > > reject the DHCPv4 response packet.
> > > >
> > > > ​You can verify yourself by applying the ovn-northd DHCP patch [1].
> > > >
> > > > ​[1] - https://patchwork.ozlabs.org/patch/651958/ <https://patchwork.ozlabs.org/patch/651958/>
> > > >
> > > > ​
> > > >
> > > > >      out_ip->ip_csum = csum(out_ip, sizeof *out_ip);
> > > > >
> > > > >      pin->packet = dp_packet_data(&pkt_out);
> > > >
> > > > lgtm...
> > > >
> > > > Acked-by: Ryan Moats <rmoats at us.ibm.com>
> > > 
> > > Ok, I'll admit this is a mistaken ack, but I think adding
> > > a comment pointing out that the line is initializing the
> > > variable for the pending csum calculation makes it crystal
> > > clear for the future.
> > 
> > Sure, that would be a welcome patch.
> 
> Such a patch now exists at
> http://patchwork.ozlabs.org/patch/652122/ <http://patchwork.ozlabs.org/patch/652122/>
> 
>