From 2bbba1279ae7b197d1b21dfb2ef7e45d0bcd8000 Mon Sep 17 00:00:00 2001
From: Mark Michelson <mmichels@redhat.com>
Date: Mon, 27 Jul 2020 16:08:21 -0400
Subject: [PATCH 11/22] Add expression writeability scopes.
Logical fields are defined as either being writeable or read-only. There
is no way to make fields writeable only in specific scenarios.
This commit changes the boolean writeability field to a field of flags
indicating contexts where a field is writeable. Any time that nested
actions are used (i.e. actions enclosed in curly braces), a new scope
may be set for the nested action. For this particular commit, no
functionality is changed, and only a "default" scope is added
that mirrors the current setup. A future commit will make use of this
feature.
Change-Id: Id7a8dbedb862e8274c70597251233eeb35f81af6
Signed-off-by: Mark Michelson <mmichels@redhat.com>
Acked-by: Numan Siddique <numans@ovn.org>
---
include/ovn/expr.h | 52 ++++++++++++++++++++++++++++++++++------------
lib/actions.c | 46 +++++++++++++++++++++-------------------
lib/expr.c | 35 ++++++++++++++++++-------------
3 files changed, 84 insertions(+), 49 deletions(-)
diff --git a/include/ovn/expr.h b/include/ovn/expr.h
index 9838251c1..11bfdad5b 100644
--- a/include/ovn/expr.h
+++ b/include/ovn/expr.h
@@ -83,6 +83,10 @@ enum expr_level {
EXPR_L_ORDINAL
};
+enum expr_write_scope {
+ WR_DEFAULT = (1 << 0), /* Writeable at "global" level */
+};
+
const char *expr_level_to_string(enum expr_level);
/* A symbol.
@@ -255,7 +259,8 @@ struct expr_symbol {
char *prereqs;
bool must_crossproduct;
- bool rw;
+ enum expr_write_scope rw; /* Bit map indicating in which nested contexts
+ * the symbol is writeable */
};
void expr_symbol_format(const struct expr_symbol *, struct ds *);
@@ -273,20 +278,40 @@ bool expr_field_parse(struct lexer *, const struct shash *symtab,
struct expr_field *, struct expr **prereqsp);
void expr_field_format(const struct expr_field *, struct ds *);
-struct expr_symbol *expr_symtab_add_field(struct shash *symtab,
- const char *name, enum mf_field_id,
- const char *prereqs,
- bool must_crossproduct);
-struct expr_symbol *expr_symtab_add_subfield(struct shash *symtab,
- const char *name,
- const char *prereqs,
- const char *subfield);
-struct expr_symbol *expr_symtab_add_string(struct shash *symtab,
- const char *name, enum mf_field_id,
- const char *prereqs);
+struct expr_symbol *expr_symtab_add_field_scoped(struct shash *symtab,
+ const char *name,
+ enum mf_field_id,
+ const char *prereqs,
+ bool must_crossproduct,
+ enum expr_write_scope scope);
+
+#define expr_symtab_add_field(SYMTAB, NAME, MF_FIELD_ID, PREREQS, \
+ MUST_CROSSPRODUCT) \
+ expr_symtab_add_field_scoped((SYMTAB), (NAME), (MF_FIELD_ID), (PREREQS), \
+ (MUST_CROSSPRODUCT), WR_DEFAULT)
+
+struct expr_symbol *expr_symtab_add_subfield_scoped(struct shash *symtab,
+ const char *name, const char *prereqs, const char *subfield,
+ enum expr_write_scope scope);
+
+#define expr_symtab_add_subfield(SYMTAB, NAME, PREREQS, SUBFIELD) \
+ expr_symtab_add_subfield_scoped((SYMTAB), (NAME), (PREREQS), \
+ (SUBFIELD), WR_DEFAULT)
+
+struct expr_symbol *expr_symtab_add_string_scoped(struct shash *symtab,
+ const char *name,
+ enum mf_field_id,
+ const char *prereqs,
+ enum expr_write_scope scope);
+
+#define expr_symtab_add_string(SYMTAB, NAME, MF_FIELD_ID, PREREQS) \
+ expr_symtab_add_string_scoped((SYMTAB), (NAME), (MF_FIELD_ID), (PREREQS), \
+ WR_DEFAULT)
+
struct expr_symbol *expr_symtab_add_predicate(struct shash *symtab,
const char *name,
const char *expansion);
+
struct expr_symbol *expr_symtab_add_ovn_field(struct shash *symtab,
const char *name,
enum ovn_field_id id);
@@ -452,7 +477,8 @@ void expr_matches_print(const struct hmap *matches, FILE *);
/* Action parsing helper. */
-char *expr_type_check(const struct expr_field *, int n_bits, bool rw)
+char *expr_type_check(const struct expr_field *, int n_bits, bool rw,
+ enum expr_write_scope scope)
OVS_WARN_UNUSED_RESULT;
struct mf_subfield expr_resolve_field(const struct expr_field *);
diff --git a/lib/actions.c b/lib/actions.c
index 23e334404..460ab0cf5 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -195,6 +195,7 @@ struct action_context {
struct ofpbuf *ovnacts; /* Actions. */
struct expr *prereqs; /* Prerequisites to apply to match. */
int depth; /* Current nested action depth. */
+ enum expr_write_scope scope; /* Current writeability scope */
};
static void parse_actions(struct action_context *, enum lex_type sentinel);
@@ -207,7 +208,7 @@ action_parse_field(struct action_context *ctx,
return false;
}
- char *error = expr_type_check(f, n_bits, rw);
+ char *error = expr_type_check(f, n_bits, rw, ctx->scope);
if (error) {
lexer_error(ctx->lexer, "%s", error);
free(error);
@@ -374,7 +375,7 @@ parse_LOAD(struct action_context *ctx, const struct expr_field *lhs)
load->dst = *lhs;
- char *error = expr_type_check(lhs, lhs->n_bits, true);
+ char *error = expr_type_check(lhs, lhs->n_bits, true, ctx->scope);
if (error) {
ctx->ovnacts->size = ofs;
lexer_error(ctx->lexer, "%s", error);
@@ -513,9 +514,9 @@ parse_assignment_action(struct action_context *ctx, bool exchange,
return;
}
- char *error = expr_type_check(lhs, lhs->n_bits, true);
+ char *error = expr_type_check(lhs, lhs->n_bits, true, ctx->scope);
if (!error) {
- error = expr_type_check(&rhs, rhs.n_bits, exchange);
+ error = expr_type_check(&rhs, rhs.n_bits, exchange, ctx->scope);
}
if (error) {
lexer_error(ctx->lexer, "%s", error);
@@ -1186,7 +1187,8 @@ static void
parse_select_action(struct action_context *ctx, struct expr_field *res_field)
{
/* Check if the result field is modifiable. */
- char *error = expr_type_check(res_field, res_field->n_bits, true);
+ char *error = expr_type_check(res_field, res_field->n_bits, true,
+ ctx->scope);
if (error) {
lexer_error(ctx->lexer, "%s", error);
free(error);
@@ -1337,7 +1339,7 @@ encode_CT_CLEAR(const struct ovnact_null *null OVS_UNUSED,
* actions on a packet derived from the one being processed. */
static void
parse_nested_action(struct action_context *ctx, enum ovnact_type type,
- const char *prereq)
+ const char *prereq, enum expr_write_scope scope)
{
if (!lexer_force_match(ctx->lexer, LEX_T_LCURLY)) {
return;
@@ -1357,6 +1359,7 @@ parse_nested_action(struct action_context *ctx, enum ovnact_type type,
.ovnacts = &nested,
.prereqs = NULL,
.depth = ctx->depth + 1,
+ .scope = scope,
};
parse_actions(&inner_ctx, LEX_T_RCURLY);
@@ -1387,61 +1390,61 @@ parse_nested_action(struct action_context *ctx, enum ovnact_type type,
static void
parse_ARP(struct action_context *ctx)
{
- parse_nested_action(ctx, OVNACT_ARP, "ip4");
+ parse_nested_action(ctx, OVNACT_ARP, "ip4", ctx->scope);
}
static void
parse_ICMP4(struct action_context *ctx)
{
- parse_nested_action(ctx, OVNACT_ICMP4, "ip4");
+ parse_nested_action(ctx, OVNACT_ICMP4, "ip4", ctx->scope);
}
static void
parse_ICMP4_ERROR(struct action_context *ctx)
{
- parse_nested_action(ctx, OVNACT_ICMP4_ERROR, "ip4");
+ parse_nested_action(ctx, OVNACT_ICMP4_ERROR, "ip4", ctx->scope);
}
static void
parse_ICMP6(struct action_context *ctx)
{
- parse_nested_action(ctx, OVNACT_ICMP6, "ip6");
+ parse_nested_action(ctx, OVNACT_ICMP6, "ip6", ctx->scope);
}
static void
parse_ICMP6_ERROR(struct action_context *ctx)
{
- parse_nested_action(ctx, OVNACT_ICMP6_ERROR, "ip6");
+ parse_nested_action(ctx, OVNACT_ICMP6_ERROR, "ip6", ctx->scope);
}
static void
parse_TCP_RESET(struct action_context *ctx)
{
- parse_nested_action(ctx, OVNACT_TCP_RESET, "tcp");
+ parse_nested_action(ctx, OVNACT_TCP_RESET, "tcp", ctx->scope);
}
static void
parse_ND_NA(struct action_context *ctx)
{
- parse_nested_action(ctx, OVNACT_ND_NA, "nd_ns");
+ parse_nested_action(ctx, OVNACT_ND_NA, "nd_ns", ctx->scope);
}
static void
parse_ND_NA_ROUTER(struct action_context *ctx)
{
- parse_nested_action(ctx, OVNACT_ND_NA_ROUTER, "nd_ns");
+ parse_nested_action(ctx, OVNACT_ND_NA_ROUTER, "nd_ns", ctx->scope);
}
static void
parse_ND_NS(struct action_context *ctx)
{
- parse_nested_action(ctx, OVNACT_ND_NS, "ip6");
+ parse_nested_action(ctx, OVNACT_ND_NS, "ip6", ctx->scope);
}
static void
parse_CLONE(struct action_context *ctx)
{
- parse_nested_action(ctx, OVNACT_CLONE, NULL);
+ parse_nested_action(ctx, OVNACT_CLONE, NULL, WR_DEFAULT);
}
static void
@@ -1947,7 +1950,7 @@ parse_lookup_mac_bind(struct action_context *ctx,
struct ovnact_lookup_mac_bind *lookup_mac)
{
/* Validate that the destination is a 1-bit, modifiable field. */
- char *error = expr_type_check(dst, 1, true);
+ char *error = expr_type_check(dst, 1, true, ctx->scope);
if (error) {
lexer_error(ctx->lexer, "%s", error);
free(error);
@@ -2053,7 +2056,7 @@ parse_lookup_mac_bind_ip(struct action_context *ctx,
struct ovnact_lookup_mac_bind_ip *lookup_mac)
{
/* Validate that the destination is a 1-bit, modifiable field. */
- char *error = expr_type_check(dst, 1, true);
+ char *error = expr_type_check(dst, 1, true, ctx->scope);
if (error) {
lexer_error(ctx->lexer, "%s", error);
free(error);
@@ -2283,7 +2286,7 @@ parse_put_opts(struct action_context *ctx, const struct expr_field *dst,
lexer_get(ctx->lexer); /* Skip '('. */
/* Validate that the destination is a 1-bit, modifiable field. */
- char *error = expr_type_check(dst, 1, true);
+ char *error = expr_type_check(dst, 1, true, ctx->scope);
if (error) {
lexer_error(ctx->lexer, "%s", error);
free(error);
@@ -2680,7 +2683,7 @@ parse_dns_lookup(struct action_context *ctx, const struct expr_field *dst,
return;
}
/* Validate that the destination is a 1-bit, modifiable field. */
- char *error = expr_type_check(dst, 1, true);
+ char *error = expr_type_check(dst, 1, true, ctx->scope);
if (error) {
lexer_error(ctx->lexer, "%s", error);
free(error);
@@ -3205,7 +3208,7 @@ parse_check_pkt_larger(struct action_context *ctx,
struct ovnact_check_pkt_larger *cipl)
{
/* Validate that the destination is a 1-bit, modifiable field. */
- char *error = expr_type_check(dst, 1, true);
+ char *error = expr_type_check(dst, 1, true, ctx->scope);
if (error) {
lexer_error(ctx->lexer, "%s", error);
free(error);
@@ -3677,6 +3680,7 @@ ovnacts_parse(struct lexer *lexer, const struct ovnact_parse_params *pp,
.lexer = lexer,
.ovnacts = ovnacts,
.prereqs = NULL,
+ .scope = WR_DEFAULT,
};
if (!lexer->error) {
parse_actions(&ctx, LEX_T_END);
diff --git a/lib/expr.c b/lib/expr.c
index 497b2accc..c07e7dd4d 100644
--- a/lib/expr.c
+++ b/lib/expr.c
@@ -1447,7 +1447,7 @@ expr_symbol_format(const struct expr_symbol *symbol, struct ds *s)
static struct expr_symbol *
add_symbol(struct shash *symtab, const char *name, int width,
const char *prereqs, enum expr_level level,
- bool must_crossproduct, bool rw)
+ bool must_crossproduct, enum expr_write_scope rw)
{
struct expr_symbol *symbol = xzalloc(sizeof *symbol);
symbol->name = xstrdup(name);
@@ -1471,9 +1471,10 @@ add_symbol(struct shash *symtab, const char *name, int width,
* Use subfields to duplicate or subset a field (you can even make a subfield
* include all the bits of the "parent" field if you like). */
struct expr_symbol *
-expr_symtab_add_field(struct shash *symtab, const char *name,
- enum mf_field_id id, const char *prereqs,
- bool must_crossproduct)
+expr_symtab_add_field_scoped(struct shash *symtab, const char *name,
+ enum mf_field_id id, const char *prereqs,
+ bool must_crossproduct,
+ enum expr_write_scope scope)
{
const struct mf_field *field = mf_from_id(id);
struct expr_symbol *symbol;
@@ -1482,7 +1483,8 @@ expr_symtab_add_field(struct shash *symtab, const char *name,
(field->maskable == MFM_FULLY
? EXPR_L_ORDINAL
: EXPR_L_NOMINAL),
- must_crossproduct, field->writable);
+ must_crossproduct,
+ field->writable ? scope : 0);
symbol->field = field;
return symbol;
}
@@ -1511,8 +1513,9 @@ parse_field_from_string(const char *s, const struct shash *symtab,
* 'subfield' must describe the subfield as a string, e.g. "vlan.tci[0..11]"
* for the low 12 bits of a larger field named "vlan.tci". */
struct expr_symbol *
-expr_symtab_add_subfield(struct shash *symtab, const char *name,
- const char *prereqs, const char *subfield)
+expr_symtab_add_subfield_scoped(struct shash *symtab, const char *name,
+ const char *prereqs, const char *subfield,
+ enum expr_write_scope scope)
{
struct expr_symbol *symbol;
struct expr_field f;
@@ -1531,7 +1534,7 @@ expr_symtab_add_subfield(struct shash *symtab, const char *name,
}
symbol = add_symbol(symtab, name, f.n_bits, prereqs, level, false,
- f.symbol->rw);
+ f.symbol->rw ? scope : 0);
symbol->parent = f.symbol;
symbol->parent_ofs = f.ofs;
return symbol;
@@ -1540,14 +1543,15 @@ expr_symtab_add_subfield(struct shash *symtab, const char *name,
/* Adds a string-valued symbol named 'name' to 'symtab' with the specified
* 'prereqs'. */
struct expr_symbol *
-expr_symtab_add_string(struct shash *symtab, const char *name,
- enum mf_field_id id, const char *prereqs)
+expr_symtab_add_string_scoped(struct shash *symtab, const char *name,
+ enum mf_field_id id, const char *prereqs,
+ enum expr_write_scope scope)
{
const struct mf_field *field = mf_from_id(id);
struct expr_symbol *symbol;
symbol = add_symbol(symtab, name, 0, prereqs, EXPR_L_NOMINAL, false,
- field->writable);
+ field->writable ? scope : 0);
symbol->field = field;
return symbol;
}
@@ -1610,7 +1614,7 @@ expr_symtab_add_predicate(struct shash *symtab, const char *name,
return NULL;
}
- symbol = add_symbol(symtab, name, 1, NULL, level, false, false);
+ symbol = add_symbol(symtab, name, 1, NULL, level, false, 0);
symbol->predicate = xstrdup(expansion);
return symbol;
}
@@ -1623,7 +1627,7 @@ expr_symtab_add_ovn_field(struct shash *symtab, const char *name,
struct expr_symbol *symbol;
symbol = add_symbol(symtab, name, ovn_field->n_bits, NULL,
- EXPR_L_NOMINAL, false, true);
+ EXPR_L_NOMINAL, false, UINT32_MAX);
symbol->ovn_field = ovn_field;
return symbol;
}
@@ -3322,7 +3326,8 @@ expr_evaluate(const struct expr *e, const struct flow *uflow,
* if 'f' is acceptable, otherwise a malloc()'d error message that the caller
* must free(). */
char * OVS_WARN_UNUSED_RESULT
-expr_type_check(const struct expr_field *f, int n_bits, bool rw)
+expr_type_check(const struct expr_field *f, int n_bits, bool rw,
+ uint32_t write_scope)
{
if (n_bits != f->n_bits) {
if (n_bits && f->n_bits) {
@@ -3340,7 +3345,7 @@ expr_type_check(const struct expr_field *f, int n_bits, bool rw)
}
}
- if (rw && !f->symbol->rw) {
+ if (rw && !(f->symbol->rw & write_scope)) {
return xasprintf("Field %s is not modifiable.", f->symbol->name);
}
--
2.26.2