Regards _Sugesh
> -----Original Message----- > From: Joe Stringer [mailto:j...@ovn.org] > Sent: Monday, June 26, 2017 6:27 PM > To: Chandran, Sugesh <sugesh.chand...@intel.com> > Cc: ovs dev <d...@openvswitch.org>; Zoltán Balogh > <zoltan.bal...@ericsson.com> > Subject: Re: [PATCH 2/4] dpif-xlate : Adding functions to update xlate > context for the tunnel push translations. > > On 16 June 2017 at 09:45, Sugesh Chandran <sugesh.chand...@intel.com> > wrote: > > Functions to update the xlate context with relevant tunnel fields to > > continue the following translation on tunneled packets. > > > > Signed-off-by: Sugesh Chandran <sugesh.chand...@intel.com> > > Signed-off-by: Zoltán Balogh <zoltan.bal...@ericsson.com> > > Co-authored-by: Zoltán Balogh <zoltan.bal...@ericsson.com> > > --- > > Usually we don't introduce unused functions in a separate patch. It can be > difficult to understand whether introduced functions are doing the right > thing without the context of how they're called. This patch and patch #3 can > be folded into patch #4. [Sugesh] Ok. Will combine them into one. > > > ofproto/ofproto-dpif-xlate.c | 87 > > ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 87 insertions(+) > > > > diff --git a/ofproto/ofproto-dpif-xlate.c > > b/ofproto/ofproto-dpif-xlate.c index 3a2b1d3..d3a1624 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -3143,6 +3143,93 @@ tnl_send_arp_request(struct xlate_ctx *ctx, > const struct xport *out_dev, > > dp_packet_uninit(&packet); > > } > > > > +/* > > + * Copy IP data of 'flow->tunnel' into 'flow' and 'base_flow'. > > + */ > > +static void > > +propagate_tunnel_data_to_flow(struct xlate_ctx *ctx, struct eth_addr > dmac, > > + struct eth_addr smac, struct in6_addr > > s_ip6, > > + ovs_be32 s_ip, bool is_tnl_ipv6, > > + uint32_t tnl_type) { > > + struct flow *base_flow, *flow; > > + flow = &ctx->xin->flow; > > + base_flow = &ctx->base_flow; > > If 'flow' and 'base_flow' are modified in the exact same way for the first > chunk of this function, then you could refactor those bits into a single > function, then call something like > > propagate_tunnel_data_to_flow__(&ctx->xin->flow, ...); > propagate_tunnel_data_to_flow__(&ctx->base_flow, ...); > > ... then do the flow-specific or base_flow-specific modifications afterwards. > This is probably a bit shorter and less error-prone. [Sugesh] Ok, will do it in V2. > > > + > > + flow->dl_dst = dmac; > > + base_flow->dl_dst = dmac; > > + > > + flow->dl_src = smac; > > + base_flow->dl_src = smac; > > + > > + /* Set packet_type. */ > > Unnecessary comment. The following line clearly sets the packet type. [Sugesh] Will remove it. > > > + flow->packet_type = htonl(PT_ETH); > > + base_flow->packet_type = htonl(PT_ETH); > > + > > + flow->nw_dst = flow->tunnel.ip_dst; > > + flow->nw_src = flow->tunnel.ip_src; > > + flow->ipv6_dst = flow->tunnel.ipv6_dst; > > + flow->ipv6_src = flow->tunnel.ipv6_src; > > + > > + flow->nw_tos = flow->tunnel.ip_tos; > > + flow->nw_ttl = flow->tunnel.ip_ttl; > > + flow->tp_dst = flow->tunnel.tp_dst; > > + flow->tp_src = flow->tunnel.tp_src; > > + > > + base_flow->nw_dst = flow->nw_dst; > > + base_flow->nw_src = flow->nw_src; > > + base_flow->ipv6_dst = flow->ipv6_dst; > > + base_flow->ipv6_src = flow->ipv6_src; > > + > > + base_flow->nw_tos = flow->nw_tos; > > + base_flow->nw_ttl = flow->nw_ttl; > > + base_flow->tp_dst = flow->tp_dst; > > + base_flow->tp_src = flow->tp_src; > > + > > + if (is_tnl_ipv6) { > > + flow->dl_type = htons(ETH_TYPE_IPV6); > > + if (ipv6_mask_is_any(&flow->ipv6_src) > > + && !ipv6_mask_is_any(&s_ip6)) { > > + flow->ipv6_src = s_ip6; > > + base_flow->ipv6_src = s_ip6; > > + } > > + } else { > > + flow->dl_type = htons(ETH_TYPE_IP); > > + if (flow->nw_src == 0 && s_ip) { > > + flow->nw_src = s_ip; > > + base_flow->nw_src = s_ip; > > + } > > + > > + } > > + > > + switch (tnl_type) { > > Usually we use the enum type for cases like this, so we can ensure that later > modifications of the enum will generate a compile failure that prompts the > developer adding additional enums to update all of the switch statements. [Sugesh] Will use the enum type for it in v2. > > > + case OVS_VPORT_TYPE_GRE: > > + flow->nw_proto = IPPROTO_GRE; > > + break; > > + case OVS_VPORT_TYPE_VXLAN: > > + case OVS_VPORT_TYPE_GENEVE: > > + flow->nw_proto = IPPROTO_UDP; > > + break; > > + case OVS_VPORT_TYPE_LISP: > > + case OVS_VPORT_TYPE_STT: > > + default: > > + OVS_NOT_REACHED(); > > + break; > > + } > > + > > + base_flow->nw_proto = ctx->xin->flow.nw_proto; > > + > > +} > > + > > +static void > > +copy_flow(struct flow *dst, const struct flow *src) { > > + if (!dst || !src) { > > + return; > > + } > > + memcpy(dst, src, sizeof *dst); > > +} > > Looking at the later usage of copy_flow, every caller provides &foo pointers > as arguments so the NULL check is never hit. It seems like this function can > be dropped and any usage of it can be replaced with "backup_flow = flow" or > this memcpy. [Sugesh] ok, will remove the function and use the memcpy in the caller. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
