[ovs-dev] [PATCH v3 2/9] ovn: Rewrite logical action parsing and encoding library.

[Ben Pfaff]

Thanks for the review.  I've applied this patch to master.  I applied
most of your comments in the obvious way, so I just snipped them below.
A few responses:

On Mon, Aug 15, 2016 at 11:12:19AM -0700, Justin Pettit wrote:
> > On Aug 8, 2016, at 11:21 AM, Ben Pfaff <blp at ovn.org> wrote:
> > +    while (!lexer_match(ctx->lexer, LEX_T_RCURLY)) {
> > +        if (!parse_action(&inner_ctx)) {
> > +            break;
> > +        }
> > +    }
> > +
> > +    expr_destroy(inner_ctx.prereqs); /* XXX what is the correct behavior? */
> 
> This comment seems a little worrying.  Is it something that we should
> fix now?  If not, it would be helpful to describe in greater detail
> what we need to fix in the future.

I've never been quite sure what to do about prerequisites generated by
nested actions.  They can't be applied directly to the outer match.
Probably we should do something with them, but for some time now we've
just discarded them (this is not new behavior).

The comment was unclear so I changed it:

@@ -1134,7 +1133,10 @@ parse_nested_action(struct action_context *ctx, enum ovnact_type type,
         }
     }
 
-    expr_destroy(inner_ctx.prereqs); /* XXX what is the correct behavior? */
+    /* XXX Not really sure what we should do with prerequisites for nested
+     * actions. */
+    expr_destroy(inner_ctx.prereqs);
+
     if (inner_ctx.error) {
         ctx->error = inner_ctx.error;
         ovnacts_free(nested.data, nested.size);

> > @@ -1169,21 +1758,21 @@ parse_actions(struct action_context *ctx)
> >  * eventually free it.
> >  *
> >  * Returns NULL on success, otherwise a malloc()'d error message that the
> > - * caller must free.  On failure, 'ofpacts' has the same contents and
> > + * caller must free.  On failure, 'ovnacts' has the same contents and
> >  * '*prereqsp' is set to NULL, but some tokens may have been consumed from
> >  * 'lexer'.
> >   */
> 
> You didn't introduce it, but I think there's an extra space here.

I didn't start it but by damn I'm going to finish it!

Removed.

> Due to the churn, this was kind of a tough patch to review.  I re-went
> though "actions.c", and it looks good.  Also, it has really good
> tests, which is reassuring.

I couldn't come up with a good incremental or easily reviewable way to
do this.  Thanks for reviewing it.

> Acked-by: Justin Pettit <jpettit at ovn.org>
> 
> Thanks for doing this.  It's very clean, and the new test output is a
> big improvement.