[ovs-dev] [PATCH 0/6] Generic Encap & Decap based NSH implementation

[Jan Scheurich]

Hi Yi,

The following incremental clarifies and cleans up the error handling during translation of encap and decap actions for NSH:

-   Rephrased the xlate_report_debug() messages when dropping packets during
    translation of encap/decap actions.

-   Encap and decap translation error handling should be complete. Removed any
    TODO from the comments.

-   As all encap/decap errors are checked during translation, the function
    commit_packet_type_change() should throw an abort when it encounters
    an unexpected combination of old and new packet types.

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 02c9859..57c6ec8 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -6765,24 +6765,31 @@ commit_packet_type_change(const struct flow *flow,
             /* TODO: Do we need to copy all the other fields too? */
             break;
         default:
-            /* Only the above protocols are supported for encap. The check
-             * is done at action decoding. */
+            /* Only the above protocols are supported for encap.
+             * The check is done at action translation. */
             OVS_NOT_REACHED();
         }
     } else {
-        /* This is a decap case. It can only happen after translation of a
-         * decap action. */
+        /* This is an explicit or implicit decap case. */
         if (pt_ns(flow->packet_type) == OFPHTN_ETHERTYPE &&
             base_flow->packet_type == htonl(PT_ETH)) {
-            /* pop_eth */
+            /* Generate pop_eth and continue without recirculation. */
             odp_put_pop_eth_action(odp_actions);
             base_flow->packet_type = flow->packet_type;
             base_flow->dl_src = eth_addr_zero;
             base_flow->dl_dst = eth_addr_zero;
-        } else if (ntohl(base_flow->packet_type) == PT_NSH) {
+        } else {
+            /* All other decap cases require recirculation.
+             * No need to update the base flow here. */
+            switch (ntohl(base_flow->packet_type)) {
+            case PT_NSH:
+                /* decap_nsh. */
                 odp_put_decap_nsh_action(odp_actions);
-                base_flow->packet_type = flow->packet_type;
-                memcpy(&base_flow->nsh, &flow->nsh, sizeof(base_flow->nsh));
+                break;
+            default:
+                /* Checks are done during translation. */
+                OVS_NOT_REACHED();
+            }
         }
     }

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 41a2dd1..9ebfd30 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5459,10 +5459,10 @@ rewrite_flow_encap_ethernet(struct xlate_ctx *ctx,
         flow->dl_dst = eth_addr_zero;
         flow->dl_type = ethertype;
     } else {
+        /* Error handling: drop packet. */
         xlate_report_debug(ctx, OFT_ACTION,
-                           "encap(ethernet) unsupported for packet type "
-                           "ethernet");
-        /* TODO: Error handling: drop packet. */
+                           "Dropping packet as encap(ethernet) is not "
+                           "supported for packet type ethernet.");
         ctx->error = 1;
     }
 }
@@ -5532,9 +5532,11 @@ rewrite_flow_encap_nsh(struct xlate_ctx *ctx,
             np = NSH_P_NSH;
             break;
         default:
+            /* Error handling: drop packet. */
             xlate_report_debug(ctx, OFT_ACTION,
-                               "encap(nsh) for unsupported packet type %x",
-                               ntohl(packet_type));
+                               "Dropping packet as encap(nsh) is not "
+                               "supported for packet type (%d,0x%x)",
+                               pt_ns(packet_type), pt_ns_type(packet_type));
             ctx->error = 1;
             return;
     }
@@ -5584,6 +5586,7 @@ xlate_generic_encap_action(struct xlate_ctx *ctx,
             rewrite_flow_encap_nsh(ctx, encap, flow, wc);
             break;
         default:
+            /* New packet type was checked during decoding. */
             OVS_NOT_REACHED();
             break;
     }
@@ -5632,10 +5635,10 @@ xlate_generic_decap_action(struct xlate_ctx *ctx,
                 flow->packet_type = htonl(PT_NSH);
                 break;
             default:
+                /* Error handling: drop packet. */
                 xlate_report_debug(ctx, OFT_ACTION,
-                                   "decap() NSH Next Protocol not supported"
-                                   " %x", flow->nsh.np);
-                /* TODO: Error handling: drop packet. */
+                                   "Dropping packet as NSH next protocol %d "
+                                   "is not supported", flow->nsh.np);
                 ctx->error = 1;
                 return false;
                 break;
@@ -5644,10 +5647,12 @@ xlate_generic_decap_action(struct xlate_ctx *ctx,
             /* Trigger recirculation. */
             return true;
         default:
-            xlate_report_debug(ctx, OFT_ACTION,
-                               "decap() for unsupported packet type %x",
-                               ntohl(flow->packet_type));
-            /* TODO: Error handling: drop packet. */
+            /* Error handling: drop packet. */
+            xlate_report_debug(
+                    ctx, OFT_ACTION,
+                    "Dropping packet as the decap() does not support "
+                    "packet type (%d,0x%x)",
+                    pt_ns(flow->packet_type), pt_ns_type(flow->packet_type));
             ctx->error = 1;
             return false;
     }