[ovs-dev] [PATCH v2 5/5] datapath-windows: Add support for OVS_KEY_ATTR_TCP set action

[Sairam Venugopal]

Please see my comment inline:




On 1/6/17, 11:33 AM, "ovs-dev-bounces at openvswitch.org on behalf of Alin Serdean" <ovs-dev-bounces at openvswitch.org on behalf of aserdean at cloudbasesolutions.com> wrote:

>This patch adds support for set action with OVS_KEY_ATTR_TCP attribute
>(change TCP source or destination port).
>
>If the source or destination TCP port was changed, update the TCP checksum.
>
>A sample flow can look like the following:
>set(tcp(src=80,dst=443))
>
>Signed-off-by: Alin Gabriel Serdean <aserdean at cloudbasesolutions.com>
>---
>v2: set action for tcp attribute changed to function: OvsUpdateTcpPorts
>---
> datapath-windows/ovsext/Actions.c | 48 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
>diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c
>index a09afdd..349592f 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -1426,6 +1426,49 @@ OvsUpdateUdpPorts(OvsForwardingContext *ovsFwdCtx,
> 
> /*
>  *----------------------------------------------------------------------------
>+ * OvsUpdateTcpPorts --
>+ *      Updates the TCP source or destination port in ovsFwdCtx.curNbl inline
>+ *      based on the specified key.
>+ *----------------------------------------------------------------------------
>+ */
>+static __inline NDIS_STATUS
>+OvsUpdateTcpPorts(OvsForwardingContext *ovsFwdCtx,
>+                  const struct ovs_key_tcp *tcpAttr)
>+{
>+    PUINT8 bufferStart;
>+    OVS_PACKET_HDR_INFO *layers = &ovsFwdCtx->layers;
>+    TCPHdr *tcpHdr = NULL;
>+
>+    ASSERT(layers->value != 0);
>+
>+    if (!layers->isUdp) {

Sai: This is incorrect. Should be checking for !layers->isTcp. This will end up failing for tcp set action.
Can you consolidate TCP/UDP actions to call into 1 function and use switch instead. Most of the code has been copied over from UDP.

>+        ovsActionStats.noCopiedNbl++;
>+        return NDIS_STATUS_FAILURE;
>+    }
>+
>+    bufferStart = OvsGetHeaderBySize(ovsFwdCtx, layers->l7Offset);
>+    if (!bufferStart) {
>+        return NDIS_STATUS_RESOURCES;
>+    }
>+
>+    tcpHdr = (TCPHdr *)(bufferStart + layers->l4Offset);
>+
>+    if (tcpHdr->source != tcpAttr->tcp_src) {
>+        tcpHdr->check = ChecksumUpdate16(tcpHdr->check, tcpHdr->source,
>+                                         tcpAttr->tcp_src);
>+        tcpHdr->source = tcpAttr->tcp_src;
>+    }
>+    if (tcpHdr->dest != tcpAttr->tcp_dst) {
>+        tcpHdr->check = ChecksumUpdate16(tcpHdr->check, tcpHdr->dest,
>+                                         tcpAttr->tcp_dst);
>+        tcpHdr->dest = tcpAttr->tcp_dst;
>+    }
>+
>+    return NDIS_STATUS_SUCCESS;
>+}
>+
>+/*
>+ *----------------------------------------------------------------------------
>  * OvsUpdateIPv4Header --
>  *      Updates the IPv4 header in ovsFwdCtx.curNbl inline based on the
>  *      specified key.
>@@ -1571,6 +1614,11 @@ OvsExecuteSetAction(OvsForwardingContext *ovsFwdCtx,
>             NlAttrGetUnspec(a, sizeof(struct ovs_key_udp)));
>         break;
> 
>+    case OVS_KEY_ATTR_TCP:
>+        status = OvsUpdateTcpPorts(ovsFwdCtx,
>+            NlAttrGetUnspec(a, sizeof(struct ovs_key_tcp)));
>+        break;
>+
>     default:
>         OVS_LOG_INFO("Unhandled attribute %#x", type);
>         break;
>-- 
>2.10.2.windows.1
>_______________________________________________
>dev mailing list
>dev at openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DgICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=1Jc0nXap12Jq8G6IRneWFHLYyX1YfozJHnO6oj2EDxk&s=NDtt57jP5w1_yZix1X4vVmI7blFXZ-h6gJ1BpzQ4-dg&e=