[ovs-dev] [PATCH ovn v5 01/15] ovn: New module for parsing OVN actions as OpenFlow.
Ben Pfaff
blp at nicira.com
Fri May 1 00:12:26 UTC 2015
On Thu, Apr 30, 2015 at 04:15:13PM -0700, Justin Pettit wrote:
> > On Apr 29, 2015, at 11:48 PM, Ben Pfaff <blp at nicira.com> wrote:
> >
> > + enum lex_type lookahead = lexer_lookahead(ctx->lexer);
> > + if (lookahead == LEX_T_EQUALS || lookahead == LEX_T_LSQUARE) {
>
> I don't think it's clear that bit-level assignment is supported in
> the ovn-sb man page. It might be nice to add.
OK, done.
> > + parse_set_action(ctx);
> > + } else if (lexer_match_id(ctx->lexer, "next")) {
> > + emit_resubmit(ctx, ctx->table_id + 1);
>
> Do you want to enforce that table_id is not equal to the max
> table_id? Maybe it's okay, because the next table won't have any
> flows so it will get dropped? It still seems surprising, and an
> error message might be nice.
OK, done.
Apparently I'd forgotten to fix up the tests to reflect
s/resubmit/next/ earlier, so I fixed that too.
> > + } else if (lexer_match_id(ctx->lexer, "output")) {
> > + emit_resubmit(ctx, 64);
>
> The next patch is the one that limits the available table ids to 32,
> so this value of 64 looks a little odd. Also, I think it's worth
> documenting somewhere what the different tables mean.
OK, I moved that change to this patch and added a comment about the
table numbers. The comment isn't really sufficient, but I don't think
the overall arrangement is finished yet.
> Acked-by: Justin Pettit <jpettit at nicira.com>
Thanks. I'm appending an incremental diff. I folded this into my
"ovn" branch in my ovs-reviews repository.
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 02b7a20..28be688 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -30,7 +30,7 @@ struct action_context {
/* Input. */
struct lexer *lexer; /* Lexer for pulling more tokens. */
const struct shash *symtab; /* Symbol table. */
- uint8_t table_id; /* Current logical table. */
+ uint8_t next_table_id; /* OpenFlow table for 'next' to resubmit. */
const struct simap *ports; /* Map from port name to number. */
/* State. */
@@ -155,8 +155,13 @@ parse_actions(struct action_context *ctx)
if (lookahead == LEX_T_EQUALS || lookahead == LEX_T_LSQUARE) {
parse_set_action(ctx);
} else if (lexer_match_id(ctx->lexer, "next")) {
- emit_resubmit(ctx, ctx->table_id + 1);
+ if (ctx->next_table_id) {
+ emit_resubmit(ctx, ctx->next_table_id);
+ } else {
+ action_error(ctx, "\"next\" action not allowed here.");
+ }
} else if (lexer_match_id(ctx->lexer, "output")) {
+ /* Table 64 does logical-to-physical translation. */
emit_resubmit(ctx, 64);
} else {
action_syntax_error(ctx, "expecting action");
@@ -181,9 +186,8 @@ parse_actions(struct action_context *ctx)
* (as one would provide to expr_to_matches()). Strings used in the actions
* that are not in 'ports' are translated to zero.
*
- * 'table_id' should be the OpenFlow table in which the actions will be used,
- * to allow OVN "next" actions to go to the correct OpenFlow table (that is,
- * table_id+1).
+ * 'next_table_id' should be the OpenFlow table to which the "next" action will
+ * resubmit, or 0 to disable "next".
*
* Some actions add extra requirements (prerequisites) to the flow's match. If
* so, this function sets '*prereqsp' to the actions' prerequisites; otherwise,
@@ -197,7 +201,7 @@ parse_actions(struct action_context *ctx)
*/
char * OVS_WARN_UNUSED_RESULT
actions_parse(struct lexer *lexer, const struct shash *symtab,
- const struct simap *ports, uint8_t table_id,
+ const struct simap *ports, uint8_t next_table_id,
struct ofpbuf *ofpacts, struct expr **prereqsp)
{
size_t ofpacts_start = ofpacts->size;
@@ -206,7 +210,7 @@ actions_parse(struct lexer *lexer, const struct shash *symtab,
ctx.lexer = lexer;
ctx.symtab = symtab;
ctx.ports = ports;
- ctx.table_id = table_id;
+ ctx.next_table_id = next_table_id;
ctx.error = NULL;
ctx.ofpacts = ofpacts;
ctx.prereqs = NULL;
@@ -227,7 +231,7 @@ actions_parse(struct lexer *lexer, const struct shash *symtab,
/* Like actions_parse(), but the actions are taken from 's'. */
char * OVS_WARN_UNUSED_RESULT
actions_parse_string(const char *s, const struct shash *symtab,
- const struct simap *ports, uint8_t table_id,
+ const struct simap *ports, uint8_t next_table_id,
struct ofpbuf *ofpacts, struct expr **prereqsp)
{
struct lexer lexer;
@@ -235,7 +239,8 @@ actions_parse_string(const char *s, const struct shash *symtab,
lexer_init(&lexer, s);
lexer_get(&lexer);
- error = actions_parse(&lexer, symtab, ports, table_id, ofpacts, prereqsp);
+ error = actions_parse(&lexer, symtab, ports, next_table_id,
+ ofpacts, prereqsp);
lexer_destroy(&lexer);
return error;
diff --git a/ovn/lib/actions.h b/ovn/lib/actions.h
index baef953..0139bfc 100644
--- a/ovn/lib/actions.h
+++ b/ovn/lib/actions.h
@@ -27,11 +27,11 @@ struct shash;
struct simap;
char *actions_parse(struct lexer *, const struct shash *symtab,
- const struct simap *ports, uint8_t table_id,
+ const struct simap *ports, uint8_t next_table_id,
struct ofpbuf *ofpacts, struct expr **prereqsp)
OVS_WARN_UNUSED_RESULT;;
char *actions_parse_string(const char *s, const struct shash *symtab,
- const struct simap *ports, uint8_t table_id,
+ const struct simap *ports, uint8_t next_table_id,
struct ofpbuf *ofpacts, struct expr **prereqsp)
OVS_WARN_UNUSED_RESULT;;
diff --git a/ovn/ovn-sb.ovsschema b/ovn/ovn-sb.ovsschema
index 9fd5363..db56211 100644
--- a/ovn/ovn-sb.ovsschema
+++ b/ovn/ovn-sb.ovsschema
@@ -36,7 +36,7 @@
"logical_datapath": {"type": "uuid"},
"table_id": {"type": {"key": {"type": "integer",
"minInteger": 0,
- "maxInteger": 127}}},
+ "maxInteger": 31}}},
"priority": {"type": {"key": {"type": "integer",
"minInteger": 0,
"maxInteger": 65535}}},
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 127d4f1..4bab4e3 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -586,11 +586,19 @@
<dt><code><var>field</var> = <var>constant</var>;</code></dt>
<dd>
Sets data or metadata field <var>field</var> to constant value
- <var>constant</var>. Assigning to a field with prerequisites
+ <var>constant</var>, e.g. <code>outport = "vif0";</code> to set the
+ logical output port. Assigning to a field with prerequisites
implicitly adds those prerequisites to <ref column="match"/>; thus,
for example, a flow that sets <code>tcp.dst</code> applies only to
TCP flows, regardless of whether its <ref column="match"/> mentions
- any TCP field.
+ any TCP field. To set only a subset of bits in a field,
+ <var>field</var> may be a subfield or <var>constant</var> may be
+ masked, e.g. <code>vlan.pcp[2] = 1;</code> and <code>vlan.pcp =
+ 4/4;</code> both set the most sigificant bit of the VLAN PCP. Not
+ all fields are modifiable (e.g. <code>eth.type</code> and
+ <code>ip.proto</code> are read-only), and not all modifiable fields
+ may be partially modified (e.g. <code>ip.ttl</code> must assigned as
+ a whole).
</dd>
</dl>
diff --git a/tests/ovn.at b/tests/ovn.at
index 205d70d..ed79192 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -385,9 +385,9 @@ dnl Text before => is input, text after => is expected output.
AT_DATA([test-cases.txt], [[
# Positive tests.
drop; => actions=drop, prereqs=1
-resubmit; => actions=resubmit(,11), prereqs=1
+next; => actions=resubmit(,11), prereqs=1
output; => actions=resubmit(,64), prereqs=1
-outport="eth0"; resubmit; outport="LOCAL"; resubmit; => actions=set_field:0x5->reg7,resubmit(,11),set_field:0xfffe->reg7,resubmit(,11), prereqs=1
+outport="eth0"; next; outport="LOCAL"; next; => actions=set_field:0x5->reg7,resubmit(,11),set_field:0xfffe->reg7,resubmit(,11), prereqs=1
tcp.dst=80; => actions=set_field:80->tcp_dst, prereqs=ip.proto == 0x6 && (eth.type == 0x800 || eth.type == 0x86dd)
eth.dst[40] = 1; => actions=set_field:01:00:00:00:00:00/01:00:00:00:00:00->eth_dst, prereqs=1
vlan.pcp = 2; => actions=set_field:0x4000/0xe000->vlan_tci, prereqs=vlan.tci[12]
@@ -397,15 +397,15 @@ vlan.tci[13..15] = 2; => actions=set_field:0x4000/0xe000->vlan_tci, prereqs=1
; => Syntax error at `;'.
xyzzy; => Syntax error at `xyzzy' expecting action.
-resubmit; 123; => Syntax error at `123'.
-resubmit; xyzzy; => Syntax error at `xyzzy' expecting action.
+next; 123; => Syntax error at `123'.
+next; xyzzy; => Syntax error at `xyzzy' expecting action.
# "drop;" must be on its own:
-drop; resubmit; => Syntax error at `resubmit' expecting end of input.
-resubmit; drop; => Syntax error at `drop' expecting action.
+drop; next; => Syntax error at `next' expecting end of input.
+next; drop; => Syntax error at `drop' expecting action.
# Missing ";":
-resubmit => Syntax error at end of input expecting ';'.
+next => Syntax error at end of input expecting ';'.
inport[1] = 1; => Cannot select subfield of string field inport.
ip.proto[1] = 1; => Cannot select subfield of nominal field ip.proto.
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 9e56cee..1a005b8 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -1136,7 +1136,7 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
char *error;
ofpbuf_init(&ofpacts, 0);
- error = actions_parse_string(ds_cstr(&input), &symtab, &ports, 10,
+ error = actions_parse_string(ds_cstr(&input), &symtab, &ports, 11,
&ofpacts, &prereqs);
if (!error) {
struct ds output;
--
1.7.10.4
More information about the dev
mailing list