Hi Flavio,
I agree with your suggestions and will include them in the next version.

Thank you,
Babu

On Thursday 23 June 2016 10:46 PM, Flaviof wrote:


On Thu, Jun 23, 2016 at 1:05 AM, <bscha...@redhat.com <mailto:bscha...@redhat.com>> wrote:

    From: Russel Bryant <russ...@ovn.org <mailto:russ...@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 <russ...@ovn.org
    <mailto:russ...@ovn.org>>
    Co-authored-by: Babu Shanmugam <bscha...@redhat.com
    <mailto:bscha...@redhat.com>>
    Signed-off-by: Babu Shanmugam <bscha...@redhat.com
    <mailto:bscha...@redhat.com>>
    ---
     ovn/controller/lflow.c |   2 +-
     ovn/lib/actions.c      |   2 +-
     ovn/lib/expr.c         | 121
    +++++++++++++++++++++++++++++++++++++++++++++++--
     ovn/lib/expr.h         |  17 +++++++
     ovn/lib/lex.c          |  16 +++++++
     ovn/lib/lex.h          |   1 +
     tests/ovn.at <http://ovn.at>           | 50 ++++++++++++++++++++
     tests/test-ovn.c       |  31 +++++++++++--
     8 files changed, 230 insertions(+), 10 deletions(-)


Please see tests I suggest adding in this commit:

https://github.com/anbu-enovance/ovs/pull/2/commits/f463a43451b85b4896f1be78e1a2ee7066b3b465



    diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
    index efc427d..263cba6 100644
    --- a/ovn/controller/lflow.c
    +++ b/ovn/controller/lflow.c
    @@ -297,7 +297,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 4a486a0..0771b61 100644
    --- a/ovn/lib/actions.c
    +++ b/ovn/lib/actions.c
    @@ -180,7 +180,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 f274ab4..f87ece0 100644
    --- a/ovn/lib/expr.c
    +++ b/ovn/lib/expr.c
    @@ -451,6 +451,7 @@ struct expr_field {
     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. */
     };
    @@ -772,6 +773,41 @@ 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;
    +    }
    +



    +   cs->type = EXPR_C_INTEGER;


// call use assign_constant_set_type() instead of blindly
// setting expr_constant_set's type. This way, we can stop
// cases when type is unxecpected by the macro parsing:
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)
     {
    @@ -802,6 +838,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;
    @@ -851,6 +893,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)
     {
    @@ -1023,12 +1131,14 @@ 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;

         ctx.lexer = lexer;
         ctx.symtab = symtab;
    +    ctx.macros = macros;
         ctx.error = NULL;
         ctx.not = false;

    @@ -1040,14 +1150,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);
    @@ -1203,7 +1314,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;
    @@ -1346,7 +1457,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 1327789..c4f8e67 100644
    --- a/ovn/lib/expr.h
    +++ b/ovn/lib/expr.h
    @@ -343,8 +343,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 *);
    @@ -391,4 +393,19 @@ char *expr_parse_field(struct lexer *, int
    n_bits, bool rw,
                            const struct shash *symtab, struct
    mf_subfield *,
                            struct expr **prereqsp);

    +
    +/* 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
    <http://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 <http://ovn.at> b/tests/ovn.at
    <http://ovn.at>
    index a52def4..4f72107 100644
    --- a/tests/ovn.at <http://ovn.at>
    +++ b/tests/ovn.at <http://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
    <http://1.2.0.0/20>, 5.5.5.0/24 <http://5.5.5.0/24>, $set1}'],
    [0], [dnl
    +ip,nw_src=1.2.0.0/20 <http://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 <http://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 051f3a7..55281af 100644
    --- a/tests/test-ovn.c
    +++ b/tests/test-ovn.c
    @@ -239,6 +239,26 @@ create_symtab(struct shash *symtab)
         expr_symtab_add_string(symtab, "big_string", MFF_XREG0, NULL);
     }

    +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)
     {
    @@ -255,10 +275,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(&macros);

         simap_init(&ports);
         simap_put(&ports, "eth0", 5);
    @@ -270,7 +292,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, &macros,
    +                                 &error);
             if (!error && steps > 0) {
                 expr = expr_annotate(expr, &symtab, &error);
             }
    @@ -307,6 +330,8 @@ test_parse_expr__(int steps)
         simap_destroy(&ports);
         expr_symtab_destroy(&symtab);
         shash_destroy(&symtab);
    +    expr_macros_destroy(&macros);
    +    shash_destroy(&macros);
     }

     static void
    @@ -451,7 +476,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);
             }
    @@ -925,7 +950,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 <mailto:dev@openvswitch.org>
    http://openvswitch.org/mailman/listinfo/dev



_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to