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

Ben Pfaff blp at nicira.com
Wed Oct 7 20:49:11 UTC 2015


On Wed, Oct 07, 2015 at 01:17:22PM -0700, Justin Pettit wrote:
> 
> > 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.

OK, I added a comment:

/* Expands 'f' repeatedly as long as it has an expansion, that is, as long as
 * it is a subfield or a predicate.  Adds any prerequisites for 'f' to
 * '*prereqs'.
 *
 * If 'rw', verifies that 'f' is a read/write field.
 *
 * 'exchange' should be true if an exchange action is being parsed.  This is
 * only used to improve error message phrasing.
 *
 * Returns true if successful, false if an error was encountered (in which case
 * 'ctx->error' reports the particular error). */

> > {
> > +    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.

OK, I changed all of them to "exchange".

> > 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.

You're right.

I've moved it now.

> > -(){}[[]]==!=<<=>>=!&&||..,;= => ( ) { } [[ ]] == != < <= > >= ! && || .. , ; =
> > +(){}[[]]==!=<<=>>=!&&||..,;=<-> => ( ) { } [[ ]] == != < <= > >= ! && || .. , ; = <->
> > & => 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.

OK, I added a comment.

> > +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.

Right again, also moved, thanks.

> It's nice to see push/pop being put to use!
> 
> Acked-by: Justin Pettit <jpettit at nicira.com>

Thanks for the review, I applied this patch to master.



More information about the dev mailing list