On Tue, Jun 14, 2016 at 09:36:10AM -0500, Ryan Moats wrote:
> "dev" <dev-boun...@openvswitch.org> wrote on 06/09/2016 12:37:58 AM:
>
> > From: Ben Pfaff <b...@ovn.org>
> > To: dev@openvswitch.org
> > Cc: Ben Pfaff <b...@ovn.org>
> > Subject: [ovs-dev] [PATCH 2/4] expr: Refactor parsing of assignments and
> exchanges.
> > Sent by: "dev" <dev-boun...@openvswitch.org>
>
> [snip]
>
> > --- a/ovn/lib/actions.c
> > +++ b/ovn/lib/actions.c
> > @@ -106,19 +106,35 @@ action_syntax_error(struct action_context *ctx,
> const char *message, ...)
> > static void
> > parse_set_action(struct action_context *ctx)
> > {
> > - struct expr *prereqs;
> > + struct expr *prereqs = NULL;
> > + struct expr_field dst;
> > char *error;
> >
> > - error = expr_parse_assignment(ctx->lexer, ctx->ap->symtab,
> > - ctx->ap->lookup_port, ctx->ap->aux,
> > - ctx->ofpacts, &prereqs);
> > + error = expr_parse_field(ctx->lexer, ctx->ap->symtab, &dst);
> > if (error) {
> > + goto exit;
> > + }
> > +
> > + if (lexer_match(ctx->lexer, LEX_T_EXCHANGE)) {
> > + error = expr_parse_exchange(ctx->lexer, &dst, ctx->ap->symtab,
> > + ctx->ap->lookup_port, ctx->ap->aux,
> > + ctx->ofpacts, &prereqs);
> > + } else if (lexer_match(ctx->lexer, LEX_T_EQUALS)) {
> > + error = expr_parse_assignment(
> > + ctx->lexer, &dst, ctx->ap->symtab, ctx->ap->lookup_port,
> > + ctx->ap->aux, ctx->ofpacts, &prereqs);
> > + } else {
> > + action_syntax_error(ctx, "expecting `=' or `<->'");
> > + }
> > +
> > +exit:
> > + if (!error) {
> > + ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, prereqs);
> > + } else {
> > + expr_destroy(prereqs);
> > action_error(ctx, "%s", error);
> > free(error);
> > - return;
> > }
> > -
> > - ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, prereqs);
> > }
> >
> > static void
>
> I looked at this long and hard, and while I don't mind the use of gotos in
> C code,
> in this case I found it a bit "jarring" and wonder if the following isn't
> more readable:
>
> error = expr_parse_field(ctx->lexer, ctx->ap->symtab, &dst);
> if (!error) {
> if (lexer_match(ctx->lexer, LEX_T_EXCHANGE)) {
> error = expr_parse_exchange(ctx->lexer, &dst, ctx->ap->symtab,
> ctx->ap->lookup_port, ctx->ap->aux,
> ctx->ofpacts, &prereqs);
> } else if (lexer_match(ctx->lexer, LEX_T_EQUALS)) {
> error = expr_parse_assignment(
> ctx->lexer, &dst, ctx->ap->symtab, ctx->ap->lookup_port,
> ctx->ap->aux, ctx->ofpacts, &prereqs);
> } else {
> action_syntax_error(ctx, "expecting `=' or `<->'");
> }
> if (!error) {
> ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, prereqs);
> }
> }
> if (error) {
> expr_destroy(prereqs);
> action_error(ctx, "%s", error);
> free(error);
> }
>
> Ryan
Fair enough. I made that change and applied this to master.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
