| Message ID | 1601463380-11769-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] lflow.c: Refactor function convert_match_to_expr. | expand |
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 <hzhou@ovn.org> > CC: Numan Siddique <numans@ovn.org> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > 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 { >
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 <hzhou@ovn.org> >> CC: Numan Siddique <numans@ovn.org> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> 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 { >> >
On 01/10/2020 11:37, Dumitru Ceara wrote: > On 10/1/20 12:19 PM, Mark Gray wrote: >> On 30/09/2020 11:56, Dumitru Ceara wrote: >> >> 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. Ok, sounds good. Also, I ran the UTs and don't see any issues/failures.
On 10/1/20 12:40 PM, Mark Gray wrote: > On 01/10/2020 11:37, Dumitru Ceara wrote: >> On 10/1/20 12:19 PM, Mark Gray wrote: >>> On 30/09/2020 11:56, Dumitru Ceara wrote: > >>> >>> 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. > > Ok, sounds good. Also, I ran the UTs and don't see any issues/failures. > v2 available at: http://patchwork.ozlabs.org/project/ovn/patch/1601551333-25368-1-git-send-email-dceara@redhat.com/ Thanks, Dumitru
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, 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 +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); 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 {
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. CC: Han Zhou <hzhou@ovn.org> CC: Numan Siddique <numans@ovn.org> Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- controller/lflow.c | 61 ++++++++++++++++++++++++------------------------------ 1 file changed, 27 insertions(+), 34 deletions(-)