[ovs-dev] [PATCH v2 3/3] ovn: Implement action to exchange two fields.

Justin Pettit jpettit at nicira.com
Wed Oct 7 20:17:22 UTC 2015


> On Sep 30, 2015, at 1:56 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> static bool
> -expand_symbol(struct expr_context *ctx, struct expr_field *f,
> -              struct expr **prereqsp)
> +expand_symbol(struct expr_context *ctx, bool rw, bool swap,
> +              struct expr_field *f, struct expr **prereqsp)

I assume "rw" is "read/write".  It might be nice to document it, though.

> {
> +    const struct expr_symbol *orig_symbol = f->symbol;
> +
>     if (f->symbol->expansion && f->symbol->level != EXPR_L_ORDINAL) {
> -        expr_error(ctx, "Predicate symbol %s cannot be used in assignment.",
> -                   f->symbol->name);
> +        expr_error(ctx, "Predicate symbol %s cannot be used in %s.",
> +                   f->symbol->name, swap ? "exchange" : "assignment");

In most internal uses, "swap" is used instead of "exchange"; but external uses seem to prefer "exchange".  It's not a big deal, but it can be a bit easier to work through the code if the names are the same.

> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> index 28b0d2c..3ae99b3 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -806,11 +806,11 @@
>           <p>
>             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>.
> +            ip4.src;</code> copies <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> copies the least-significant bits of
> +            <code>reg0</code> into the VLAN PCP.

This isn't a big deal, but it seems like it belongs more in the previous patch.

> -(){}[[]]==!=<<=>>=!&&||..,;= => ( ) { } [[ ]] == != < <= > >= ! && || .. , ; =
> +(){}[[]]==!=<<=>>=!&&||..,;=<-> => ( ) { } [[ ]] == != < <= > >= ! && || .. , ; = <->
> & => error("`&' is only valid as part of `&&'.")
> | => error("`|' is only valid as part of `||'.")

Not related to this patch in particular, but it might be worth adding a comment about this test, since it looks odd.

> +ip.proto = reg0[0..7]; => Field ip.proto is not modifiable.

Also not a big deal, but it seems like this test belongs in the previous patch.

It's nice to see push/pop being put to use!

Acked-by: Justin Pettit <jpettit at nicira.com>

--Justin





More information about the dev mailing list