Re: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET command handler.

2014-09-30 Thread Samuel Ghinet 
Sorry Ankur,

My bad, I was not clear enough on my suggestion. I wanted to reply later, but 
did not find the time.
Thinking about the diversity of situations and scenarios (both for flow set and 
flow delete), I see the current implementation, with one handler to deal with 
all netlink flow commands very intricate. And it is likely that more 
differences between cases will arise.
This, I believe, will make the code difficult to read and maintain.

My suggestion would be - now I see this patch is merged, but if you consider, 
for later on - to split the handler functions.
This will result into code duplication for now, yes, but the code will become 
more manageable and more readable. And when the netlink components are 
finished, and when all scenarios can be tested, all the differences between 
flow new and flow set or flow new and flow delete will become more visible,
allowing a refactor then to better sort out the common functionalities, leaving 
the code more readable and maintainable. Putting now if-s and else-s all over 
within the same flow handler function may only make code intricate and 
difficult to spot potential problems.

This is my opinion. If you believe otherwise, that is ok as well.

Best Regards,
Sam

From: Ankur Sharma [ankursha...@vmware.com]
Sent: Friday, September 26, 2014 7:51 PM
To: Samuel Ghinet; dev@openvswitch.org
Subject: RE: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET   
  command handler.

Hi Sam,

We plan to handle the error cases properly, but not in this series.
Let me know if it sounds fine.

Thanks.

Regards,
Ankur

From: Samuel Ghinet sghi...@cloudbasesolutions.com
Sent: Thursday, September 25, 2014 9:28 AM
To: dev@openvswitch.org
Cc: Ankur Sharma
Subject: RE: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET   
  command handler.

Hey Ankur,

There are a few differences between flow new and flow set, with regard to the 
transactional errors that will need to be implemented:
Flow New: can do NEW or SET.
If netlink command flow new is issued, with netlink flags create and 
exclusive, and the flow exists, it must return EEXIST.

Flow Set: can do SET only: if the flow does not exist, it must return ENOENT.

Also, Flow Set has actions optional, while for Flow New the actions are 
required.

Regards,
Sam


Date: Wed, 24 Sep 2014 00:15:42 -0700
From: Ankur Sharma ankursha...@vmware.com
To: dev@openvswitch.org
Subject: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET
command handler.
Message-ID: 1411542944-19374-8-git-send-email-ankursha...@vmware.com

Registered FLOW_SET command handler. The same command
handler as FLOW_ADD is good enough to handle FLOW_SET
case as well.
---
 datapath-windows/ovsext/Datapath.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Datapath.c 
b/datapath-windows/ovsext/Datapath.c
index 7b064f9..5008aab 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -193,7 +193,12 @@ NETLINK_FAMILY nlVportFamilyOps = {
 /* Netlink flow family. */

 NETLINK_CMD nlFlowFamilyCmdOps[] = {
-{ .cmd  = OVS_FLOW_CMD_NEW,
+{ .cmd  = OVS_FLOW_CMD_NEW,
+  .handler  = OvsFlowNlNewCmdHandler,
+  .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
+  .validateDpIndex  = FALSE
+},
+{ .cmd  = OVS_FLOW_CMD_SET,
   .handler  = OvsFlowNlNewCmdHandler,
   .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
   .validateDpIndex  = FALSE
--
1.9.1


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET command handler.

2014-09-30 Thread Ankur Sharma 
Hi Samuel,

My intention was to remove code duplication.

But i have no strong preferences. I think your point makes sense, in long term 
having separate handler would make code easy to understand (even if it leads to 
code duplication).
I have a bunch of TODOs for netlink , i have noted this point as well and it 
will be handled in a follow up checkin.

Thanks.

Regards,
Ankur

From: Samuel Ghinet sghi...@cloudbasesolutions.com
Sent: Tuesday, September 30, 2014 8:44 AM
To: Ankur Sharma; dev@openvswitch.org
Cc: dev@openvswitch.org
Subject: RE: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET   
  command handler.

Sorry Ankur,

My bad, I was not clear enough on my suggestion. I wanted to reply later, but 
did not find the time.
Thinking about the diversity of situations and scenarios (both for flow set and 
flow delete), I see the current implementation, with one handler to deal with 
all netlink flow commands very intricate. And it is likely that more 
differences between cases will arise.
This, I believe, will make the code difficult to read and maintain.

My suggestion would be - now I see this patch is merged, but if you consider, 
for later on - to split the handler functions.
This will result into code duplication for now, yes, but the code will become 
more manageable and more readable. And when the netlink components are 
finished, and when all scenarios can be tested, all the differences between 
flow new and flow set or flow new and flow delete will become more visible,
allowing a refactor then to better sort out the common functionalities, leaving 
the code more readable and maintainable. Putting now if-s and else-s all over 
within the same flow handler function may only make code intricate and 
difficult to spot potential problems.

This is my opinion. If you believe otherwise, that is ok as well.

Best Regards,
Sam

From: Ankur Sharma [ankursha...@vmware.com]
Sent: Friday, September 26, 2014 7:51 PM
To: Samuel Ghinet; dev@openvswitch.org
Subject: RE: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET   
  command handler.

Hi Sam,

We plan to handle the error cases properly, but not in this series.
Let me know if it sounds fine.

Thanks.

Regards,
Ankur

From: Samuel Ghinet sghi...@cloudbasesolutions.com
Sent: Thursday, September 25, 2014 9:28 AM
To: dev@openvswitch.org
Cc: Ankur Sharma
Subject: RE: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET   
  command handler.

Hey Ankur,

There are a few differences between flow new and flow set, with regard to the 
transactional errors that will need to be implemented:
Flow New: can do NEW or SET.
If netlink command flow new is issued, with netlink flags create and 
exclusive, and the flow exists, it must return EEXIST.

Flow Set: can do SET only: if the flow does not exist, it must return ENOENT.

Also, Flow Set has actions optional, while for Flow New the actions are 
required.

Regards,
Sam


Date: Wed, 24 Sep 2014 00:15:42 -0700
From: Ankur Sharma ankursha...@vmware.com
To: dev@openvswitch.org
Subject: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET
command handler.
Message-ID: 1411542944-19374-8-git-send-email-ankursha...@vmware.com

Registered FLOW_SET command handler. The same command
handler as FLOW_ADD is good enough to handle FLOW_SET
case as well.
---
 datapath-windows/ovsext/Datapath.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Datapath.c 
b/datapath-windows/ovsext/Datapath.c
index 7b064f9..5008aab 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -193,7 +193,12 @@ NETLINK_FAMILY nlVportFamilyOps = {
 /* Netlink flow family. */

 NETLINK_CMD nlFlowFamilyCmdOps[] = {
-{ .cmd  = OVS_FLOW_CMD_NEW,
+{ .cmd  = OVS_FLOW_CMD_NEW,
+  .handler  = OvsFlowNlNewCmdHandler,
+  .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
+  .validateDpIndex  = FALSE
+},
+{ .cmd  = OVS_FLOW_CMD_SET,
   .handler  = OvsFlowNlNewCmdHandler,
   .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
   .validateDpIndex  = FALSE
--
1.9.1


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET command handler.

2014-09-26 Thread Ankur Sharma 
Hi Sam,

We plan to handle the error cases properly, but not in this series.
Let me know if it sounds fine.

Thanks.

Regards,
Ankur

From: Samuel Ghinet sghi...@cloudbasesolutions.com
Sent: Thursday, September 25, 2014 9:28 AM
To: dev@openvswitch.org
Cc: Ankur Sharma
Subject: RE: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET   
  command handler.

Hey Ankur,

There are a few differences between flow new and flow set, with regard to the 
transactional errors that will need to be implemented:
Flow New: can do NEW or SET.
If netlink command flow new is issued, with netlink flags create and 
exclusive, and the flow exists, it must return EEXIST.

Flow Set: can do SET only: if the flow does not exist, it must return ENOENT.

Also, Flow Set has actions optional, while for Flow New the actions are 
required.

Regards,
Sam


Date: Wed, 24 Sep 2014 00:15:42 -0700
From: Ankur Sharma ankursha...@vmware.com
To: dev@openvswitch.org
Subject: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET
command handler.
Message-ID: 1411542944-19374-8-git-send-email-ankursha...@vmware.com

Registered FLOW_SET command handler. The same command
handler as FLOW_ADD is good enough to handle FLOW_SET
case as well.
---
 datapath-windows/ovsext/Datapath.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Datapath.c 
b/datapath-windows/ovsext/Datapath.c
index 7b064f9..5008aab 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -193,7 +193,12 @@ NETLINK_FAMILY nlVportFamilyOps = {
 /* Netlink flow family. */

 NETLINK_CMD nlFlowFamilyCmdOps[] = {
-{ .cmd  = OVS_FLOW_CMD_NEW,
+{ .cmd  = OVS_FLOW_CMD_NEW,
+  .handler  = OvsFlowNlNewCmdHandler,
+  .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
+  .validateDpIndex  = FALSE
+},
+{ .cmd  = OVS_FLOW_CMD_SET,
   .handler  = OvsFlowNlNewCmdHandler,
   .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
   .validateDpIndex  = FALSE
--
1.9.1


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET command handler.

2014-09-25 Thread Samuel Ghinet 
Hey Ankur,

There are a few differences between flow new and flow set, with regard to the 
transactional errors that will need to be implemented:
Flow New: can do NEW or SET. 
If netlink command flow new is issued, with netlink flags create and 
exclusive, and the flow exists, it must return EEXIST.

Flow Set: can do SET only: if the flow does not exist, it must return ENOENT.

Also, Flow Set has actions optional, while for Flow New the actions are 
required.

Regards,
Sam


Date: Wed, 24 Sep 2014 00:15:42 -0700
From: Ankur Sharma ankursha...@vmware.com
To: dev@openvswitch.org
Subject: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET
command handler.
Message-ID: 1411542944-19374-8-git-send-email-ankursha...@vmware.com

Registered FLOW_SET command handler. The same command
handler as FLOW_ADD is good enough to handle FLOW_SET
case as well.
---
 datapath-windows/ovsext/Datapath.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Datapath.c 
b/datapath-windows/ovsext/Datapath.c
index 7b064f9..5008aab 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -193,7 +193,12 @@ NETLINK_FAMILY nlVportFamilyOps = {
 /* Netlink flow family. */

 NETLINK_CMD nlFlowFamilyCmdOps[] = {
-{ .cmd  = OVS_FLOW_CMD_NEW,
+{ .cmd  = OVS_FLOW_CMD_NEW,
+  .handler  = OvsFlowNlNewCmdHandler,
+  .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
+  .validateDpIndex  = FALSE
+},
+{ .cmd  = OVS_FLOW_CMD_SET,
   .handler  = OvsFlowNlNewCmdHandler,
   .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
   .validateDpIndex  = FALSE
--
1.9.1


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET command handler.

2014-09-24 Thread Ankur Sharma 
Registered FLOW_SET command handler. The same command
handler as FLOW_ADD is good enough to handle FLOW_SET
case as well.
---
 datapath-windows/ovsext/Datapath.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Datapath.c 
b/datapath-windows/ovsext/Datapath.c
index 7b064f9..5008aab 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -193,7 +193,12 @@ NETLINK_FAMILY nlVportFamilyOps = {
 /* Netlink flow family. */
 
 NETLINK_CMD nlFlowFamilyCmdOps[] = {
-{ .cmd  = OVS_FLOW_CMD_NEW, 
+{ .cmd  = OVS_FLOW_CMD_NEW,
+  .handler  = OvsFlowNlNewCmdHandler,
+  .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
+  .validateDpIndex  = FALSE
+},
+{ .cmd  = OVS_FLOW_CMD_SET,
   .handler  = OvsFlowNlNewCmdHandler,
   .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
   .validateDpIndex  = FALSE
-- 
1.9.1

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET command handler.

2014-09-24 Thread Eitan Eliahu 
Acked-by: Eitan Eliahu elia...@vmware.com


-Original Message-
From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Ankur Sharma
Sent: Wednesday, September 24, 2014 12:16 AM
To: dev@openvswitch.org
Subject: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET command 
handler.

Registered FLOW_SET command handler. The same command handler as FLOW_ADD is 
good enough to handle FLOW_SET case as well.
---
 datapath-windows/ovsext/Datapath.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Datapath.c 
b/datapath-windows/ovsext/Datapath.c
index 7b064f9..5008aab 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -193,7 +193,12 @@ NETLINK_FAMILY nlVportFamilyOps = {
 /* Netlink flow family. */
 
 NETLINK_CMD nlFlowFamilyCmdOps[] = {
-{ .cmd  = OVS_FLOW_CMD_NEW, 
+{ .cmd  = OVS_FLOW_CMD_NEW,
+  .handler  = OvsFlowNlNewCmdHandler,
+  .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
+  .validateDpIndex  = FALSE
+},
+{ .cmd  = OVS_FLOW_CMD_SET,
   .handler  = OvsFlowNlNewCmdHandler,
   .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
   .validateDpIndex  = FALSE
--
1.9.1

___
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/devk=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0Ar=yTvML8OxA42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0Am=xTLe4yvn6aQq2LrM7e1xXHI05JkZ8kFY%2BUpy4aGhAXc%3D%0As=21cad77534deffc6584ccc45756b865986fb98ce9170fb43839b50a53ef7ff60
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET command handler.

2014-09-24 Thread Alin Serdean 
Acked-by: Alin Gabriel Serdean aserd...@cloudbasesolutions.com



-Mesaj original-
De la: dev [mailto:dev-boun...@openvswitch.org] În numele Ankur Sharma
Trimis: Wednesday, September 24, 2014 10:16 AM
Către: dev@openvswitch.org
Subiect: [ovs-dev] [PATCH v1 08/10] datapath-windows/Flow.c: FLOW_SET command 
handler.

Registered FLOW_SET command handler. The same command handler as FLOW_ADD is 
good enough to handle FLOW_SET case as well.
---
 datapath-windows/ovsext/Datapath.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Datapath.c 
b/datapath-windows/ovsext/Datapath.c
index 7b064f9..5008aab 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -193,7 +193,12 @@ NETLINK_FAMILY nlVportFamilyOps = {
 /* Netlink flow family. */
 
 NETLINK_CMD nlFlowFamilyCmdOps[] = {
-{ .cmd  = OVS_FLOW_CMD_NEW, 
+{ .cmd  = OVS_FLOW_CMD_NEW,
+  .handler  = OvsFlowNlNewCmdHandler,
+  .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
+  .validateDpIndex  = FALSE
+},
+{ .cmd  = OVS_FLOW_CMD_SET,
   .handler  = OvsFlowNlNewCmdHandler,
   .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
   .validateDpIndex  = FALSE
--
1.9.1

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev