Numan Siddique <[email protected]> wrote on 07/24/2016 11:21:07 AM:
> From: Numan Siddique <[email protected]> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: nickcooper-zhangtonghao <nickcooper- > [email protected]>, ovs dev <[email protected]> > 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 <[email protected]> wrote: > "dev" <[email protected]> wrote on 07/21/2016 05:36:13 AM: > > > From: nickcooper-zhangtonghao <[email protected]> > > To: [email protected] > > Cc: nickcooper-zhangtonghao <[email protected]> > > Date: 07/21/2016 05:36 AM > > Subject: [ovs-dev] [PATCH] ovn: remove the dead code. > > Sent by: "dev" <[email protected]> > > > > It is not necessary to assign 0 to ip_csum. > > > > Signed-off-by: nickcooper-zhangtonghao <nickcooper- > > [email protected]> > > --- > > 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 <[email protected]> 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. Ryan _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
