Re: [ovs-dev] [PATCH] datapath-windows: Add trace level logs in conntrack for invalid ct state.

2018-02-02 Thread aserdean 


-Mesaj original-
De la: Anand Kumar [mailto:kumaran...@vmware.com] 
Trimis: Saturday, February 3, 2018 12:28 AM
Către: Alin Serdean <aserd...@cloudbasesolutions.com>; d...@openvswitch.org
Subiect: Re: [ovs-dev] [PATCH] datapath-windows: Add trace level logs in 
conntrack for invalid ct state.

Hi Alin,

Thanks for the quick review. I will expand log messages to “Invalid XXX packet 
detected”.
I’m not sure if I follow your comment about “s/syn/SYN/g”. 

Do you want me to change it to Uppercase?
[Alin Serdean] Yes please .

Thanks,
Anand Kumar

On 2/2/18, 2:06 PM, "Alin Serdean" <aserd...@cloudbasesolutions.com> wrote:

Looks good just a small nit:
s/syn/SYN/g
s/ack/ACK/g

Also I would prefer if you drop the text `Invalid!`. Either just remove it 
or expand it, i.e.:
"Invalid! ICMPhdr cannot be NULL" => "Invalid ICMP packet detected the 
header cannot be NULL"

Thanks,
Alin.

-Mesaj original-
De la: ovs-dev-boun...@openvswitch.org 
[mailto:ovs-dev-boun...@openvswitch.org] În numele Anand Kumar
Trimis: Friday, February 2, 2018 11:19 PM
Către: d...@openvswitch.org
Subiect: [ovs-dev] [PATCH] datapath-windows: Add trace level logs in 
conntrack for invalid ct state.

Signed-off-by: Anand Kumar <kumaran...@vmware.com>
---
 datapath-windows/ovsext/Conntrack-icmp.c | 1 +  
datapath-windows/ovsext/Conntrack-tcp.c  | 4 
 datapath-windows/ovsext/Conntrack.c  | 4 
 3 files changed, 9 insertions(+)

diff --git a/datapath-windows/ovsext/Conntrack-icmp.c 
b/datapath-windows/ovsext/Conntrack-icmp.c
index 4da0665..d86feed 100644
--- a/datapath-windows/ovsext/Conntrack-icmp.c
+++ b/datapath-windows/ovsext/Conntrack-icmp.c
@@ -61,6 +61,7 @@ BOOLEAN
 OvsConntrackValidateIcmpPacket(const ICMPHdr *icmp)  {
 if (!icmp) {
+OVS_LOG_TRACE("Invalid! ICMPhdr cannot be NULL");
 return FALSE;
 }
 
diff --git a/datapath-windows/ovsext/Conntrack-tcp.c 
b/datapath-windows/ovsext/Conntrack-tcp.c
index f8e85a2..65eaac5 100644
--- a/datapath-windows/ovsext/Conntrack-tcp.c
+++ b/datapath-windows/ovsext/Conntrack-tcp.c
@@ -444,12 +444,14 @@ BOOLEAN
 OvsConntrackValidateTcpPacket(const TCPHdr *tcp)  {
 if (!tcp) {
+OVS_LOG_TRACE("Invalid! TCPHdr cannot be NULL");
 return FALSE;
 }
 
 UINT16 tcp_flags = ntohs(tcp->flags);
 
 if (OvsCtInvalidTcpFlags(tcp_flags)) {
+OVS_LOG_TRACE("Invalid! tcp_flags %hu", tcp_flags);
 return FALSE;
 }
 
@@ -457,6 +459,8 @@ OvsConntrackValidateTcpPacket(const TCPHdr *tcp)
  * totally new connections (syn) or already established, not partially
  * open (syn+ack). */
 if ((tcp_flags & TCP_SYN) && (tcp_flags & TCP_ACK)) {
+OVS_LOG_TRACE("Invalid! syn+ack flags not allowed, tcp_flags %hu",
+  tcp_flags);
 return FALSE;
 }
 
diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index 43c9dd3..7e413c6 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -317,6 +317,9 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
 const ICMPHdr *icmp;
 icmp = OvsGetIcmp(curNbl, l4Offset, );
 if (!OvsConntrackValidateIcmpPacket(icmp)) {
+if(icmp) {
+OVS_LOG_TRACE("Invalid! icmp->type %u", icmp->type);
+}
 state = OVS_CS_F_INVALID;
 break;
 }
@@ -334,6 +337,7 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
 break;
 }
 default:
+OVS_LOG_TRACE("Invalid! Not supported protocol, ipProto %u", 
+ ipProto);
 state = OVS_CS_F_INVALID;
 break;
 }
--
2.9.3.windows.1

___
dev mailing list
d...@openvswitch.org

https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwIFBA=uilaK90D4TOVoH58JNXRgQ=Q5z9tBe-nAOpE7LIHSPV8uy5-437agMXvkeHHMkR8Us=yuxpORhg-xij1o9VvWANs9QOyywwfr7YO_EDT-QvqQ4=ainXigtUgyesCYog0X4639gDSFv1mrY4OTsctKNaH0M=



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows: Add trace level logs in conntrack for invalid ct state.

2018-02-02 Thread Anand Kumar 
Hi Alin,

Thanks for the quick review. I will expand log messages to “Invalid XXX packet 
detected”.
I’m not sure if I follow your comment about “s/syn/SYN/g”. 

Do you want me to change it to Uppercase?

Thanks,
Anand Kumar

On 2/2/18, 2:06 PM, "Alin Serdean"  wrote:

Looks good just a small nit:
s/syn/SYN/g
s/ack/ACK/g

Also I would prefer if you drop the text `Invalid!`. Either just remove it 
or expand it, i.e.:
"Invalid! ICMPhdr cannot be NULL" => "Invalid ICMP packet detected the 
header cannot be NULL"

Thanks,
Alin.

-Mesaj original-
De la: ovs-dev-boun...@openvswitch.org 
[mailto:ovs-dev-boun...@openvswitch.org] În numele Anand Kumar
Trimis: Friday, February 2, 2018 11:19 PM
Către: d...@openvswitch.org
Subiect: [ovs-dev] [PATCH] datapath-windows: Add trace level logs in 
conntrack for invalid ct state.

Signed-off-by: Anand Kumar 
---
 datapath-windows/ovsext/Conntrack-icmp.c | 1 +  
datapath-windows/ovsext/Conntrack-tcp.c  | 4 
 datapath-windows/ovsext/Conntrack.c  | 4 
 3 files changed, 9 insertions(+)

diff --git a/datapath-windows/ovsext/Conntrack-icmp.c 
b/datapath-windows/ovsext/Conntrack-icmp.c
index 4da0665..d86feed 100644
--- a/datapath-windows/ovsext/Conntrack-icmp.c
+++ b/datapath-windows/ovsext/Conntrack-icmp.c
@@ -61,6 +61,7 @@ BOOLEAN
 OvsConntrackValidateIcmpPacket(const ICMPHdr *icmp)  {
 if (!icmp) {
+OVS_LOG_TRACE("Invalid! ICMPhdr cannot be NULL");
 return FALSE;
 }
 
diff --git a/datapath-windows/ovsext/Conntrack-tcp.c 
b/datapath-windows/ovsext/Conntrack-tcp.c
index f8e85a2..65eaac5 100644
--- a/datapath-windows/ovsext/Conntrack-tcp.c
+++ b/datapath-windows/ovsext/Conntrack-tcp.c
@@ -444,12 +444,14 @@ BOOLEAN
 OvsConntrackValidateTcpPacket(const TCPHdr *tcp)  {
 if (!tcp) {
+OVS_LOG_TRACE("Invalid! TCPHdr cannot be NULL");
 return FALSE;
 }
 
 UINT16 tcp_flags = ntohs(tcp->flags);
 
 if (OvsCtInvalidTcpFlags(tcp_flags)) {
+OVS_LOG_TRACE("Invalid! tcp_flags %hu", tcp_flags);
 return FALSE;
 }
 
@@ -457,6 +459,8 @@ OvsConntrackValidateTcpPacket(const TCPHdr *tcp)
  * totally new connections (syn) or already established, not partially
  * open (syn+ack). */
 if ((tcp_flags & TCP_SYN) && (tcp_flags & TCP_ACK)) {
+OVS_LOG_TRACE("Invalid! syn+ack flags not allowed, tcp_flags %hu",
+  tcp_flags);
 return FALSE;
 }
 
diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index 43c9dd3..7e413c6 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -317,6 +317,9 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
 const ICMPHdr *icmp;
 icmp = OvsGetIcmp(curNbl, l4Offset, );
 if (!OvsConntrackValidateIcmpPacket(icmp)) {
+if(icmp) {
+OVS_LOG_TRACE("Invalid! icmp->type %u", icmp->type);
+}
 state = OVS_CS_F_INVALID;
 break;
 }
@@ -334,6 +337,7 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
 break;
 }
 default:
+OVS_LOG_TRACE("Invalid! Not supported protocol, ipProto %u", 
+ ipProto);
 state = OVS_CS_F_INVALID;
 break;
 }
--
2.9.3.windows.1

___
dev mailing list
d...@openvswitch.org

https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwIFBA=uilaK90D4TOVoH58JNXRgQ=Q5z9tBe-nAOpE7LIHSPV8uy5-437agMXvkeHHMkR8Us=yuxpORhg-xij1o9VvWANs9QOyywwfr7YO_EDT-QvqQ4=ainXigtUgyesCYog0X4639gDSFv1mrY4OTsctKNaH0M=


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows: Add trace level logs in conntrack for invalid ct state.

2018-02-02 Thread Alin Serdean 
Looks good just a small nit:
s/syn/SYN/g
s/ack/ACK/g

Also I would prefer if you drop the text `Invalid!`. Either just remove it or 
expand it, i.e.:
"Invalid! ICMPhdr cannot be NULL" => "Invalid ICMP packet detected the header 
cannot be NULL"

Thanks,
Alin.

-Mesaj original-
De la: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-boun...@openvswitch.org] 
În numele Anand Kumar
Trimis: Friday, February 2, 2018 11:19 PM
Către: d...@openvswitch.org
Subiect: [ovs-dev] [PATCH] datapath-windows: Add trace level logs in conntrack 
for invalid ct state.

Signed-off-by: Anand Kumar 
---
 datapath-windows/ovsext/Conntrack-icmp.c | 1 +  
datapath-windows/ovsext/Conntrack-tcp.c  | 4 
 datapath-windows/ovsext/Conntrack.c  | 4 
 3 files changed, 9 insertions(+)

diff --git a/datapath-windows/ovsext/Conntrack-icmp.c 
b/datapath-windows/ovsext/Conntrack-icmp.c
index 4da0665..d86feed 100644
--- a/datapath-windows/ovsext/Conntrack-icmp.c
+++ b/datapath-windows/ovsext/Conntrack-icmp.c
@@ -61,6 +61,7 @@ BOOLEAN
 OvsConntrackValidateIcmpPacket(const ICMPHdr *icmp)  {
 if (!icmp) {
+OVS_LOG_TRACE("Invalid! ICMPhdr cannot be NULL");
 return FALSE;
 }
 
diff --git a/datapath-windows/ovsext/Conntrack-tcp.c 
b/datapath-windows/ovsext/Conntrack-tcp.c
index f8e85a2..65eaac5 100644
--- a/datapath-windows/ovsext/Conntrack-tcp.c
+++ b/datapath-windows/ovsext/Conntrack-tcp.c
@@ -444,12 +444,14 @@ BOOLEAN
 OvsConntrackValidateTcpPacket(const TCPHdr *tcp)  {
 if (!tcp) {
+OVS_LOG_TRACE("Invalid! TCPHdr cannot be NULL");
 return FALSE;
 }
 
 UINT16 tcp_flags = ntohs(tcp->flags);
 
 if (OvsCtInvalidTcpFlags(tcp_flags)) {
+OVS_LOG_TRACE("Invalid! tcp_flags %hu", tcp_flags);
 return FALSE;
 }
 
@@ -457,6 +459,8 @@ OvsConntrackValidateTcpPacket(const TCPHdr *tcp)
  * totally new connections (syn) or already established, not partially
  * open (syn+ack). */
 if ((tcp_flags & TCP_SYN) && (tcp_flags & TCP_ACK)) {
+OVS_LOG_TRACE("Invalid! syn+ack flags not allowed, tcp_flags %hu",
+  tcp_flags);
 return FALSE;
 }
 
diff --git a/datapath-windows/ovsext/Conntrack.c 
b/datapath-windows/ovsext/Conntrack.c
index 43c9dd3..7e413c6 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -317,6 +317,9 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
 const ICMPHdr *icmp;
 icmp = OvsGetIcmp(curNbl, l4Offset, );
 if (!OvsConntrackValidateIcmpPacket(icmp)) {
+if(icmp) {
+OVS_LOG_TRACE("Invalid! icmp->type %u", icmp->type);
+}
 state = OVS_CS_F_INVALID;
 break;
 }
@@ -334,6 +337,7 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
 break;
 }
 default:
+OVS_LOG_TRACE("Invalid! Not supported protocol, ipProto %u", 
+ ipProto);
 state = OVS_CS_F_INVALID;
 break;
 }
--
2.9.3.windows.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev