| Message ID | 1443646592-20290-3-git-send-email-blp@nicira.com |
|---|---|
| State | Accepted |
| Headers | show |
Acked-by: Justin Pettit <jpettit@nicira.com> --Justin > On Sep 30, 2015, at 1:56 PM, Ben Pfaff <blp@nicira.com> wrote: > > Signed-off-by: Ben Pfaff <blp@nicira.com> > --- > ovn/TODO | 4 +- > ovn/lib/expr.c | 186 +++++++++++++++++++++++++++++++++++-------------------- > ovn/ovn-sb.xml | 35 ++++++++--- > tests/ovn.at | 14 ++++- > tests/test-ovn.c | 1 + > 5 files changed, 159 insertions(+), 81 deletions(-) > > diff --git a/ovn/TODO b/ovn/TODO > index c83a287..740ea17 100644 > --- a/ovn/TODO > +++ b/ovn/TODO > @@ -132,8 +132,8 @@ the "arp" action, and an action for generating > > *** Other actions. > > -Possibly we'll need to implement "field1 = field2;" for copying > -between fields and "field1 <-> field2;" for swapping fields. > +Possibly we'll need to implement "field1 <-> field2;" for swapping > +fields. > > *** ovn-controller translation to OpenFlow > > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c > index 71d817b..b7a3f7f 100644 > --- a/ovn/lib/expr.c > +++ b/ovn/lib/expr.c > @@ -2619,113 +2619,165 @@ expr_is_normalized(const struct expr *expr) > > /* Action parsing helper. */ > > -static struct expr * > -parse_assignment(struct expr_context *ctx, const struct simap *ports, > - struct ofpbuf *ofpacts) > +static bool > +expand_symbol(struct expr_context *ctx, struct expr_field *f, > + struct expr **prereqsp) > { > - struct expr *prereqs = NULL; > - > - struct expr_field f; > - if (!parse_field(ctx, &f)) { > - goto exit; > - } > - if (!lexer_match(ctx->lexer, LEX_T_EQUALS)) { > - expr_syntax_error(ctx, "expecting `='."); > - goto exit; > - } > - > - if (f.symbol->expansion && f.symbol->level != EXPR_L_ORDINAL) { > - expr_error(ctx, "Can't assign to predicate symbol %s.", > - f.symbol->name); > - goto exit; > - } > - > - struct expr_constant_set cs; > - if (!parse_constant_set(ctx, &cs)) { > - goto exit; > - } > - > - if (!type_check(ctx, &f, &cs)) { > - goto exit_destroy_cs; > - } > - if (cs.in_curlies) { > - expr_error(ctx, "Assignments require a single value."); > - goto exit_destroy_cs; > + if (f->symbol->expansion && f->symbol->level != EXPR_L_ORDINAL) { > + expr_error(ctx, "Predicate symbol %s cannot be used in assignment.", > + f->symbol->name); > + return false; > } > > - const struct expr_symbol *orig_symbol = f.symbol; > - union expr_constant *c = cs.values; > for (;;) { > /* Accumulate prerequisites. */ > - if (f.symbol->prereqs) { > + if (f->symbol->prereqs) { > struct ovs_list nesting = OVS_LIST_INITIALIZER(&nesting); > char *error; > struct expr *e; > - e = parse_and_annotate(f.symbol->prereqs, ctx->symtab, &nesting, > + e = parse_and_annotate(f->symbol->prereqs, ctx->symtab, &nesting, > &error); > if (error) { > expr_error(ctx, "%s", error); > free(error); > - goto exit_destroy_cs; > + return false; > } > - prereqs = expr_combine(EXPR_T_AND, prereqs, e); > + *prereqsp = expr_combine(EXPR_T_AND, *prereqsp, e); > } > > /* If there's no expansion, we're done. */ > - if (!f.symbol->expansion) { > + if (!f->symbol->expansion) { > break; > } > > /* Expand. */ > struct expr_field expansion; > char *error; > - if (!parse_field_from_string(f.symbol->expansion, ctx->symtab, > + if (!parse_field_from_string(f->symbol->expansion, ctx->symtab, > &expansion, &error)) { > expr_error(ctx, "%s", error); > free(error); > - goto exit_destroy_cs; > + return false; > } > - f.symbol = expansion.symbol; > - f.ofs += expansion.ofs; > + f->symbol = expansion.symbol; > + f->ofs += expansion.ofs; > } > > - if (!f.symbol->field->writable) { > - expr_error(ctx, "Field %s is not modifiable.", orig_symbol->name); > - goto exit_destroy_cs; > + return true; > +} > + > +static struct expr * > +parse_assignment(struct expr_context *ctx, const struct simap *ports, > + struct ofpbuf *ofpacts) > +{ > + struct expr *prereqs = NULL; > + > + /* Parse destination and do basic checking. */ > + struct expr_field dst; > + if (!parse_field(ctx, &dst)) { > + goto exit; > + } > + if (!lexer_match(ctx->lexer, LEX_T_EQUALS)) { > + expr_syntax_error(ctx, "expecting `='."); > + goto exit; > + } > + const struct expr_symbol *orig_dst = dst.symbol; > + if (!expand_symbol(ctx, &dst, &prereqs)) { > + goto exit; > + } > + if (!dst.symbol->field->writable) { > + expr_error(ctx, "Field %s is not modifiable.", orig_dst->name); > + goto exit; > } > > - struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts); > - sf->field = f.symbol->field; > - if (f.symbol->width) { > - mf_subvalue_shift(&c->value, f.ofs); > - if (!c->masked) { > - memset(&c->mask, 0, sizeof c->mask); > - bitwise_one(&c->mask, sizeof c->mask, f.ofs, f.n_bits); > - } else { > - mf_subvalue_shift(&c->mask, f.ofs); > + if (ctx->lexer->token.type == LEX_T_ID) { > + struct expr_field src; > + if (!parse_field(ctx, &src)) { > + goto exit; > + } > + const struct expr_symbol *orig_src = src.symbol; > + if (!expand_symbol(ctx, &src, &prereqs)) { > + goto exit; > + } > + > + if ((dst.symbol->width != 0) != (src.symbol->width != 0)) { > + expr_error(ctx, "Can't assign %s field (%s) to %s field (%s).", > + orig_src->width ? "integer" : "string", > + orig_src->name, > + orig_dst->width ? "integer" : "string", > + orig_dst->name); > + goto exit; > + } > + > + if (dst.n_bits != src.n_bits) { > + expr_error(ctx, "Can't assign %d-bit value to %d-bit destination.", > + src.n_bits, dst.n_bits); > + goto exit; > + } else if (!dst.n_bits > + && dst.symbol->field->n_bits != src.symbol->field->n_bits) { > + expr_error(ctx, "String fields %s and %s are incompatible for " > + "assignment.", orig_dst->name, orig_src->name); > + goto exit; > } > > - memcpy(&sf->value, &c->value.u8[sizeof c->value - sf->field->n_bytes], > - sf->field->n_bytes); > - memcpy(&sf->mask, &c->mask.u8[sizeof c->mask - sf->field->n_bytes], > - sf->field->n_bytes); > + struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts); > + move->src.field = src.symbol->field; > + move->src.ofs = src.ofs; > + move->src.n_bits = src.n_bits ? src.n_bits : src.symbol->field->n_bits; > + move->dst.field = dst.symbol->field; > + move->dst.ofs = dst.ofs; > + move->dst.n_bits = dst.n_bits ? dst.n_bits : dst.symbol->field->n_bits; > } else { > - uint32_t port = simap_get(ports, c->string); > - bitwise_put(port, &sf->value, > - sf->field->n_bytes, 0, sf->field->n_bits); > - bitwise_put(UINT64_MAX, &sf->mask, > - sf->field->n_bytes, 0, sf->field->n_bits); > + struct expr_constant_set cs; > + if (!parse_constant_set(ctx, &cs)) { > + goto exit; > + } > + > + if (!type_check(ctx, &dst, &cs)) { > + goto exit_destroy_cs; > + } > + if (cs.in_curlies) { > + expr_error(ctx, "Assignments require a single value."); > + goto exit_destroy_cs; > + } > + > + union expr_constant *c = cs.values; > + struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts); > + sf->field = dst.symbol->field; > + if (dst.symbol->width) { > + mf_subvalue_shift(&c->value, dst.ofs); > + if (!c->masked) { > + memset(&c->mask, 0, sizeof c->mask); > + bitwise_one(&c->mask, sizeof c->mask, dst.ofs, dst.n_bits); > + } else { > + mf_subvalue_shift(&c->mask, dst.ofs); > + } > + > + memcpy(&sf->value, > + &c->value.u8[sizeof c->value - sf->field->n_bytes], > + sf->field->n_bytes); > + memcpy(&sf->mask, > + &c->mask.u8[sizeof c->mask - sf->field->n_bytes], > + sf->field->n_bytes); > + } else { > + uint32_t port = simap_get(ports, c->string); > + bitwise_put(port, &sf->value, > + sf->field->n_bytes, 0, sf->field->n_bits); > + bitwise_put(UINT64_MAX, &sf->mask, > + sf->field->n_bytes, 0, sf->field->n_bits); > + } > + > + exit_destroy_cs: > + expr_constant_set_destroy(&cs); > } > > -exit_destroy_cs: > - expr_constant_set_destroy(&cs); > exit: > return prereqs; > } > > /* A helper for actions_parse(), to parse an OVN assignment action in the form > - * "field = value" into 'ofpacts'. The parameters and return value match those > - * for actions_parse(). */ > + * "field = value" or "field1 = field2" into 'ofpacts'. The parameters and > + * return value match those for actions_parse(). */ > char * > expr_parse_assignment(struct lexer *lexer, const struct shash *symtab, > const struct simap *ports, > diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml > index da14fe4..28b0d2c 100644 > --- a/ovn/ovn-sb.xml > +++ b/ovn/ovn-sb.xml > @@ -800,26 +800,41 @@ > pipeline. > </p> > </dd> > - </dl> > > - <p> > - The following actions will likely be useful later, but they have not > - been thought out carefully. > - </p> > - > - <dl> > <dt><code><var>field1</var> = <var>field2</var>;</code></dt> > <dd> > <p> > - Extends the assignment action to allow copying between fields. > + Sets data or metadata field <var>field1</var> to the value of data > + or metadata field <var>field2</var>, e.g. <code>reg0 = > + ip4.src;</code> to copy <code>ip4.src</code> into > + <code>reg0</code>. To modify only a subset of a field's bits, > + specify a subfield for <var>field1</var> or <var>field2</var> or > + both, e.g. <code>vlan.pcp = reg0[0..2];</code> to set the VLAN PCP > + from the least-significant bits of <code>reg0</code>. > </p> > > <p> > - An assignment adds prerequisites from the source and the > - destination fields. > + <var>field1</var> and <var>field2</var> must be the same type, > + either both string or both integer fields. If they are both > + integer fields, they must have the same width. > + </p> > + > + <p> > + If <var>field1</var> or <var>field2</var> has prerequisites, they > + are added implicitly to <ref column="match"/>. It is possible to > + write an assignment with contradictory prerequisites, such as > + <code>ip4.src = ip6.src[0..31];</code>, but the contradiction means > + that a logical flow with such an assignment will never be matched. > </p> > </dd> > + </dl> > + > + <p> > + The following actions will likely be useful later, but they have not > + been thought out carefully. > + </p> > > + <dl> > <dt><code>ip.ttl--;</code></dt> > <dd> > <p> > diff --git a/tests/ovn.at b/tests/ovn.at > index 70fc086..96f190d 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -444,6 +444,12 @@ tcp.dst=80; => actions=set_field:80->tcp_dst, prereqs=ip.proto == 0x6 && (eth.ty > 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] > vlan.tci[13..15] = 2; => actions=set_field:0x4000/0xe000->vlan_tci, prereqs=1 > +reg0 = reg1; => actions=move:OXM_OF_PKT_REG0[0..31]->OXM_OF_PKT_REG0[32..63], prereqs=1 > +vlan.pcp = reg0[0..2]; => actions=move:OXM_OF_PKT_REG0[32..34]->NXM_OF_VLAN_TCI[13..15], prereqs=vlan.tci[12] > +reg0[10] = vlan.pcp[1]; => actions=move:NXM_OF_VLAN_TCI[14]->OXM_OF_PKT_REG0[42], prereqs=vlan.tci[12] > +outport = inport; => actions=move:NXM_NX_REG6[]->NXM_NX_REG7[], prereqs=1 > +# Contradictionary prerequisites (allowed but not useful): > +ip4.src = ip6.src[0..31]; => actions=move:NXM_NX_IPV6_SRC[0..31]->NXM_OF_IP_SRC[], prereqs=eth.type == 0x800 && eth.type == 0x86dd > > ## Negative tests. > > @@ -462,13 +468,17 @@ 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. > eth.dst[40] == 1; => Syntax error at `==' expecting `='. > -ip = 1; => Can't assign to predicate symbol ip. > +ip = 1; => Predicate symbol ip cannot be used in assignment. > ip.proto = 6; => Field ip.proto is not modifiable. > inport = {"a", "b"}; => Assignments require a single value. > inport = {}; => Syntax error at `}' expecting constant. > bad_prereq = 123; => Error parsing expression `xyzzy' encountered as prerequisite or predicate of initial expression: Syntax error at `xyzzy' expecting field name. > self_recurse = 123; => Error parsing expression `self_recurse != 0' encountered as prerequisite or predicate of initial expression: Error parsing expression `self_recurse != 0' encountered as prerequisite or predicate of initial expression: Recursive expansion of symbol `self_recurse'. > -vlan.present = 0; => Can't assign to predicate symbol vlan.present. > +vlan.present = 0; => Predicate symbol vlan.present cannot be used in assignment. > +reg0[0] = vlan.present; => Predicate symbol vlan.present cannot be used in assignment. > +reg0 = reg1[0..10]; => Can't assign 11-bit value to 32-bit destination. > +inport = reg0; => Can't assign integer field (reg0) to string field (inport). > +inport = big_string; => String fields inport and big_string are incompatible for assignment. > ]]) > sed 's/ =>.*//' test-cases.txt > input.txt > sed 's/.* => //' test-cases.txt > expout > diff --git a/tests/test-ovn.c b/tests/test-ovn.c > index ecb3b5c..c806068 100644 > --- a/tests/test-ovn.c > +++ b/tests/test-ovn.c > @@ -235,6 +235,7 @@ create_symtab(struct shash *symtab) > "mutual_recurse_2 != 0", false); > expr_symtab_add_field(symtab, "mutual_recurse_2", MFF_XREG0, > "mutual_recurse_1 != 0", false); > + expr_symtab_add_string(symtab, "big_string", MFF_XREG0, NULL); > } > > static void > -- > 2.1.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
Thanks, applied to master. On Wed, Oct 07, 2015 at 11:32:28AM -0700, Justin Pettit wrote: > Acked-by: Justin Pettit <jpettit@nicira.com> > > --Justin > > > > On Sep 30, 2015, at 1:56 PM, Ben Pfaff <blp@nicira.com> wrote: > > > > Signed-off-by: Ben Pfaff <blp@nicira.com> > > --- > > ovn/TODO | 4 +- > > ovn/lib/expr.c | 186 +++++++++++++++++++++++++++++++++++-------------------- > > ovn/ovn-sb.xml | 35 ++++++++--- > > tests/ovn.at | 14 ++++- > > tests/test-ovn.c | 1 + > > 5 files changed, 159 insertions(+), 81 deletions(-) > > > > diff --git a/ovn/TODO b/ovn/TODO > > index c83a287..740ea17 100644 > > --- a/ovn/TODO > > +++ b/ovn/TODO > > @@ -132,8 +132,8 @@ the "arp" action, and an action for generating > > > > *** Other actions. > > > > -Possibly we'll need to implement "field1 = field2;" for copying > > -between fields and "field1 <-> field2;" for swapping fields. > > +Possibly we'll need to implement "field1 <-> field2;" for swapping > > +fields. > > > > *** ovn-controller translation to OpenFlow > > > > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c > > index 71d817b..b7a3f7f 100644 > > --- a/ovn/lib/expr.c > > +++ b/ovn/lib/expr.c > > @@ -2619,113 +2619,165 @@ expr_is_normalized(const struct expr *expr) > > > > /* Action parsing helper. */ > > > > -static struct expr * > > -parse_assignment(struct expr_context *ctx, const struct simap *ports, > > - struct ofpbuf *ofpacts) > > +static bool > > +expand_symbol(struct expr_context *ctx, struct expr_field *f, > > + struct expr **prereqsp) > > { > > - struct expr *prereqs = NULL; > > - > > - struct expr_field f; > > - if (!parse_field(ctx, &f)) { > > - goto exit; > > - } > > - if (!lexer_match(ctx->lexer, LEX_T_EQUALS)) { > > - expr_syntax_error(ctx, "expecting `='."); > > - goto exit; > > - } > > - > > - if (f.symbol->expansion && f.symbol->level != EXPR_L_ORDINAL) { > > - expr_error(ctx, "Can't assign to predicate symbol %s.", > > - f.symbol->name); > > - goto exit; > > - } > > - > > - struct expr_constant_set cs; > > - if (!parse_constant_set(ctx, &cs)) { > > - goto exit; > > - } > > - > > - if (!type_check(ctx, &f, &cs)) { > > - goto exit_destroy_cs; > > - } > > - if (cs.in_curlies) { > > - expr_error(ctx, "Assignments require a single value."); > > - goto exit_destroy_cs; > > + if (f->symbol->expansion && f->symbol->level != EXPR_L_ORDINAL) { > > + expr_error(ctx, "Predicate symbol %s cannot be used in assignment.", > > + f->symbol->name); > > + return false; > > } > > > > - const struct expr_symbol *orig_symbol = f.symbol; > > - union expr_constant *c = cs.values; > > for (;;) { > > /* Accumulate prerequisites. */ > > - if (f.symbol->prereqs) { > > + if (f->symbol->prereqs) { > > struct ovs_list nesting = OVS_LIST_INITIALIZER(&nesting); > > char *error; > > struct expr *e; > > - e = parse_and_annotate(f.symbol->prereqs, ctx->symtab, &nesting, > > + e = parse_and_annotate(f->symbol->prereqs, ctx->symtab, &nesting, > > &error); > > if (error) { > > expr_error(ctx, "%s", error); > > free(error); > > - goto exit_destroy_cs; > > + return false; > > } > > - prereqs = expr_combine(EXPR_T_AND, prereqs, e); > > + *prereqsp = expr_combine(EXPR_T_AND, *prereqsp, e); > > } > > > > /* If there's no expansion, we're done. */ > > - if (!f.symbol->expansion) { > > + if (!f->symbol->expansion) { > > break; > > } > > > > /* Expand. */ > > struct expr_field expansion; > > char *error; > > - if (!parse_field_from_string(f.symbol->expansion, ctx->symtab, > > + if (!parse_field_from_string(f->symbol->expansion, ctx->symtab, > > &expansion, &error)) { > > expr_error(ctx, "%s", error); > > free(error); > > - goto exit_destroy_cs; > > + return false; > > } > > - f.symbol = expansion.symbol; > > - f.ofs += expansion.ofs; > > + f->symbol = expansion.symbol; > > + f->ofs += expansion.ofs; > > } > > > > - if (!f.symbol->field->writable) { > > - expr_error(ctx, "Field %s is not modifiable.", orig_symbol->name); > > - goto exit_destroy_cs; > > + return true; > > +} > > + > > +static struct expr * > > +parse_assignment(struct expr_context *ctx, const struct simap *ports, > > + struct ofpbuf *ofpacts) > > +{ > > + struct expr *prereqs = NULL; > > + > > + /* Parse destination and do basic checking. */ > > + struct expr_field dst; > > + if (!parse_field(ctx, &dst)) { > > + goto exit; > > + } > > + if (!lexer_match(ctx->lexer, LEX_T_EQUALS)) { > > + expr_syntax_error(ctx, "expecting `='."); > > + goto exit; > > + } > > + const struct expr_symbol *orig_dst = dst.symbol; > > + if (!expand_symbol(ctx, &dst, &prereqs)) { > > + goto exit; > > + } > > + if (!dst.symbol->field->writable) { > > + expr_error(ctx, "Field %s is not modifiable.", orig_dst->name); > > + goto exit; > > } > > > > - struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts); > > - sf->field = f.symbol->field; > > - if (f.symbol->width) { > > - mf_subvalue_shift(&c->value, f.ofs); > > - if (!c->masked) { > > - memset(&c->mask, 0, sizeof c->mask); > > - bitwise_one(&c->mask, sizeof c->mask, f.ofs, f.n_bits); > > - } else { > > - mf_subvalue_shift(&c->mask, f.ofs); > > + if (ctx->lexer->token.type == LEX_T_ID) { > > + struct expr_field src; > > + if (!parse_field(ctx, &src)) { > > + goto exit; > > + } > > + const struct expr_symbol *orig_src = src.symbol; > > + if (!expand_symbol(ctx, &src, &prereqs)) { > > + goto exit; > > + } > > + > > + if ((dst.symbol->width != 0) != (src.symbol->width != 0)) { > > + expr_error(ctx, "Can't assign %s field (%s) to %s field (%s).", > > + orig_src->width ? "integer" : "string", > > + orig_src->name, > > + orig_dst->width ? "integer" : "string", > > + orig_dst->name); > > + goto exit; > > + } > > + > > + if (dst.n_bits != src.n_bits) { > > + expr_error(ctx, "Can't assign %d-bit value to %d-bit destination.", > > + src.n_bits, dst.n_bits); > > + goto exit; > > + } else if (!dst.n_bits > > + && dst.symbol->field->n_bits != src.symbol->field->n_bits) { > > + expr_error(ctx, "String fields %s and %s are incompatible for " > > + "assignment.", orig_dst->name, orig_src->name); > > + goto exit; > > } > > > > - memcpy(&sf->value, &c->value.u8[sizeof c->value - sf->field->n_bytes], > > - sf->field->n_bytes); > > - memcpy(&sf->mask, &c->mask.u8[sizeof c->mask - sf->field->n_bytes], > > - sf->field->n_bytes); > > + struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts); > > + move->src.field = src.symbol->field; > > + move->src.ofs = src.ofs; > > + move->src.n_bits = src.n_bits ? src.n_bits : src.symbol->field->n_bits; > > + move->dst.field = dst.symbol->field; > > + move->dst.ofs = dst.ofs; > > + move->dst.n_bits = dst.n_bits ? dst.n_bits : dst.symbol->field->n_bits; > > } else { > > - uint32_t port = simap_get(ports, c->string); > > - bitwise_put(port, &sf->value, > > - sf->field->n_bytes, 0, sf->field->n_bits); > > - bitwise_put(UINT64_MAX, &sf->mask, > > - sf->field->n_bytes, 0, sf->field->n_bits); > > + struct expr_constant_set cs; > > + if (!parse_constant_set(ctx, &cs)) { > > + goto exit; > > + } > > + > > + if (!type_check(ctx, &dst, &cs)) { > > + goto exit_destroy_cs; > > + } > > + if (cs.in_curlies) { > > + expr_error(ctx, "Assignments require a single value."); > > + goto exit_destroy_cs; > > + } > > + > > + union expr_constant *c = cs.values; > > + struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts); > > + sf->field = dst.symbol->field; > > + if (dst.symbol->width) { > > + mf_subvalue_shift(&c->value, dst.ofs); > > + if (!c->masked) { > > + memset(&c->mask, 0, sizeof c->mask); > > + bitwise_one(&c->mask, sizeof c->mask, dst.ofs, dst.n_bits); > > + } else { > > + mf_subvalue_shift(&c->mask, dst.ofs); > > + } > > + > > + memcpy(&sf->value, > > + &c->value.u8[sizeof c->value - sf->field->n_bytes], > > + sf->field->n_bytes); > > + memcpy(&sf->mask, > > + &c->mask.u8[sizeof c->mask - sf->field->n_bytes], > > + sf->field->n_bytes); > > + } else { > > + uint32_t port = simap_get(ports, c->string); > > + bitwise_put(port, &sf->value, > > + sf->field->n_bytes, 0, sf->field->n_bits); > > + bitwise_put(UINT64_MAX, &sf->mask, > > + sf->field->n_bytes, 0, sf->field->n_bits); > > + } > > + > > + exit_destroy_cs: > > + expr_constant_set_destroy(&cs); > > } > > > > -exit_destroy_cs: > > - expr_constant_set_destroy(&cs); > > exit: > > return prereqs; > > } > > > > /* A helper for actions_parse(), to parse an OVN assignment action in the form > > - * "field = value" into 'ofpacts'. The parameters and return value match those > > - * for actions_parse(). */ > > + * "field = value" or "field1 = field2" into 'ofpacts'. The parameters and > > + * return value match those for actions_parse(). */ > > char * > > expr_parse_assignment(struct lexer *lexer, const struct shash *symtab, > > const struct simap *ports, > > diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml > > index da14fe4..28b0d2c 100644 > > --- a/ovn/ovn-sb.xml > > +++ b/ovn/ovn-sb.xml > > @@ -800,26 +800,41 @@ > > pipeline. > > </p> > > </dd> > > - </dl> > > > > - <p> > > - The following actions will likely be useful later, but they have not > > - been thought out carefully. > > - </p> > > - > > - <dl> > > <dt><code><var>field1</var> = <var>field2</var>;</code></dt> > > <dd> > > <p> > > - Extends the assignment action to allow copying between fields. > > + Sets data or metadata field <var>field1</var> to the value of data > > + or metadata field <var>field2</var>, e.g. <code>reg0 = > > + ip4.src;</code> to copy <code>ip4.src</code> into > > + <code>reg0</code>. To modify only a subset of a field's bits, > > + specify a subfield for <var>field1</var> or <var>field2</var> or > > + both, e.g. <code>vlan.pcp = reg0[0..2];</code> to set the VLAN PCP > > + from the least-significant bits of <code>reg0</code>. > > </p> > > > > <p> > > - An assignment adds prerequisites from the source and the > > - destination fields. > > + <var>field1</var> and <var>field2</var> must be the same type, > > + either both string or both integer fields. If they are both > > + integer fields, they must have the same width. > > + </p> > > + > > + <p> > > + If <var>field1</var> or <var>field2</var> has prerequisites, they > > + are added implicitly to <ref column="match"/>. It is possible to > > + write an assignment with contradictory prerequisites, such as > > + <code>ip4.src = ip6.src[0..31];</code>, but the contradiction means > > + that a logical flow with such an assignment will never be matched. > > </p> > > </dd> > > + </dl> > > + > > + <p> > > + The following actions will likely be useful later, but they have not > > + been thought out carefully. > > + </p> > > > > + <dl> > > <dt><code>ip.ttl--;</code></dt> > > <dd> > > <p> > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 70fc086..96f190d 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -444,6 +444,12 @@ tcp.dst=80; => actions=set_field:80->tcp_dst, prereqs=ip.proto == 0x6 && (eth.ty > > 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] > > vlan.tci[13..15] = 2; => actions=set_field:0x4000/0xe000->vlan_tci, prereqs=1 > > +reg0 = reg1; => actions=move:OXM_OF_PKT_REG0[0..31]->OXM_OF_PKT_REG0[32..63], prereqs=1 > > +vlan.pcp = reg0[0..2]; => actions=move:OXM_OF_PKT_REG0[32..34]->NXM_OF_VLAN_TCI[13..15], prereqs=vlan.tci[12] > > +reg0[10] = vlan.pcp[1]; => actions=move:NXM_OF_VLAN_TCI[14]->OXM_OF_PKT_REG0[42], prereqs=vlan.tci[12] > > +outport = inport; => actions=move:NXM_NX_REG6[]->NXM_NX_REG7[], prereqs=1 > > +# Contradictionary prerequisites (allowed but not useful): > > +ip4.src = ip6.src[0..31]; => actions=move:NXM_NX_IPV6_SRC[0..31]->NXM_OF_IP_SRC[], prereqs=eth.type == 0x800 && eth.type == 0x86dd > > > > ## Negative tests. > > > > @@ -462,13 +468,17 @@ 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. > > eth.dst[40] == 1; => Syntax error at `==' expecting `='. > > -ip = 1; => Can't assign to predicate symbol ip. > > +ip = 1; => Predicate symbol ip cannot be used in assignment. > > ip.proto = 6; => Field ip.proto is not modifiable. > > inport = {"a", "b"}; => Assignments require a single value. > > inport = {}; => Syntax error at `}' expecting constant. > > bad_prereq = 123; => Error parsing expression `xyzzy' encountered as prerequisite or predicate of initial expression: Syntax error at `xyzzy' expecting field name. > > self_recurse = 123; => Error parsing expression `self_recurse != 0' encountered as prerequisite or predicate of initial expression: Error parsing expression `self_recurse != 0' encountered as prerequisite or predicate of initial expression: Recursive expansion of symbol `self_recurse'. > > -vlan.present = 0; => Can't assign to predicate symbol vlan.present. > > +vlan.present = 0; => Predicate symbol vlan.present cannot be used in assignment. > > +reg0[0] = vlan.present; => Predicate symbol vlan.present cannot be used in assignment. > > +reg0 = reg1[0..10]; => Can't assign 11-bit value to 32-bit destination. > > +inport = reg0; => Can't assign integer field (reg0) to string field (inport). > > +inport = big_string; => String fields inport and big_string are incompatible for assignment. > > ]]) > > sed 's/ =>.*//' test-cases.txt > input.txt > > sed 's/.* => //' test-cases.txt > expout > > diff --git a/tests/test-ovn.c b/tests/test-ovn.c > > index ecb3b5c..c806068 100644 > > --- a/tests/test-ovn.c > > +++ b/tests/test-ovn.c > > @@ -235,6 +235,7 @@ create_symtab(struct shash *symtab) > > "mutual_recurse_2 != 0", false); > > expr_symtab_add_field(symtab, "mutual_recurse_2", MFF_XREG0, > > "mutual_recurse_1 != 0", false); > > + expr_symtab_add_string(symtab, "big_string", MFF_XREG0, NULL); > > } > > > > static void > > -- > > 2.1.3 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev >
diff --git a/ovn/TODO b/ovn/TODO index c83a287..740ea17 100644 --- a/ovn/TODO +++ b/ovn/TODO @@ -132,8 +132,8 @@ the "arp" action, and an action for generating *** Other actions. -Possibly we'll need to implement "field1 = field2;" for copying -between fields and "field1 <-> field2;" for swapping fields. +Possibly we'll need to implement "field1 <-> field2;" for swapping +fields. *** ovn-controller translation to OpenFlow diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c index 71d817b..b7a3f7f 100644 --- a/ovn/lib/expr.c +++ b/ovn/lib/expr.c @@ -2619,113 +2619,165 @@ expr_is_normalized(const struct expr *expr) /* Action parsing helper. */ -static struct expr * -parse_assignment(struct expr_context *ctx, const struct simap *ports, - struct ofpbuf *ofpacts) +static bool +expand_symbol(struct expr_context *ctx, struct expr_field *f, + struct expr **prereqsp) { - struct expr *prereqs = NULL; - - struct expr_field f; - if (!parse_field(ctx, &f)) { - goto exit; - } - if (!lexer_match(ctx->lexer, LEX_T_EQUALS)) { - expr_syntax_error(ctx, "expecting `='."); - goto exit; - } - - if (f.symbol->expansion && f.symbol->level != EXPR_L_ORDINAL) { - expr_error(ctx, "Can't assign to predicate symbol %s.", - f.symbol->name); - goto exit; - } - - struct expr_constant_set cs; - if (!parse_constant_set(ctx, &cs)) { - goto exit; - } - - if (!type_check(ctx, &f, &cs)) { - goto exit_destroy_cs; - } - if (cs.in_curlies) { - expr_error(ctx, "Assignments require a single value."); - goto exit_destroy_cs; + if (f->symbol->expansion && f->symbol->level != EXPR_L_ORDINAL) { + expr_error(ctx, "Predicate symbol %s cannot be used in assignment.", + f->symbol->name); + return false; } - const struct expr_symbol *orig_symbol = f.symbol; - union expr_constant *c = cs.values; for (;;) { /* Accumulate prerequisites. */ - if (f.symbol->prereqs) { + if (f->symbol->prereqs) { struct ovs_list nesting = OVS_LIST_INITIALIZER(&nesting); char *error; struct expr *e; - e = parse_and_annotate(f.symbol->prereqs, ctx->symtab, &nesting, + e = parse_and_annotate(f->symbol->prereqs, ctx->symtab, &nesting, &error); if (error) { expr_error(ctx, "%s", error); free(error); - goto exit_destroy_cs; + return false; } - prereqs = expr_combine(EXPR_T_AND, prereqs, e); + *prereqsp = expr_combine(EXPR_T_AND, *prereqsp, e); } /* If there's no expansion, we're done. */ - if (!f.symbol->expansion) { + if (!f->symbol->expansion) { break; } /* Expand. */ struct expr_field expansion; char *error; - if (!parse_field_from_string(f.symbol->expansion, ctx->symtab, + if (!parse_field_from_string(f->symbol->expansion, ctx->symtab, &expansion, &error)) { expr_error(ctx, "%s", error); free(error); - goto exit_destroy_cs; + return false; } - f.symbol = expansion.symbol; - f.ofs += expansion.ofs; + f->symbol = expansion.symbol; + f->ofs += expansion.ofs; } - if (!f.symbol->field->writable) { - expr_error(ctx, "Field %s is not modifiable.", orig_symbol->name); - goto exit_destroy_cs; + return true; +} + +static struct expr * +parse_assignment(struct expr_context *ctx, const struct simap *ports, + struct ofpbuf *ofpacts) +{ + struct expr *prereqs = NULL; + + /* Parse destination and do basic checking. */ + struct expr_field dst; + if (!parse_field(ctx, &dst)) { + goto exit; + } + if (!lexer_match(ctx->lexer, LEX_T_EQUALS)) { + expr_syntax_error(ctx, "expecting `='."); + goto exit; + } + const struct expr_symbol *orig_dst = dst.symbol; + if (!expand_symbol(ctx, &dst, &prereqs)) { + goto exit; + } + if (!dst.symbol->field->writable) { + expr_error(ctx, "Field %s is not modifiable.", orig_dst->name); + goto exit; } - struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts); - sf->field = f.symbol->field; - if (f.symbol->width) { - mf_subvalue_shift(&c->value, f.ofs); - if (!c->masked) { - memset(&c->mask, 0, sizeof c->mask); - bitwise_one(&c->mask, sizeof c->mask, f.ofs, f.n_bits); - } else { - mf_subvalue_shift(&c->mask, f.ofs); + if (ctx->lexer->token.type == LEX_T_ID) { + struct expr_field src; + if (!parse_field(ctx, &src)) { + goto exit; + } + const struct expr_symbol *orig_src = src.symbol; + if (!expand_symbol(ctx, &src, &prereqs)) { + goto exit; + } + + if ((dst.symbol->width != 0) != (src.symbol->width != 0)) { + expr_error(ctx, "Can't assign %s field (%s) to %s field (%s).", + orig_src->width ? "integer" : "string", + orig_src->name, + orig_dst->width ? "integer" : "string", + orig_dst->name); + goto exit; + } + + if (dst.n_bits != src.n_bits) { + expr_error(ctx, "Can't assign %d-bit value to %d-bit destination.", + src.n_bits, dst.n_bits); + goto exit; + } else if (!dst.n_bits + && dst.symbol->field->n_bits != src.symbol->field->n_bits) { + expr_error(ctx, "String fields %s and %s are incompatible for " + "assignment.", orig_dst->name, orig_src->name); + goto exit; } - memcpy(&sf->value, &c->value.u8[sizeof c->value - sf->field->n_bytes], - sf->field->n_bytes); - memcpy(&sf->mask, &c->mask.u8[sizeof c->mask - sf->field->n_bytes], - sf->field->n_bytes); + struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts); + move->src.field = src.symbol->field; + move->src.ofs = src.ofs; + move->src.n_bits = src.n_bits ? src.n_bits : src.symbol->field->n_bits; + move->dst.field = dst.symbol->field; + move->dst.ofs = dst.ofs; + move->dst.n_bits = dst.n_bits ? dst.n_bits : dst.symbol->field->n_bits; } else { - uint32_t port = simap_get(ports, c->string); - bitwise_put(port, &sf->value, - sf->field->n_bytes, 0, sf->field->n_bits); - bitwise_put(UINT64_MAX, &sf->mask, - sf->field->n_bytes, 0, sf->field->n_bits); + struct expr_constant_set cs; + if (!parse_constant_set(ctx, &cs)) { + goto exit; + } + + if (!type_check(ctx, &dst, &cs)) { + goto exit_destroy_cs; + } + if (cs.in_curlies) { + expr_error(ctx, "Assignments require a single value."); + goto exit_destroy_cs; + } + + union expr_constant *c = cs.values; + struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts); + sf->field = dst.symbol->field; + if (dst.symbol->width) { + mf_subvalue_shift(&c->value, dst.ofs); + if (!c->masked) { + memset(&c->mask, 0, sizeof c->mask); + bitwise_one(&c->mask, sizeof c->mask, dst.ofs, dst.n_bits); + } else { + mf_subvalue_shift(&c->mask, dst.ofs); + } + + memcpy(&sf->value, + &c->value.u8[sizeof c->value - sf->field->n_bytes], + sf->field->n_bytes); + memcpy(&sf->mask, + &c->mask.u8[sizeof c->mask - sf->field->n_bytes], + sf->field->n_bytes); + } else { + uint32_t port = simap_get(ports, c->string); + bitwise_put(port, &sf->value, + sf->field->n_bytes, 0, sf->field->n_bits); + bitwise_put(UINT64_MAX, &sf->mask, + sf->field->n_bytes, 0, sf->field->n_bits); + } + + exit_destroy_cs: + expr_constant_set_destroy(&cs); } -exit_destroy_cs: - expr_constant_set_destroy(&cs); exit: return prereqs; } /* A helper for actions_parse(), to parse an OVN assignment action in the form - * "field = value" into 'ofpacts'. The parameters and return value match those - * for actions_parse(). */ + * "field = value" or "field1 = field2" into 'ofpacts'. The parameters and + * return value match those for actions_parse(). */ char * expr_parse_assignment(struct lexer *lexer, const struct shash *symtab, const struct simap *ports, diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml index da14fe4..28b0d2c 100644 --- a/ovn/ovn-sb.xml +++ b/ovn/ovn-sb.xml @@ -800,26 +800,41 @@ pipeline. </p> </dd> - </dl> - <p> - The following actions will likely be useful later, but they have not - been thought out carefully. - </p> - - <dl> <dt><code><var>field1</var> = <var>field2</var>;</code></dt> <dd> <p> - Extends the assignment action to allow copying between fields. + Sets data or metadata field <var>field1</var> to the value of data + or metadata field <var>field2</var>, e.g. <code>reg0 = + ip4.src;</code> to copy <code>ip4.src</code> into + <code>reg0</code>. To modify only a subset of a field's bits, + specify a subfield for <var>field1</var> or <var>field2</var> or + both, e.g. <code>vlan.pcp = reg0[0..2];</code> to set the VLAN PCP + from the least-significant bits of <code>reg0</code>. </p> <p> - An assignment adds prerequisites from the source and the - destination fields. + <var>field1</var> and <var>field2</var> must be the same type, + either both string or both integer fields. If they are both + integer fields, they must have the same width. + </p> + + <p> + If <var>field1</var> or <var>field2</var> has prerequisites, they + are added implicitly to <ref column="match"/>. It is possible to + write an assignment with contradictory prerequisites, such as + <code>ip4.src = ip6.src[0..31];</code>, but the contradiction means + that a logical flow with such an assignment will never be matched. </p> </dd> + </dl> + + <p> + The following actions will likely be useful later, but they have not + been thought out carefully. + </p> + <dl> <dt><code>ip.ttl--;</code></dt> <dd> <p> diff --git a/tests/ovn.at b/tests/ovn.at index 70fc086..96f190d 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -444,6 +444,12 @@ tcp.dst=80; => actions=set_field:80->tcp_dst, prereqs=ip.proto == 0x6 && (eth.ty 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] vlan.tci[13..15] = 2; => actions=set_field:0x4000/0xe000->vlan_tci, prereqs=1 +reg0 = reg1; => actions=move:OXM_OF_PKT_REG0[0..31]->OXM_OF_PKT_REG0[32..63], prereqs=1 +vlan.pcp = reg0[0..2]; => actions=move:OXM_OF_PKT_REG0[32..34]->NXM_OF_VLAN_TCI[13..15], prereqs=vlan.tci[12] +reg0[10] = vlan.pcp[1]; => actions=move:NXM_OF_VLAN_TCI[14]->OXM_OF_PKT_REG0[42], prereqs=vlan.tci[12] +outport = inport; => actions=move:NXM_NX_REG6[]->NXM_NX_REG7[], prereqs=1 +# Contradictionary prerequisites (allowed but not useful): +ip4.src = ip6.src[0..31]; => actions=move:NXM_NX_IPV6_SRC[0..31]->NXM_OF_IP_SRC[], prereqs=eth.type == 0x800 && eth.type == 0x86dd ## Negative tests. @@ -462,13 +468,17 @@ 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. eth.dst[40] == 1; => Syntax error at `==' expecting `='. -ip = 1; => Can't assign to predicate symbol ip. +ip = 1; => Predicate symbol ip cannot be used in assignment. ip.proto = 6; => Field ip.proto is not modifiable. inport = {"a", "b"}; => Assignments require a single value. inport = {}; => Syntax error at `}' expecting constant. bad_prereq = 123; => Error parsing expression `xyzzy' encountered as prerequisite or predicate of initial expression: Syntax error at `xyzzy' expecting field name. self_recurse = 123; => Error parsing expression `self_recurse != 0' encountered as prerequisite or predicate of initial expression: Error parsing expression `self_recurse != 0' encountered as prerequisite or predicate of initial expression: Recursive expansion of symbol `self_recurse'. -vlan.present = 0; => Can't assign to predicate symbol vlan.present. +vlan.present = 0; => Predicate symbol vlan.present cannot be used in assignment. +reg0[0] = vlan.present; => Predicate symbol vlan.present cannot be used in assignment. +reg0 = reg1[0..10]; => Can't assign 11-bit value to 32-bit destination. +inport = reg0; => Can't assign integer field (reg0) to string field (inport). +inport = big_string; => String fields inport and big_string are incompatible for assignment. ]]) sed 's/ =>.*//' test-cases.txt > input.txt sed 's/.* => //' test-cases.txt > expout diff --git a/tests/test-ovn.c b/tests/test-ovn.c index ecb3b5c..c806068 100644 --- a/tests/test-ovn.c +++ b/tests/test-ovn.c @@ -235,6 +235,7 @@ create_symtab(struct shash *symtab) "mutual_recurse_2 != 0", false); expr_symtab_add_field(symtab, "mutual_recurse_2", MFF_XREG0, "mutual_recurse_1 != 0", false); + expr_symtab_add_string(symtab, "big_string", MFF_XREG0, NULL); } static void
Signed-off-by: Ben Pfaff <blp@nicira.com> --- ovn/TODO | 4 +- ovn/lib/expr.c | 186 +++++++++++++++++++++++++++++++++++-------------------- ovn/ovn-sb.xml | 35 ++++++++--- tests/ovn.at | 14 ++++- tests/test-ovn.c | 1 + 5 files changed, 159 insertions(+), 81 deletions(-)