| Message ID | 1601551333-25368-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] lflow.c: Refactor function convert_match_to_expr. | expand |
Acked-by: Mark Michelson <mmichels@redhat.com> On 10/1/20 7:22 AM, Dumitru Ceara wrote: > To make it easier to understand what the function does we now explicitly > pass the input/output arguments instead of the complete > lflow_ctx_in/lflow_ctx_out context. > > Also: > - change the error handling to avoid forcing the caller to supply > (and free) an error string pointer. > - update function comments in expr.c to mention that port_groups sets > are also used for parsing. > > CC: Han Zhou <hzhou@ovn.org> > CC: Numan Siddique <numans@ovn.org> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > v2: Fix typos and comments as suggested by Mark Gray. > --- > controller/lflow.c | 68 +++++++++++++++++++++++++----------------------------- > lib/expr.c | 18 +++++++-------- > 2 files changed, 40 insertions(+), 46 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index 4d71dfd..f631679 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -755,41 +755,48 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, > ofpbuf_uninit(&ofpacts); > } > > -/* Converts the match and returns the simplified expre tree. > - * The caller should evaluate the conditions and normalize the > - * expr tree. */ > +/* Converts the match and returns the simplified expr tree. > + * > + * The caller should evaluate the conditions and normalize the expr tree. > + */ > static struct expr * > convert_match_to_expr(const struct sbrec_logical_flow *lflow, > struct expr *prereqs, > - struct lflow_ctx_in *l_ctx_in, > - struct lflow_ctx_out *l_ctx_out, > - bool *pg_addr_set_ref, char **errorp) > + const struct shash *addr_sets, > + const struct shash *port_groups, > + struct lflow_resource_ref *lfrr, > + bool *pg_addr_set_ref) > { > struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); > struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); > - char *error; > + char *error = NULL; > > - struct expr *e = expr_parse_string(lflow->match, &symtab, > - l_ctx_in->addr_sets, > - l_ctx_in->port_groups, &addr_sets_ref, > + struct expr *e = expr_parse_string(lflow->match, &symtab, addr_sets, > + port_groups, &addr_sets_ref, > &port_groups_ref, > lflow->logical_datapath->tunnel_key, > &error); > const char *addr_set_name; > SSET_FOR_EACH (addr_set_name, &addr_sets_ref) { > - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET, addr_set_name, > + lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name, > &lflow->header_.uuid); > } > const char *port_group_name; > SSET_FOR_EACH (port_group_name, &port_groups_ref) { > - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP, > - port_group_name, &lflow->header_.uuid); > + lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name, > + &lflow->header_.uuid); > } > > + if (pg_addr_set_ref) { > + *pg_addr_set_ref = (!sset_is_empty(&port_groups_ref) || > + !sset_is_empty(&addr_sets_ref)); > + } > + sset_destroy(&addr_sets_ref); > + sset_destroy(&port_groups_ref); > + > if (!error) { > if (prereqs) { > e = expr_combine(EXPR_T_AND, e, prereqs); > - prereqs = NULL; > } > e = expr_annotate(e, &symtab, &error); > } > @@ -797,24 +804,11 @@ convert_match_to_expr(const struct sbrec_logical_flow *lflow, > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s", > lflow->match, error); > - sset_destroy(&addr_sets_ref); > - sset_destroy(&port_groups_ref); > - *errorp = error; > + free(error); > return NULL; > } > > - e = expr_simplify(e); > - > - if (pg_addr_set_ref) { > - *pg_addr_set_ref = (!sset_is_empty(&port_groups_ref) || > - !sset_is_empty(&addr_sets_ref)); > - } > - > - sset_destroy(&addr_sets_ref); > - sset_destroy(&port_groups_ref); > - > - *errorp = NULL; > - return e; > + return expr_simplify(e); > } > > static bool > @@ -896,13 +890,13 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, > struct expr *expr = NULL; > if (!l_ctx_out->lflow_cache_map) { > /* Caching is disabled. */ > - expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, > - l_ctx_out, NULL, &error); > - if (error) { > + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in->addr_sets, > + l_ctx_in->port_groups, l_ctx_out->lfrr, > + NULL); > + if (!expr) { > expr_destroy(prereqs); > ovnacts_free(ovnacts.data, ovnacts.size); > ofpbuf_uninit(&ovnacts); > - free(error); > return true; > } > > @@ -959,13 +953,13 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, > > bool pg_addr_set_ref = false; > if (!expr) { > - expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, l_ctx_out, > - &pg_addr_set_ref, &error); > - if (error) { > + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in->addr_sets, > + l_ctx_in->port_groups, l_ctx_out->lfrr, > + &pg_addr_set_ref); > + if (!expr) { > expr_destroy(prereqs); > ovnacts_free(ovnacts.data, ovnacts.size); > ofpbuf_uninit(&ovnacts); > - free(error); > return true; > } > } else { > diff --git a/lib/expr.c b/lib/expr.c > index f6b22cf..acb1f3a 100644 > --- a/lib/expr.c > +++ b/lib/expr.c > @@ -1317,9 +1317,9 @@ expr_parse__(struct expr_context *ctx) > } > > /* Parses an expression from 'lexer' using the symbols in 'symtab' and > - * address set table in 'addr_sets'. If successful, returns the new > - * expression; on failure, returns NULL. Returns nonnull if and only if > - * lexer->error is NULL. */ > + * address set table in 'addr_sets' and 'port_groups'. If successful, returns > + * the new expression; on failure, returns NULL. Returns nonnull if and only > + * if lexer->error is NULL. */ > struct expr * > expr_parse(struct lexer *lexer, const struct shash *symtab, > const struct shash *addr_sets, > @@ -1339,9 +1339,9 @@ expr_parse(struct lexer *lexer, const struct shash *symtab, > } > > /* Parses the expression in 's' using the symbols in 'symtab' and > - * address set table in 'addr_sets'. If successful, returns the new > - * expression and sets '*errorp' to NULL. On failure, returns NULL and > - * sets '*errorp' to an explanatory error message. The caller must > + * address set table in 'addr_sets' and 'port_groups'. If successful, returns > + * the new expression and sets '*errorp' to NULL. On failure, returns NULL > + * and sets '*errorp' to an explanatory error message. The caller must > * eventually free the returned expression (with expr_destroy()) or > * error (with free()). */ > struct expr * > @@ -3481,9 +3481,9 @@ expr_parse_microflow__(struct lexer *lexer, > } > > /* Parses 's' as a microflow, using symbols from 'symtab', address set > - * table from 'addr_sets', and looking up port numbers using 'lookup_port' > - * and 'aux'. On success, stores the result in 'uflow' and returns > - * NULL, otherwise zeros 'uflow' and returns an error message that the > + * table from 'addr_sets' and 'port_groups', and looking up port numbers using > + * 'lookup_port' and 'aux'. On success, stores the result in 'uflow' and > + * returns NULL, otherwise zeros 'uflow' and returns an error message that the > * caller must free(). > * > * A "microflow" is a description of a single stream of packets, such as half a >
Thanks Dumitru and Mark. I applied this to master. Han On Thu, Oct 1, 2020 at 7:17 AM Mark Michelson <mmichels@redhat.com> wrote: > Acked-by: Mark Michelson <mmichels@redhat.com> > > On 10/1/20 7:22 AM, Dumitru Ceara wrote: > > To make it easier to understand what the function does we now explicitly > > pass the input/output arguments instead of the complete > > lflow_ctx_in/lflow_ctx_out context. > > > > Also: > > - change the error handling to avoid forcing the caller to supply > > (and free) an error string pointer. > > - update function comments in expr.c to mention that port_groups sets > > are also used for parsing. > > > > CC: Han Zhou <hzhou@ovn.org> > > CC: Numan Siddique <numans@ovn.org> > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > > v2: Fix typos and comments as suggested by Mark Gray. > > --- > > controller/lflow.c | 68 > +++++++++++++++++++++++++----------------------------- > > lib/expr.c | 18 +++++++-------- > > 2 files changed, 40 insertions(+), 46 deletions(-) > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > index 4d71dfd..f631679 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -755,41 +755,48 @@ add_matches_to_flow_table(const struct > sbrec_logical_flow *lflow, > > ofpbuf_uninit(&ofpacts); > > } > > > > -/* Converts the match and returns the simplified expre tree. > > - * The caller should evaluate the conditions and normalize the > > - * expr tree. */ > > +/* Converts the match and returns the simplified expr tree. > > + * > > + * The caller should evaluate the conditions and normalize the expr > tree. > > + */ > > static struct expr * > > convert_match_to_expr(const struct sbrec_logical_flow *lflow, > > struct expr *prereqs, > > - struct lflow_ctx_in *l_ctx_in, > > - struct lflow_ctx_out *l_ctx_out, > > - bool *pg_addr_set_ref, char **errorp) > > + const struct shash *addr_sets, > > + const struct shash *port_groups, > > + struct lflow_resource_ref *lfrr, > > + bool *pg_addr_set_ref) > > { > > struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); > > struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); > > - char *error; > > + char *error = NULL; > > > > - struct expr *e = expr_parse_string(lflow->match, &symtab, > > - l_ctx_in->addr_sets, > > - l_ctx_in->port_groups, > &addr_sets_ref, > > + struct expr *e = expr_parse_string(lflow->match, &symtab, addr_sets, > > + port_groups, &addr_sets_ref, > > &port_groups_ref, > > > lflow->logical_datapath->tunnel_key, > > &error); > > const char *addr_set_name; > > SSET_FOR_EACH (addr_set_name, &addr_sets_ref) { > > - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET, > addr_set_name, > > + lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name, > > &lflow->header_.uuid); > > } > > const char *port_group_name; > > SSET_FOR_EACH (port_group_name, &port_groups_ref) { > > - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP, > > - port_group_name, &lflow->header_.uuid); > > + lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name, > > + &lflow->header_.uuid); > > } > > > > + if (pg_addr_set_ref) { > > + *pg_addr_set_ref = (!sset_is_empty(&port_groups_ref) || > > + !sset_is_empty(&addr_sets_ref)); > > + } > > + sset_destroy(&addr_sets_ref); > > + sset_destroy(&port_groups_ref); > > + > > if (!error) { > > if (prereqs) { > > e = expr_combine(EXPR_T_AND, e, prereqs); > > - prereqs = NULL; > > } > > e = expr_annotate(e, &symtab, &error); > > } > > @@ -797,24 +804,11 @@ convert_match_to_expr(const struct > sbrec_logical_flow *lflow, > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > > VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s", > > lflow->match, error); > > - sset_destroy(&addr_sets_ref); > > - sset_destroy(&port_groups_ref); > > - *errorp = error; > > + free(error); > > return NULL; > > } > > > > - e = expr_simplify(e); > > - > > - if (pg_addr_set_ref) { > > - *pg_addr_set_ref = (!sset_is_empty(&port_groups_ref) || > > - !sset_is_empty(&addr_sets_ref)); > > - } > > - > > - sset_destroy(&addr_sets_ref); > > - sset_destroy(&port_groups_ref); > > - > > - *errorp = NULL; > > - return e; > > + return expr_simplify(e); > > } > > > > static bool > > @@ -896,13 +890,13 @@ consider_logical_flow(const struct > sbrec_logical_flow *lflow, > > struct expr *expr = NULL; > > if (!l_ctx_out->lflow_cache_map) { > > /* Caching is disabled. */ > > - expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, > > - l_ctx_out, NULL, &error); > > - if (error) { > > + expr = convert_match_to_expr(lflow, prereqs, > l_ctx_in->addr_sets, > > + l_ctx_in->port_groups, > l_ctx_out->lfrr, > > + NULL); > > + if (!expr) { > > expr_destroy(prereqs); > > ovnacts_free(ovnacts.data, ovnacts.size); > > ofpbuf_uninit(&ovnacts); > > - free(error); > > return true; > > } > > > > @@ -959,13 +953,13 @@ consider_logical_flow(const struct > sbrec_logical_flow *lflow, > > > > bool pg_addr_set_ref = false; > > if (!expr) { > > - expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, > l_ctx_out, > > - &pg_addr_set_ref, &error); > > - if (error) { > > + expr = convert_match_to_expr(lflow, prereqs, > l_ctx_in->addr_sets, > > + l_ctx_in->port_groups, > l_ctx_out->lfrr, > > + &pg_addr_set_ref); > > + if (!expr) { > > expr_destroy(prereqs); > > ovnacts_free(ovnacts.data, ovnacts.size); > > ofpbuf_uninit(&ovnacts); > > - free(error); > > return true; > > } > > } else { > > diff --git a/lib/expr.c b/lib/expr.c > > index f6b22cf..acb1f3a 100644 > > --- a/lib/expr.c > > +++ b/lib/expr.c > > @@ -1317,9 +1317,9 @@ expr_parse__(struct expr_context *ctx) > > } > > > > /* Parses an expression from 'lexer' using the symbols in 'symtab' and > > - * address set table in 'addr_sets'. If successful, returns the new > > - * expression; on failure, returns NULL. Returns nonnull if and only if > > - * lexer->error is NULL. */ > > + * address set table in 'addr_sets' and 'port_groups'. If successful, > returns > > + * the new expression; on failure, returns NULL. Returns nonnull if > and only > > + * if lexer->error is NULL. */ > > struct expr * > > expr_parse(struct lexer *lexer, const struct shash *symtab, > > const struct shash *addr_sets, > > @@ -1339,9 +1339,9 @@ expr_parse(struct lexer *lexer, const struct shash > *symtab, > > } > > > > /* Parses the expression in 's' using the symbols in 'symtab' and > > - * address set table in 'addr_sets'. If successful, returns the new > > - * expression and sets '*errorp' to NULL. On failure, returns NULL and > > - * sets '*errorp' to an explanatory error message. The caller must > > + * address set table in 'addr_sets' and 'port_groups'. If successful, > returns > > + * the new expression and sets '*errorp' to NULL. On failure, returns > NULL > > + * and sets '*errorp' to an explanatory error message. The caller must > > * eventually free the returned expression (with expr_destroy()) or > > * error (with free()). */ > > struct expr * > > @@ -3481,9 +3481,9 @@ expr_parse_microflow__(struct lexer *lexer, > > } > > > > /* Parses 's' as a microflow, using symbols from 'symtab', address set > > - * table from 'addr_sets', and looking up port numbers using > 'lookup_port' > > - * and 'aux'. On success, stores the result in 'uflow' and returns > > - * NULL, otherwise zeros 'uflow' and returns an error message that the > > + * table from 'addr_sets' and 'port_groups', and looking up port > numbers using > > + * 'lookup_port' and 'aux'. On success, stores the result in 'uflow' > and > > + * returns NULL, otherwise zeros 'uflow' and returns an error message > that the > > * caller must free(). > > * > > * A "microflow" is a description of a single stream of packets, such > as half a > > > >
On 01/10/2020 12:22, Dumitru Ceara wrote: > To make it easier to understand what the function does we now explicitly > pass the input/output arguments instead of the complete > lflow_ctx_in/lflow_ctx_out context. > > Also: > - change the error handling to avoid forcing the caller to supply > (and free) an error string pointer. > - update function comments in expr.c to mention that port_groups sets > are also used for parsing. > > CC: Han Zhou <hzhou@ovn.org> > CC: Numan Siddique <numans@ovn.org> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > v2: Fix typos and comments as suggested by Mark Gray. > --- > controller/lflow.c | 68 +++++++++++++++++++++++++----------------------------- > lib/expr.c | 18 +++++++-------- > 2 files changed, 40 insertions(+), 46 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index 4d71dfd..f631679 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -755,41 +755,48 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, > ofpbuf_uninit(&ofpacts); > } > > -/* Converts the match and returns the simplified expre tree. > - * The caller should evaluate the conditions and normalize the > - * expr tree. */ > +/* Converts the match and returns the simplified expr tree. > + * > + * The caller should evaluate the conditions and normalize the expr tree. > + */ > static struct expr * > convert_match_to_expr(const struct sbrec_logical_flow *lflow, > struct expr *prereqs, > - struct lflow_ctx_in *l_ctx_in, > - struct lflow_ctx_out *l_ctx_out, > - bool *pg_addr_set_ref, char **errorp) > + const struct shash *addr_sets, > + const struct shash *port_groups, > + struct lflow_resource_ref *lfrr, > + bool *pg_addr_set_ref) > { > struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); > struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); > - char *error; > + char *error = NULL; > > - struct expr *e = expr_parse_string(lflow->match, &symtab, > - l_ctx_in->addr_sets, > - l_ctx_in->port_groups, &addr_sets_ref, > + struct expr *e = expr_parse_string(lflow->match, &symtab, addr_sets, > + port_groups, &addr_sets_ref, > &port_groups_ref, > lflow->logical_datapath->tunnel_key, > &error); > const char *addr_set_name; > SSET_FOR_EACH (addr_set_name, &addr_sets_ref) { > - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET, addr_set_name, > + lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name, > &lflow->header_.uuid); > } > const char *port_group_name; > SSET_FOR_EACH (port_group_name, &port_groups_ref) { > - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP, > - port_group_name, &lflow->header_.uuid); > + lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name, > + &lflow->header_.uuid); > } > > + if (pg_addr_set_ref) { > + *pg_addr_set_ref = (!sset_is_empty(&port_groups_ref) || > + !sset_is_empty(&addr_sets_ref)); > + } > + sset_destroy(&addr_sets_ref); > + sset_destroy(&port_groups_ref); > + > if (!error) { > if (prereqs) { > e = expr_combine(EXPR_T_AND, e, prereqs); > - prereqs = NULL; > } > e = expr_annotate(e, &symtab, &error); > } > @@ -797,24 +804,11 @@ convert_match_to_expr(const struct sbrec_logical_flow *lflow, > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s", > lflow->match, error); > - sset_destroy(&addr_sets_ref); > - sset_destroy(&port_groups_ref); > - *errorp = error; > + free(error); > return NULL; > } > > - e = expr_simplify(e); > - > - if (pg_addr_set_ref) { > - *pg_addr_set_ref = (!sset_is_empty(&port_groups_ref) || > - !sset_is_empty(&addr_sets_ref)); > - } > - > - sset_destroy(&addr_sets_ref); > - sset_destroy(&port_groups_ref); > - > - *errorp = NULL; > - return e; > + return expr_simplify(e); > } > > static bool > @@ -896,13 +890,13 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, > struct expr *expr = NULL; > if (!l_ctx_out->lflow_cache_map) { > /* Caching is disabled. */ > - expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, > - l_ctx_out, NULL, &error); > - if (error) { > + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in->addr_sets, > + l_ctx_in->port_groups, l_ctx_out->lfrr, > + NULL); > + if (!expr) { > expr_destroy(prereqs); > ovnacts_free(ovnacts.data, ovnacts.size); > ofpbuf_uninit(&ovnacts); > - free(error); > return true; > } > > @@ -959,13 +953,13 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, > > bool pg_addr_set_ref = false; > if (!expr) { > - expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, l_ctx_out, > - &pg_addr_set_ref, &error); > - if (error) { > + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in->addr_sets, > + l_ctx_in->port_groups, l_ctx_out->lfrr, > + &pg_addr_set_ref); > + if (!expr) { > expr_destroy(prereqs); > ovnacts_free(ovnacts.data, ovnacts.size); > ofpbuf_uninit(&ovnacts); > - free(error); > return true; > } > } else { > diff --git a/lib/expr.c b/lib/expr.c > index f6b22cf..acb1f3a 100644 > --- a/lib/expr.c > +++ b/lib/expr.c > @@ -1317,9 +1317,9 @@ expr_parse__(struct expr_context *ctx) > } > > /* Parses an expression from 'lexer' using the symbols in 'symtab' and > - * address set table in 'addr_sets'. If successful, returns the new > - * expression; on failure, returns NULL. Returns nonnull if and only if > - * lexer->error is NULL. */ > + * address set table in 'addr_sets' and 'port_groups'. If successful, returns > + * the new expression; on failure, returns NULL. Returns nonnull if and only > + * if lexer->error is NULL. */ > struct expr * > expr_parse(struct lexer *lexer, const struct shash *symtab, > const struct shash *addr_sets, > @@ -1339,9 +1339,9 @@ expr_parse(struct lexer *lexer, const struct shash *symtab, > } > > /* Parses the expression in 's' using the symbols in 'symtab' and > - * address set table in 'addr_sets'. If successful, returns the new > - * expression and sets '*errorp' to NULL. On failure, returns NULL and > - * sets '*errorp' to an explanatory error message. The caller must > + * address set table in 'addr_sets' and 'port_groups'. If successful, returns > + * the new expression and sets '*errorp' to NULL. On failure, returns NULL > + * and sets '*errorp' to an explanatory error message. The caller must > * eventually free the returned expression (with expr_destroy()) or > * error (with free()). */ > struct expr * > @@ -3481,9 +3481,9 @@ expr_parse_microflow__(struct lexer *lexer, > } > > /* Parses 's' as a microflow, using symbols from 'symtab', address set > - * table from 'addr_sets', and looking up port numbers using 'lookup_port' > - * and 'aux'. On success, stores the result in 'uflow' and returns > - * NULL, otherwise zeros 'uflow' and returns an error message that the > + * table from 'addr_sets' and 'port_groups', and looking up port numbers using > + * 'lookup_port' and 'aux'. On success, stores the result in 'uflow' and > + * returns NULL, otherwise zeros 'uflow' and returns an error message that the > * caller must free(). > * > * A "microflow" is a description of a single stream of packets, such as half a > LGTM Acked-by: Mark Gray <mark.d.gray@redhat.com> Thanks, Mark
On 10/2/20 9:39 AM, Mark Gray wrote: > > LGTM > > Acked-by: Mark Gray <mark.d.gray@redhat.com> > > Thanks, > > Mark > Thanks Mark, Mark, and Han for reviews and applying this!
diff --git a/controller/lflow.c b/controller/lflow.c index 4d71dfd..f631679 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -755,41 +755,48 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, ofpbuf_uninit(&ofpacts); } -/* Converts the match and returns the simplified expre tree. - * The caller should evaluate the conditions and normalize the - * expr tree. */ +/* Converts the match and returns the simplified expr tree. + * + * The caller should evaluate the conditions and normalize the expr tree. + */ static struct expr * convert_match_to_expr(const struct sbrec_logical_flow *lflow, struct expr *prereqs, - struct lflow_ctx_in *l_ctx_in, - struct lflow_ctx_out *l_ctx_out, - bool *pg_addr_set_ref, char **errorp) + const struct shash *addr_sets, + const struct shash *port_groups, + struct lflow_resource_ref *lfrr, + bool *pg_addr_set_ref) { struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); - char *error; + char *error = NULL; - struct expr *e = expr_parse_string(lflow->match, &symtab, - l_ctx_in->addr_sets, - l_ctx_in->port_groups, &addr_sets_ref, + struct expr *e = expr_parse_string(lflow->match, &symtab, addr_sets, + port_groups, &addr_sets_ref, &port_groups_ref, lflow->logical_datapath->tunnel_key, &error); const char *addr_set_name; SSET_FOR_EACH (addr_set_name, &addr_sets_ref) { - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET, addr_set_name, + lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name, &lflow->header_.uuid); } const char *port_group_name; SSET_FOR_EACH (port_group_name, &port_groups_ref) { - lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP, - port_group_name, &lflow->header_.uuid); + lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name, + &lflow->header_.uuid); } + if (pg_addr_set_ref) { + *pg_addr_set_ref = (!sset_is_empty(&port_groups_ref) || + !sset_is_empty(&addr_sets_ref)); + } + sset_destroy(&addr_sets_ref); + sset_destroy(&port_groups_ref); + if (!error) { if (prereqs) { e = expr_combine(EXPR_T_AND, e, prereqs); - prereqs = NULL; } e = expr_annotate(e, &symtab, &error); } @@ -797,24 +804,11 @@ convert_match_to_expr(const struct sbrec_logical_flow *lflow, static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s", lflow->match, error); - sset_destroy(&addr_sets_ref); - sset_destroy(&port_groups_ref); - *errorp = error; + free(error); return NULL; } - e = expr_simplify(e); - - if (pg_addr_set_ref) { - *pg_addr_set_ref = (!sset_is_empty(&port_groups_ref) || - !sset_is_empty(&addr_sets_ref)); - } - - sset_destroy(&addr_sets_ref); - sset_destroy(&port_groups_ref); - - *errorp = NULL; - return e; + return expr_simplify(e); } static bool @@ -896,13 +890,13 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, struct expr *expr = NULL; if (!l_ctx_out->lflow_cache_map) { /* Caching is disabled. */ - expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, - l_ctx_out, NULL, &error); - if (error) { + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in->addr_sets, + l_ctx_in->port_groups, l_ctx_out->lfrr, + NULL); + if (!expr) { expr_destroy(prereqs); ovnacts_free(ovnacts.data, ovnacts.size); ofpbuf_uninit(&ovnacts); - free(error); return true; } @@ -959,13 +953,13 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, bool pg_addr_set_ref = false; if (!expr) { - expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, l_ctx_out, - &pg_addr_set_ref, &error); - if (error) { + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in->addr_sets, + l_ctx_in->port_groups, l_ctx_out->lfrr, + &pg_addr_set_ref); + if (!expr) { expr_destroy(prereqs); ovnacts_free(ovnacts.data, ovnacts.size); ofpbuf_uninit(&ovnacts); - free(error); return true; } } else { diff --git a/lib/expr.c b/lib/expr.c index f6b22cf..acb1f3a 100644 --- a/lib/expr.c +++ b/lib/expr.c @@ -1317,9 +1317,9 @@ expr_parse__(struct expr_context *ctx) } /* Parses an expression from 'lexer' using the symbols in 'symtab' and - * address set table in 'addr_sets'. If successful, returns the new - * expression; on failure, returns NULL. Returns nonnull if and only if - * lexer->error is NULL. */ + * address set table in 'addr_sets' and 'port_groups'. If successful, returns + * the new expression; on failure, returns NULL. Returns nonnull if and only + * if lexer->error is NULL. */ struct expr * expr_parse(struct lexer *lexer, const struct shash *symtab, const struct shash *addr_sets, @@ -1339,9 +1339,9 @@ expr_parse(struct lexer *lexer, const struct shash *symtab, } /* Parses the expression in 's' using the symbols in 'symtab' and - * address set table in 'addr_sets'. If successful, returns the new - * expression and sets '*errorp' to NULL. On failure, returns NULL and - * sets '*errorp' to an explanatory error message. The caller must + * address set table in 'addr_sets' and 'port_groups'. If successful, returns + * the new expression and sets '*errorp' to NULL. On failure, returns NULL + * and sets '*errorp' to an explanatory error message. The caller must * eventually free the returned expression (with expr_destroy()) or * error (with free()). */ struct expr * @@ -3481,9 +3481,9 @@ expr_parse_microflow__(struct lexer *lexer, } /* Parses 's' as a microflow, using symbols from 'symtab', address set - * table from 'addr_sets', and looking up port numbers using 'lookup_port' - * and 'aux'. On success, stores the result in 'uflow' and returns - * NULL, otherwise zeros 'uflow' and returns an error message that the + * table from 'addr_sets' and 'port_groups', and looking up port numbers using + * 'lookup_port' and 'aux'. On success, stores the result in 'uflow' and + * returns NULL, otherwise zeros 'uflow' and returns an error message that the * caller must free(). * * A "microflow" is a description of a single stream of packets, such as half a
To make it easier to understand what the function does we now explicitly pass the input/output arguments instead of the complete lflow_ctx_in/lflow_ctx_out context. Also: - change the error handling to avoid forcing the caller to supply (and free) an error string pointer. - update function comments in expr.c to mention that port_groups sets are also used for parsing. CC: Han Zhou <hzhou@ovn.org> CC: Numan Siddique <numans@ovn.org> Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- v2: Fix typos and comments as suggested by Mark Gray. --- controller/lflow.c | 68 +++++++++++++++++++++++++----------------------------- lib/expr.c | 18 +++++++-------- 2 files changed, 40 insertions(+), 46 deletions(-)