| Message ID | 1507125256-7420-1-git-send-email-iwase.yusuke0@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ofproto-dpif-xlate: Remove assertion for truncated | expand |
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: >
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
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
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
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"); }
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"); > } >
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.
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. >
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:
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(-)