[ovs-dev] ofproto-dpif-xlate: Remove assertion for truncated

Message ID 1507125256-7420-1-git-send-email-iwase.yusuke0@gmail.com
State New
Headers show
Series
  • [ovs-dev] ofproto-dpif-xlate: Remove assertion for truncated
Related show

Commit Message

Iwase Yusuke Oct. 4, 2017, 1:54 p.m.
Because OpenFlow Spec does not clearly stipulate that "max_len" in
OUTPUT action must be zero when "port" is other than OFPP_CONTROLLER,
it is too strict assertion that confirm "max_len" is not zero, and
"max_len" should be ignored when not used.
Also this assertion causes the lack of the interoperability with some
controller implementations.

This patch removes these redundant assertions of if truncated or not.

Signed-off-by: IWASE Yusuke <iwase.yusuke0@gmail.com>
---
 ofproto/ofproto-dpif-xlate.c | 5 -----
 1 file changed, 5 deletions(-)

Comments

Iwase Yusuke Oct. 10, 2017, 2:39 a.m. | #1
Hi,

I'm very sorry for disturbing you.

Could someone review this patch? Or are there some more procedures for submitting patch?

Thanks,
Iwase


On 2017年10月04日 22:54, IWASE Yusuke wrote:
> Because OpenFlow Spec does not clearly stipulate that "max_len" in
> OUTPUT action must be zero when "port" is other than OFPP_CONTROLLER,
> it is too strict assertion that confirm "max_len" is not zero, and
> "max_len" should be ignored when not used.
> Also this assertion causes the lack of the interoperability with some
> controller implementations.
> 
> This patch removes these redundant assertions of if truncated or not.
> 
> Signed-off-by: IWASE Yusuke <iwase.yusuke0@gmail.com>
> ---
>   ofproto/ofproto-dpif-xlate.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index d320d57..c5ed6a0 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3700,7 +3700,6 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
>       }
>   
>       if (xport->peer) {
> -       ovs_assert(!truncate)
>          patch_port_output(ctx, xport, xport->peer);
>          return;
>       }
> @@ -4839,21 +4838,17 @@ xlate_output_action(struct xlate_ctx *ctx,
>                                 is_last_action, truncate);
>           break;
>       case OFPP_TABLE:
> -        ovs_assert(!truncate);
>           xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
>                              0, may_packet_in, true, false, false,
>                              do_xlate_actions);
>           break;
>       case OFPP_NORMAL:
> -        ovs_assert(!truncate);
>           xlate_normal(ctx);
>           break;
>       case OFPP_FLOOD:
> -        ovs_assert(!truncate);
>           flood_packets(ctx, false, is_last_action);
>           break;
>       case OFPP_ALL:
> -        ovs_assert(!truncate);
>           flood_packets(ctx, true, is_last_action);
>           break;
>       case OFPP_CONTROLLER:
>
Ben Pfaff Oct. 10, 2017, 3:05 a.m. | #2
Andy, can you take a quick look at this?  It appears to me that it is
trivial to trigger this assertion by specifying a max_len in any
OpenFlow output action.  I guess that wasn't the intention, but maybe
there is some other importance purpose for the assertion that should be
retained.

Thanks,

Ben.

On Wed, Oct 04, 2017 at 10:54:16PM +0900, IWASE Yusuke wrote:
> Because OpenFlow Spec does not clearly stipulate that "max_len" in
> OUTPUT action must be zero when "port" is other than OFPP_CONTROLLER,
> it is too strict assertion that confirm "max_len" is not zero, and
> "max_len" should be ignored when not used.
> Also this assertion causes the lack of the interoperability with some
> controller implementations.
> 
> This patch removes these redundant assertions of if truncated or not.
> 
> Signed-off-by: IWASE Yusuke <iwase.yusuke0@gmail.com>
> ---
>  ofproto/ofproto-dpif-xlate.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index d320d57..c5ed6a0 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3700,7 +3700,6 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
>      }
>  
>      if (xport->peer) {
> -       ovs_assert(!truncate)
>         patch_port_output(ctx, xport, xport->peer);
>         return;
>      }
> @@ -4839,21 +4838,17 @@ xlate_output_action(struct xlate_ctx *ctx,
>                                is_last_action, truncate);
>          break;
>      case OFPP_TABLE:
> -        ovs_assert(!truncate);
>          xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
>                             0, may_packet_in, true, false, false,
>                             do_xlate_actions);
>          break;
>      case OFPP_NORMAL:
> -        ovs_assert(!truncate);
>          xlate_normal(ctx);
>          break;
>      case OFPP_FLOOD:
> -        ovs_assert(!truncate);
>          flood_packets(ctx, false, is_last_action);
>          break;
>      case OFPP_ALL:
> -        ovs_assert(!truncate);
>          flood_packets(ctx, true, is_last_action);
>          break;
>      case OFPP_CONTROLLER:
> -- 
> 2.7.4
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ben Pfaff Oct. 10, 2017, 3:08 a.m. | #3
You submitted the patch correctly.  It is my fault because I've been
slow about reviews lately.

I think that Andy Zhou should take a look at this patch since he added
the assertions.  I sent him a message asking about it.

On Tue, Oct 10, 2017 at 11:39:42AM +0900, Iwase Yusuke wrote:
> Hi,
> 
> I'm very sorry for disturbing you.
> 
> Could someone review this patch? Or are there some more procedures for submitting patch?
> 
> Thanks,
> Iwase
> 
> 
> On 2017年10月04日 22:54, IWASE Yusuke wrote:
> >Because OpenFlow Spec does not clearly stipulate that "max_len" in
> >OUTPUT action must be zero when "port" is other than OFPP_CONTROLLER,
> >it is too strict assertion that confirm "max_len" is not zero, and
> >"max_len" should be ignored when not used.
> >Also this assertion causes the lack of the interoperability with some
> >controller implementations.
> >
> >This patch removes these redundant assertions of if truncated or not.
> >
> >Signed-off-by: IWASE Yusuke <iwase.yusuke0@gmail.com>
> >---
> >  ofproto/ofproto-dpif-xlate.c | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> >diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> >index d320d57..c5ed6a0 100644
> >--- a/ofproto/ofproto-dpif-xlate.c
> >+++ b/ofproto/ofproto-dpif-xlate.c
> >@@ -3700,7 +3700,6 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
> >      }
> >      if (xport->peer) {
> >-       ovs_assert(!truncate)
> >         patch_port_output(ctx, xport, xport->peer);
> >         return;
> >      }
> >@@ -4839,21 +4838,17 @@ xlate_output_action(struct xlate_ctx *ctx,
> >                                is_last_action, truncate);
> >          break;
> >      case OFPP_TABLE:
> >-        ovs_assert(!truncate);
> >          xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
> >                             0, may_packet_in, true, false, false,
> >                             do_xlate_actions);
> >          break;
> >      case OFPP_NORMAL:
> >-        ovs_assert(!truncate);
> >          xlate_normal(ctx);
> >          break;
> >      case OFPP_FLOOD:
> >-        ovs_assert(!truncate);
> >          flood_packets(ctx, false, is_last_action);
> >          break;
> >      case OFPP_ALL:
> >-        ovs_assert(!truncate);
> >          flood_packets(ctx, true, is_last_action);
> >          break;
> >      case OFPP_CONTROLLER:
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Iwase Yusuke Oct. 10, 2017, 3:18 a.m. | #4
Hi Ben and Andy,

Thank you very much!
I'm looking forward to review comments.


On 2017年10月10日 12:08, Ben Pfaff wrote:
> You submitted the patch correctly.  It is my fault because I've been
> slow about reviews lately.
> 
> I think that Andy Zhou should take a look at this patch since he added
> the assertions.  I sent him a message asking about it.
> 
> On Tue, Oct 10, 2017 at 11:39:42AM +0900, Iwase Yusuke wrote:
>> Hi,
>>
>> I'm very sorry for disturbing you.
>>
>> Could someone review this patch? Or are there some more procedures for submitting patch?
>>
>> Thanks,
>> Iwase
>>
>>
>> On 2017年10月04日 22:54, IWASE Yusuke wrote:
>>> Because OpenFlow Spec does not clearly stipulate that "max_len" in
>>> OUTPUT action must be zero when "port" is other than OFPP_CONTROLLER,
>>> it is too strict assertion that confirm "max_len" is not zero, and
>>> "max_len" should be ignored when not used.
>>> Also this assertion causes the lack of the interoperability with some
>>> controller implementations.
>>>
>>> This patch removes these redundant assertions of if truncated or not.
>>>
>>> Signed-off-by: IWASE Yusuke <iwase.yusuke0@gmail.com>
>>> ---
>>>   ofproto/ofproto-dpif-xlate.c | 5 -----
>>>   1 file changed, 5 deletions(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index d320d57..c5ed6a0 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -3700,7 +3700,6 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
>>>       }
>>>       if (xport->peer) {
>>> -       ovs_assert(!truncate)
>>>          patch_port_output(ctx, xport, xport->peer);
>>>          return;
>>>       }
>>> @@ -4839,21 +4838,17 @@ xlate_output_action(struct xlate_ctx *ctx,
>>>                                 is_last_action, truncate);
>>>           break;
>>>       case OFPP_TABLE:
>>> -        ovs_assert(!truncate);
>>>           xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
>>>                              0, may_packet_in, true, false, false,
>>>                              do_xlate_actions);
>>>           break;
>>>       case OFPP_NORMAL:
>>> -        ovs_assert(!truncate);
>>>           xlate_normal(ctx);
>>>           break;
>>>       case OFPP_FLOOD:
>>> -        ovs_assert(!truncate);
>>>           flood_packets(ctx, false, is_last_action);
>>>           break;
>>>       case OFPP_ALL:
>>> -        ovs_assert(!truncate);
>>>           flood_packets(ctx, true, is_last_action);
>>>           break;
>>>       case OFPP_CONTROLLER:
>>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Andy Zhou Oct. 10, 2017, 7:21 a.m. | #5
On Mon, Oct 9, 2017 at 8:18 PM, Iwase Yusuke <iwase.yusuke0@gmail.com> wrote:
> Hi Ben and Andy,
>
> Thank you very much!
> I'm looking forward to review comments.
>
>
Hi, Iwase,

I miss interpreted max_len in the spec, and thought (wrongly) that
only the output controller action should have a non-zero value.
Apparently, this
is too restrictive as you have pointed out.

In terms of the fix. I think we can remove the 'truncate" variable
completely. If the following incremental looks O.K. to you, I'd like
to fold it in before pushing your patch to master.

Thanks for reporting the bug!


diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index d320d570b304..a492c2215851 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4829,31 +4829,26 @@ xlate_output_action(struct xlate_ctx *ctx,
                     bool is_last_action)
 {
     ofp_port_t prev_nf_output_iface = ctx->nf_output_iface;
-    bool truncate = max_len != 0;

     ctx->nf_output_iface = NF_OUT_DROP;

     switch (port) {
     case OFPP_IN_PORT:
         compose_output_action(ctx, ctx->xin->flow.in_port.ofp_port, NULL,
-                              is_last_action, truncate);
+                              is_last_action, false);
         break;
     case OFPP_TABLE:
-        ovs_assert(!truncate);
         xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
                            0, may_packet_in, true, false, false,
                            do_xlate_actions);
         break;
     case OFPP_NORMAL:
-        ovs_assert(!truncate);
         xlate_normal(ctx);
         break;
     case OFPP_FLOOD:
-        ovs_assert(!truncate);
         flood_packets(ctx, false, is_last_action);
         break;
     case OFPP_ALL:
-        ovs_assert(!truncate);
         flood_packets(ctx, true, is_last_action);
         break;
     case OFPP_CONTROLLER:
@@ -4869,7 +4864,7 @@ xlate_output_action(struct xlate_ctx *ctx,
     case OFPP_LOCAL:
     default:
         if (port != ctx->xin->flow.in_port.ofp_port) {
-            compose_output_action(ctx, port, NULL, is_last_action, truncate);
+            compose_output_action(ctx, port, NULL, is_last_action, false);
         } else {
             xlate_report(ctx, OFT_WARN, "skipping output to input port");
         }
Iwase Yusuke Oct. 10, 2017, 7:28 a.m. | #6
Hi Andy,

Thank you for your quick response.

Your patch looks good to me.
I couldn't judge whether we can "truncate" completely or not, that sounds great!

Thanks,
Iwase


On 2017年10月10日 16:21, Andy Zhou wrote:
> On Mon, Oct 9, 2017 at 8:18 PM, Iwase Yusuke <iwase.yusuke0@gmail.com> wrote:
>> Hi Ben and Andy,
>>
>> Thank you very much!
>> I'm looking forward to review comments.
>>
>>
> Hi, Iwase,
> 
> I miss interpreted max_len in the spec, and thought (wrongly) that
> only the output controller action should have a non-zero value.
> Apparently, this
> is too restrictive as you have pointed out.
> 
> In terms of the fix. I think we can remove the 'truncate" variable
> completely. If the following incremental looks O.K. to you, I'd like
> to fold it in before pushing your patch to master.
> 
> Thanks for reporting the bug!
> 
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index d320d570b304..a492c2215851 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4829,31 +4829,26 @@ xlate_output_action(struct xlate_ctx *ctx,
>                       bool is_last_action)
>   {
>       ofp_port_t prev_nf_output_iface = ctx->nf_output_iface;
> -    bool truncate = max_len != 0;
> 
>       ctx->nf_output_iface = NF_OUT_DROP;
> 
>       switch (port) {
>       case OFPP_IN_PORT:
>           compose_output_action(ctx, ctx->xin->flow.in_port.ofp_port, NULL,
> -                              is_last_action, truncate);
> +                              is_last_action, false);
>           break;
>       case OFPP_TABLE:
> -        ovs_assert(!truncate);
>           xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
>                              0, may_packet_in, true, false, false,
>                              do_xlate_actions);
>           break;
>       case OFPP_NORMAL:
> -        ovs_assert(!truncate);
>           xlate_normal(ctx);
>           break;
>       case OFPP_FLOOD:
> -        ovs_assert(!truncate);
>           flood_packets(ctx, false, is_last_action);
>           break;
>       case OFPP_ALL:
> -        ovs_assert(!truncate);
>           flood_packets(ctx, true, is_last_action);
>           break;
>       case OFPP_CONTROLLER:
> @@ -4869,7 +4864,7 @@ xlate_output_action(struct xlate_ctx *ctx,
>       case OFPP_LOCAL:
>       default:
>           if (port != ctx->xin->flow.in_port.ofp_port) {
> -            compose_output_action(ctx, port, NULL, is_last_action, truncate);
> +            compose_output_action(ctx, port, NULL, is_last_action, false);
>           } else {
>               xlate_report(ctx, OFT_WARN, "skipping output to input port");
>           }
>
Andy Zhou Oct. 10, 2017, 7:29 p.m. | #7
On Tue, Oct 10, 2017 at 12:28 AM, Iwase Yusuke <iwase.yusuke0@gmail.com> wrote:
> Hi Andy,
>
> Thank you for your quick response.
>
> Your patch looks good to me.
> I couldn't judge whether we can "truncate" completely or not, that sounds
> great!
>
> Thanks,
> Iwase
>
Thanks. I have pushed the patch to master. Also added your name to the
AUTHORS.rst file.
Iwase Yusuke Oct. 11, 2017, 1:46 a.m. | #8
Hi Andy,

Thanks so much!
I will test it with my controller application.

Thanks,
Iwase

On 2017年10月11日 04:29, Andy Zhou wrote:
> On Tue, Oct 10, 2017 at 12:28 AM, Iwase Yusuke <iwase.yusuke0@gmail.com> wrote:
>> Hi Andy,
>>
>> Thank you for your quick response.
>>
>> Your patch looks good to me.
>> I couldn't judge whether we can "truncate" completely or not, that sounds
>> great!
>>
>> Thanks,
>> Iwase
>>
> Thanks. I have pushed the patch to master. Also added your name to the
> AUTHORS.rst file.
>

Patch

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index d320d57..c5ed6a0 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3700,7 +3700,6 @@  compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
     }
 
     if (xport->peer) {
-       ovs_assert(!truncate)
        patch_port_output(ctx, xport, xport->peer);
        return;
     }
@@ -4839,21 +4838,17 @@  xlate_output_action(struct xlate_ctx *ctx,
                               is_last_action, truncate);
         break;
     case OFPP_TABLE:
-        ovs_assert(!truncate);
         xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
                            0, may_packet_in, true, false, false,
                            do_xlate_actions);
         break;
     case OFPP_NORMAL:
-        ovs_assert(!truncate);
         xlate_normal(ctx);
         break;
     case OFPP_FLOOD:
-        ovs_assert(!truncate);
         flood_packets(ctx, false, is_last_action);
         break;
     case OFPP_ALL:
-        ovs_assert(!truncate);
         flood_packets(ctx, true, is_last_action);
         break;
     case OFPP_CONTROLLER: