[ovs-dev,2/4] ovs-ofctl: Fix OpenFlow versions with '--bundle'
diff mbox

Message ID 1444944484-117214-2-git-send-email-jrajahalme@nicira.com
State Accepted
Headers show

Commit Message

Jarno Rajahalme Oct. 15, 2015, 9:28 p.m. UTC
While the presence of the '--bundle' option implicitly added the
OpenFlow 1.4 to the allowed protocols, it failed to remove OpenFlow
1.0 from the allowed protocols.  This is changed so that '--bundle'
option now also implicitly removes versions lesser than 1.4 from the
allowed protocols.  This has no behavioral difference when ovs-ofctl
is paired with OVS that supports OpenFlow 1.4, as the greatest common
version is negotiated, but prevents negotiation of OpenFlow 1.0 when
OVS does not support OpenFlow 1.4.

Found by inspection.

Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
---
 tests/ofproto.at      | 12 ++++++------
 tests/ovs-ofctl.at    |  8 ++++----
 utilities/ovs-ofctl.c |  3 +++
 3 files changed, 13 insertions(+), 10 deletions(-)

Comments

Takashi Yamamoto Oct. 16, 2015, 3:32 a.m. UTC | #1
On Fri, Oct 16, 2015 at 6:28 AM, Jarno Rajahalme <jrajahalme@nicira.com> wrote:
> While the presence of the '--bundle' option implicitly added the
> OpenFlow 1.4 to the allowed protocols, it failed to remove OpenFlow
> 1.0 from the allowed protocols.  This is changed so that '--bundle'
> option now also implicitly removes versions lesser than 1.4 from the
> allowed protocols.  This has no behavioral difference when ovs-ofctl
> is paired with OVS that supports OpenFlow 1.4, as the greatest common
> version is negotiated, but prevents negotiation of OpenFlow 1.0 when
> OVS does not support OpenFlow 1.4.
>
> Found by inspection.
>
> Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>

Acked-by: YAMAMOTO Takashi <yamamoto@midokura.com>

> ---
>  tests/ofproto.at      | 12 ++++++------
>  tests/ovs-ofctl.at    |  8 ++++----
>  utilities/ovs-ofctl.c |  3 +++
>  3 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 5e4441c..4c6dd29 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -3962,8 +3962,8 @@ vconn|DBG|unix: sent (Success): OFPT_BARRIER_REPLY:
>  vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
>   version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
>  vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
> - version bitmap: 0x01, 0x05
> -vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
> + version bitmap: 0x05
> +vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
>  vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
>   bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
>  vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
> @@ -4014,8 +4014,8 @@ vconn|DBG|unix: sent (Success): NXST_FLOW reply:
>  vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
>   version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
>  vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
> - version bitmap: 0x01, 0x05
> -vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
> + version bitmap: 0x05
> +vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
>  vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
>   bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
>  vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
> @@ -4045,8 +4045,8 @@ vconn|DBG|unix: sent (Success): NXST_FLOW reply:
>  vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
>   version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
>  vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
> - version bitmap: 0x01, 0x05
> -vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
> + version bitmap: 0x05
> +vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
>  vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
>   bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
>  vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
> diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
> index 6f03adb..7375cad 100644
> --- a/tests/ovs-ofctl.at
> +++ b/tests/ovs-ofctl.at
> @@ -2889,8 +2889,8 @@ AT_CHECK([print_vconn_debug | vconn_windows_sub | ofctl_strip], [0], [dnl
>  vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
>   version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
>  vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
> - version bitmap: 0x01, 0x05
> -vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
> + version bitmap: 0x05
> +vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
>  vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
>   bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
>  vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
> @@ -2926,8 +2926,8 @@ vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
>  vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
>   version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
>  vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
> - version bitmap: 0x01, 0x05
> -vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
> + version bitmap: 0x05
> +vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
>  vconn|DBG|unix: received: OFPST_FLOW request (OF1.4):
>  vconn|DBG|unix: sent (Success): OFPST_FLOW reply (OF1.4):
>   importance=1, dl_vlan=1 actions=drop
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index fbc9da4..ee15e1a 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -312,6 +312,9 @@ parse_options(int argc, char *argv[])
>          /* Add implicit allowance for OpenFlow 1.4. */
>          add_allowed_ofp_versions(ofputil_protocols_to_version_bitmap(
>                                       OFPUTIL_P_OF14_OXM));
> +        /* Remove all prior versions. */
> +        mask_allowed_ofp_versions(ofputil_protocols_to_version_bitmap(
> +                                     OFPUTIL_P_OF14_UP));
>      }
>      versions = get_allowed_ofp_versions();
>      version_protocols = ofputil_protocols_from_version_bitmap(versions);
> --
> 2.1.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
Jarno Rajahalme Oct. 19, 2015, 10:11 p.m. UTC | #2
> On Oct 15, 2015, at 8:32 PM, Takashi Yamamoto <yamamoto@midokura.com> wrote:
> 
> On Fri, Oct 16, 2015 at 6:28 AM, Jarno Rajahalme <jrajahalme@nicira.com <mailto:jrajahalme@nicira.com>> wrote:
>> While the presence of the '--bundle' option implicitly added the
>> OpenFlow 1.4 to the allowed protocols, it failed to remove OpenFlow
>> 1.0 from the allowed protocols.  This is changed so that '--bundle'
>> option now also implicitly removes versions lesser than 1.4 from the
>> allowed protocols.  This has no behavioral difference when ovs-ofctl
>> is paired with OVS that supports OpenFlow 1.4, as the greatest common
>> version is negotiated, but prevents negotiation of OpenFlow 1.0 when
>> OVS does not support OpenFlow 1.4.
>> 
>> Found by inspection.
>> 
>> Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com <mailto:jrajahalme@nicira.com>>
> 
> Acked-by: YAMAMOTO Takashi <yamamoto@midokura.com <mailto:yamamoto@midokura.com>>
> 

Thanks for the review, pushed to master.

  Jarno


>> ---
>> tests/ofproto.at      | 12 ++++++------
>> tests/ovs-ofctl.at    |  8 ++++----
>> utilities/ovs-ofctl.c |  3 +++
>> 3 files changed, 13 insertions(+), 10 deletions(-)
>> 
>> diff --git a/tests/ofproto.at b/tests/ofproto.at
>> index 5e4441c..4c6dd29 100644
>> --- a/tests/ofproto.at
>> +++ b/tests/ofproto.at
>> @@ -3962,8 +3962,8 @@ vconn|DBG|unix: sent (Success): OFPT_BARRIER_REPLY:
>> vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
>>  version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
>> vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
>> - version bitmap: 0x01, 0x05
>> -vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
>> + version bitmap: 0x05
>> +vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
>> vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
>>  bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
>> vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
>> @@ -4014,8 +4014,8 @@ vconn|DBG|unix: sent (Success): NXST_FLOW reply:
>> vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
>>  version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
>> vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
>> - version bitmap: 0x01, 0x05
>> -vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
>> + version bitmap: 0x05
>> +vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
>> vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
>>  bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
>> vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
>> @@ -4045,8 +4045,8 @@ vconn|DBG|unix: sent (Success): NXST_FLOW reply:
>> vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
>>  version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
>> vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
>> - version bitmap: 0x01, 0x05
>> -vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
>> + version bitmap: 0x05
>> +vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
>> vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
>>  bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
>> vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
>> diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
>> index 6f03adb..7375cad 100644
>> --- a/tests/ovs-ofctl.at
>> +++ b/tests/ovs-ofctl.at
>> @@ -2889,8 +2889,8 @@ AT_CHECK([print_vconn_debug | vconn_windows_sub | ofctl_strip], [0], [dnl
>> vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
>>  version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
>> vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
>> - version bitmap: 0x01, 0x05
>> -vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
>> + version bitmap: 0x05
>> +vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
>> vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
>>  bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
>> vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
>> @@ -2926,8 +2926,8 @@ vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
>> vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
>>  version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
>> vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
>> - version bitmap: 0x01, 0x05
>> -vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
>> + version bitmap: 0x05
>> +vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
>> vconn|DBG|unix: received: OFPST_FLOW request (OF1.4):
>> vconn|DBG|unix: sent (Success): OFPST_FLOW reply (OF1.4):
>>  importance=1, dl_vlan=1 actions=drop
>> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
>> index fbc9da4..ee15e1a 100644
>> --- a/utilities/ovs-ofctl.c
>> +++ b/utilities/ovs-ofctl.c
>> @@ -312,6 +312,9 @@ parse_options(int argc, char *argv[])
>>         /* Add implicit allowance for OpenFlow 1.4. */
>>         add_allowed_ofp_versions(ofputil_protocols_to_version_bitmap(
>>                                      OFPUTIL_P_OF14_OXM));
>> +        /* Remove all prior versions. */
>> +        mask_allowed_ofp_versions(ofputil_protocols_to_version_bitmap(
>> +                                     OFPUTIL_P_OF14_UP));
>>     }
>>     versions = get_allowed_ofp_versions();
>>     version_protocols = ofputil_protocols_from_version_bitmap(versions);
>> --
>> 2.1.4
>> 
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org <mailto:dev@openvswitch.org>
>> http://openvswitch.org/mailman/listinfo/dev <http://openvswitch.org/mailman/listinfo/dev>

Patch
diff mbox

diff --git a/tests/ofproto.at b/tests/ofproto.at
index 5e4441c..4c6dd29 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -3962,8 +3962,8 @@  vconn|DBG|unix: sent (Success): OFPT_BARRIER_REPLY:
 vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
  version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
 vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
- version bitmap: 0x01, 0x05
-vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
+ version bitmap: 0x05
+vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
 vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
  bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
 vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
@@ -4014,8 +4014,8 @@  vconn|DBG|unix: sent (Success): NXST_FLOW reply:
 vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
  version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
 vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
- version bitmap: 0x01, 0x05
-vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
+ version bitmap: 0x05
+vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
 vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
  bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
 vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
@@ -4045,8 +4045,8 @@  vconn|DBG|unix: sent (Success): NXST_FLOW reply:
 vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
  version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
 vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
- version bitmap: 0x01, 0x05
-vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
+ version bitmap: 0x05
+vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
 vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
  bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
 vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index 6f03adb..7375cad 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -2889,8 +2889,8 @@  AT_CHECK([print_vconn_debug | vconn_windows_sub | ofctl_strip], [0], [dnl
 vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
  version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
 vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
- version bitmap: 0x01, 0x05
-vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
+ version bitmap: 0x05
+vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
 vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
  bundle_id=0 type=OPEN_REQUEST flags=atomic ordered
 vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
@@ -2926,8 +2926,8 @@  vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
 vconn|DBG|unix: sent (Success): OFPT_HELLO (OF1.5):
  version bitmap: 0x01, 0x02, 0x03, 0x04, 0x05, 0x06
 vconn|DBG|unix: received: OFPT_HELLO (OF1.4):
- version bitmap: 0x01, 0x05
-vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports versions 0x01, 0x05)
+ version bitmap: 0x05
+vconn|DBG|unix: negotiated OpenFlow version 0x05 (we support version 0x06 and earlier, peer supports version 0x05)
 vconn|DBG|unix: received: OFPST_FLOW request (OF1.4):
 vconn|DBG|unix: sent (Success): OFPST_FLOW reply (OF1.4):
  importance=1, dl_vlan=1 actions=drop
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index fbc9da4..ee15e1a 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -312,6 +312,9 @@  parse_options(int argc, char *argv[])
         /* Add implicit allowance for OpenFlow 1.4. */
         add_allowed_ofp_versions(ofputil_protocols_to_version_bitmap(
                                      OFPUTIL_P_OF14_OXM));
+        /* Remove all prior versions. */
+        mask_allowed_ofp_versions(ofputil_protocols_to_version_bitmap(
+                                     OFPUTIL_P_OF14_UP));
     }
     versions = get_allowed_ofp_versions();
     version_protocols = ofputil_protocols_from_version_bitmap(versions);