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

[Ryan Moats]

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/
> > >
> > > ​
> > >
> > > >      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/