On Wed, Oct 07, 2015 at 01:17:22PM -0700, Justin Pettit wrote:
>
> > On Sep 30, 2015, at 1:56 PM, Ben Pfaff <[email protected]> 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 <[email protected]>
Thanks for the review, I applied this patch to master.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
