As written, it was difficult for the OVN logical action code to add support
for new actions of the form "dst = ...", because the code to parse the left
side of the assignment was a monolithic part of the expr library. This
commit refactors the code division so that an upcoming patch can support a
new "dst = func(args);" kind of action.
Signed-off-by: Ben Pfaff <b...@ovn.org>
---
ovn/lib/actions.c | 52 +++++++++++-----
ovn/lib/expr.c | 173 +++++++++++++++++++++++++++++++-----------------------
ovn/lib/expr.h | 36 ++++++++++--
tests/ovn.at | 2 +-
4 files changed, 165 insertions(+), 98 deletions(-)
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 5f0bf19..f9309da 100644
--- 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
@@ -282,19 +298,23 @@ static bool
action_parse_field(struct action_context *ctx,
int n_bits, struct mf_subfield *sf)
{
- struct expr *prereqs;
+ struct expr_field field;
char *error;
- error = expr_parse_field(ctx->lexer, n_bits, false, ctx->ap->symtab, sf,
- &prereqs);
- if (error) {
- action_error(ctx, "%s", error);
- free(error);
- return false;
+ error = expr_parse_field(ctx->lexer, ctx->ap->symtab, &field);
+ if (!error) {
+ struct expr *prereqs;
+ error = expr_expand_field(ctx->lexer, ctx->ap->symtab,
+ &field, n_bits, false, sf, &prereqs);
+ if (!error) {
+ ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, prereqs);
+ return true;
+ }
}
- ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, prereqs);
- return true;
+ action_error(ctx, "%s", error);
+ free(error);
+ return false;
}
static void
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index 7ff9538..31b94d8 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -438,15 +438,6 @@ struct expr_constant_set {
bool in_curlies; /* Whether the constants were in {}. */
};
-/* A reference to a symbol or a subfield of a symbol.
- *
- * For string fields, ofs and n_bits are 0. */
-struct expr_field {
- const struct expr_symbol *symbol; /* The symbol. */
- int ofs; /* Starting bit offset. */
- int n_bits; /* Number of bits. */
-};
-
/* Context maintained during expr_parse(). */
struct expr_context {
struct lexer *lexer; /* Lexer for pulling more tokens. */
@@ -2698,49 +2689,44 @@ init_stack_action(const struct expr_field *f, struct
ofpact_stack *stack)
mf_subfield_from_expr_field(f, &stack->subfield);
}
-static struct expr *
-parse_assignment(struct expr_context *ctx,
+static char * OVS_WARN_UNUSED_RESULT
+parse_assignment(struct lexer *lexer, struct expr_field *dst,
+ const struct shash *symtab, bool exchange,
bool (*lookup_port)(const void *aux, const char *port_name,
unsigned int *portp),
- const void *aux, struct ofpbuf *ofpacts)
+ const void *aux, struct ofpbuf *ofpacts,
+ struct expr **prereqsp)
{
+ struct expr_context ctx = { .lexer = lexer, .symtab = symtab };
struct expr *prereqs = NULL;
/* Parse destination and do basic checking. */
- struct expr_field dst;
- if (!parse_field(ctx, &dst)) {
- goto exit;
- }
- bool exchange = lexer_match(ctx->lexer, LEX_T_EXCHANGE);
- if (!exchange && !lexer_match(ctx->lexer, LEX_T_EQUALS)) {
- expr_syntax_error(ctx, "expecting `='.");
- goto exit;
- }
- const struct expr_symbol *orig_dst = dst.symbol;
- if (!expand_symbol(ctx, true, &dst, &prereqs)) {
+ const struct expr_symbol *orig_dst = dst->symbol;
+ if (!expand_symbol(&ctx, true, dst, &prereqs)) {
goto exit;
}
- if (exchange || ctx->lexer->token.type == LEX_T_ID) {
+ if (exchange || ctx.lexer->token.type == LEX_T_ID) {
struct expr_field src;
- if (!parse_field(ctx, &src)) {
+ if (!parse_field(&ctx, &src)) {
goto exit;
}
const struct expr_symbol *orig_src = src.symbol;
- if (!expand_symbol(ctx, exchange, &src, &prereqs)) {
+ if (!expand_symbol(&ctx, exchange, &src, &prereqs)) {
goto exit;
}
- if ((dst.symbol->width != 0) != (src.symbol->width != 0)) {
+ if ((dst->symbol->width != 0) != (src.symbol->width != 0)) {
if (exchange) {
- expr_error(ctx,
+ expr_error(&ctx,
"Can't exchange %s field (%s) with %s field (%s).",
orig_dst->width ? "integer" : "string",
orig_dst->name,
orig_src->width ? "integer" : "string",
orig_src->name);
} else {
- expr_error(ctx, "Can't assign %s field (%s) to %s field (%s).",
+ expr_error(&ctx,
+ "Can't assign %s field (%s) to %s field (%s).",
orig_src->width ? "integer" : "string",
orig_src->name,
orig_dst->width ? "integer" : "string",
@@ -2749,20 +2735,20 @@ parse_assignment(struct expr_context *ctx,
goto exit;
}
- if (dst.n_bits != src.n_bits) {
+ if (dst->n_bits != src.n_bits) {
if (exchange) {
- expr_error(ctx,
+ expr_error(&ctx,
"Can't exchange %d-bit field with %d-bit field.",
- dst.n_bits, src.n_bits);
+ dst->n_bits, src.n_bits);
} else {
- expr_error(ctx,
+ expr_error(&ctx,
"Can't assign %d-bit value to %d-bit destination.",
- src.n_bits, dst.n_bits);
+ src.n_bits, dst->n_bits);
}
goto exit;
- } else if (!dst.n_bits
- && dst.symbol->field->n_bits != src.symbol->field->n_bits) {
- expr_error(ctx, "String fields %s and %s are incompatible for "
+ } else if (!dst->n_bits &&
+ dst->symbol->field->n_bits != src.symbol->field->n_bits) {
+ expr_error(&ctx, "String fields %s and %s are incompatible for "
"%s.", orig_dst->name, orig_src->name,
exchange ? "exchange" : "assignment");
goto exit;
@@ -2770,38 +2756,38 @@ parse_assignment(struct expr_context *ctx,
if (exchange) {
init_stack_action(&src, ofpact_put_STACK_PUSH(ofpacts));
- init_stack_action(&dst, ofpact_put_STACK_PUSH(ofpacts));
+ init_stack_action(dst, ofpact_put_STACK_PUSH(ofpacts));
init_stack_action(&src, ofpact_put_STACK_POP(ofpacts));
- init_stack_action(&dst, ofpact_put_STACK_POP(ofpacts));
+ init_stack_action(dst, ofpact_put_STACK_POP(ofpacts));
} else {
struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts);
mf_subfield_from_expr_field(&src, &move->src);
- mf_subfield_from_expr_field(&dst, &move->dst);
+ mf_subfield_from_expr_field(dst, &move->dst);
}
} else {
struct expr_constant_set cs;
- if (!parse_constant_set(ctx, &cs)) {
+ if (!parse_constant_set(&ctx, &cs)) {
goto exit;
}
- if (!type_check(ctx, &dst, &cs)) {
+ if (!type_check(&ctx, dst, &cs)) {
goto exit_destroy_cs;
}
if (cs.in_curlies) {
- expr_error(ctx, "Assignments require a single value.");
+ expr_error(&ctx, "Assignments require a single value.");
goto exit_destroy_cs;
}
union expr_constant *c = cs.values;
struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts);
- sf->field = dst.symbol->field;
- if (dst.symbol->width) {
- mf_subvalue_shift(&c->value, dst.ofs);
+ sf->field = dst->symbol->field;
+ if (dst->symbol->width) {
+ mf_subvalue_shift(&c->value, dst->ofs);
if (!c->masked) {
memset(&c->mask, 0, sizeof c->mask);
- bitwise_one(&c->mask, sizeof c->mask, dst.ofs, dst.n_bits);
+ bitwise_one(&c->mask, sizeof c->mask, dst->ofs, dst->n_bits);
} else {
- mf_subvalue_shift(&c->mask, dst.ofs);
+ mf_subvalue_shift(&c->mask, dst->ofs);
}
memcpy(&sf->value,
@@ -2835,65 +2821,102 @@ parse_assignment(struct expr_context *ctx,
}
exit:
- return prereqs;
+ if (ctx.error) {
+ expr_destroy(prereqs);
+ prereqs = NULL;
+ }
+ *prereqsp = prereqs;
+ return ctx.error;
}
/* A helper for actions_parse(), to parse an OVN assignment action in the form
- * "field = value" or "field1 = field2", or a "exchange" action in the form
- * "field1 <-> field2", into 'ofpacts'. The parameters and return value match
- * those for actions_parse(). */
-char *
-expr_parse_assignment(struct lexer *lexer, const struct shash *symtab,
+ * "field = value" or "field = field2" into 'ofpacts'. The caller must have
+ * already parsed and skipped the left-hand side "field =" and pass in the
+ * field as 'dst'. Other parameters and return value match those for
+ * actions_parse(). */
+char * OVS_WARN_UNUSED_RESULT
+expr_parse_assignment(struct lexer *lexer, struct expr_field *dst,
+ const struct shash *symtab,
bool (*lookup_port)(const void *aux,
const char *port_name,
unsigned int *portp),
const void *aux,
struct ofpbuf *ofpacts, struct expr **prereqsp)
{
+ return parse_assignment(lexer, dst, symtab, false, lookup_port, aux,
+ ofpacts, prereqsp);
+}
+
+/* A helper for actions_parse(), to parse an OVN exchange action in the form
+ * "field1 <-> field2" into 'ofpacts'. The caller must have already parsed and
+ * skipped the left-hand side "field1 <->" and pass in 'field1'. Other
+ * parameters and return value match those for actions_parse(). */
+char * OVS_WARN_UNUSED_RESULT
+expr_parse_exchange(struct lexer *lexer, struct expr_field *field,
+ const struct shash *symtab,
+ bool (*lookup_port)(const void *aux,
+ const char *port_name,
+ unsigned int *portp),
+ const void *aux,
+ struct ofpbuf *ofpacts, struct expr **prereqsp)
+{
+ return parse_assignment(lexer, field, symtab, true, lookup_port, aux,
+ ofpacts, prereqsp);
+}
+
+/* Parses a field or subfield from 'lexer' into 'field', obtaining field names
+ * from 'symtab'. Returns NULL if successful, otherwise an error message owned
+ * by the caller. */
+char * OVS_WARN_UNUSED_RESULT
+expr_parse_field(struct lexer *lexer, const struct shash *symtab,
+ struct expr_field *field)
+{
struct expr_context ctx = { .lexer = lexer, .symtab = symtab };
- struct expr *prereqs = parse_assignment(&ctx, lookup_port, aux, ofpacts);
- if (ctx.error) {
- expr_destroy(prereqs);
- prereqs = NULL;
+ if (!parse_field(&ctx, field)) {
+ memset(field, 0, sizeof *field);
}
- *prereqsp = prereqs;
return ctx.error;
}
-char *
-expr_parse_field(struct lexer *lexer, int n_bits, bool rw,
- const struct shash *symtab,
- struct mf_subfield *sf, struct expr **prereqsp)
+/* Takes 'field', which was presumably parsed by expr_parse_field(), and
+ * converts it into mf_subfield 'sf' and a set of prerequisites in '*prereqsp'.
+ *
+ * 'n_bits' specifies the number of bits that the field must have, and 0
+ * indicates a string field; reports an error if 'field' has a different type
+ * or width. If 'rw' is true, it is an error if 'field' is read-only. Uses
+ * 'symtab 'for expanding references and 'lexer' for error reporting.
+ *
+ * Returns NULL if successful, otherwise an error message owned by the
+ * caller. */
+char * OVS_WARN_UNUSED_RESULT
+expr_expand_field(struct lexer *lexer, const struct shash *symtab,
+ const struct expr_field *orig_field, int n_bits, bool rw,
+ struct mf_subfield *sf, struct expr **prereqsp)
{
struct expr_context ctx = { .lexer = lexer, .symtab = symtab };
struct expr *prereqs = NULL;
- struct expr_field field;
- if (!parse_field(&ctx, &field)) {
- goto exit;
- }
-
- const struct expr_field orig_field = field;
+ struct expr_field field = *orig_field;
if (!expand_symbol(&ctx, rw, &field, &prereqs)) {
goto exit;
}
- ovs_assert(field.n_bits == orig_field.n_bits);
+ ovs_assert(field.n_bits == orig_field->n_bits);
if (n_bits != field.n_bits) {
if (n_bits && field.n_bits) {
expr_error(&ctx, "Cannot use %d-bit field %s[%d..%d] "
"where %d-bit field is required.",
- orig_field.n_bits, orig_field.symbol->name,
- orig_field.ofs, orig_field.ofs + orig_field.n_bits - 1,
- n_bits);
+ orig_field->n_bits, orig_field->symbol->name,
+ orig_field->ofs,
+ orig_field->ofs + orig_field->n_bits - 1, n_bits);
} else if (n_bits) {
expr_error(&ctx, "Cannot use string field %s where numeric "
"field is required.",
- orig_field.symbol->name);
+ orig_field->symbol->name);
} else {
expr_error(&ctx, "Cannot use numeric field %s where string "
"field is required.",
- orig_field.symbol->name);
+ orig_field->symbol->name);
}
}
diff --git a/ovn/lib/expr.h b/ovn/lib/expr.h
index 1327789..351d0ff 100644
--- a/ovn/lib/expr.h
+++ b/ovn/lib/expr.h
@@ -248,6 +248,15 @@ struct expr_symbol {
bool must_crossproduct;
};
+/* A reference to a symbol or a subfield of a symbol.
+ *
+ * For string fields, ofs and n_bits are 0. */
+struct expr_field {
+ const struct expr_symbol *symbol; /* The symbol. */
+ int ofs; /* Starting bit offset. */
+ int n_bits; /* Number of bits. */
+};
+
struct expr_symbol *expr_symtab_add_field(struct shash *symtab,
const char *name, enum mf_field_id,
const char *prereqs,
@@ -381,14 +390,29 @@ void expr_matches_print(const struct hmap *matches, FILE
*);
/* Action parsing helper. */
-char *expr_parse_assignment(struct lexer *lexer, const struct shash *symtab,
+char *expr_parse_assignment(struct lexer *lexer, struct expr_field *dst,
+ const struct shash *symtab,
bool (*lookup_port)(const void *aux,
const char *port_name,
unsigned int *portp),
- const void *aux, struct ofpbuf *ofpacts,
- struct expr **prereqsp);
-char *expr_parse_field(struct lexer *, int n_bits, bool rw,
- const struct shash *symtab, struct mf_subfield *,
- struct expr **prereqsp);
+ const void *aux,
+ struct ofpbuf *ofpacts, struct expr **prereqsp)
+ OVS_WARN_UNUSED_RESULT;
+char *expr_parse_exchange(struct lexer *lexer, struct expr_field *dst,
+ const struct shash *symtab,
+ bool (*lookup_port)(const void *aux,
+ const char *port_name,
+ unsigned int *portp),
+ const void *aux,
+ struct ofpbuf *ofpacts, struct expr **prereqsp)
+ OVS_WARN_UNUSED_RESULT;
+char *expr_parse_field(struct lexer *lexer, const struct shash *symtab,
+ struct expr_field *field)
+ OVS_WARN_UNUSED_RESULT;
+char *expr_expand_field(struct lexer *lexer, const struct shash *symtab,
+ const struct expr_field *orig_field,
+ int n_bits, bool rw,
+ struct mf_subfield *sf, struct expr **prereqsp)
+ OVS_WARN_UNUSED_RESULT;
#endif /* ovn/expr.h */
diff --git a/tests/ovn.at b/tests/ovn.at
index 633cf35..a05f1ef 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -465,7 +465,7 @@ outport="eth0"; next; outport="LOCAL"; next; =>
actions=set_field:0x5->reg7,resu
inport[1] = 1; => Cannot select subfield of string field inport.
ip.proto[1] = 1; => Cannot select subfield of nominal field ip.proto.
-eth.dst[40] == 1; => Syntax error at `==' expecting `='.
+eth.dst[40] == 1; => Syntax error at `==' expecting `=' or `<->'.
ip = 1; => Predicate symbol ip used where lvalue required.
ip.proto = 6; => Field ip.proto is not modifiable.
inport = {"a", "b"}; => Assignments require a single value.
--
2.1.3
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
