On Thu, Jan 12, 2023 at 11:09 AM Dumitru Ceara <[email protected]> wrote:
> On 1/12/23 10:58, Ales Musil wrote: > > On Thu, Jan 12, 2023 at 10:45 AM Dumitru Ceara <[email protected]> > wrote: > > > >> On 12/15/22 07:27, Ales Musil wrote: > >>> To speed up search of possible recursive symbols > >>> use sset. This also prevents the dangling pointer > >>> warning wtih GCC 12 [0]. > >>> > >>> [0] https://bugzilla.redhat.com/2145107 > >>> Signed-off-by: Ales Musil <[email protected]> > >>> --- > >> > >> Hi Ales, > >> > > > > Hi Dumitru, > > > > > >> > >>> v2: Rebase on top of current main. > >>> Address comments from Dumitru. > >>> --- > >>> lib/expr.c | 72 ++++++++++++++++++++++++------------------------------ > >>> 1 file changed, 32 insertions(+), 40 deletions(-) > >>> > >>> diff --git a/lib/expr.c b/lib/expr.c > >>> index 43f44771c..c22a7bd89 100644 > >>> --- a/lib/expr.c > >>> +++ b/lib/expr.c > >>> @@ -35,7 +35,7 @@ VLOG_DEFINE_THIS_MODULE(expr); > >>> > >>> static struct expr *parse_and_annotate(const char *s, > >>> const struct shash *symtab, > >>> - struct ovs_list *nesting, > >>> + struct sset *nesting, > >>> char **errorp); > >>> > >>> /* Returns the name of measurement level 'level'. */ > >>> @@ -1582,9 +1582,10 @@ expr_field_parse(struct lexer *lexer, const > >> struct shash *symtab, > >>> while (symbol) { > >>> if (symbol->prereqs) { > >>> char *error; > >>> - struct ovs_list nesting = > >> OVS_LIST_INITIALIZER(&nesting); > >>> + struct sset nesting = SSET_INITIALIZER(&nesting); > >>> struct expr *e = parse_and_annotate(symbol->prereqs, > >> symtab, > >>> &nesting, &error); > >>> + sset_destroy(&nesting); > >>> if (error) { > >>> lexer_error(lexer, "%s", error); > >>> free(error); > >>> @@ -1940,21 +1941,12 @@ expr_destroy(struct expr *expr) > >>> > >>> /* Annotation. */ > >>> > >>> -/* An element in a linked list of symbols. > >>> - * > >>> - * Used to detect when a symbol is being expanded recursively, to > allow > >>> - * flagging an error. */ > >>> -struct annotation_nesting { > >>> - struct ovs_list node; > >>> - const struct expr_symbol *symbol; > >>> -}; > >>> - > >>> static struct expr *expr_annotate_(struct expr *, const struct shash > >> *symtab, > >>> - struct ovs_list *nesting, char > >> **errorp); > >>> + struct sset *nesting, char > **errorp); > >>> > >>> static struct expr * > >>> parse_and_annotate(const char *s, const struct shash *symtab, > >>> - struct ovs_list *nesting, char **errorp) > >>> + struct sset *nesting, char **errorp) > >>> { > >>> char *error; > >>> struct expr *expr; > >>> @@ -1976,28 +1968,23 @@ parse_and_annotate(const char *s, const struct > >> shash *symtab, > >>> > >>> static struct expr * > >>> expr_annotate_cmp(struct expr *expr, const struct shash *symtab, > >>> - bool append_prereqs, struct ovs_list *nesting, char > >> **errorp) > >>> + bool append_prereqs, struct sset *nesting, char > >> **errorp) > >>> { > >>> const struct expr_symbol *symbol = expr->cmp.symbol; > >>> - const struct annotation_nesting *iter; > >>> - LIST_FOR_EACH (iter, node, nesting) { > >>> - if (iter->symbol == symbol) { > >>> - *errorp = xasprintf("Recursive expansion of symbol `%s'.", > >>> - symbol->name); > >>> - expr_destroy(expr); > >>> - return NULL; > >>> - } > >>> - } > >>> + struct sset_node *nested_node; > >>> > >>> - struct annotation_nesting an; > >>> - an.symbol = symbol; > >>> - ovs_list_push_back(nesting, &an.node); > >>> + if (!(nested_node = sset_add(nesting, symbol->name))) { > > I forgot to mention this in the previous email. While this is correct, > I would move the assignment out of the condition. It saves us potential > typos later on IMO. > > >>> + *errorp = xasprintf("Recursive expansion of symbol `%s'.", > >>> + symbol->name); > >>> + expr_destroy(expr); > >>> + return NULL; > >>> + } > >>> > >>> struct expr *prereqs = NULL; > >>> if (append_prereqs && symbol->prereqs) { > >>> prereqs = parse_and_annotate(symbol->prereqs, symtab, nesting, > >> errorp); > >>> if (!prereqs) { > >>> - goto error; > >>> + goto out; > >>> } > >>> } > >>> > >>> @@ -2011,7 +1998,7 @@ expr_annotate_cmp(struct expr *expr, const struct > >> shash *symtab, > >>> predicate = parse_and_annotate(symbol->predicate, symtab, > >>> nesting, errorp); > >>> if (!predicate) { > >>> - goto error; > >>> + goto out; > >>> } > >>> > >>> bool positive = (expr->cmp.value.integer & htonll(1)) != 0; > >>> @@ -2025,20 +2012,22 @@ expr_annotate_cmp(struct expr *expr, const > >> struct shash *symtab, > >>> } > >>> > >>> *errorp = NULL; > >>> - ovs_list_remove(&an.node); > >>> - return prereqs ? expr_combine(EXPR_T_AND, expr, prereqs) : expr; > >>> > >>> -error: > >>> - expr_destroy(expr); > >>> - expr_destroy(prereqs); > >>> - ovs_list_remove(&an.node); > >>> - return NULL; > >>> +out: > >>> + sset_delete(nesting, nested_node); > >>> + if (*errorp) { > >> > >> I'm not sure I like the dependency we're creating between *errorp != > >> NULL and "any error has happened in the rest of the function". > >> > >> Why can't we keep the original "goto error" and do the cleanup there? > >> > > > > We certainly can, this seems more readable to me. There is not a > functional > > reason behind that. It's effectively the same however if you feel that it > > should > > use two labels I can change it back. > > > > I might be missing something but why would we need two labels? > > In the "success" case we only need to delete the nested_node from the > sset. And for the "error" case we can use the old "error" label. In > that case we can check if nested_node is not NULL and if so, remove it. > > We duplicate the nested_node removal but we don't create a dependency > between errorp and "any error happened". > > Thanks, > Dumitru > > > > >> > >>> + expr_destroy(expr); > >>> + expr_destroy(prereqs); > >>> + return NULL; > >>> + } > >>> + > >>> + return prereqs ? expr_combine(EXPR_T_AND, expr, prereqs) : expr; > >>> } > >>> > >>> /* Append (logical AND) prerequisites for given symbol to the > >> expression. */ > >>> static struct expr * > >>> expr_append_prereqs(struct expr *expr, const struct expr_symbol > *symbol, > >>> - const struct shash *symtab, struct ovs_list > >> *nesting, > >>> + const struct shash *symtab, struct sset *nesting, > >>> char **errorp) > >>> { > >>> struct expr *prereqs = NULL; > >>> @@ -2063,7 +2052,7 @@ static const struct expr_symbol > >> *expr_get_unique_symbol( > >>> * have arranged to deal with them). */ > >>> static struct expr * > >>> expr_annotate__(struct expr *expr, const struct shash *symtab, > >>> - bool append_prereqs, struct ovs_list *nesting, char > >> **errorp) > >>> + bool append_prereqs, struct sset *nesting, char > >> **errorp) > >>> { > >>> switch (expr->type) { > >>> case EXPR_T_CMP: > >>> @@ -2122,7 +2111,7 @@ expr_annotate__(struct expr *expr, const struct > >> shash *symtab, > >>> * Uses 'nesting' to ensure that a given symbol is not recursively > >> expanded. */ > >>> static struct expr * > >>> expr_annotate_(struct expr *expr, const struct shash *symtab, > >>> - struct ovs_list *nesting, char **errorp) > >>> + struct sset *nesting, char **errorp) > >>> { > >>> return expr_annotate__(expr, symtab, true, nesting, errorp); > >>> } > >>> @@ -2148,8 +2137,11 @@ expr_annotate_(struct expr *expr, const struct > >> shash *symtab, > >>> struct expr * > >>> expr_annotate(struct expr *expr, const struct shash *symtab, char > >> **errorp) > >>> { > >>> - struct ovs_list nesting = OVS_LIST_INITIALIZER(&nesting); > >>> - return expr_annotate_(expr, symtab, &nesting, errorp); > >>> + struct sset nesting = SSET_INITIALIZER(&nesting); > >>> + struct expr *result = expr_annotate_(expr, symtab, &nesting, > >> errorp); > >>> + sset_destroy(&nesting); > >>> + > >>> + return result; > >>> } > >>> > >>> static struct expr * > >> > >> Regards, > >> Dumitru > >> > >> > > Thanks, > > Ales > > > > Right, both addressed in v3. Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
