[ovs-dev] ofproto.at: Fix minor typo in test.
diff mbox

Message ID 1499669501-80363-3-git-send-email-jpettit@ovn.org
State Superseded
Headers show

Commit Message

Justin Pettit July 10, 2017, 6:51 a.m. UTC
Signed-off-by: Justin Pettit <jpettit@ovn.org>
---
 tests/ofproto.at | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ben Pfaff July 14, 2017, 7:59 p.m. UTC | #1
On Sun, Jul 09, 2017 at 11:51:40PM -0700, Justin Pettit wrote:
> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> ---
>  tests/ofproto.at | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 9e6acfad653d..c7ea31a77ce9 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -3430,7 +3430,7 @@ udp,vlan_tci=0x0000,dl_src=00:26:b9:8c:b0:f9,dl_dst=00:25:83:df:b4:00,nw_src=172
>      ovs-ofctl -O OpenFlow13 add-flow br0 send_flow_rem,actions=group:1234
>      ovs-ofctl -O OpenFlow13 --strict del-groups br0 group_id=1234
>      if test X"$1" = X"OFPRR_DELETE"; then shift;
> -        echo >>expout "OFPT_FLOW_REMOVED (OF1.3):  reason=gropu_delete table_id=0"
> +        echo >>expout "OFPT_FLOW_REMOVED (OF1.3):  reason=group_delete table_id=0"

Why didn't this cause a test failure?  (Is this code never actually
executed?)
Justin Pettit July 15, 2017, 10:02 p.m. UTC | #2
> On Jul 14, 2017, at 12:59 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Sun, Jul 09, 2017 at 11:51:40PM -0700, Justin Pettit wrote:
>> Signed-off-by: Justin Pettit <jpettit@ovn.org>
>> ---
>> tests/ofproto.at | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/tests/ofproto.at b/tests/ofproto.at
>> index 9e6acfad653d..c7ea31a77ce9 100644
>> --- a/tests/ofproto.at
>> +++ b/tests/ofproto.at
>> @@ -3430,7 +3430,7 @@ udp,vlan_tci=0x0000,dl_src=00:26:b9:8c:b0:f9,dl_dst=00:25:83:df:b4:00,nw_src=172
>>     ovs-ofctl -O OpenFlow13 add-flow br0 send_flow_rem,actions=group:1234
>>     ovs-ofctl -O OpenFlow13 --strict del-groups br0 group_id=1234
>>     if test X"$1" = X"OFPRR_DELETE"; then shift;
>> -        echo >>expout "OFPT_FLOW_REMOVED (OF1.3):  reason=gropu_delete table_id=0"
>> +        echo >>expout "OFPT_FLOW_REMOVED (OF1.3):  reason=group_delete table_id=0"
> 
> Why didn't this cause a test failure?  (Is this code never actually
> executed?)

Yes, there were multiple errors in the test that prevented it from running.  It's easy to correct them, but our support for OpenFlow 1.3 doesn't include generating a flow removed due to the "group delete".  The OpenFlow 1.3 specification defines such a reason but we seem to break things into "OpenFlow 1.4+" or "Open Flow 1.0". so we don't generate it for 1.3.

Do you think we should add support for 1.3 or just remove that bit of test code?

Here's the commit that introduced that test:

	https://github.com/openvswitch/ovs/commit/cc40d06bf5ca3

--Justin
Ben Pfaff July 15, 2017, 10:21 p.m. UTC | #3
On Sat, Jul 15, 2017 at 03:02:18PM -0700, Justin Pettit wrote:
> 
> > On Jul 14, 2017, at 12:59 PM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > On Sun, Jul 09, 2017 at 11:51:40PM -0700, Justin Pettit wrote:
> >> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> >> ---
> >> tests/ofproto.at | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/tests/ofproto.at b/tests/ofproto.at
> >> index 9e6acfad653d..c7ea31a77ce9 100644
> >> --- a/tests/ofproto.at
> >> +++ b/tests/ofproto.at
> >> @@ -3430,7 +3430,7 @@ udp,vlan_tci=0x0000,dl_src=00:26:b9:8c:b0:f9,dl_dst=00:25:83:df:b4:00,nw_src=172
> >>     ovs-ofctl -O OpenFlow13 add-flow br0 send_flow_rem,actions=group:1234
> >>     ovs-ofctl -O OpenFlow13 --strict del-groups br0 group_id=1234
> >>     if test X"$1" = X"OFPRR_DELETE"; then shift;
> >> -        echo >>expout "OFPT_FLOW_REMOVED (OF1.3):  reason=gropu_delete table_id=0"
> >> +        echo >>expout "OFPT_FLOW_REMOVED (OF1.3):  reason=group_delete table_id=0"
> > 
> > Why didn't this cause a test failure?  (Is this code never actually
> > executed?)
> 
> Yes, there were multiple errors in the test that prevented it from running.  It's easy to correct them, but our support for OpenFlow 1.3 doesn't include generating a flow removed due to the "group delete".  The OpenFlow 1.3 specification defines such a reason but we seem to break things into "OpenFlow 1.4+" or "Open Flow 1.0". so we don't generate it for 1.3.
> 
> Do you think we should add support for 1.3 or just remove that bit of test code?

I think it would be better to support OF1.3.  I don't know of a reason
that it's particularly hard.
Justin Pettit July 15, 2017, 11:53 p.m. UTC | #4
> On Jul 15, 2017, at 3:21 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Sat, Jul 15, 2017 at 03:02:18PM -0700, Justin Pettit wrote:
>> 
>>> On Jul 14, 2017, at 12:59 PM, Ben Pfaff <blp@ovn.org> wrote:
>>> 
>>> On Sun, Jul 09, 2017 at 11:51:40PM -0700, Justin Pettit wrote:
>>>> Signed-off-by: Justin Pettit <jpettit@ovn.org>
>>>> ---
>>>> tests/ofproto.at | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/tests/ofproto.at b/tests/ofproto.at
>>>> index 9e6acfad653d..c7ea31a77ce9 100644
>>>> --- a/tests/ofproto.at
>>>> +++ b/tests/ofproto.at
>>>> @@ -3430,7 +3430,7 @@ udp,vlan_tci=0x0000,dl_src=00:26:b9:8c:b0:f9,dl_dst=00:25:83:df:b4:00,nw_src=172
>>>>    ovs-ofctl -O OpenFlow13 add-flow br0 send_flow_rem,actions=group:1234
>>>>    ovs-ofctl -O OpenFlow13 --strict del-groups br0 group_id=1234
>>>>    if test X"$1" = X"OFPRR_DELETE"; then shift;
>>>> -        echo >>expout "OFPT_FLOW_REMOVED (OF1.3):  reason=gropu_delete table_id=0"
>>>> +        echo >>expout "OFPT_FLOW_REMOVED (OF1.3):  reason=group_delete table_id=0"
>>> 
>>> Why didn't this cause a test failure?  (Is this code never actually
>>> executed?)
>> 
>> Yes, there were multiple errors in the test that prevented it from running.  It's easy to correct them, but our support for OpenFlow 1.3 doesn't include generating a flow removed due to the "group delete".  The OpenFlow 1.3 specification defines such a reason but we seem to break things into "OpenFlow 1.4+" or "Open Flow 1.0". so we don't generate it for 1.3.
>> 
>> Do you think we should add support for 1.3 or just remove that bit of test code?
> 
> I think it would be better to support OF1.3.  I don't know of a reason
> that it's particularly hard.

I sent out a patch to add support:

	https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335724.html

--Justin

Patch
diff mbox

diff --git a/tests/ofproto.at b/tests/ofproto.at
index 9e6acfad653d..c7ea31a77ce9 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -3430,7 +3430,7 @@  udp,vlan_tci=0x0000,dl_src=00:26:b9:8c:b0:f9,dl_dst=00:25:83:df:b4:00,nw_src=172
     ovs-ofctl -O OpenFlow13 add-flow br0 send_flow_rem,actions=group:1234
     ovs-ofctl -O OpenFlow13 --strict del-groups br0 group_id=1234
     if test X"$1" = X"OFPRR_DELETE"; then shift;
-        echo >>expout "OFPT_FLOW_REMOVED (OF1.3):  reason=gropu_delete table_id=0"
+        echo >>expout "OFPT_FLOW_REMOVED (OF1.3):  reason=group_delete table_id=0"
     fi
 
     AT_FAIL_IF([test X"$1" != X])