diff mbox

[ovs-dev] ofp-actions: Properly interpret "output:in_port".

Message ID 20170612154104.14591-1-blp@ovn.org
State Accepted
Headers show

Commit Message

Ben Pfaff June 12, 2017, 3:41 p.m. UTC 
It was being misinterpreted as output:NXM_OF_IN_PORT[], which is subtly
different and doesn't do anything useful.

CC: Jarno Rajahalme <jarno@ovn.org>
Fixes: 21b2fa617126 ("ofp-parse: Allow match field names in actions and brackets in matches.")
Reported-by: nickcooper-zhangtonghao <nic@opencloud.tech>
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/ofp-actions.c  | 36 +++++++++++++++++++-----------------
 tests/ovs-ofctl.at |  2 ++
 2 files changed, 21 insertions(+), 17 deletions(-)

Comments

Jarno Rajahalme June 12, 2017, 5:24 p.m. UTC | #1 
Acked-by: Jarno Rajahalme <jarno@ovn.org <mailto:jarno@ovn.org>>

Maybe it would be worthwhile elaborating on the subtle difference in the commit message? I.e., “in_port” should match to special OpenFlow port number that outputs to the incoming port, while normally output to the incoming port would be suppressed?

  Jarno

> On Jun 12, 2017, at 8:41 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
> It was being misinterpreted as output:NXM_OF_IN_PORT[], which is subtly
> different and doesn't do anything useful.
> 
> CC: Jarno Rajahalme <jarno@ovn.org>
> Fixes: 21b2fa617126 ("ofp-parse: Allow match field names in actions and brackets in matches.")
> Reported-by: nickcooper-zhangtonghao <nic@opencloud.tech>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
> lib/ofp-actions.c  | 36 +++++++++++++++++++-----------------
> tests/ovs-ofctl.at |  2 ++
> 2 files changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index d5e4623d0291..f9140f4e9a7e 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -635,27 +635,29 @@ parse_OUTPUT(const char *arg,
> 
>         output_trunc = ofpact_put_OUTPUT_TRUNC(ofpacts);
>         return parse_truncate_subfield(output_trunc, arg, port_map);
> -    } else {
> -        struct mf_subfield src;
> -        char *error = mf_parse_subfield(&src, arg);
> -        if (!error) {
> -            struct ofpact_output_reg *output_reg;
> +    }
> 
> -            output_reg = ofpact_put_OUTPUT_REG(ofpacts);
> -            output_reg->max_len = UINT16_MAX;
> -            output_reg->src = src;
> -        } else {
> -            free(error);
> -            struct ofpact_output *output;
> +    ofp_port_t port;
> +    if (ofputil_port_from_string(arg, port_map, &port)) {
> +        struct ofpact_output *output = ofpact_put_OUTPUT(ofpacts);
> +        output->port = port;
> +        output->max_len = output->port == OFPP_CONTROLLER ? UINT16_MAX : 0;
> +        return NULL;
> +    }
> 
> -            output = ofpact_put_OUTPUT(ofpacts);
> -            if (!ofputil_port_from_string(arg, port_map, &output->port)) {
> -                return xasprintf("%s: output to unknown port", arg);
> -            }
> -            output->max_len = output->port == OFPP_CONTROLLER ? UINT16_MAX : 0;
> -        }
> +    struct mf_subfield src;
> +    char *error = mf_parse_subfield(&src, arg);
> +    if (!error) {
> +        struct ofpact_output_reg *output_reg;
> +
> +        output_reg = ofpact_put_OUTPUT_REG(ofpacts);
> +        output_reg->max_len = UINT16_MAX;
> +        output_reg->src = src;
>         return NULL;
>     }
> +    free(error);
> +
> +    return xasprintf("%s: output to unknown port", arg);
> }
> 
> static void
> diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
> index 6afe8f766627..52eaf0320cd5 100644
> --- a/tests/ovs-ofctl.at
> +++ b/tests/ovs-ofctl.at
> @@ -207,6 +207,7 @@ ipv6,actions=ct(commit,nat(src=fe80::20c:29ff:fe88:a18b,random))
> ipv6,actions=ct(commit,nat(src=fe80::20c:29ff:fe88:1-fe80::20c:29ff:fe88:a18b,random))
> ipv6,actions=ct(commit,nat(src=[fe80::20c:29ff:fe88:1]-[fe80::20c:29ff:fe88:a18b]:255-4096,random))
> tcp,actions=ct(commit,nat(src=10.1.1.240-10.1.1.255),alg=ftp)
> +actions=in_port,output:in_port
> ]])
> 
> AT_CHECK([ovs-ofctl parse-flows flows.txt
> @@ -240,6 +241,7 @@ OFPT_FLOW_MOD: ADD ipv6 actions=ct(commit,nat(src=fe80::20c:29ff:fe88:a18b,rando
> OFPT_FLOW_MOD: ADD ipv6 actions=ct(commit,nat(src=fe80::20c:29ff:fe88:1-fe80::20c:29ff:fe88:a18b,random))
> OFPT_FLOW_MOD: ADD ipv6 actions=ct(commit,nat(src=[fe80::20c:29ff:fe88:1]-[fe80::20c:29ff:fe88:a18b]:255-4096,random))
> OFPT_FLOW_MOD: ADD tcp actions=ct(commit,nat(src=10.1.1.240-10.1.1.255),alg=ftp)
> +OFPT_FLOW_MOD: ADD actions=IN_PORT,IN_PORT
> ]])
> AT_CLEANUP
> 
> -- 
> 2.10.2
>
Ben Pfaff June 12, 2017, 6:04 p.m. UTC | #2 
Thanks for the ack.  I applied this to master and branch-2.7.

On Mon, Jun 12, 2017 at 10:24:06AM -0700, Jarno Rajahalme wrote:
> Acked-by: Jarno Rajahalme <jarno@ovn.org <mailto:jarno@ovn.org>>
> 
> Maybe it would be worthwhile elaborating on the subtle difference in the commit message? I.e., “in_port” should match to special OpenFlow port number that outputs to the incoming port, while normally output to the incoming port would be suppressed?
> 
>   Jarno
> 
> > On Jun 12, 2017, at 8:41 AM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > It was being misinterpreted as output:NXM_OF_IN_PORT[], which is subtly
> > different and doesn't do anything useful.
> > 
> > CC: Jarno Rajahalme <jarno@ovn.org>
> > Fixes: 21b2fa617126 ("ofp-parse: Allow match field names in actions and brackets in matches.")
> > Reported-by: nickcooper-zhangtonghao <nic@opencloud.tech>
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> > lib/ofp-actions.c  | 36 +++++++++++++++++++-----------------
> > tests/ovs-ofctl.at |  2 ++
> > 2 files changed, 21 insertions(+), 17 deletions(-)
> > 
> > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> > index d5e4623d0291..f9140f4e9a7e 100644
> > --- a/lib/ofp-actions.c
> > +++ b/lib/ofp-actions.c
> > @@ -635,27 +635,29 @@ parse_OUTPUT(const char *arg,
> > 
> >         output_trunc = ofpact_put_OUTPUT_TRUNC(ofpacts);
> >         return parse_truncate_subfield(output_trunc, arg, port_map);
> > -    } else {
> > -        struct mf_subfield src;
> > -        char *error = mf_parse_subfield(&src, arg);
> > -        if (!error) {
> > -            struct ofpact_output_reg *output_reg;
> > +    }
> > 
> > -            output_reg = ofpact_put_OUTPUT_REG(ofpacts);
> > -            output_reg->max_len = UINT16_MAX;
> > -            output_reg->src = src;
> > -        } else {
> > -            free(error);
> > -            struct ofpact_output *output;
> > +    ofp_port_t port;
> > +    if (ofputil_port_from_string(arg, port_map, &port)) {
> > +        struct ofpact_output *output = ofpact_put_OUTPUT(ofpacts);
> > +        output->port = port;
> > +        output->max_len = output->port == OFPP_CONTROLLER ? UINT16_MAX : 0;
> > +        return NULL;
> > +    }
> > 
> > -            output = ofpact_put_OUTPUT(ofpacts);
> > -            if (!ofputil_port_from_string(arg, port_map, &output->port)) {
> > -                return xasprintf("%s: output to unknown port", arg);
> > -            }
> > -            output->max_len = output->port == OFPP_CONTROLLER ? UINT16_MAX : 0;
> > -        }
> > +    struct mf_subfield src;
> > +    char *error = mf_parse_subfield(&src, arg);
> > +    if (!error) {
> > +        struct ofpact_output_reg *output_reg;
> > +
> > +        output_reg = ofpact_put_OUTPUT_REG(ofpacts);
> > +        output_reg->max_len = UINT16_MAX;
> > +        output_reg->src = src;
> >         return NULL;
> >     }
> > +    free(error);
> > +
> > +    return xasprintf("%s: output to unknown port", arg);
> > }
> > 
> > static void
> > diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
> > index 6afe8f766627..52eaf0320cd5 100644
> > --- a/tests/ovs-ofctl.at
> > +++ b/tests/ovs-ofctl.at
> > @@ -207,6 +207,7 @@ ipv6,actions=ct(commit,nat(src=fe80::20c:29ff:fe88:a18b,random))
> > ipv6,actions=ct(commit,nat(src=fe80::20c:29ff:fe88:1-fe80::20c:29ff:fe88:a18b,random))
> > ipv6,actions=ct(commit,nat(src=[fe80::20c:29ff:fe88:1]-[fe80::20c:29ff:fe88:a18b]:255-4096,random))
> > tcp,actions=ct(commit,nat(src=10.1.1.240-10.1.1.255),alg=ftp)
> > +actions=in_port,output:in_port
> > ]])
> > 
> > AT_CHECK([ovs-ofctl parse-flows flows.txt
> > @@ -240,6 +241,7 @@ OFPT_FLOW_MOD: ADD ipv6 actions=ct(commit,nat(src=fe80::20c:29ff:fe88:a18b,rando
> > OFPT_FLOW_MOD: ADD ipv6 actions=ct(commit,nat(src=fe80::20c:29ff:fe88:1-fe80::20c:29ff:fe88:a18b,random))
> > OFPT_FLOW_MOD: ADD ipv6 actions=ct(commit,nat(src=[fe80::20c:29ff:fe88:1]-[fe80::20c:29ff:fe88:a18b]:255-4096,random))
> > OFPT_FLOW_MOD: ADD tcp actions=ct(commit,nat(src=10.1.1.240-10.1.1.255),alg=ftp)
> > +OFPT_FLOW_MOD: ADD actions=IN_PORT,IN_PORT
> > ]])
> > AT_CLEANUP
> > 
> > -- 
> > 2.10.2
> > 
>
diff mbox

Patch

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index d5e4623d0291..f9140f4e9a7e 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -635,27 +635,29 @@  parse_OUTPUT(const char *arg,
 
         output_trunc = ofpact_put_OUTPUT_TRUNC(ofpacts);
         return parse_truncate_subfield(output_trunc, arg, port_map);
-    } else {
-        struct mf_subfield src;
-        char *error = mf_parse_subfield(&src, arg);
-        if (!error) {
-            struct ofpact_output_reg *output_reg;
+    }
 
-            output_reg = ofpact_put_OUTPUT_REG(ofpacts);
-            output_reg->max_len = UINT16_MAX;
-            output_reg->src = src;
-        } else {
-            free(error);
-            struct ofpact_output *output;
+    ofp_port_t port;
+    if (ofputil_port_from_string(arg, port_map, &port)) {
+        struct ofpact_output *output = ofpact_put_OUTPUT(ofpacts);
+        output->port = port;
+        output->max_len = output->port == OFPP_CONTROLLER ? UINT16_MAX : 0;
+        return NULL;
+    }
 
-            output = ofpact_put_OUTPUT(ofpacts);
-            if (!ofputil_port_from_string(arg, port_map, &output->port)) {
-                return xasprintf("%s: output to unknown port", arg);
-            }
-            output->max_len = output->port == OFPP_CONTROLLER ? UINT16_MAX : 0;
-        }
+    struct mf_subfield src;
+    char *error = mf_parse_subfield(&src, arg);
+    if (!error) {
+        struct ofpact_output_reg *output_reg;
+
+        output_reg = ofpact_put_OUTPUT_REG(ofpacts);
+        output_reg->max_len = UINT16_MAX;
+        output_reg->src = src;
         return NULL;
     }
+    free(error);
+
+    return xasprintf("%s: output to unknown port", arg);
 }
 
 static void
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index 6afe8f766627..52eaf0320cd5 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -207,6 +207,7 @@  ipv6,actions=ct(commit,nat(src=fe80::20c:29ff:fe88:a18b,random))
 ipv6,actions=ct(commit,nat(src=fe80::20c:29ff:fe88:1-fe80::20c:29ff:fe88:a18b,random))
 ipv6,actions=ct(commit,nat(src=[fe80::20c:29ff:fe88:1]-[fe80::20c:29ff:fe88:a18b]:255-4096,random))
 tcp,actions=ct(commit,nat(src=10.1.1.240-10.1.1.255),alg=ftp)
+actions=in_port,output:in_port
 ]])
 
 AT_CHECK([ovs-ofctl parse-flows flows.txt
@@ -240,6 +241,7 @@  OFPT_FLOW_MOD: ADD ipv6 actions=ct(commit,nat(src=fe80::20c:29ff:fe88:a18b,rando
 OFPT_FLOW_MOD: ADD ipv6 actions=ct(commit,nat(src=fe80::20c:29ff:fe88:1-fe80::20c:29ff:fe88:a18b,random))
 OFPT_FLOW_MOD: ADD ipv6 actions=ct(commit,nat(src=[fe80::20c:29ff:fe88:1]-[fe80::20c:29ff:fe88:a18b]:255-4096,random))
 OFPT_FLOW_MOD: ADD tcp actions=ct(commit,nat(src=10.1.1.240-10.1.1.255),alg=ftp)
+OFPT_FLOW_MOD: ADD actions=IN_PORT,IN_PORT
 ]])
 AT_CLEANUP