Thanks, applied to master.
On Wed, Oct 07, 2015 at 11:32:28AM -0700, Justin Pettit wrote:
> Acked-by: Justin Pettit <jpet...@nicira.com>
>
> --Justin
>
>
> > On Sep 30, 2015, at 1:56 PM, Ben Pfaff <b...@nicira.com> wrote:
> >
> > Signed-off-by: Ben Pfaff <b...@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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
