Re: [ovs-dev] [PATCH v7 1/4] datapath-windows: Add support for NAT in conntrack

2017-05-16 Thread Sairam Venugopal 
Looks like outlook messed up my formatting.  I had 2 comments in this patch.

The default case statement is missing and 2 other cases aren't handled:
case OVS_NAT_ATTR_UNSPEC: break;
case __OVS_NAT_ATTR_MAX: break;
default: ;

OvsCtExecute_ - parameters exceed the length to almost 87

Thanks,
Sairam

-Original Message-
From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-boun...@openvswitch.org] 
On Behalf Of Yin Lin
Sent: Tuesday, May 9, 2017 4:00 PM
To: d...@openvswitch.org
Cc: Yin Lin 
Subject: [ovs-dev] [PATCH v7 1/4] datapath-windows: Add support for NAT in 
conntrack

From: Anand Kumar 

Add support for parsing netlink attributes related to NAT in conntrack.

Co-Authored-by: Anand Kumar 
Co-Authored-by: Darrell Ball 
Signed-off-by: Yin Lin 
---
 datapath-windows/ovsext/Conntrack.c | 73 -
 datapath-windows/ovsext/Conntrack.h | 17 +
 datapath-windows/ovsext/Flow.c  |  4 +-
 3 files changed, 90 insertions(+), 4 deletions(-)

diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index dce0c1b..9824368 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -645,7 +645,8 @@ OvsCtExecute_(PNET_BUFFER_LIST curNbl,
   UINT16 zone,
   MD_MARK *mark,
   MD_LABELS *labels,
-  PCHAR helper)
+  PCHAR helper,
+  PNAT_ACTION_INFO natInfo)
 {
 NDIS_STATUS status = NDIS_STATUS_SUCCESS;
 POVS_CT_ENTRY entry = NULL;
@@ -654,6 +655,9 @@ OvsCtExecute_(PNET_BUFFER_LIST curNbl,
 UINT64 currentTime;
 NdisGetCurrentSystemTime((LARGE_INTEGER *) );
 
+/* XXX: Not referenced for now */
+UNREFERENCED_PARAMETER(natInfo);
+
 /* Retrieve the Conntrack Key related fields from packet */
 OvsCtSetupLookupCtx(key, zone, , curNbl, layers->l4Offset);
 
@@ -730,11 +734,14 @@ OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx,
 MD_MARK *mark = NULL;
 MD_LABELS *labels = NULL;
 PCHAR helper = NULL;
+NAT_ACTION_INFO natActionInfo;
 PNET_BUFFER_LIST curNbl = fwdCtx->curNbl;
 OVS_PACKET_HDR_INFO *layers = >layers;
 PNET_BUFFER_LIST newNbl = NULL;
+NAT_ACTION_INFO natActionInfo;
 NDIS_STATUS status;
 
+memset(, 0, sizeof natActionInfo);
 status = OvsDetectCtPacket(fwdCtx, key, );
 if (status != NDIS_STATUS_SUCCESS) {
 return status;
@@ -757,6 +764,68 @@ OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx,
 if (ctAttr) {
 labels = NlAttrGet(ctAttr);
 }
+natActionInfo.natAction = NAT_ACTION_NONE;
+ctAttr = NlAttrFindNested(a, OVS_CT_ATTR_NAT);
+if (ctAttr) {
+/* Pares Nested NAT attributes. */
+PNL_ATTR natAttr;
+unsigned int left;
+BOOLEAN hasMinIp = FALSE;
+BOOLEAN hasMinPort = FALSE;
+BOOLEAN hasMaxIp = FALSE;
+BOOLEAN hasMaxPort = FALSE;
+NL_NESTED_FOR_EACH_UNSAFE (natAttr, left, ctAttr) {
+enum ovs_nat_attr sub_type_nest = NlAttrType(natAttr);
+switch(sub_type_nest) {
+case OVS_NAT_ATTR_SRC:
+case OVS_NAT_ATTR_DST:
+natActionInfo.natAction |=
+((sub_type_nest == OVS_NAT_ATTR_SRC)
+? NAT_ACTION_SRC : NAT_ACTION_DST);
+break;
+case OVS_NAT_ATTR_IP_MIN:
+memcpy(,
+   NlAttrData(natAttr), natAttr->nlaLen - NLA_HDRLEN);
+hasMinIp = TRUE;
+break;
+case OVS_NAT_ATTR_IP_MAX:
+memcpy(,
+   NlAttrData(natAttr), natAttr->nlaLen - NLA_HDRLEN);
+hasMaxIp = TRUE;
+break;
+case OVS_NAT_ATTR_PROTO_MIN:
+natActionInfo.minPort = NlAttrGetU16(natAttr);
+hasMinPort = TRUE;
+break;
+case OVS_NAT_ATTR_PROTO_MAX:
+natActionInfo.maxPort = NlAttrGetU16(natAttr);
+hasMaxPort = TRUE;
+break;
+case OVS_NAT_ATTR_PERSISTENT:
+case OVS_NAT_ATTR_PROTO_HASH:
+case OVS_NAT_ATTR_PROTO_RANDOM:
+break;
+}
+}
+if (natActionInfo.natAction == NAT_ACTION_NONE) {
+natActionInfo.natAction = NAT_ACTION_REVERSE;
+}
+if (hasMinIp && !hasMaxIp) {
+memcpy(,
+   ,
+   sizeof(natActionInfo.maxAddr));
+}
+if (hasMinPort && !hasMaxPort) {
+natActionInfo.maxPort = natActionInfo.minPort;
+}
+if (hasMinPort || hasMaxPort) {
+if (natActionInfo.natAction & NAT_ACTION_SRC) {
+natActionInfo.natAction |= NAT_ACTION_SRC_PORT;
+} else if

Re: [ovs-dev] [PATCH v7 1/4] datapath-windows: Add support for NAT in conntrack

2017-05-16 Thread Sairam Venugopal 
Not sure why the "From: Anand Kumar ” shows up in the 
commit description.

Since they are already listed as co-author, it should be safe to remove this.

Acked-by: Sairam Venugopal 






On 5/9/17, 3:59 PM, "ovs-dev-boun...@openvswitch.org on behalf of Yin Lin" 
 wrote:

>From: Anand Kumar 
>
>Add support for parsing netlink attributes related to NAT
>in conntrack.
>
>Co-Authored-by: Anand Kumar 
>Co-Authored-by: Darrell Ball 
>Signed-off-by: Yin Lin 
>---
> datapath-windows/ovsext/Conntrack.c | 73 -
> datapath-windows/ovsext/Conntrack.h | 17 +
> datapath-windows/ovsext/Flow.c  |  4 +-
> 3 files changed, 90 insertions(+), 4 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Conntrack.c 
>b/datapath-windows/ovsext/Conntrack.c
>index dce0c1b..9824368 100644
>--- a/datapath-windows/ovsext/Conntrack.c
>+++ b/datapath-windows/ovsext/Conntrack.c
>@@ -645,7 +645,8 @@ OvsCtExecute_(PNET_BUFFER_LIST curNbl,
>   UINT16 zone,
>   MD_MARK *mark,
>   MD_LABELS *labels,
>-  PCHAR helper)
>+  PCHAR helper,
>+  PNAT_ACTION_INFO natInfo)
> {
> NDIS_STATUS status = NDIS_STATUS_SUCCESS;
> POVS_CT_ENTRY entry = NULL;
>@@ -654,6 +655,9 @@ OvsCtExecute_(PNET_BUFFER_LIST curNbl,
> UINT64 currentTime;
> NdisGetCurrentSystemTime((LARGE_INTEGER *) );
> 
>+/* XXX: Not referenced for now */
>+UNREFERENCED_PARAMETER(natInfo);
>+
> /* Retrieve the Conntrack Key related fields from packet */
> OvsCtSetupLookupCtx(key, zone, , curNbl, layers->l4Offset);
> 
>@@ -730,11 +734,14 @@ OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx,
> MD_MARK *mark = NULL;
> MD_LABELS *labels = NULL;
> PCHAR helper = NULL;
>+NAT_ACTION_INFO natActionInfo;
> PNET_BUFFER_LIST curNbl = fwdCtx->curNbl;
> OVS_PACKET_HDR_INFO *layers = >layers;
> PNET_BUFFER_LIST newNbl = NULL;
>+NAT_ACTION_INFO natActionInfo;
> NDIS_STATUS status;
> 
>+memset(, 0, sizeof natActionInfo);
> status = OvsDetectCtPacket(fwdCtx, key, );
> if (status != NDIS_STATUS_SUCCESS) {
> return status;
>@@ -757,6 +764,68 @@ OvsExecuteConntrackAction(OvsForwardingContext *fwdCtx,
> if (ctAttr) {
> labels = NlAttrGet(ctAttr);
> }
>+natActionInfo.natAction = NAT_ACTION_NONE;
>+ctAttr = NlAttrFindNested(a, OVS_CT_ATTR_NAT);
>+if (ctAttr) {
>+/* Pares Nested NAT attributes. */
>+PNL_ATTR natAttr;
>+unsigned int left;
>+BOOLEAN hasMinIp = FALSE;
>+BOOLEAN hasMinPort = FALSE;
>+BOOLEAN hasMaxIp = FALSE;
>+BOOLEAN hasMaxPort = FALSE;
>+NL_NESTED_FOR_EACH_UNSAFE (natAttr, left, ctAttr) {
>+enum ovs_nat_attr sub_type_nest = NlAttrType(natAttr);
>+switch(sub_type_nest) {
>+case OVS_NAT_ATTR_SRC:
>+case OVS_NAT_ATTR_DST:
>+natActionInfo.natAction |=
>+((sub_type_nest == OVS_NAT_ATTR_SRC)
>+? NAT_ACTION_SRC : NAT_ACTION_DST);
>+break;
>+case OVS_NAT_ATTR_IP_MIN:
>+memcpy(,
>+   NlAttrData(natAttr), natAttr->nlaLen - NLA_HDRLEN);
>+hasMinIp = TRUE;
>+break;
>+case OVS_NAT_ATTR_IP_MAX:
>+memcpy(,
>+   NlAttrData(natAttr), natAttr->nlaLen - NLA_HDRLEN);
>+hasMaxIp = TRUE;
>+break;
>+case OVS_NAT_ATTR_PROTO_MIN:
>+natActionInfo.minPort = NlAttrGetU16(natAttr);
>+hasMinPort = TRUE;
>+break;
>+case OVS_NAT_ATTR_PROTO_MAX:
>+natActionInfo.maxPort = NlAttrGetU16(natAttr);
>+hasMaxPort = TRUE;
>+break;
>+case OVS_NAT_ATTR_PERSISTENT:
>+case OVS_NAT_ATTR_PROTO_HASH:
>+case OVS_NAT_ATTR_PROTO_RANDOM:
>+break;
>+}
>+}
>+if (natActionInfo.natAction == NAT_ACTION_NONE) {
>+natActionInfo.natAction = NAT_ACTION_REVERSE;
>+}
>+if (hasMinIp && !hasMaxIp) {
>+memcpy(,
>+   ,
>+   sizeof(natActionInfo.maxAddr));
>+}
>+if (hasMinPort && !hasMaxPort) {
>+natActionInfo.maxPort = natActionInfo.minPort;
>+}
>+if (hasMinPort || hasMaxPort) {
>+if (natActionInfo.natAction & NAT_ACTION_SRC) {
>+natActionInfo.natAction |= NAT_ACTION_SRC_PORT;
>+} else if (natActionInfo.natAction & NAT_ACTION_DST) {
>+natActionInfo.natAction |= NAT_ACTION_DST_PORT;
>+}
>+}
>+}
> ctAttr =