On 30/09/2020 11:56, 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.
I think this is a good idea as it should make it easier to understand what is going on. Creating the contexts was a good first step but refactoring like this will help readability. Thanks for the patch. > > Also, change the error handling to avoid forcing the caller to supply > (and free) an error string pointer. > > CC: Han Zhou <[email protected]> > CC: Numan Siddique <[email protected]> > Signed-off-by: Dumitru Ceara <[email protected]> > --- > controller/lflow.c | 61 > ++++++++++++++++++++++++------------------------------ > 1 file changed, 27 insertions(+), 34 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index 4d71dfd..d9d1477 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -761,35 +761,41 @@ add_matches_to_flow_table(const struct > sbrec_logical_flow *lflow, Could you fix the comment on this aswell? There is a typo s/expre/expr > 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); The comment on this function (and expr_parse() is wrong as it specifies that it only used addr_sets to generate the expression. Can you update both please? > 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 +803,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); On failure do we need to undo the flow_resource_add()? > 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 +889,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 +952,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 { > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
