[ovs-dev,v2,2/3] expr: Implement action to copy one field into another.
diff mbox

Message ID 1443646592-20290-3-git-send-email-blp@nicira.com
State Accepted
Headers show

Commit Message

Ben Pfaff Sept. 30, 2015, 8:56 p.m. UTC
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(-)

Comments

Justin Pettit Oct. 7, 2015, 6:32 p.m. UTC | #1
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
Ben Pfaff Oct. 7, 2015, 8:49 p.m. UTC | #2
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
>

Patch
diff mbox

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