diff mbox series

[ovs-dev,v2,ovn] Encode the virtual port key in vport_bind action in network byte order

Message ID 20190805134925.14056-1-nusiddiq@redhat.com
State Accepted
Headers show
Series [ovs-dev,v2,ovn] Encode the virtual port key in vport_bind action in network byte order | expand

Commit Message

Numan Siddique Aug. 5, 2019, 1:49 p.m. UTC 
From: Numan Siddique <nusiddiq@redhat.com>

The commit [1] encoded the vport key using uint32_t and the test case
"action parsing" is failing for s380 arch.

This patch fixes this issue by encoding the vport key in the network byte
order.

[1] - 054f4c85c413("Add a new logical switch port type - 'virtual'")
Fixes: 054f4c85c413("Add a new logical switch port type - 'virtual'")

Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
---
v1 -> v2
=======
  * There was a sparse compilation error when I missed checking when
    submitting v1.  Corrected it.


 controller/pinctrl.c | 11 ++++++-----
 lib/actions.c        |  3 ++-
 tests/ovn.at         |  2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

Comments

Dumitru Ceara Aug. 5, 2019, 2:49 p.m. UTC | #1 
On Mon, Aug 5, 2019 at 3:50 PM <nusiddiq@redhat.com> wrote:
>
> From: Numan Siddique <nusiddiq@redhat.com>
>
> The commit [1] encoded the vport key using uint32_t and the test case
> "action parsing" is failing for s380 arch.
>
> This patch fixes this issue by encoding the vport key in the network byte
> order.
>
> [1] - 054f4c85c413("Add a new logical switch port type - 'virtual'")
> Fixes: 054f4c85c413("Add a new logical switch port type - 'virtual'")
>
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>

Looks good to me. Thanks!

Acked-by: Dumitru Ceara <dceara@redhat.com>

> ---
> v1 -> v2
> =======
>   * There was a sparse compilation error when I missed checking when
>     submitting v1.  Corrected it.
>
>
>  controller/pinctrl.c | 11 ++++++-----
>  lib/actions.c        |  3 ++-
>  tests/ovn.at         |  2 +-
>  3 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index f05579fcc..f27718f55 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -4489,16 +4489,17 @@ pinctrl_handle_bind_vport(
>      uint32_t vport_parent_key = md->regs[MFF_LOG_INPORT - MFF_REG0];
>
>      /* Get the virtual port key from the userdata buffer. */
> -    uint32_t *vport_key = ofpbuf_try_pull(userdata, sizeof *vport_key);
> +    ovs_be32 *vp_key = ofpbuf_try_pull(userdata, sizeof *vp_key);
>
> -    if (!vport_key) {
> +    if (!vp_key) {
>          return;
>      }
>
> -    uint32_t hash = hash_2words(dp_key, *vport_key);
> +    uint32_t vport_key = ntohl(*vp_key);
> +    uint32_t hash = hash_2words(dp_key, vport_key);
>
>      struct put_vport_binding *vpb
> -        = pinctrl_find_put_vport_binding(dp_key, *vport_key, hash);
> +        = pinctrl_find_put_vport_binding(dp_key, vport_key, hash);
>      if (!vpb) {
>          if (hmap_count(&put_vport_bindings) >= 1000) {
>              COVERAGE_INC(pinctrl_drop_put_vport_binding);
> @@ -4510,7 +4511,7 @@ pinctrl_handle_bind_vport(
>      }
>
>      vpb->dp_key = dp_key;
> -    vpb->vport_key = *vport_key;
> +    vpb->vport_key = vport_key;
>      vpb->vport_parent_key = vport_parent_key;
>
>      notify_pinctrl_main();
> diff --git a/lib/actions.c b/lib/actions.c
> index 66916a837..b0cb3490b 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -2645,7 +2645,8 @@ encode_BIND_VPORT(const struct ovnact_bind_vport *vp,
>      size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_BIND_VPORT,
>                                                    false, NX_CTLR_NO_METER,
>                                                    ofpacts);
> -    ofpbuf_put(ofpacts, &vport_key, sizeof(uint32_t));
> +    ovs_be32 vp_key = htonl(vport_key);
> +    ofpbuf_put(ofpacts, &vp_key, sizeof(ovs_be32));
>      encode_finish_controller_op(oc_offset, ofpacts);
>      encode_restore_args(args, ARRAY_SIZE(args), ofpacts);
>  }
> diff --git a/tests/ovn.at b/tests/ovn.at
> index e88cffa20..344efad26 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1371,7 +1371,7 @@ reg0[0] = check_pkt_larger(foo);
>  # bind_vport
>  # lsp1's port key is 0x11.
>  bind_vport("lsp1", inport);
> -    encodes as controller(userdata=00.00.00.11.00.00.00.00.11.00.00.00)
> +    encodes as controller(userdata=00.00.00.11.00.00.00.00.00.00.00.11)
>  # lsp2 doesn't exist. So it should be encoded as drop.
>  bind_vport("lsp2", inport);
>      encodes as drop
> --
> 2.21.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Mark Michelson Aug. 5, 2019, 8:37 p.m. UTC | #2 
I applied the change to master.

On 8/5/19 10:49 AM, Dumitru Ceara wrote:
> On Mon, Aug 5, 2019 at 3:50 PM <nusiddiq@redhat.com> wrote:
>>
>> From: Numan Siddique <nusiddiq@redhat.com>
>>
>> The commit [1] encoded the vport key using uint32_t and the test case
>> "action parsing" is failing for s380 arch.
>>
>> This patch fixes this issue by encoding the vport key in the network byte
>> order.
>>
>> [1] - 054f4c85c413("Add a new logical switch port type - 'virtual'")
>> Fixes: 054f4c85c413("Add a new logical switch port type - 'virtual'")
>>
>> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> 
> Looks good to me. Thanks!
> 
> Acked-by: Dumitru Ceara <dceara@redhat.com>
> 
>> ---
>> v1 -> v2
>> =======
>>    * There was a sparse compilation error when I missed checking when
>>      submitting v1.  Corrected it.
>>
>>
>>   controller/pinctrl.c | 11 ++++++-----
>>   lib/actions.c        |  3 ++-
>>   tests/ovn.at         |  2 +-
>>   3 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>> index f05579fcc..f27718f55 100644
>> --- a/controller/pinctrl.c
>> +++ b/controller/pinctrl.c
>> @@ -4489,16 +4489,17 @@ pinctrl_handle_bind_vport(
>>       uint32_t vport_parent_key = md->regs[MFF_LOG_INPORT - MFF_REG0];
>>
>>       /* Get the virtual port key from the userdata buffer. */
>> -    uint32_t *vport_key = ofpbuf_try_pull(userdata, sizeof *vport_key);
>> +    ovs_be32 *vp_key = ofpbuf_try_pull(userdata, sizeof *vp_key);
>>
>> -    if (!vport_key) {
>> +    if (!vp_key) {
>>           return;
>>       }
>>
>> -    uint32_t hash = hash_2words(dp_key, *vport_key);
>> +    uint32_t vport_key = ntohl(*vp_key);
>> +    uint32_t hash = hash_2words(dp_key, vport_key);
>>
>>       struct put_vport_binding *vpb
>> -        = pinctrl_find_put_vport_binding(dp_key, *vport_key, hash);
>> +        = pinctrl_find_put_vport_binding(dp_key, vport_key, hash);
>>       if (!vpb) {
>>           if (hmap_count(&put_vport_bindings) >= 1000) {
>>               COVERAGE_INC(pinctrl_drop_put_vport_binding);
>> @@ -4510,7 +4511,7 @@ pinctrl_handle_bind_vport(
>>       }
>>
>>       vpb->dp_key = dp_key;
>> -    vpb->vport_key = *vport_key;
>> +    vpb->vport_key = vport_key;
>>       vpb->vport_parent_key = vport_parent_key;
>>
>>       notify_pinctrl_main();
>> diff --git a/lib/actions.c b/lib/actions.c
>> index 66916a837..b0cb3490b 100644
>> --- a/lib/actions.c
>> +++ b/lib/actions.c
>> @@ -2645,7 +2645,8 @@ encode_BIND_VPORT(const struct ovnact_bind_vport *vp,
>>       size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_BIND_VPORT,
>>                                                     false, NX_CTLR_NO_METER,
>>                                                     ofpacts);
>> -    ofpbuf_put(ofpacts, &vport_key, sizeof(uint32_t));
>> +    ovs_be32 vp_key = htonl(vport_key);
>> +    ofpbuf_put(ofpacts, &vp_key, sizeof(ovs_be32));
>>       encode_finish_controller_op(oc_offset, ofpacts);
>>       encode_restore_args(args, ARRAY_SIZE(args), ofpacts);
>>   }
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index e88cffa20..344efad26 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -1371,7 +1371,7 @@ reg0[0] = check_pkt_larger(foo);
>>   # bind_vport
>>   # lsp1's port key is 0x11.
>>   bind_vport("lsp1", inport);
>> -    encodes as controller(userdata=00.00.00.11.00.00.00.00.11.00.00.00)
>> +    encodes as controller(userdata=00.00.00.11.00.00.00.00.00.00.00.11)
>>   # lsp2 doesn't exist. So it should be encoded as drop.
>>   bind_vport("lsp2", inport);
>>       encodes as drop
>> --
>> 2.21.0
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index f05579fcc..f27718f55 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -4489,16 +4489,17 @@  pinctrl_handle_bind_vport(
     uint32_t vport_parent_key = md->regs[MFF_LOG_INPORT - MFF_REG0];
 
     /* Get the virtual port key from the userdata buffer. */
-    uint32_t *vport_key = ofpbuf_try_pull(userdata, sizeof *vport_key);
+    ovs_be32 *vp_key = ofpbuf_try_pull(userdata, sizeof *vp_key);
 
-    if (!vport_key) {
+    if (!vp_key) {
         return;
     }
 
-    uint32_t hash = hash_2words(dp_key, *vport_key);
+    uint32_t vport_key = ntohl(*vp_key);
+    uint32_t hash = hash_2words(dp_key, vport_key);
 
     struct put_vport_binding *vpb
-        = pinctrl_find_put_vport_binding(dp_key, *vport_key, hash);
+        = pinctrl_find_put_vport_binding(dp_key, vport_key, hash);
     if (!vpb) {
         if (hmap_count(&put_vport_bindings) >= 1000) {
             COVERAGE_INC(pinctrl_drop_put_vport_binding);
@@ -4510,7 +4511,7 @@  pinctrl_handle_bind_vport(
     }
 
     vpb->dp_key = dp_key;
-    vpb->vport_key = *vport_key;
+    vpb->vport_key = vport_key;
     vpb->vport_parent_key = vport_parent_key;
 
     notify_pinctrl_main();
diff --git a/lib/actions.c b/lib/actions.c
index 66916a837..b0cb3490b 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -2645,7 +2645,8 @@  encode_BIND_VPORT(const struct ovnact_bind_vport *vp,
     size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_BIND_VPORT,
                                                   false, NX_CTLR_NO_METER,
                                                   ofpacts);
-    ofpbuf_put(ofpacts, &vport_key, sizeof(uint32_t));
+    ovs_be32 vp_key = htonl(vport_key);
+    ofpbuf_put(ofpacts, &vp_key, sizeof(ovs_be32));
     encode_finish_controller_op(oc_offset, ofpacts);
     encode_restore_args(args, ARRAY_SIZE(args), ofpacts);
 }
diff --git a/tests/ovn.at b/tests/ovn.at
index e88cffa20..344efad26 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1371,7 +1371,7 @@  reg0[0] = check_pkt_larger(foo);
 # bind_vport
 # lsp1's port key is 0x11.
 bind_vport("lsp1", inport);
-    encodes as controller(userdata=00.00.00.11.00.00.00.00.11.00.00.00)
+    encodes as controller(userdata=00.00.00.11.00.00.00.00.00.00.00.11)
 # lsp2 doesn't exist. So it should be encoded as drop.
 bind_vport("lsp2", inport);
     encodes as drop