implement review comments

Author: mogaika

upf/flowtable.h

@@ -186,7 +186,7 @@ typedef struct flow_entry
   u8 tcp_state : 4; // TODO: needs only 3 bits?
   // should be updated in classify and based on PDR during flow creation
   u8 uplink_direction : 2;
-  // do not need to perform ipfix operations for this flow anymore
+  // do not perform ipfix operations for this flow anymore
   u8 ipfix_disabled : 1;
 
   // use macro since unsigned will not expand to ~0

upf/upf_classify.c

@@ -432,15 +432,13 @@ upf_acl_classify_return (vlib_main_t *vm, u32 teid, flow_entry_t *flow,
 // if impossible to detect
 always_inline flow_direction_t
 upf_classify_detect_flow_direction (vlib_buffer_t *b, struct rules *r,
-                                    flow_direction_t direction, bool *found)
+                                    flow_direction_t direction)
 {
   // Here we rely on the fact that interface of type ACCESS is one which
   // directed to UE
   // TODO: it should be possible to calculate and save this value per PDR
   // during session creation/modification
 
-  *found = false;
-
   u32 pdr_idx = upf_buffer_opaque (b)->gtpu.pdr_idx;
   if (pdr_idx == ~0)
     {
@@ -460,7 +458,6 @@ upf_classify_detect_flow_direction (vlib_buffer_t *b, struct rules *r,
       upf_debug ("detected uplink direction from pdr: %d matched on %v",
                  FTD_OP_SAME ^ direction,
                  pool_elt_at_index (upf_main.nwis, pdr->pdi.nwi_index)->name);
-      *found = true;
       return FTD_OP_SAME ^ direction;
     }
 
@@ -483,7 +480,6 @@ upf_classify_detect_flow_direction (vlib_buffer_t *b, struct rules *r,
         "detected uplink direction from far: %d forwarded to %v",
         FTD_OP_FLIP ^ direction,
         pool_elt_at_index (upf_main.nwis, far->forward.nwi_index)->name);
-      *found = true;
       return FTD_OP_FLIP ^ direction;
     }
 
@@ -619,11 +615,9 @@ upf_classify_fn (vlib_main_t *vm, vlib_node_runtime_t *node,
 
           if (flow->uplink_direction == FLOW_ENTRY_UPLINK_DIRECTION_UNDEFINED)
             {
-              bool found;
               flow_direction_t uplink_direction =
-                upf_classify_detect_flow_direction (b, active, direction,
-                                                    &found);
-              if (found)
+                upf_classify_detect_flow_direction (b, active, direction);
+              if (uplink_direction != FLOW_ENTRY_UPLINK_DIRECTION_UNDEFINED)
                 flow->uplink_direction = uplink_direction;
             }
 

upf/upf_ipfix.c

@@ -117,16 +117,16 @@ upf_ipfix_template_rewrite (ipfix_exporter_t *exp, flow_report_t *fr,
   fib_protocol_t fproto =
     context->key.is_ip4 ? FIB_PROTOCOL_IP4 : FIB_PROTOCOL_IP6;
   upf_ipfix_template_t *template = &upf_ipfix_templates[context->key.policy];
-  upf_ipfix_template_proto_t *ptemplate = &template->per_ip[fproto];
+  upf_ipfix_template_proto_t *template_proto = &template->per_ip[fproto];
 
-  ASSERT (ptemplate->field_count);
+  ASSERT (template_proto->field_count);
 
   /* allocate rewrite space */
 
   vec_validate_aligned (
     rewrite,
     sizeof (ip4_ipfix_template_packet_t) +
-      ptemplate->field_count * sizeof (ipfix_field_specifier_t) - 1,
+      template_proto->field_count * sizeof (ipfix_field_specifier_t) - 1,
     CLIB_CACHE_LINE_BYTES);
 
   tp = (ip4_ipfix_template_packet_t *) rewrite;
@@ -150,7 +150,7 @@ upf_ipfix_template_rewrite (ipfix_exporter_t *exp, flow_report_t *fr,
   h->domain_id = clib_host_to_net_u32 (stream->domain_id);
 
   /* Add TLVs to the template */
-  f = ptemplate->add_fields (f);
+  f = template_proto->add_fields (f);
 
   /* Back to the template packet... */
   ip = (ip4_header_t *) &tp->ip4;
@@ -173,7 +173,7 @@ upf_ipfix_template_rewrite (ipfix_exporter_t *exp, flow_report_t *fr,
              vec_len (rewrite));
   upf_debug ("n of fields: %u, hdr size %u, part hdr size %u, "
              "single field spec len %u",
-             ptemplate->field_count, sizeof (ip4_ipfix_template_packet_t),
+             template_proto->field_count, sizeof (ip4_ipfix_template_packet_t),
              sizeof (ipfix_template_packet_t),
              sizeof (ipfix_field_specifier_t));
   ASSERT ((u8 *) f - (u8 *) ip == vec_len (rewrite));