On 10/1/20 12:19 PM, Mark Gray wrote: > 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. >
Thanks for the review! >> >> 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 Ack. >> 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? > Ack, and we should also update the comment for expr_parse_microflow(). >> 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()? > We didn't do before and I think it's fine to leave as is because failure to parse will trigger a recompute of the I-P engine and call en_flow_output_run() which should cleanup the resource refs. Unfortunately I don't see an easier way to do cleanup. I'll send a v2 soon fixing the points above. Thanks, Dumitru >> 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
