Addressed code review inputs

(On behalf of Prabhakar)
Closes-Bug: #1398625

Change-Id: I46a313dc3b9b527b38078fc348051ad96b43f37f

dp-core/vr_flow.c

@@ -794,17 +794,17 @@ __vr_flow_forward(flow_result_t result, struct vr_packet *pkt,
 }
 
 static flow_result_t
-__vr_flow_lookup(struct vrouter *router, struct vr_packet *pkt,
+vr_do_flow_lookup(struct vrouter *router, struct vr_packet *pkt,
                 struct vr_forwarding_md *fmd)
 {
     flow_result_t result = FLOW_FORWARD;
 
-    /* Flow processig is only for untagged unicast IP packets */
+    /* Flow processing is only for untagged unicast IP packets */
     if (pkt->vp_type == VP_TYPE_IP)
         result = vr_inet_flow_lookup(router, pkt, fmd);
     else if (pkt->vp_type == VP_TYPE_IP6)
         result = vr_inet6_flow_lookup(router, pkt, fmd);
-    
+
     return result;
 }
 
@@ -816,7 +816,7 @@ vr_flow_forward(struct vrouter *router, struct vr_packet *pkt,
 
     if ((!(pkt->vp_flags & VP_FLAG_MULTICAST))
         && ((fmd->fmd_vlan == VLAN_ID_INVALID) || vif_is_service(pkt->vp_if)))
-        result = __vr_flow_lookup(router, pkt, fmd);
+        result = vr_do_flow_lookup(router, pkt, fmd);
 
     return __vr_flow_forward(result, pkt, fmd);
 }
@@ -1013,14 +1013,21 @@ vr_add_flow_req(vr_flow_req *req, unsigned int *fe_index)
     struct vr_flow key;
     struct vr_flow_entry *fe;
 
-    if (req->fr_family == AF_INET6) {
+    switch (req->fr_family) {
+    case  AF_INET6:
         type = VP_TYPE_IP6;
         vr_inet6_fill_flow(&key, req->fr_flow_nh_id, req->fr_flow_ip,
             req->fr_flow_proto, req->fr_flow_sport, req->fr_flow_dport);
-    } else {
+        break;
+
+    case  AF_INET:
         type = VP_TYPE_IP;
         vr_inet_fill_flow(&key, req->fr_flow_nh_id, req->fr_flow_ip,
             req->fr_flow_proto, req->fr_flow_sport, req->fr_flow_dport);
+        break;
+
+    default:
+        return NULL;
     }
 
     if (req->fr_action == VR_FLOW_ACTION_HOLD)
@@ -1046,7 +1053,7 @@ vr_flow_req_is_invalid(struct vrouter *router, vr_flow_req *req,
     if (fe) {
         if ((fe->fe_type == VP_TYPE_IP) || (fe->fe_type == VP_TYPE_IP6)) {
             if (memcmp(req->fr_flow_ip, fe->fe_key.flow_ip, 
-                       2 * VR_FLOW_IP_ADDR_SIZE(fe->fe_type)) ||
+                       2 * VR_IP_ADDR_SIZE(fe->fe_type)) ||
                     (unsigned short)req->fr_flow_sport != fe->fe_key.flow_sport ||
                     (unsigned short)req->fr_flow_dport != fe->fe_key.flow_dport||
                     (unsigned short)req->fr_flow_nh_id != fe->fe_key.flow_nh_id ||
@@ -1141,8 +1148,8 @@ vr_flow_udp_src_port (struct vrouter *router, struct vr_flow_entry *fe)
 
     hash_key[0] = fe->fe_vrf;
     hash_key[1] = (fe->fe_key.flow_sport << 16) | fe->fe_key.flow_dport;
-    memcpy(&hash_key[2], fe->fe_key.flow_ip, 2*VR_FLOW_IP_ADDR_SIZE(fe->fe_type));
-    hash_len = VR_FLOW_KEY_SIZE(fe->fe_type);
+    memcpy(&hash_key[2], fe->fe_key.flow_ip, 2 * VR_IP_ADDR_SIZE(fe->fe_type));
+    hash_len = VR_FLOW_HASH_SIZE(fe->fe_type);
 
     hashval = jhash(hash_key, hash_len, vr_hashrnd);
     port_range = VR_MUDP_PORT_RANGE_END - VR_MUDP_PORT_RANGE_START;

dp-core/vr_nexthop.c

@@ -1357,7 +1357,7 @@ nh_mpls_udp_tunnel(struct vr_packet *pkt, struct vr_nexthop *nh,
 
     /*
      * The UDP source port is a hash of the inner IP src/dst address and
-     * vrf. 
+     * vrf.
      */
     if ((!fmd->fmd_udp_src_port)  && vr_get_udp_src_port) {
         udp_src_port = vr_get_udp_src_port(pkt, fmd, fmd->fmd_dvrf);

dp-core/vr_proto_ip.c

@@ -789,14 +789,14 @@ vr_inet_fill_flow(struct vr_flow *flow_p, unsigned short nh_id,
         unsigned char *ip, uint8_t proto, uint16_t sport, uint16_t dport)
 {
     /* copy both source and destinations */
-    memcpy(flow_p->flow_ip, ip, 2*VR_FLOW_IPV4_ADDR_SIZE);
+    memcpy(flow_p->flow_ip, ip, 2 * VR_IP_ADDRESS_LEN);
     flow_p->flow4_proto = proto;
     flow_p->flow4_nh_id = nh_id;
     flow_p->flow4_sport = sport;
     flow_p->flow4_dport = dport;
     flow_p->flow4_family = AF_INET;
 
-    flow_p->flow_key_len = VR_FLOW_IPV4_KEY_SIZE; 
+    flow_p->flow_key_len = VR_FLOW_IPV4_HASH_SIZE;
 
     return;
 }

dp-core/vr_proto_ip6.c

@@ -117,18 +117,18 @@ vr_icmp6_input(struct vrouter *router, struct vr_packet *pkt,
 }
 
 void
-vr_inet6_fill_flow(struct vr_flow *flow_p, unsigned short nh_id, 
+vr_inet6_fill_flow(struct vr_flow *flow_p, unsigned short nh_id,
         unsigned char *ip, uint8_t proto, uint16_t sport, uint16_t dport)
 {
     /* copy both source and destinations */
-    memcpy(flow_p->flow_ip, ip, 2*VR_FLOW_IPV6_ADDR_SIZE);
+    memcpy(flow_p->flow_ip, ip, 2 * VR_IP6_ADDRESS_LEN);
     flow_p->flow6_proto = proto;
     flow_p->flow6_nh_id = nh_id;
     flow_p->flow6_sport = sport;
     flow_p->flow6_dport = dport;
     flow_p->flow6_family = AF_INET6;
 
-    flow_p->flow_key_len = VR_FLOW_IPV6_KEY_SIZE; 
+    flow_p->flow_key_len = VR_FLOW_IPV6_HASH_SIZE;
 
     return;
 }
@@ -143,10 +143,6 @@ vr_inet6_form_flow(struct vrouter *router, unsigned short vrf,
 
     struct vr_icmp *icmph;
 
-    /* Skip flow lookup for V6 frags */
-    if (ip6->ip6_nxt == VR_IP6_PROTO_FRAG) 
-        return 1;
-
     t_hdr = (unsigned short *)((char *)ip6 + sizeof(struct vr_ip6));
     if (ip6->ip6_nxt == VR_IP_PROTO_ICMP6) {
         icmph = (struct vr_icmp *)t_hdr;
@@ -186,6 +182,10 @@ vr_inet6_flow_lookup(struct vrouter *router, struct vr_packet *pkt,
     if (pkt->vp_flags & VP_FLAG_FLOW_SET)
         return FLOW_FORWARD;
 
+    /* Skip flow lookup for V6 frags */
+    if (ip6->ip6_nxt == VR_IP6_PROTO_FRAG)
+        return FLOW_FORWARD;
+
     ret = vr_inet6_form_flow(router, fmd->fmd_dvrf, pkt, fmd->fmd_vlan, ip6, flow_p);
     if (ret < 0)
         return FLOW_CONSUMED;
@@ -201,7 +201,6 @@ vr_inet6_flow_lookup(struct vrouter *router, struct vr_packet *pkt,
 
 
     if (lookup) {
-
         return vr_flow_lookup(router, flow_p, pkt, fmd);
     }
 

include/vr_flow.h

@@ -62,17 +62,13 @@ typedef enum {
 #define VR_FLOW_DR_REVERSE_SG           0x11
 #define VR_FLOW_DR_REVERSE_OUT_SG       0x12
 
-#define VR_FLOW_IPV6_ADDR_SIZE          16
-#define VR_FLOW_IPV4_ADDR_SIZE          4
-#define VR_FLOW_IP_ADDR_SIZE(type) \
-        ((type == VP_TYPE_IP6) ? VR_FLOW_IPV6_ADDR_SIZE \
-                               : VR_FLOW_IPV4_ADDR_SIZE)
-
-#define VR_FLOW_IPV6_KEY_SIZE           40
-#define VR_FLOW_IPV4_KEY_SIZE           16
-#define VR_FLOW_KEY_SIZE(type) \
-        ((type == VP_TYPE_IP6) ? VR_FLOW_IPV6_KEY_SIZE \
-                               : VR_FLOW_IPV4_KEY_SIZE)
+#define VR_FLOW_IPV6_HASH_SIZE           40
+#define VR_FLOW_IPV4_HASH_SIZE           16
+#define VR_FLOW_HASH_SIZE(type) \
+        ((type == VP_TYPE_IP6) ? VR_FLOW_IPV6_HASH_SIZE \
+                               : VR_FLOW_IPV4_HASH_SIZE)
+
+#define VR_IP6_ADDRESS_LEN               16
 
 #define VR_FLOW_FAMILY(type) \
         ((type == VP_TYPE_IP6) ? AF_INET6 \
@@ -85,7 +81,7 @@ struct vr_common_flow{
     unsigned short ip_nh_id;
     unsigned short ip_sport;
     unsigned short ip_dport;
-    unsigned char  ip_addr[2*VR_FLOW_IPV6_ADDR_SIZE];
+    unsigned char  ip_addr[2 * VR_IP6_ADDRESS_LEN];
 } __attribute__((packed));
 
 
@@ -105,8 +101,8 @@ struct vr_inet6_flow {
     unsigned short ip6_nh_id;
     unsigned short ip6_sport;
     unsigned short ip6_dport;
-    unsigned char  ip6_sip[VR_FLOW_IPV6_ADDR_SIZE];
-    unsigned char  ip6_dip[VR_FLOW_IPV6_ADDR_SIZE];
+    unsigned char  ip6_sip[VR_IP6_ADDRESS_LEN];
+    unsigned char  ip6_dip[VR_IP6_ADDRESS_LEN];
 } __attribute__((packed));
 
 struct vr_flow {
@@ -115,7 +111,7 @@ struct vr_flow {
         struct vr_inet_flow ip4_key;
         struct vr_inet6_flow ip6_key;
     } key_u;
-    uint32_t   vr_flow_keylen;    
+    uint8_t   vr_flow_keylen;
 } __attribute__((packed));
 
 #define flow_key_len   vr_flow_keylen
@@ -140,7 +136,7 @@ struct vr_flow {
 #define flow6_nh_id    key_u.ip6_key.ip6_nh_id
 #define flow6_proto    key_u.ip6_key.ip6_proto
 
-/* 
+/*
  * Limit the number of outstanding flows in hold state. The flow rate can
  * be much more than what agent can handle. In such cases, to make sure that
  *
@@ -180,7 +176,7 @@ struct vr_flow_table_info {
     uint32_t vfti_hold_count[0];
 };
 
-/* 
+/*
  * flow bytes and packets are of same width. this should be
  * ok since agent really has to take care of overflows. this
  * is also better probably because processor does not have to
@@ -214,6 +210,7 @@ struct vr_flow_queue {
 
 struct vr_dummy_flow_entry {
     struct vr_flow fe_key;
+    uint8_t vr_flow_key_padding[7];
     struct vr_flow_queue *fe_hold_list;
     unsigned short fe_action;
     unsigned short fe_flags;
@@ -235,6 +232,7 @@ struct vr_dummy_flow_entry {
 /* do not change. any field positions as it might lead to incompatibility */
 struct vr_flow_entry {
     struct vr_flow fe_key;
+    uint8_t vr_flow_key_padding[7];
     struct vr_flow_queue *fe_hold_list;
     unsigned short fe_action;
     unsigned short fe_flags;
@@ -311,7 +309,7 @@ unsigned short
 vr_inet_flow_nexthop(struct vr_packet *pkt, unsigned short vlan);
 extern flow_result_t vr_inet_flow_nat(struct vr_flow_entry *,
         struct vr_packet *, struct vr_forwarding_md *);
-extern void vr_inet_fill_flow(struct vr_flow *, unsigned short, 
+extern void vr_inet_fill_flow(struct vr_flow *, unsigned short,
        unsigned char *, uint8_t, uint16_t, uint16_t);
 extern void vr_inet6_fill_flow(struct vr_flow *, unsigned short, 
        unsigned char *, uint8_t, uint16_t, uint16_t);

include/vr_packet.h

@@ -7,6 +7,7 @@
 #define __VR_PACKET_H__
 
 #include "vr_defs.h"
+#include "vr_flow.h"
 #include "vrouter.h"
 
 /* ethernet header */
@@ -359,8 +360,6 @@ struct vr_neighbor_option {
 } __attribute__((packed));
 
 
-#define VR_IP6_ADDRESS_LEN      16
-
 struct vr_ip6_pseudo {
     unsigned char ip6_src[VR_IP6_ADDRESS_LEN];
     unsigned char ip6_dst[VR_IP6_ADDRESS_LEN];
@@ -408,6 +407,10 @@ struct vr_ip6 {
 #define IS_BMCAST_IP(ip) \
             (((ntohl(ip) & MCAST_IP_MASK) == MCAST_IP) || (ip == 0xFFFFFFFF))
 
+#define VR_IP_ADDR_SIZE(type) \
+        ((type == VP_TYPE_IP6) ? VR_IP6_ADDRESS_LEN \
+                               : VR_IP_ADDRESS_LEN)
+
 static inline unsigned char
 vr_eth_proto_to_pkt_type(unsigned short eth_proto)
 {

include/vrouter.h

@@ -15,7 +15,6 @@ extern "C" {
 #include "vr_flow.h"
 #include "vr_nexthop.h"
 #include "vr_route.h"
-#include "vr_flow.h"
 #include "vr_response.h"
 #include "vr_mpls.h"
 #include "vr_index_table.h"