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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
