| Message ID | 1467103841-31640-2-git-send-email-bschanmu@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Jun 28, 2016 at 3:50 AM, <bschanmu@redhat.com> wrote: > From: Russel Bryant <russell@ovn.org> > > 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 <russell@ovn.org> > Co-authored-by: Babu Shanmugam <bschanmu@redhat.com> > Signed-off-by: Babu Shanmugam <bschanmu@redhat.com> Tested-by: Kyle Mestery <mestery@mestery.com> I've run these through the ovn-scale-test CI setup [1]. We have a test which creates ports and puts ACLs on them. This is a pure OVN control plane test with Rally driving creating and managing the resources under test. The test uses the following parameters: * 5 networks * 200 ports per network * 5 ACLs per port Before the address sets patches, these tests took an average of 820 seconds to run. With the address sets patches, they take an average of 460 seconds. That's quite the improvement! Thanks for the work here Russell and Babu! [1] https://github.com/openvswitch/ovn-scale-test/tree/master/ci > --- > ovn/controller/lflow.c | 2 +- > ovn/lib/actions.c | 2 +- > ovn/lib/expr.c | 126 ++++++++++++++++++++++++++++++++++++++++++++++--- > ovn/lib/expr.h | 17 +++++++ > ovn/lib/lex.c | 16 +++++++ > ovn/lib/lex.h | 1 + > tests/ovn.at | 50 ++++++++++++++++++++ > tests/test-ovn.c | 31 ++++++++++-- > 8 files changed, 234 insertions(+), 11 deletions(-) > > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > index 52e6131..433df70 100644 > --- a/ovn/controller/lflow.c > +++ b/ovn/controller/lflow.c > @@ -306,7 +306,7 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports, > struct hmap matches; > struct expr *expr; > > - expr = expr_parse_string(lflow->match, &symtab, &error); > + expr = expr_parse_string(lflow->match, &symtab, NULL, &error); > if (!error) { > if (prereqs) { > expr = expr_combine(EXPR_T_AND, expr, prereqs); > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c > index 569970e..ebd7159 100644 > --- a/ovn/lib/actions.c > +++ b/ovn/lib/actions.c > @@ -208,7 +208,7 @@ add_prerequisite(struct action_context *ctx, const char *prerequisite) > struct expr *expr; > char *error; > > - expr = expr_parse_string(prerequisite, ctx->ap->symtab, &error); > + expr = expr_parse_string(prerequisite, ctx->ap->symtab, NULL, &error); > ovs_assert(!error); > ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, expr); > } > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c > index 91bff07..2883ca9 100644 > --- a/ovn/lib/expr.c > +++ b/ovn/lib/expr.c > @@ -409,6 +409,7 @@ expr_print(const struct expr *e) > struct expr_context { > struct lexer *lexer; /* Lexer for pulling more tokens. */ > const struct shash *symtab; /* Symbol table. */ > + const struct shash *macros; /* Table of macros. */ > char *error; /* Error, if any, otherwise NULL. */ > bool not; /* True inside odd number of NOT operators. */ > }; > @@ -729,6 +730,44 @@ 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) > +{ > + 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); > + return false; > + } > + > + if (!assign_constant_set_type(ctx, cs, EXPR_C_INTEGER)) { > + return false; > + } > + > + size_t n_values = cs->n_values + addr_set->n_values; > + if (n_values >= *allocated_values) { > + 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; > + } > + > + return true; > +} > + > +static bool > parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, > size_t *allocated_values) > { > @@ -759,6 +798,12 @@ parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, > } > lexer_get(ctx->lexer); > return true; > + } else if (ctx->lexer->token.type == LEX_T_MACRO) { > + if (!parse_macros(ctx, cs, allocated_values)) { > + return false; > + } > + lexer_get(ctx->lexer); > + return true; > } else { > expr_syntax_error(ctx, "expecting constant."); > return false; > @@ -808,6 +853,72 @@ expr_constant_set_destroy(struct expr_constant_set *cs) > } > } > > +void > +expr_macros_add(struct shash *macros, const char *name, > + 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++) { > + /* Use the lexer to convert each macro into the proper > + * integer format. */ > + struct lexer lex; > + lexer_init(&lex, values[i]); > + lexer_get(&lex); > + if (lex.token.type != LEX_T_INTEGER > + && 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]; > + c->value = lex.token.value; > + c->format = lex.token.format; > + c->masked = lex.token.type == LEX_T_MASKED_INTEGER; > + if (c->masked) { > + c->mask = lex.token.mask; > + } > + } > + lexer_destroy(&lex); > + } > + cset->n_values -= errors; > + > + shash_add(macros, name, cset); > +} > + > +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); > + } > +} > + > +/* 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; > + > + shash_delete(macros, node); > + expr_constant_set_destroy(cset); > + } > +} > + > static struct expr * > expr_parse_primary(struct expr_context *ctx, bool *atomic) > { > @@ -980,9 +1091,11 @@ expr_parse__(struct expr_context *ctx) > * The caller must eventually free the returned expression (with > * expr_destroy()) or error (with free()). */ > struct expr * > -expr_parse(struct lexer *lexer, const struct shash *symtab, char **errorp) > +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; > ovs_assert((ctx.error != NULL) != (e != NULL)); > @@ -991,14 +1104,15 @@ expr_parse(struct lexer *lexer, const struct shash *symtab, char **errorp) > > /* Like expr_parse(), but the expression is taken from 's'. */ > struct expr * > -expr_parse_string(const char *s, const struct shash *symtab, char **errorp) > +expr_parse_string(const char *s, const struct shash *symtab, > + const struct shash *macros, char **errorp) > { > struct lexer lexer; > struct expr *expr; > > lexer_init(&lexer, s); > lexer_get(&lexer); > - expr = expr_parse(&lexer, symtab, errorp); > + expr = expr_parse(&lexer, symtab, macros, errorp); > if (!*errorp && lexer.token.type != LEX_T_END) { > *errorp = xstrdup("Extra tokens at end of input."); > expr_destroy(expr); > @@ -1149,7 +1263,7 @@ expr_get_level(const struct expr *expr) > static enum expr_level > expr_parse_level(const char *s, const struct shash *symtab, char **errorp) > { > - struct expr *expr = expr_parse_string(s, symtab, errorp); > + struct expr *expr = expr_parse_string(s, symtab, NULL, errorp); > enum expr_level level = expr ? expr_get_level(expr) : EXPR_L_NOMINAL; > expr_destroy(expr); > return level; > @@ -1292,7 +1406,7 @@ parse_and_annotate(const char *s, const struct shash *symtab, > char *error; > struct expr *expr; > > - expr = expr_parse_string(s, symtab, &error); > + expr = expr_parse_string(s, symtab, NULL, &error); > if (expr) { > expr = expr_annotate__(expr, symtab, nesting, &error); > } > diff --git a/ovn/lib/expr.h b/ovn/lib/expr.h > index d6f8489..56f6bbb 100644 > --- a/ovn/lib/expr.h > +++ b/ovn/lib/expr.h > @@ -352,8 +352,10 @@ expr_from_node(const struct ovs_list *node) > void expr_format(const struct expr *, struct ds *); > void expr_print(const struct expr *); > struct expr *expr_parse(struct lexer *, const struct shash *symtab, > + const struct shash *macros, > char **errorp); > struct expr *expr_parse_string(const char *, const struct shash *symtab, > + const struct shash *macros, > char **errorp); > > struct expr *expr_clone(struct expr *); > @@ -453,4 +455,19 @@ char *expr_parse_constant_set(struct lexer *, const struct shash *symtab, > OVS_WARN_UNUSED_RESULT; > void expr_constant_set_destroy(struct expr_constant_set *cs); > > + > +/* MACRO Variables > + * > + * 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 > + */ > +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); > +void expr_macros_destroy(struct shash *macros); > + > #endif /* ovn/expr.h */ > diff --git a/ovn/lib/lex.c b/ovn/lib/lex.c > index 556288f..bbbc7fc 100644 > --- a/ovn/lib/lex.c > +++ b/ovn/lib/lex.c > @@ -227,6 +227,10 @@ lex_token_format(const struct lex_token *token, struct ds *s) > lex_token_format_masked_integer(token, s); > break; > > + case LEX_T_MACRO: > + ds_put_cstr(s, token->s); > + break; > + > case LEX_T_LPAREN: > ds_put_cstr(s, "("); > break; > @@ -527,6 +531,14 @@ lex_parse_id(const char *p, struct lex_token *token) > return p; > } > > +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; > +} > + > /* Initializes 'token' and parses the first token from the beginning of > * null-terminated string 'p' into 'token'. Stores a pointer to the start of > * the token (after skipping white space and comments, if any) into '*startp'. > @@ -697,6 +709,10 @@ next: > } > break; > > + case '$': > + p = lex_parse_macro(p, token); > + break; > + > case '0': case '1': case '2': case '3': case '4': > case '5': case '6': case '7': case '8': case '9': > case ':': > diff --git a/ovn/lib/lex.h b/ovn/lib/lex.h > index 578ef40..826465c 100644 > --- a/ovn/lib/lex.h > +++ b/ovn/lib/lex.h > @@ -36,6 +36,7 @@ enum lex_type { > LEX_T_STRING, /* "foo" */ > LEX_T_INTEGER, /* 12345 or 1.2.3.4 or ::1 or 01:02:03:04:05 */ > LEX_T_MASKED_INTEGER, /* 12345/10 or 1.2.0.0/16 or ::2/127 or... */ > + LEX_T_MACRO, /* $NAME */ > LEX_T_ERROR, /* invalid input */ > > /* Bare tokens. */ > diff --git a/tests/ovn.at b/tests/ovn.at > index 297070c..840d0ef 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -434,6 +434,56 @@ AT_CHECK([expr_to_flow 'inport == "eth0" && inport == "eth1"'], [0], [dnl > ]) > AT_CLEANUP > > +AT_SETUP([ovn -- converting expressions to flows -- address sets]) > +expr_to_flow () { > + echo "$1" | ovstest test-ovn expr-to-flows | sort > +} > +AT_CHECK([expr_to_flow 'ip4.src == {10.0.0.1, 10.0.0.2, 10.0.0.3}'], [0], [dnl > +ip,nw_src=10.0.0.1 > +ip,nw_src=10.0.0.2 > +ip,nw_src=10.0.0.3 > +]) > +AT_CHECK([expr_to_flow 'ip4.src == $set1'], [0], [dnl > +ip,nw_src=10.0.0.1 > +ip,nw_src=10.0.0.2 > +ip,nw_src=10.0.0.3 > +]) > +AT_CHECK([expr_to_flow 'ip4.src == {1.2.3.4, $set1}'], [0], [dnl > +ip,nw_src=1.2.3.4 > +ip,nw_src=10.0.0.1 > +ip,nw_src=10.0.0.2 > +ip,nw_src=10.0.0.3 > +]) > +AT_CHECK([expr_to_flow 'ip4.src == {1.2.0.0/20, 5.5.5.0/24, $set1}'], [0], [dnl > +ip,nw_src=1.2.0.0/20 > +ip,nw_src=10.0.0.1 > +ip,nw_src=10.0.0.2 > +ip,nw_src=10.0.0.3 > +ip,nw_src=5.5.5.0/24 > +]) > +AT_CHECK([expr_to_flow 'ip6.src == {::1, ::2, ::3}'], [0], [dnl > +ipv6,ipv6_src=::1 > +ipv6,ipv6_src=::2 > +ipv6,ipv6_src=::3 > +]) > +AT_CHECK([expr_to_flow 'ip6.src == {::1, $set2, ::4}'], [0], [dnl > +ipv6,ipv6_src=::1 > +ipv6,ipv6_src=::2 > +ipv6,ipv6_src=::3 > +ipv6,ipv6_src=::4 > +]) > +AT_CHECK([expr_to_flow 'eth.src == {00:00:00:00:00:01, 00:00:00:00:00:02, 00:00:00:00:00:03}'], [0], [dnl > +dl_src=00:00:00:00:00:01 > +dl_src=00:00:00:00:00:02 > +dl_src=00:00:00:00:00:03 > +]) > +AT_CHECK([expr_to_flow 'eth.src == {$set3}'], [0], [dnl > +dl_src=00:00:00:00:00:01 > +dl_src=00:00:00:00:00:02 > +dl_src=00:00:00:00:00:03 > +]) > +AT_CLEANUP > + > AT_SETUP([ovn -- action parsing]) > dnl Text before => is input, text after => is expected output. > AT_DATA([test-cases.txt], [[ > diff --git a/tests/test-ovn.c b/tests/test-ovn.c > index 18e5aca..15ebba7 100644 > --- a/tests/test-ovn.c > +++ b/tests/test-ovn.c > @@ -268,6 +268,26 @@ create_dhcp_opts(struct hmap *dhcp_opts) > dhcp_opt_add(dhcp_opts, "lease_time", 51, "uint32"); > } > > +static void > +create_macros(struct shash *macros) > +{ > + shash_init(macros); > + > + const char * const addrs1[] = { > + "10.0.0.1", "10.0.0.2", "10.0.0.3", > + }; > + const char * const addrs2[] = { > + "::1", "::2", "::3", > + }; > + const char * const addrs3[] = { > + "00:00:00:00:00:01", "00:00:00:00:00:02", "00:00:00:00:00:03", > + }; > + > + expr_macros_add(macros, "set1", addrs1, 3); > + expr_macros_add(macros, "set2", addrs2, 3); > + expr_macros_add(macros, "set3", addrs3, 3); > +} > + > static bool > lookup_port_cb(const void *ports_, const char *port_name, unsigned int *portp) > { > @@ -284,10 +304,12 @@ static void > test_parse_expr__(int steps) > { > struct shash symtab; > + struct shash macros; > struct simap ports; > struct ds input; > > create_symtab(&symtab); > + create_macros(¯os); > > simap_init(&ports); > simap_put(&ports, "eth0", 5); > @@ -299,7 +321,8 @@ test_parse_expr__(int steps) > struct expr *expr; > char *error; > > - expr = expr_parse_string(ds_cstr(&input), &symtab, &error); > + expr = expr_parse_string(ds_cstr(&input), &symtab, ¯os, > + &error); > if (!error && steps > 0) { > expr = expr_annotate(expr, &symtab, &error); > } > @@ -336,6 +359,8 @@ test_parse_expr__(int steps) > simap_destroy(&ports); > expr_symtab_destroy(&symtab); > shash_destroy(&symtab); > + expr_macros_destroy(¯os); > + shash_destroy(¯os); > } > > static void > @@ -480,7 +505,7 @@ test_evaluate_expr(struct ovs_cmdl_context *ctx) > struct expr *expr; > char *error; > > - expr = expr_parse_string(ds_cstr(&input), &symtab, &error); > + expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, &error); > if (!error) { > expr = expr_annotate(expr, &symtab, &error); > } > @@ -954,7 +979,7 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab, > expr_format(expr, &s); > > char *error; > - modified = expr_parse_string(ds_cstr(&s), symtab, &error); > + modified = expr_parse_string(ds_cstr(&s), symtab, NULL, &error); > if (error) { > fprintf(stderr, "%s fails to parse (%s)\n", > ds_cstr(&s), error); > -- > 2.5.5 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On Thursday, June 30, 2016, Kyle Mestery <mestery@mestery.com> wrote: > On Tue, Jun 28, 2016 at 3:50 AM, <bschanmu@redhat.com <javascript:;>> > wrote: > > From: Russel Bryant <russell@ovn.org <javascript:;>> > > > > 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 <russell@ovn.org <javascript:;>> > > Co-authored-by: Babu Shanmugam <bschanmu@redhat.com <javascript:;>> > > Signed-off-by: Babu Shanmugam <bschanmu@redhat.com <javascript:;>> > > Tested-by: Kyle Mestery <mestery@mestery.com <javascript:;>> > > I've run these through the ovn-scale-test CI setup [1]. We have a test > which creates ports and puts ACLs on them. This is a pure OVN control > plane test with Rally driving creating and managing the resources > under test. The test uses the following parameters: > > * 5 networks > * 200 ports per network > * 5 ACLs per port > > Before the address sets patches, these tests took an average of 820 > seconds to run. With the address sets patches, they take an average of > 460 seconds. That's quite the improvement! Thanks for the work here > Russell and Babu! > > [1] https://github.com/openvswitch/ovn-scale-test/tree/master/ci Wow, that's a nice speed up. What do the ACLs look like that get created? What did they look like after changing them to use address sets? Russell > > > --- > > ovn/controller/lflow.c | 2 +- > > ovn/lib/actions.c | 2 +- > > ovn/lib/expr.c | 126 > ++++++++++++++++++++++++++++++++++++++++++++++--- > > ovn/lib/expr.h | 17 +++++++ > > ovn/lib/lex.c | 16 +++++++ > > ovn/lib/lex.h | 1 + > > tests/ovn.at | 50 ++++++++++++++++++++ > > tests/test-ovn.c | 31 ++++++++++-- > > 8 files changed, 234 insertions(+), 11 deletions(-) > > > > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > > index 52e6131..433df70 100644 > > --- a/ovn/controller/lflow.c > > +++ b/ovn/controller/lflow.c > > @@ -306,7 +306,7 @@ add_logical_flows(struct controller_ctx *ctx, const > struct lport_index *lports, > > struct hmap matches; > > struct expr *expr; > > > > - expr = expr_parse_string(lflow->match, &symtab, &error); > > + expr = expr_parse_string(lflow->match, &symtab, NULL, &error); > > if (!error) { > > if (prereqs) { > > expr = expr_combine(EXPR_T_AND, expr, prereqs); > > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c > > index 569970e..ebd7159 100644 > > --- a/ovn/lib/actions.c > > +++ b/ovn/lib/actions.c > > @@ -208,7 +208,7 @@ add_prerequisite(struct action_context *ctx, const > char *prerequisite) > > struct expr *expr; > > char *error; > > > > - expr = expr_parse_string(prerequisite, ctx->ap->symtab, &error); > > + expr = expr_parse_string(prerequisite, ctx->ap->symtab, NULL, > &error); > > ovs_assert(!error); > > ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, expr); > > } > > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c > > index 91bff07..2883ca9 100644 > > --- a/ovn/lib/expr.c > > +++ b/ovn/lib/expr.c > > @@ -409,6 +409,7 @@ expr_print(const struct expr *e) > > struct expr_context { > > struct lexer *lexer; /* Lexer for pulling more tokens. */ > > const struct shash *symtab; /* Symbol table. */ > > + const struct shash *macros; /* Table of macros. */ > > char *error; /* Error, if any, otherwise NULL. */ > > bool not; /* True inside odd number of NOT > operators. */ > > }; > > @@ -729,6 +730,44 @@ 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) > > +{ > > + 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); > > + return false; > > + } > > + > > + if (!assign_constant_set_type(ctx, cs, EXPR_C_INTEGER)) { > > + return false; > > + } > > + > > + size_t n_values = cs->n_values + addr_set->n_values; > > + if (n_values >= *allocated_values) { > > + 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; > > + } > > + > > + return true; > > +} > > + > > +static bool > > parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, > > size_t *allocated_values) > > { > > @@ -759,6 +798,12 @@ parse_constant(struct expr_context *ctx, struct > expr_constant_set *cs, > > } > > lexer_get(ctx->lexer); > > return true; > > + } else if (ctx->lexer->token.type == LEX_T_MACRO) { > > + if (!parse_macros(ctx, cs, allocated_values)) { > > + return false; > > + } > > + lexer_get(ctx->lexer); > > + return true; > > } else { > > expr_syntax_error(ctx, "expecting constant."); > > return false; > > @@ -808,6 +853,72 @@ expr_constant_set_destroy(struct expr_constant_set > *cs) > > } > > } > > > > +void > > +expr_macros_add(struct shash *macros, const char *name, > > + 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++) { > > + /* Use the lexer to convert each macro into the proper > > + * integer format. */ > > + struct lexer lex; > > + lexer_init(&lex, values[i]); > > + lexer_get(&lex); > > + if (lex.token.type != LEX_T_INTEGER > > + && 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]; > > + c->value = lex.token.value; > > + c->format = lex.token.format; > > + c->masked = lex.token.type == LEX_T_MASKED_INTEGER; > > + if (c->masked) { > > + c->mask = lex.token.mask; > > + } > > + } > > + lexer_destroy(&lex); > > + } > > + cset->n_values -= errors; > > + > > + shash_add(macros, name, cset); > > +} > > + > > +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); > > + } > > +} > > + > > +/* 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; > > + > > + shash_delete(macros, node); > > + expr_constant_set_destroy(cset); > > + } > > +} > > + > > static struct expr * > > expr_parse_primary(struct expr_context *ctx, bool *atomic) > > { > > @@ -980,9 +1091,11 @@ expr_parse__(struct expr_context *ctx) > > * The caller must eventually free the returned expression (with > > * expr_destroy()) or error (with free()). */ > > struct expr * > > -expr_parse(struct lexer *lexer, const struct shash *symtab, char > **errorp) > > +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; > > ovs_assert((ctx.error != NULL) != (e != NULL)); > > @@ -991,14 +1104,15 @@ expr_parse(struct lexer *lexer, const struct > shash *symtab, char **errorp) > > > > /* Like expr_parse(), but the expression is taken from 's'. */ > > struct expr * > > -expr_parse_string(const char *s, const struct shash *symtab, char > **errorp) > > +expr_parse_string(const char *s, const struct shash *symtab, > > + const struct shash *macros, char **errorp) > > { > > struct lexer lexer; > > struct expr *expr; > > > > lexer_init(&lexer, s); > > lexer_get(&lexer); > > - expr = expr_parse(&lexer, symtab, errorp); > > + expr = expr_parse(&lexer, symtab, macros, errorp); > > if (!*errorp && lexer.token.type != LEX_T_END) { > > *errorp = xstrdup("Extra tokens at end of input."); > > expr_destroy(expr); > > @@ -1149,7 +1263,7 @@ expr_get_level(const struct expr *expr) > > static enum expr_level > > expr_parse_level(const char *s, const struct shash *symtab, char > **errorp) > > { > > - struct expr *expr = expr_parse_string(s, symtab, errorp); > > + struct expr *expr = expr_parse_string(s, symtab, NULL, errorp); > > enum expr_level level = expr ? expr_get_level(expr) : > EXPR_L_NOMINAL; > > expr_destroy(expr); > > return level; > > @@ -1292,7 +1406,7 @@ parse_and_annotate(const char *s, const struct > shash *symtab, > > char *error; > > struct expr *expr; > > > > - expr = expr_parse_string(s, symtab, &error); > > + expr = expr_parse_string(s, symtab, NULL, &error); > > if (expr) { > > expr = expr_annotate__(expr, symtab, nesting, &error); > > } > > diff --git a/ovn/lib/expr.h b/ovn/lib/expr.h > > index d6f8489..56f6bbb 100644 > > --- a/ovn/lib/expr.h > > +++ b/ovn/lib/expr.h > > @@ -352,8 +352,10 @@ expr_from_node(const struct ovs_list *node) > > void expr_format(const struct expr *, struct ds *); > > void expr_print(const struct expr *); > > struct expr *expr_parse(struct lexer *, const struct shash *symtab, > > + const struct shash *macros, > > char **errorp); > > struct expr *expr_parse_string(const char *, const struct shash *symtab, > > + const struct shash *macros, > > char **errorp); > > > > struct expr *expr_clone(struct expr *); > > @@ -453,4 +455,19 @@ char *expr_parse_constant_set(struct lexer *, const > struct shash *symtab, > > OVS_WARN_UNUSED_RESULT; > > void expr_constant_set_destroy(struct expr_constant_set *cs); > > > > + > > +/* MACRO Variables > > + * > > + * 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 > > + */ > > +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); > > +void expr_macros_destroy(struct shash *macros); > > + > > #endif /* ovn/expr.h */ > > diff --git a/ovn/lib/lex.c b/ovn/lib/lex.c > > index 556288f..bbbc7fc 100644 > > --- a/ovn/lib/lex.c > > +++ b/ovn/lib/lex.c > > @@ -227,6 +227,10 @@ lex_token_format(const struct lex_token *token, > struct ds *s) > > lex_token_format_masked_integer(token, s); > > break; > > > > + case LEX_T_MACRO: > > + ds_put_cstr(s, token->s); > > + break; > > + > > case LEX_T_LPAREN: > > ds_put_cstr(s, "("); > > break; > > @@ -527,6 +531,14 @@ lex_parse_id(const char *p, struct lex_token *token) > > return p; > > } > > > > +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; > > +} > > + > > /* Initializes 'token' and parses the first token from the beginning of > > * null-terminated string 'p' into 'token'. Stores a pointer to the > start of > > * the token (after skipping white space and comments, if any) into > '*startp'. > > @@ -697,6 +709,10 @@ next: > > } > > break; > > > > + case '$': > > + p = lex_parse_macro(p, token); > > + break; > > + > > case '0': case '1': case '2': case '3': case '4': > > case '5': case '6': case '7': case '8': case '9': > > case ':': > > diff --git a/ovn/lib/lex.h b/ovn/lib/lex.h > > index 578ef40..826465c 100644 > > --- a/ovn/lib/lex.h > > +++ b/ovn/lib/lex.h > > @@ -36,6 +36,7 @@ enum lex_type { > > LEX_T_STRING, /* "foo" */ > > LEX_T_INTEGER, /* 12345 or 1.2.3.4 or ::1 or > 01:02:03:04:05 */ > > LEX_T_MASKED_INTEGER, /* 12345/10 or 1.2.0.0/16 or ::2/127 > or... */ > > + LEX_T_MACRO, /* $NAME */ > > LEX_T_ERROR, /* invalid input */ > > > > /* Bare tokens. */ > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 297070c..840d0ef 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -434,6 +434,56 @@ AT_CHECK([expr_to_flow 'inport == "eth0" && inport > == "eth1"'], [0], [dnl > > ]) > > AT_CLEANUP > > > > +AT_SETUP([ovn -- converting expressions to flows -- address sets]) > > +expr_to_flow () { > > + echo "$1" | ovstest test-ovn expr-to-flows | sort > > +} > > +AT_CHECK([expr_to_flow 'ip4.src == {10.0.0.1, 10.0.0.2, 10.0.0.3}'], > [0], [dnl > > +ip,nw_src=10.0.0.1 > > +ip,nw_src=10.0.0.2 > > +ip,nw_src=10.0.0.3 > > +]) > > +AT_CHECK([expr_to_flow 'ip4.src == $set1'], [0], [dnl > > +ip,nw_src=10.0.0.1 > > +ip,nw_src=10.0.0.2 > > +ip,nw_src=10.0.0.3 > > +]) > > +AT_CHECK([expr_to_flow 'ip4.src == {1.2.3.4, $set1}'], [0], [dnl > > +ip,nw_src=1.2.3.4 > > +ip,nw_src=10.0.0.1 > > +ip,nw_src=10.0.0.2 > > +ip,nw_src=10.0.0.3 > > +]) > > +AT_CHECK([expr_to_flow 'ip4.src == {1.2.0.0/20, 5.5.5.0/24, $set1}'], > [0], [dnl > > +ip,nw_src=1.2.0.0/20 > > +ip,nw_src=10.0.0.1 > > +ip,nw_src=10.0.0.2 > > +ip,nw_src=10.0.0.3 > > +ip,nw_src=5.5.5.0/24 > > +]) > > +AT_CHECK([expr_to_flow 'ip6.src == {::1, ::2, ::3}'], [0], [dnl > > +ipv6,ipv6_src=::1 > > +ipv6,ipv6_src=::2 > > +ipv6,ipv6_src=::3 > > +]) > > +AT_CHECK([expr_to_flow 'ip6.src == {::1, $set2, ::4}'], [0], [dnl > > +ipv6,ipv6_src=::1 > > +ipv6,ipv6_src=::2 > > +ipv6,ipv6_src=::3 > > +ipv6,ipv6_src=::4 > > +]) > > +AT_CHECK([expr_to_flow 'eth.src == {00:00:00:00:00:01, > 00:00:00:00:00:02, 00:00:00:00:00:03}'], [0], [dnl > > +dl_src=00:00:00:00:00:01 > > +dl_src=00:00:00:00:00:02 > > +dl_src=00:00:00:00:00:03 > > +]) > > +AT_CHECK([expr_to_flow 'eth.src == {$set3}'], [0], [dnl > > +dl_src=00:00:00:00:00:01 > > +dl_src=00:00:00:00:00:02 > > +dl_src=00:00:00:00:00:03 > > +]) > > +AT_CLEANUP > > + > > AT_SETUP([ovn -- action parsing]) > > dnl Text before => is input, text after => is expected output. > > AT_DATA([test-cases.txt], [[ > > diff --git a/tests/test-ovn.c b/tests/test-ovn.c > > index 18e5aca..15ebba7 100644 > > --- a/tests/test-ovn.c > > +++ b/tests/test-ovn.c > > @@ -268,6 +268,26 @@ create_dhcp_opts(struct hmap *dhcp_opts) > > dhcp_opt_add(dhcp_opts, "lease_time", 51, "uint32"); > > } > > > > +static void > > +create_macros(struct shash *macros) > > +{ > > + shash_init(macros); > > + > > + const char * const addrs1[] = { > > + "10.0.0.1", "10.0.0.2", "10.0.0.3", > > + }; > > + const char * const addrs2[] = { > > + "::1", "::2", "::3", > > + }; > > + const char * const addrs3[] = { > > + "00:00:00:00:00:01", "00:00:00:00:00:02", "00:00:00:00:00:03", > > + }; > > + > > + expr_macros_add(macros, "set1", addrs1, 3); > > + expr_macros_add(macros, "set2", addrs2, 3); > > + expr_macros_add(macros, "set3", addrs3, 3); > > +} > > + > > static bool > > lookup_port_cb(const void *ports_, const char *port_name, unsigned int > *portp) > > { > > @@ -284,10 +304,12 @@ static void > > test_parse_expr__(int steps) > > { > > struct shash symtab; > > + struct shash macros; > > struct simap ports; > > struct ds input; > > > > create_symtab(&symtab); > > + create_macros(¯os); > > > > simap_init(&ports); > > simap_put(&ports, "eth0", 5); > > @@ -299,7 +321,8 @@ test_parse_expr__(int steps) > > struct expr *expr; > > char *error; > > > > - expr = expr_parse_string(ds_cstr(&input), &symtab, &error); > > + expr = expr_parse_string(ds_cstr(&input), &symtab, ¯os, > > + &error); > > if (!error && steps > 0) { > > expr = expr_annotate(expr, &symtab, &error); > > } > > @@ -336,6 +359,8 @@ test_parse_expr__(int steps) > > simap_destroy(&ports); > > expr_symtab_destroy(&symtab); > > shash_destroy(&symtab); > > + expr_macros_destroy(¯os); > > + shash_destroy(¯os); > > } > > > > static void > > @@ -480,7 +505,7 @@ test_evaluate_expr(struct ovs_cmdl_context *ctx) > > struct expr *expr; > > char *error; > > > > - expr = expr_parse_string(ds_cstr(&input), &symtab, &error); > > + expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, > &error); > > if (!error) { > > expr = expr_annotate(expr, &symtab, &error); > > } > > @@ -954,7 +979,7 @@ test_tree_shape_exhaustively(struct expr *expr, > struct shash *symtab, > > expr_format(expr, &s); > > > > char *error; > > - modified = expr_parse_string(ds_cstr(&s), symtab, &error); > > + modified = expr_parse_string(ds_cstr(&s), symtab, NULL, > &error); > > if (error) { > > fprintf(stderr, "%s fails to parse (%s)\n", > > ds_cstr(&s), error); > > -- > > 2.5.5 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <javascript:;> > > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ > dev mailing list > dev@openvswitch.org <javascript:;> > http://openvswitch.org/mailman/listinfo/dev >
"dev" <dev-bounces@openvswitch.org> wrote on 06/30/2016 06:36:17 PM: > From: Russell Bryant <russell@ovn.org> > To: Kyle Mestery <mestery@mestery.com> > Cc: "dev@openvswitch.org" <dev@openvswitch.org> > Date: 06/30/2016 06:36 PM > Subject: Re: [ovs-dev] [PATCH v3 1/2] Add address set support. > Sent by: "dev" <dev-bounces@openvswitch.org> > > On Thursday, June 30, 2016, Kyle Mestery <mestery@mestery.com> wrote: > > > On Tue, Jun 28, 2016 at 3:50 AM, <bschanmu@redhat.com <javascript:;>> > > wrote: > > > From: Russel Bryant <russell@ovn.org <javascript:;>> > > > > > > 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 <russell@ovn.org <javascript:;>> > > > Co-authored-by: Babu Shanmugam <bschanmu@redhat.com <javascript:;>> > > > Signed-off-by: Babu Shanmugam <bschanmu@redhat.com <javascript:;>> > > > > Tested-by: Kyle Mestery <mestery@mestery.com <javascript:;>> > > > > I've run these through the ovn-scale-test CI setup [1]. We have a test > > which creates ports and puts ACLs on them. This is a pure OVN control > > plane test with Rally driving creating and managing the resources > > under test. The test uses the following parameters: > > > > * 5 networks > > * 200 ports per network > > * 5 ACLs per port > > > > Before the address sets patches, these tests took an average of 820 > > seconds to run. With the address sets patches, they take an average of > > 460 seconds. That's quite the improvement! Thanks for the work here > > Russell and Babu! > > > > [1] https://github.com/openvswitch/ovn-scale-test/tree/master/ci > > > Wow, that's a nice speed up. What do the ACLs look like that get created? > What did they look like after changing them to use address sets? > > Russell > > That's on the docket next, Russell - to look at what is in the ACL tables, to verify that the OF flows still come out the same, and to throw some end-to-end OpenStack Rally testing at it... Ryan
"dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:30:29 PM: > From: Ben Pfaff <blp@ovn.org> > To: bschanmu@redhat.com > Cc: dev@openvswitch.org > Date: 07/02/2016 10:30 PM > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > Sent by: "dev" <dev-bounces@openvswitch.org> > > On Tue, Jun 28, 2016 at 02:20:40PM +0530, bschanmu@redhat.com wrote: > > From: Russel Bryant <russell@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 <russell@ovn.org> > > Co-authored-by: Babu Shanmugam <bschanmu@redhat.com> > > Signed-off-by: Babu Shanmugam <bschanmu@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. Because of our interest in this set, I've already taken a pass at merging part 2 with the incremental patch series and can share the needed delta (if it would be of interested here...) Ryan
Ryan Moats/Omaha/IBM wrote on 07/02/2016 10:37:24 PM: > From: Ryan Moats/Omaha/IBM > To: Ben Pfaff <blp@ovn.org> > Cc: bschanmu@redhat.com, dev@openvswitch.org > Date: 07/02/2016 10:37 PM > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > "dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:30:29 PM: > > > From: Ben Pfaff <blp@ovn.org> > > To: bschanmu@redhat.com > > Cc: dev@openvswitch.org > > Date: 07/02/2016 10:30 PM > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > On Tue, Jun 28, 2016 at 02:20:40PM +0530, bschanmu@redhat.com wrote: > > > From: Russel Bryant <russell@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 <russell@ovn.org> > > > Co-authored-by: Babu Shanmugam <bschanmu@redhat.com> > > > Signed-off-by: Babu Shanmugam <bschanmu@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. > > Because of our interest in this set, I've already taken a pass > at merging part 2 with the incremental patch series and can > share the needed delta (if it would be of interested here...) Oh well, I conveniently forgot to mention in the above that you've already nailed part 3 of my latest series, so I'll be spinning a v21... Ryan
"dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:41:18 PM: > From: Ryan Moats/Omaha/IBM@IBMUS > To: Ben Pfaff <blp@ovn.org> > Cc: dev@openvswitch.org > Date: 07/02/2016 10:41 PM > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > Sent by: "dev" <dev-bounces@openvswitch.org> > > Ryan Moats/Omaha/IBM wrote on 07/02/2016 10:37:24 PM: > > > From: Ryan Moats/Omaha/IBM > > To: Ben Pfaff <blp@ovn.org> > > Cc: bschanmu@redhat.com, dev@openvswitch.org > > Date: 07/02/2016 10:37 PM > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > "dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:30:29 PM: > > > > > From: Ben Pfaff <blp@ovn.org> > > > To: bschanmu@redhat.com > > > Cc: dev@openvswitch.org > > > Date: 07/02/2016 10:30 PM > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > On Tue, Jun 28, 2016 at 02:20:40PM +0530, bschanmu@redhat.com wrote: > > > > From: Russel Bryant <russell@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 <russell@ovn.org> > > > > Co-authored-by: Babu Shanmugam <bschanmu@redhat.com> > > > > Signed-off-by: Babu Shanmugam <bschanmu@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. > > > > Because of our interest in this set, I've already taken a pass > > at merging part 2 with the incremental patch series and can > > share the needed delta (if it would be of interested here...) > > Oh well, I conveniently forgot to mention in the above that you've already > nailed part 3 of my latest series, so I'll be spinning a v21... > > Ryan Ben, the rebase delta for part 3 is a single line (L338 getting NULL added as the third arguement) - I can spin you another patch series for a single line change, but it likely won't land until tomorrow morning... Ryan
On Sat, Jul 02, 2016 at 10:41:18PM -0500, Ryan Moats wrote: > Ryan Moats/Omaha/IBM wrote on 07/02/2016 10:37:24 PM: > > > From: Ryan Moats/Omaha/IBM > > To: Ben Pfaff <blp@ovn.org> > > Cc: bschanmu@redhat.com, dev@openvswitch.org > > Date: 07/02/2016 10:37 PM > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > "dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:30:29 PM: > > > > > From: Ben Pfaff <blp@ovn.org> > > > To: bschanmu@redhat.com > > > Cc: dev@openvswitch.org > > > Date: 07/02/2016 10:30 PM > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > On Tue, Jun 28, 2016 at 02:20:40PM +0530, bschanmu@redhat.com wrote: > > > > From: Russel Bryant <russell@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 <russell@ovn.org> > > > > Co-authored-by: Babu Shanmugam <bschanmu@redhat.com> > > > > Signed-off-by: Babu Shanmugam <bschanmu@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. > > > > Because of our interest in this set, I've already taken a pass > > at merging part 2 with the incremental patch series and can > > share the needed delta (if it would be of interested here...) > > Oh well, I conveniently forgot to mention in the above that you've already > nailed part 3 of my latest series, so I'll be spinning a v21... Can you just add patch 2 here as the first patch in v21?
Ben Pfaff <blp@ovn.org> wrote on 07/02/2016 11:12:00 PM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: bschanmu@redhat.com, dev@openvswitch.org > Date: 07/02/2016 11:14 PM > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > On Sat, Jul 02, 2016 at 10:41:18PM -0500, Ryan Moats wrote: > > Ryan Moats/Omaha/IBM wrote on 07/02/2016 10:37:24 PM: > > > > > From: Ryan Moats/Omaha/IBM > > > To: Ben Pfaff <blp@ovn.org> > > > Cc: bschanmu@redhat.com, dev@openvswitch.org > > > Date: 07/02/2016 10:37 PM > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > > > "dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:30:29 PM: > > > > > > > From: Ben Pfaff <blp@ovn.org> > > > > To: bschanmu@redhat.com > > > > Cc: dev@openvswitch.org > > > > Date: 07/02/2016 10:30 PM > > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > > > On Tue, Jun 28, 2016 at 02:20:40PM +0530, bschanmu@redhat.com wrote: > > > > > From: Russel Bryant <russell@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 <russell@ovn.org> > > > > > Co-authored-by: Babu Shanmugam <bschanmu@redhat.com> > > > > > Signed-off-by: Babu Shanmugam <bschanmu@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. > > > > > > Because of our interest in this set, I've already taken a pass > > > at merging part 2 with the incremental patch series and can > > > share the needed delta (if it would be of interested here...) > > > > Oh well, I conveniently forgot to mention in the above that you've already > > nailed part 3 of my latest series, so I'll be spinning a v21... > > Can you just add patch 2 here as the first patch in v21? > You mean with the requested changes from your review? I could probably do that... Ryan
On Sat, Jul 02, 2016 at 11:19:03PM -0500, Ryan Moats wrote: > Ben Pfaff <blp@ovn.org> wrote on 07/02/2016 11:12:00 PM: > > > From: Ben Pfaff <blp@ovn.org> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: bschanmu@redhat.com, dev@openvswitch.org > > Date: 07/02/2016 11:14 PM > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > On Sat, Jul 02, 2016 at 10:41:18PM -0500, Ryan Moats wrote: > > > Ryan Moats/Omaha/IBM wrote on 07/02/2016 10:37:24 PM: > > > > > > > From: Ryan Moats/Omaha/IBM > > > > To: Ben Pfaff <blp@ovn.org> > > > > Cc: bschanmu@redhat.com, dev@openvswitch.org > > > > Date: 07/02/2016 10:37 PM > > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > > > > > "dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:30:29 PM: > > > > > > > > > From: Ben Pfaff <blp@ovn.org> > > > > > To: bschanmu@redhat.com > > > > > Cc: dev@openvswitch.org > > > > > Date: 07/02/2016 10:30 PM > > > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > > > > > On Tue, Jun 28, 2016 at 02:20:40PM +0530, bschanmu@redhat.com > wrote: > > > > > > From: Russel Bryant <russell@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 <russell@ovn.org> > > > > > > Co-authored-by: Babu Shanmugam <bschanmu@redhat.com> > > > > > > Signed-off-by: Babu Shanmugam <bschanmu@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. > > > > > > > > Because of our interest in this set, I've already taken a pass > > > > at merging part 2 with the incremental patch series and can > > > > share the needed delta (if it would be of interested here...) > > > > > > Oh well, I conveniently forgot to mention in the above that you've > already > > > nailed part 3 of my latest series, so I'll be spinning a v21... > > > > Can you just add patch 2 here as the first patch in v21? > > > > You mean with the requested changes from your review? I could probably do > that... Yes, if possible.
Ben Pfaff <blp@ovn.org> wrote on 07/02/2016 11:31:28 PM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: bschanmu@redhat.com, dev@openvswitch.org > Date: 07/02/2016 11:31 PM > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > On Sat, Jul 02, 2016 at 11:19:03PM -0500, Ryan Moats wrote: > > Ben Pfaff <blp@ovn.org> wrote on 07/02/2016 11:12:00 PM: > > > > > From: Ben Pfaff <blp@ovn.org> > > > To: Ryan Moats/Omaha/IBM@IBMUS > > > Cc: bschanmu@redhat.com, dev@openvswitch.org > > > Date: 07/02/2016 11:14 PM > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > > > On Sat, Jul 02, 2016 at 10:41:18PM -0500, Ryan Moats wrote: > > > > Ryan Moats/Omaha/IBM wrote on 07/02/2016 10:37:24 PM: > > > > > > > > > From: Ryan Moats/Omaha/IBM > > > > > To: Ben Pfaff <blp@ovn.org> > > > > > Cc: bschanmu@redhat.com, dev@openvswitch.org > > > > > Date: 07/02/2016 10:37 PM > > > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > > > > > > > "dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:30:29 PM: > > > > > > > > > > > From: Ben Pfaff <blp@ovn.org> > > > > > > To: bschanmu@redhat.com > > > > > > Cc: dev@openvswitch.org > > > > > > Date: 07/02/2016 10:30 PM > > > > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > > > > > > > On Tue, Jun 28, 2016 at 02:20:40PM +0530, bschanmu@redhat.com > > wrote: > > > > > > > From: Russel Bryant <russell@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 <russell@ovn.org> > > > > > > > Co-authored-by: Babu Shanmugam <bschanmu@redhat.com> > > > > > > > Signed-off-by: Babu Shanmugam <bschanmu@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. > > > > > > > > > > Because of our interest in this set, I've already taken a pass > > > > > at merging part 2 with the incremental patch series and can > > > > > share the needed delta (if it would be of interested here...) > > > > > > > > Oh well, I conveniently forgot to mention in the above that you've > > already > > > > nailed part 3 of my latest series, so I'll be spinning a v21... > > > > > > Can you just add patch 2 here as the first patch in v21? > > > > > > > You mean with the requested changes from your review? I could probably do > > that... > > Yes, if possible. > I'll give it a shot - should have the rebase landed tomorrow am (CDT) Ryan
On Sat, Jul 02, 2016 at 10:51:39PM -0500, Ryan Moats wrote: > > > "dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:41:18 PM: > > > From: Ryan Moats/Omaha/IBM@IBMUS > > To: Ben Pfaff <blp@ovn.org> > > Cc: dev@openvswitch.org > > Date: 07/02/2016 10:41 PM > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > Ryan Moats/Omaha/IBM wrote on 07/02/2016 10:37:24 PM: > > > > > From: Ryan Moats/Omaha/IBM > > > To: Ben Pfaff <blp@ovn.org> > > > Cc: bschanmu@redhat.com, dev@openvswitch.org > > > Date: 07/02/2016 10:37 PM > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > > > "dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:30:29 PM: > > > > > > > From: Ben Pfaff <blp@ovn.org> > > > > To: bschanmu@redhat.com > > > > Cc: dev@openvswitch.org > > > > Date: 07/02/2016 10:30 PM > > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > > > On Tue, Jun 28, 2016 at 02:20:40PM +0530, bschanmu@redhat.com wrote: > > > > > From: Russel Bryant <russell@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 <russell@ovn.org> > > > > > Co-authored-by: Babu Shanmugam <bschanmu@redhat.com> > > > > > Signed-off-by: Babu Shanmugam <bschanmu@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. > > > > > > Because of our interest in this set, I've already taken a pass > > > at merging part 2 with the incremental patch series and can > > > share the needed delta (if it would be of interested here...) > > > > Oh well, I conveniently forgot to mention in the above that you've > already > > nailed part 3 of my latest series, so I'll be spinning a v21... > > > > Ryan > > Ben, the rebase delta for part 3 is a single line (L338 getting NULL added > as the third arguement) - I can spin you another patch series for a single > line change, but it likely won't land until tomorrow morning... Trivial rebases are a waste of time. Given that it's so simple, I'll work from v20.
Ben Pfaff <blp@ovn.org> wrote on 07/03/2016 09:45:27 AM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 07/03/2016 09:45 AM > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > On Sat, Jul 02, 2016 at 10:51:39PM -0500, Ryan Moats wrote: > > > > > > "dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:41:18 PM: > > > > > From: Ryan Moats/Omaha/IBM@IBMUS > > > To: Ben Pfaff <blp@ovn.org> > > > Cc: dev@openvswitch.org > > > Date: 07/02/2016 10:41 PM > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > Ryan Moats/Omaha/IBM wrote on 07/02/2016 10:37:24 PM: > > > > > > > From: Ryan Moats/Omaha/IBM > > > > To: Ben Pfaff <blp@ovn.org> > > > > Cc: bschanmu@redhat.com, dev@openvswitch.org > > > > Date: 07/02/2016 10:37 PM > > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > > > > > "dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:30:29 PM: > > > > > > > > > From: Ben Pfaff <blp@ovn.org> > > > > > To: bschanmu@redhat.com > > > > > Cc: dev@openvswitch.org > > > > > Date: 07/02/2016 10:30 PM > > > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > > > > > On Tue, Jun 28, 2016 at 02:20:40PM +0530, bschanmu@redhat.com wrote: > > > > > > From: Russel Bryant <russell@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 <russell@ovn.org> > > > > > > Co-authored-by: Babu Shanmugam <bschanmu@redhat.com> > > > > > > Signed-off-by: Babu Shanmugam <bschanmu@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. > > > > > > > > Because of our interest in this set, I've already taken a pass > > > > at merging part 2 with the incremental patch series and can > > > > share the needed delta (if it would be of interested here...) > > > > > > Oh well, I conveniently forgot to mention in the above that you've > > already > > > nailed part 3 of my latest series, so I'll be spinning a v21... > > > > > > Ryan > > > > Ben, the rebase delta for part 3 is a single line (L338 getting NULL added > > as the third arguement) - I can spin you another patch series for a single > > line change, but it likely won't land until tomorrow morning... > > Trivial rebases are a waste of time. Given that it's so simple, I'll > work from v20. > Your call - I have v21 done (starting with patch 2 of Address Sets as patch 1 of the new series) but I'm happy to have you work from v20 since I'm not sure I correctly addressed (sorry for the pun) all of your review comments on that newly included patch... Ryan
On Sun, Jul 03, 2016 at 09:59:38AM -0500, Ryan Moats wrote: > Ben Pfaff <blp@ovn.org> wrote on 07/03/2016 09:45:27 AM: > > > From: Ben Pfaff <blp@ovn.org> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: dev@openvswitch.org > > Date: 07/03/2016 09:45 AM > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > On Sat, Jul 02, 2016 at 10:51:39PM -0500, Ryan Moats wrote: > > > > > > > > > "dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:41:18 PM: > > > > > > > From: Ryan Moats/Omaha/IBM@IBMUS > > > > To: Ben Pfaff <blp@ovn.org> > > > > Cc: dev@openvswitch.org > > > > Date: 07/02/2016 10:41 PM > > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > > > Ryan Moats/Omaha/IBM wrote on 07/02/2016 10:37:24 PM: > > > > > > > > > From: Ryan Moats/Omaha/IBM > > > > > To: Ben Pfaff <blp@ovn.org> > > > > > Cc: bschanmu@redhat.com, dev@openvswitch.org > > > > > Date: 07/02/2016 10:37 PM > > > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > > > > > > > "dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:30:29 > PM: > > > > > > > > > > > From: Ben Pfaff <blp@ovn.org> > > > > > > To: bschanmu@redhat.com > > > > > > Cc: dev@openvswitch.org > > > > > > Date: 07/02/2016 10:30 PM > > > > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > > > > > > > On Tue, Jun 28, 2016 at 02:20:40PM +0530, bschanmu@redhat.com > wrote: > > > > > > > From: Russel Bryant <russell@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 <russell@ovn.org> > > > > > > > Co-authored-by: Babu Shanmugam <bschanmu@redhat.com> > > > > > > > Signed-off-by: Babu Shanmugam <bschanmu@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. > > > > > > > > > > Because of our interest in this set, I've already taken a pass > > > > > at merging part 2 with the incremental patch series and can > > > > > share the needed delta (if it would be of interested here...) > > > > > > > > Oh well, I conveniently forgot to mention in the above that you've > > > already > > > > nailed part 3 of my latest series, so I'll be spinning a v21... > > > > > > > > Ryan > > > > > > Ben, the rebase delta for part 3 is a single line (L338 getting NULL > added > > > as the third arguement) - I can spin you another patch series for a > single > > > line change, but it likely won't land until tomorrow morning... > > > > Trivial rebases are a waste of time. Given that it's so simple, I'll > > work from v20. > > > > Your call - I have v21 done (starting with patch 2 of Address Sets as > patch 1 of the new series) but I'm happy to have you work from v20 > since I'm not sure I correctly addressed (sorry for the pun) all of > your review comments on that newly included patch... Go ahead and post it then, no point in duplicating work ;-)
Ben Pfaff <blp@ovn.org> wrote on 07/03/2016 10:29:08 AM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 07/03/2016 10:29 AM > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > On Sun, Jul 03, 2016 at 09:59:38AM -0500, Ryan Moats wrote: > > Ben Pfaff <blp@ovn.org> wrote on 07/03/2016 09:45:27 AM: > > > > > From: Ben Pfaff <blp@ovn.org> > > > To: Ryan Moats/Omaha/IBM@IBMUS > > > Cc: dev@openvswitch.org > > > Date: 07/03/2016 09:45 AM > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > > > On Sat, Jul 02, 2016 at 10:51:39PM -0500, Ryan Moats wrote: > > > > > > > > > > > > "dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:41:18 PM: > > > > > > > > > From: Ryan Moats/Omaha/IBM@IBMUS > > > > > To: Ben Pfaff <blp@ovn.org> > > > > > Cc: dev@openvswitch.org > > > > > Date: 07/02/2016 10:41 PM > > > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > > > > > Ryan Moats/Omaha/IBM wrote on 07/02/2016 10:37:24 PM: > > > > > > > > > > > From: Ryan Moats/Omaha/IBM > > > > > > To: Ben Pfaff <blp@ovn.org> > > > > > > Cc: bschanmu@redhat.com, dev@openvswitch.org > > > > > > Date: 07/02/2016 10:37 PM > > > > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > > > > > > > > > "dev" <dev-bounces@openvswitch.org> wrote on 07/02/2016 10:30:29 > > PM: > > > > > > > > > > > > > From: Ben Pfaff <blp@ovn.org> > > > > > > > To: bschanmu@redhat.com > > > > > > > Cc: dev@openvswitch.org > > > > > > > Date: 07/02/2016 10:30 PM > > > > > > > Subject: Re: [ovs-dev] [PATCH v6 1/2] Add address set support. > > > > > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > > > > > > > > > On Tue, Jun 28, 2016 at 02:20:40PM +0530, bschanmu@redhat.com > > wrote: > > > > > > > > From: Russel Bryant <russell@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 <russell@ovn.org> > > > > > > > > Co-authored-by: Babu Shanmugam <bschanmu@redhat.com> > > > > > > > > Signed-off-by: Babu Shanmugam <bschanmu@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. > > > > > > > > > > > > Because of our interest in this set, I've already taken a pass > > > > > > at merging part 2 with the incremental patch series and can > > > > > > share the needed delta (if it would be of interested here...) > > > > > > > > > > Oh well, I conveniently forgot to mention in the above that you've > > > > already > > > > > nailed part 3 of my latest series, so I'll be spinning a v21... > > > > > > > > > > Ryan > > > > > > > > Ben, the rebase delta for part 3 is a single line (L338 getting NULL > > added > > > > as the third arguement) - I can spin you another patch series for a > > single > > > > line change, but it likely won't land until tomorrow morning... > > > > > > Trivial rebases are a waste of time. Given that it's so simple, I'll > > > work from v20. > > > > > > > Your call - I have v21 done (starting with patch 2 of Address Sets as > > patch 1 of the new series) but I'm happy to have you work from v20 > > since I'm not sure I correctly addressed (sorry for the pun) all of > > your review comments on that newly included patch... > > Go ahead and post it then, no point in duplicating work ;-) > Done and done, v21 is at patchworks - I'll mark v20 as "not applicable"
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c index 52e6131..433df70 100644 --- a/ovn/controller/lflow.c +++ b/ovn/controller/lflow.c @@ -306,7 +306,7 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports, struct hmap matches; struct expr *expr; - expr = expr_parse_string(lflow->match, &symtab, &error); + expr = expr_parse_string(lflow->match, &symtab, NULL, &error); if (!error) { if (prereqs) { expr = expr_combine(EXPR_T_AND, expr, prereqs); diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c index 569970e..ebd7159 100644 --- a/ovn/lib/actions.c +++ b/ovn/lib/actions.c @@ -208,7 +208,7 @@ add_prerequisite(struct action_context *ctx, const char *prerequisite) struct expr *expr; char *error; - expr = expr_parse_string(prerequisite, ctx->ap->symtab, &error); + expr = expr_parse_string(prerequisite, ctx->ap->symtab, NULL, &error); ovs_assert(!error); ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, expr); } diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c index 91bff07..2883ca9 100644 --- a/ovn/lib/expr.c +++ b/ovn/lib/expr.c @@ -409,6 +409,7 @@ expr_print(const struct expr *e) struct expr_context { struct lexer *lexer; /* Lexer for pulling more tokens. */ const struct shash *symtab; /* Symbol table. */ + const struct shash *macros; /* Table of macros. */ char *error; /* Error, if any, otherwise NULL. */ bool not; /* True inside odd number of NOT operators. */ }; @@ -729,6 +730,44 @@ 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) +{ + 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); + return false; + } + + if (!assign_constant_set_type(ctx, cs, EXPR_C_INTEGER)) { + return false; + } + + size_t n_values = cs->n_values + addr_set->n_values; + if (n_values >= *allocated_values) { + 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; + } + + return true; +} + +static bool parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, size_t *allocated_values) { @@ -759,6 +798,12 @@ parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, } lexer_get(ctx->lexer); return true; + } else if (ctx->lexer->token.type == LEX_T_MACRO) { + if (!parse_macros(ctx, cs, allocated_values)) { + return false; + } + lexer_get(ctx->lexer); + return true; } else { expr_syntax_error(ctx, "expecting constant."); return false; @@ -808,6 +853,72 @@ expr_constant_set_destroy(struct expr_constant_set *cs) } } +void +expr_macros_add(struct shash *macros, const char *name, + 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++) { + /* Use the lexer to convert each macro into the proper + * integer format. */ + struct lexer lex; + lexer_init(&lex, values[i]); + lexer_get(&lex); + if (lex.token.type != LEX_T_INTEGER + && 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]; + c->value = lex.token.value; + c->format = lex.token.format; + c->masked = lex.token.type == LEX_T_MASKED_INTEGER; + if (c->masked) { + c->mask = lex.token.mask; + } + } + lexer_destroy(&lex); + } + cset->n_values -= errors; + + shash_add(macros, name, cset); +} + +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); + } +} + +/* 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; + + shash_delete(macros, node); + expr_constant_set_destroy(cset); + } +} + static struct expr * expr_parse_primary(struct expr_context *ctx, bool *atomic) { @@ -980,9 +1091,11 @@ expr_parse__(struct expr_context *ctx) * The caller must eventually free the returned expression (with * expr_destroy()) or error (with free()). */ struct expr * -expr_parse(struct lexer *lexer, const struct shash *symtab, char **errorp) +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; ovs_assert((ctx.error != NULL) != (e != NULL)); @@ -991,14 +1104,15 @@ expr_parse(struct lexer *lexer, const struct shash *symtab, char **errorp) /* Like expr_parse(), but the expression is taken from 's'. */ struct expr * -expr_parse_string(const char *s, const struct shash *symtab, char **errorp) +expr_parse_string(const char *s, const struct shash *symtab, + const struct shash *macros, char **errorp) { struct lexer lexer; struct expr *expr; lexer_init(&lexer, s); lexer_get(&lexer); - expr = expr_parse(&lexer, symtab, errorp); + expr = expr_parse(&lexer, symtab, macros, errorp); if (!*errorp && lexer.token.type != LEX_T_END) { *errorp = xstrdup("Extra tokens at end of input."); expr_destroy(expr); @@ -1149,7 +1263,7 @@ expr_get_level(const struct expr *expr) static enum expr_level expr_parse_level(const char *s, const struct shash *symtab, char **errorp) { - struct expr *expr = expr_parse_string(s, symtab, errorp); + struct expr *expr = expr_parse_string(s, symtab, NULL, errorp); enum expr_level level = expr ? expr_get_level(expr) : EXPR_L_NOMINAL; expr_destroy(expr); return level; @@ -1292,7 +1406,7 @@ parse_and_annotate(const char *s, const struct shash *symtab, char *error; struct expr *expr; - expr = expr_parse_string(s, symtab, &error); + expr = expr_parse_string(s, symtab, NULL, &error); if (expr) { expr = expr_annotate__(expr, symtab, nesting, &error); } diff --git a/ovn/lib/expr.h b/ovn/lib/expr.h index d6f8489..56f6bbb 100644 --- a/ovn/lib/expr.h +++ b/ovn/lib/expr.h @@ -352,8 +352,10 @@ expr_from_node(const struct ovs_list *node) void expr_format(const struct expr *, struct ds *); void expr_print(const struct expr *); struct expr *expr_parse(struct lexer *, const struct shash *symtab, + const struct shash *macros, char **errorp); struct expr *expr_parse_string(const char *, const struct shash *symtab, + const struct shash *macros, char **errorp); struct expr *expr_clone(struct expr *); @@ -453,4 +455,19 @@ char *expr_parse_constant_set(struct lexer *, const struct shash *symtab, OVS_WARN_UNUSED_RESULT; void expr_constant_set_destroy(struct expr_constant_set *cs); + +/* MACRO Variables + * + * 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 + */ +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); +void expr_macros_destroy(struct shash *macros); + #endif /* ovn/expr.h */ diff --git a/ovn/lib/lex.c b/ovn/lib/lex.c index 556288f..bbbc7fc 100644 --- a/ovn/lib/lex.c +++ b/ovn/lib/lex.c @@ -227,6 +227,10 @@ lex_token_format(const struct lex_token *token, struct ds *s) lex_token_format_masked_integer(token, s); break; + case LEX_T_MACRO: + ds_put_cstr(s, token->s); + break; + case LEX_T_LPAREN: ds_put_cstr(s, "("); break; @@ -527,6 +531,14 @@ lex_parse_id(const char *p, struct lex_token *token) return p; } +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; +} + /* Initializes 'token' and parses the first token from the beginning of * null-terminated string 'p' into 'token'. Stores a pointer to the start of * the token (after skipping white space and comments, if any) into '*startp'. @@ -697,6 +709,10 @@ next: } break; + case '$': + p = lex_parse_macro(p, token); + break; + case '0': case '1': case '2': case '3': case '4': case '5': case '6': case '7': case '8': case '9': case ':': diff --git a/ovn/lib/lex.h b/ovn/lib/lex.h index 578ef40..826465c 100644 --- a/ovn/lib/lex.h +++ b/ovn/lib/lex.h @@ -36,6 +36,7 @@ enum lex_type { LEX_T_STRING, /* "foo" */ LEX_T_INTEGER, /* 12345 or 1.2.3.4 or ::1 or 01:02:03:04:05 */ LEX_T_MASKED_INTEGER, /* 12345/10 or 1.2.0.0/16 or ::2/127 or... */ + LEX_T_MACRO, /* $NAME */ LEX_T_ERROR, /* invalid input */ /* Bare tokens. */ diff --git a/tests/ovn.at b/tests/ovn.at index 297070c..840d0ef 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -434,6 +434,56 @@ AT_CHECK([expr_to_flow 'inport == "eth0" && inport == "eth1"'], [0], [dnl ]) AT_CLEANUP +AT_SETUP([ovn -- converting expressions to flows -- address sets]) +expr_to_flow () { + echo "$1" | ovstest test-ovn expr-to-flows | sort +} +AT_CHECK([expr_to_flow 'ip4.src == {10.0.0.1, 10.0.0.2, 10.0.0.3}'], [0], [dnl +ip,nw_src=10.0.0.1 +ip,nw_src=10.0.0.2 +ip,nw_src=10.0.0.3 +]) +AT_CHECK([expr_to_flow 'ip4.src == $set1'], [0], [dnl +ip,nw_src=10.0.0.1 +ip,nw_src=10.0.0.2 +ip,nw_src=10.0.0.3 +]) +AT_CHECK([expr_to_flow 'ip4.src == {1.2.3.4, $set1}'], [0], [dnl +ip,nw_src=1.2.3.4 +ip,nw_src=10.0.0.1 +ip,nw_src=10.0.0.2 +ip,nw_src=10.0.0.3 +]) +AT_CHECK([expr_to_flow 'ip4.src == {1.2.0.0/20, 5.5.5.0/24, $set1}'], [0], [dnl +ip,nw_src=1.2.0.0/20 +ip,nw_src=10.0.0.1 +ip,nw_src=10.0.0.2 +ip,nw_src=10.0.0.3 +ip,nw_src=5.5.5.0/24 +]) +AT_CHECK([expr_to_flow 'ip6.src == {::1, ::2, ::3}'], [0], [dnl +ipv6,ipv6_src=::1 +ipv6,ipv6_src=::2 +ipv6,ipv6_src=::3 +]) +AT_CHECK([expr_to_flow 'ip6.src == {::1, $set2, ::4}'], [0], [dnl +ipv6,ipv6_src=::1 +ipv6,ipv6_src=::2 +ipv6,ipv6_src=::3 +ipv6,ipv6_src=::4 +]) +AT_CHECK([expr_to_flow 'eth.src == {00:00:00:00:00:01, 00:00:00:00:00:02, 00:00:00:00:00:03}'], [0], [dnl +dl_src=00:00:00:00:00:01 +dl_src=00:00:00:00:00:02 +dl_src=00:00:00:00:00:03 +]) +AT_CHECK([expr_to_flow 'eth.src == {$set3}'], [0], [dnl +dl_src=00:00:00:00:00:01 +dl_src=00:00:00:00:00:02 +dl_src=00:00:00:00:00:03 +]) +AT_CLEANUP + AT_SETUP([ovn -- action parsing]) dnl Text before => is input, text after => is expected output. AT_DATA([test-cases.txt], [[ diff --git a/tests/test-ovn.c b/tests/test-ovn.c index 18e5aca..15ebba7 100644 --- a/tests/test-ovn.c +++ b/tests/test-ovn.c @@ -268,6 +268,26 @@ create_dhcp_opts(struct hmap *dhcp_opts) dhcp_opt_add(dhcp_opts, "lease_time", 51, "uint32"); } +static void +create_macros(struct shash *macros) +{ + shash_init(macros); + + const char * const addrs1[] = { + "10.0.0.1", "10.0.0.2", "10.0.0.3", + }; + const char * const addrs2[] = { + "::1", "::2", "::3", + }; + const char * const addrs3[] = { + "00:00:00:00:00:01", "00:00:00:00:00:02", "00:00:00:00:00:03", + }; + + expr_macros_add(macros, "set1", addrs1, 3); + expr_macros_add(macros, "set2", addrs2, 3); + expr_macros_add(macros, "set3", addrs3, 3); +} + static bool lookup_port_cb(const void *ports_, const char *port_name, unsigned int *portp) { @@ -284,10 +304,12 @@ static void test_parse_expr__(int steps) { struct shash symtab; + struct shash macros; struct simap ports; struct ds input; create_symtab(&symtab); + create_macros(¯os); simap_init(&ports); simap_put(&ports, "eth0", 5); @@ -299,7 +321,8 @@ test_parse_expr__(int steps) struct expr *expr; char *error; - expr = expr_parse_string(ds_cstr(&input), &symtab, &error); + expr = expr_parse_string(ds_cstr(&input), &symtab, ¯os, + &error); if (!error && steps > 0) { expr = expr_annotate(expr, &symtab, &error); } @@ -336,6 +359,8 @@ test_parse_expr__(int steps) simap_destroy(&ports); expr_symtab_destroy(&symtab); shash_destroy(&symtab); + expr_macros_destroy(¯os); + shash_destroy(¯os); } static void @@ -480,7 +505,7 @@ test_evaluate_expr(struct ovs_cmdl_context *ctx) struct expr *expr; char *error; - expr = expr_parse_string(ds_cstr(&input), &symtab, &error); + expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, &error); if (!error) { expr = expr_annotate(expr, &symtab, &error); } @@ -954,7 +979,7 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab, expr_format(expr, &s); char *error; - modified = expr_parse_string(ds_cstr(&s), symtab, &error); + modified = expr_parse_string(ds_cstr(&s), symtab, NULL, &error); if (error) { fprintf(stderr, "%s fails to parse (%s)\n", ds_cstr(&s), error);