On Tue, Jun 28, 2016 at 02:20:40PM +0530, bscha...@redhat.com wrote:
> From: Russel Bryant <russ...@ovn.org>
This misspells Russell's name (I fixed it).
> Update the OVN expression parser to support address sets. Previously,
> you could have a set of IP or MAC addresses in this form:
>
> {addr1, addr2, ..., addrN}
>
> This patch adds support for a bit of indirection where we can define a
> set of addresses and refer to them by name.
>
> $name
>
> This '$name' can be used in the expresssions like
>
> {addr1, addr2, $name, ... }
> {$name}
> $name
>
> A future patch will expose the ability to define address sets for use.
>
> Signed-off-by: Russell Bryant <russ...@ovn.org>
> Co-authored-by: Babu Shanmugam <bscha...@redhat.com>
> Signed-off-by: Babu Shanmugam <bscha...@redhat.com>
Thanks a lot!
The results that Kyle reported are really impressive, so I want to start
getting this in. Unfortunately I think that patch 2 probably conflicts
with Ryan's incremental series, which I'm trying to prioritize because
it's been in progress so long. So I'm just looking at patch 1 in this
review round.
I've applied this to master with the following incremental folded in.
Besides some style changes, this has the following notable changes:
- Add tests for the new lexer functionality.
- Error messages that make better sense in context (note that
expr_syntax_error() takes a string that gets appended to a
description of the token), along with tests that exercise each
of the error messages.
- Use name 'cs' for an expr_constant_set, since that's the name
used elsewhere in expr.c.
- Make sure that $names can be formatted correctly with
lex_token_format().
- Use same syntax for $names as for identifiers.
- Comment updates.
--8<--------------------------cut here-------------------------->8--
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index 2883ca9..8c0768d 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -731,17 +731,12 @@ assign_constant_set_type(struct expr_context *ctx,
static bool
parse_macros(struct expr_context *ctx, struct expr_constant_set *cs,
- size_t *allocated_values)
+ size_t *allocated_values)
{
- if (!ctx->macros) {
- expr_syntax_error(ctx, "No macros defined.");
- return false;
- }
-
struct expr_constant_set *addr_set
= shash_find_data(ctx->macros, ctx->lexer->token.s);
if (!addr_set) {
- expr_syntax_error(ctx, "Unknown macro: '%s'", ctx->lexer->token.s);
+ expr_syntax_error(ctx, "expecting address set name.");
return false;
}
@@ -754,14 +749,8 @@ parse_macros(struct expr_context *ctx, struct
expr_constant_set *cs,
cs->values = xrealloc(cs->values, n_values * sizeof *cs->values);
*allocated_values = n_values;
}
- size_t i;
- for (i = 0; i < addr_set->n_values; i++) {
- union expr_constant *c1 = &cs->values[cs->n_values++];
- union expr_constant *c2 = &addr_set->values[i];
- c1->value = c2->value;
- c1->format = c2->format;
- c1->masked = c2->masked;
- c1->mask = c2->mask;
+ for (size_t i = 0; i < addr_set->n_values; i++) {
+ cs->values[cs->n_values++] = addr_set->values[i];
}
return true;
@@ -853,20 +842,21 @@ expr_constant_set_destroy(struct expr_constant_set *cs)
}
}
+/* Adds a macro named 'name' to 'macros', replacing any existing macro with the
+ * given name. */
void
expr_macros_add(struct shash *macros, const char *name,
- const char * const *values, size_t n_values)
+ const char *const *values, size_t n_values)
{
/* Replace any existing entry for this name. */
expr_macros_remove(macros, name);
- struct expr_constant_set *cset = xzalloc(sizeof *cset);
- cset->type = EXPR_C_INTEGER;
- cset->in_curlies = true;
- cset->n_values = n_values;
- cset->values = xmalloc(cset->n_values * sizeof *cset->values);
- size_t i, errors = 0;
- for (i = 0; i < n_values; i++) {
+ struct expr_constant_set *cs = xzalloc(sizeof *cs);
+ cs->type = EXPR_C_INTEGER;
+ cs->in_curlies = true;
+ cs->n_values = 0;
+ cs->values = xmalloc(n_values * sizeof *cs->values);
+ for (size_t i = 0; i < n_values; i++) {
/* Use the lexer to convert each macro into the proper
* integer format. */
struct lexer lex;
@@ -876,9 +866,8 @@ expr_macros_add(struct shash *macros, const char *name,
&& lex.token.type != LEX_T_MASKED_INTEGER) {
VLOG_WARN("Invalid address set entry: '%s', token type: %d",
values[i], lex.token.type);
- errors += 1;
} else {
- union expr_constant *c = &cset->values[i - errors];
+ union expr_constant *c = &cs->values[cs->n_values++];
c->value = lex.token.value;
c->format = lex.token.format;
c->masked = lex.token.type == LEX_T_MASKED_INTEGER;
@@ -888,34 +877,31 @@ expr_macros_add(struct shash *macros, const char *name,
}
lexer_destroy(&lex);
}
- cset->n_values -= errors;
- shash_add(macros, name, cset);
+ shash_add(macros, name, cs);
}
void
expr_macros_remove(struct shash *macros, const char *name)
{
- struct expr_constant_set *cset
- = shash_find_and_delete(macros, name);
-
- if (cset) {
- expr_constant_set_destroy(cset);
- free(cset);
+ struct expr_constant_set *cs = shash_find_and_delete(macros, name);
+ if (cs) {
+ expr_constant_set_destroy(cs);
+ free(cs);
}
}
-/* Destroy all contents of macros. */
+/* Destroy all contents of 'macros'. */
void
expr_macros_destroy(struct shash *macros)
{
struct shash_node *node, *next;
SHASH_FOR_EACH_SAFE (node, next, macros) {
- struct expr_constant_set *cset = node->data;
+ struct expr_constant_set *cs = node->data;
shash_delete(macros, node);
- expr_constant_set_destroy(cset);
+ expr_constant_set_destroy(cs);
}
}
@@ -1094,7 +1080,8 @@ struct expr *
expr_parse(struct lexer *lexer, const struct shash *symtab,
const struct shash *macros, char **errorp)
{
- struct expr_context ctx = { .lexer = lexer, .symtab = symtab,
+ struct expr_context ctx = { .lexer = lexer,
+ .symtab = symtab,
.macros = macros };
struct expr *e = expr_parse__(&ctx);
*errorp = ctx.error;
diff --git a/ovn/lib/expr.h b/ovn/lib/expr.h
index 56f6bbb..ed5300f 100644
--- a/ovn/lib/expr.h
+++ b/ovn/lib/expr.h
@@ -456,15 +456,16 @@ char *expr_parse_constant_set(struct lexer *, const
struct shash *symtab,
void expr_constant_set_destroy(struct expr_constant_set *cs);
-/* MACRO Variables
+/* Address sets, aka "macros".
*
* Instead of referring to a set of value as:
* {addr1, addr2, ..., addrN}
* You can register a set of values and refer to them as:
* $name
* The macros should all have integer/masked-integer values.
- * The values that don't qualify are ingnored
+ * The values that don't qualify are ignored.
*/
+
void expr_macros_add(struct shash *macros, const char *name,
const char * const *values, size_t n_values);
void expr_macros_remove(struct shash *macros, const char *name);
diff --git a/ovn/lib/lex.c b/ovn/lib/lex.c
index bbbc7fc..1467720 100644
--- a/ovn/lib/lex.c
+++ b/ovn/lib/lex.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2015 Nicira, Inc.
+ * Copyright (c) 2015, 2016 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -228,7 +228,7 @@ lex_token_format(const struct lex_token *token, struct ds
*s)
break;
case LEX_T_MACRO:
- ds_put_cstr(s, token->s);
+ ds_put_format(s, "$%s", token->s);
break;
case LEX_T_LPAREN:
@@ -518,7 +518,7 @@ lex_is_idn(unsigned char c)
}
static const char *
-lex_parse_id(const char *p, struct lex_token *token)
+lex_parse_id(const char *p, enum lex_type type, struct lex_token *token)
{
const char *start = p;
@@ -526,7 +526,7 @@ lex_parse_id(const char *p, struct lex_token *token)
p++;
} while (lex_is_idn(*p));
- token->type = LEX_T_ID;
+ token->type = type;
lex_token_strcpy(token, start, p - start);
return p;
}
@@ -534,9 +534,13 @@ lex_parse_id(const char *p, struct lex_token *token)
static const char *
lex_parse_macro(const char *p, struct lex_token *token)
{
- p = lex_parse_id(++p, token);
- token->type = LEX_T_MACRO;
- return p;
+ p++;
+ if (!lex_is_id1(*p)) {
+ lex_error(token, "`$' must be followed by a valid identifier.");
+ return p;
+ }
+
+ return lex_parse_id(p, LEX_T_MACRO, token);
}
/* Initializes 'token' and parses the first token from the beginning of
@@ -731,12 +735,12 @@ next:
* digits followed by a colon, but identifiers never do. */
p = (p[strspn(p, "0123456789abcdefABCDEF")] == ':'
? lex_parse_integer(p, token)
- : lex_parse_id(p, token));
+ : lex_parse_id(p, LEX_T_ID, token));
break;
default:
if (lex_is_id1(*p)) {
- p = lex_parse_id(p, token);
+ p = lex_parse_id(p, LEX_T_ID, token);
} else {
if (isprint((unsigned char) *p)) {
lex_error(token, "Invalid character `%c' in input.", *p);
diff --git a/ovn/lib/lex.h b/ovn/lib/lex.h
index 826465c..ee71a8b 100644
--- a/ovn/lib/lex.h
+++ b/ovn/lib/lex.h
@@ -78,20 +78,32 @@ enum lex_format {
};
const char *lex_format_to_string(enum lex_format);
-/* A token.
- *
- * 's' may point to 'buffer'; otherwise, it points to malloc()ed memory owned
- * by the token. */
+/* A token. */
struct lex_token {
- enum lex_type type; /* One of LEX_*. */
- char *s; /* LEX_T_ID, LEX_T_STRING, LEX_T_ERROR only. */
- enum lex_format format; /* LEX_T_INTEGER, LEX_T_MASKED_INTEGER only. */
+ /* One of LEX_*. */
+ enum lex_type type;
+
+ /* Meaningful for LEX_T_ID, LEX_T_STRING, LEX_T_ERROR, LEX_T_MACRO only.
+ * For these token types, 's' may point to 'buffer'; otherwise, it points
+ * to malloc()ed memory owned by the token.
+ *
+ * Must be NULL for other token types.
+ *
+ * For LEX_T_MACRO, 's' does not include the leading $. */
+ char *s;
+
+ /* LEX_T_INTEGER, LEX_T_MASKED_INTEGER only. */
+ enum lex_format format;
+
union {
+ /* LEX_T_INTEGER, LEX_T_MASKED_INTEGER only. */
struct {
union mf_subvalue value; /* LEX_T_INTEGER, LEX_T_MASKED_INTEGER. */
union mf_subvalue mask; /* LEX_T_MASKED_INTEGER only. */
};
- char buffer[256]; /* Buffer for LEX_T_ID/LEX_T_STRING. */
+
+ /* LEX_T_ID, LEX_T_STRING, LEX_T_ERROR, LEX_T_MACRO only. */
+ char buffer[256];
};
};
diff --git a/tests/ovn.at b/tests/ovn.at
index b93251d..decc90d 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -8,6 +8,9 @@ foo bar baz quuxquuxquux _abcd_ a.b.c.d a123_.456
"abc\u0020def" => "abc def"
" => error("Input ends inside quoted string.")dnl "
+$foo $bar $baz $quuxquuxquux $_abcd_ $a.b.c.d $a123_.456
+$1 => error("`$' must be followed by a valid identifier.") 1
+
a/*b*/c => a c
a//b c => a
a/**/b => a b
@@ -215,6 +218,8 @@ eth.type != 0x800 => Nominal field eth.type may only be
tested for equality (tak
123 == 123 => Syntax error at `123' expecting field name.
+$name => Syntax error at `$name' expecting address set name.
+
123 == xyzzy => Syntax error at `xyzzy' expecting field name.
xyzzy == 1 => Syntax error at `xyzzy' expecting field name.
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 15ebba7..87251ee 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -273,13 +273,13 @@ create_macros(struct shash *macros)
{
shash_init(macros);
- const char * const addrs1[] = {
+ static const char *const addrs1[] = {
"10.0.0.1", "10.0.0.2", "10.0.0.3",
};
- const char * const addrs2[] = {
+ static const char *const addrs2[] = {
"::1", "::2", "::3",
};
- const char * const addrs3[] = {
+ static const char *const addrs3[] = {
"00:00:00:00:00:01", "00:00:00:00:00:02", "00:00:00:00:00:03",
};
@@ -321,8 +321,7 @@ test_parse_expr__(int steps)
struct expr *expr;
char *error;
- expr = expr_parse_string(ds_cstr(&input), &symtab, ¯os,
- &error);
+ expr = expr_parse_string(ds_cstr(&input), &symtab, ¯os, &error);
if (!error && steps > 0) {
expr = expr_annotate(expr, &symtab, &error);
}
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
