On Thu 14 Feb 2019 at 20:49, Stefano Brivio <sbri...@redhat.com> wrote:
> On Thu, 14 Feb 2019 09:47:04 +0200
> Vlad Buslov <vla...@mellanox.com> wrote:
>
>> +static int __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
>> + bool *last, struct netlink_ext_ack *extack)
>
> This would be easier to follow (at least for me):
>
>> {
>> struct cls_fl_head *head = fl_head_dereference(tp);
>> bool async = tcf_exts_get_net(&f->exts);
>> - bool last;
>> -
>> - idr_remove(&head->handle_idr, f->handle);
>> - list_del_rcu(&f->list);
>> - last = fl_mask_put(head, f->mask, async);
>> - if (!tc_skip_hw(f->flags))
>> - fl_hw_destroy_filter(tp, f, extack);
>> - tcf_unbind_filter(tp, &f->res);
>> - __fl_put(f);
>> + int err = 0;
>
> without this
>
>> +
>> + (*last) = false;
>
> with *last = false;
>
>> +
>> + if (!f->deleted) {
>
> with:
> if (f->deleted)
> return -ENOENT;
>
>> + f->deleted = true;
>> + rhashtable_remove_fast(&f->mask->ht, &f->ht_node,
>> + f->mask->filter_ht_params);
>> + idr_remove(&head->handle_idr, f->handle);
>> + list_del_rcu(&f->list);
>> + (*last) = fl_mask_put(head, f->mask, async);
>
> with:
> *last = fl_mask_put(head, f->mask, async);
>
>> + if (!tc_skip_hw(f->flags))
>> + fl_hw_destroy_filter(tp, f, extack);
>> + tcf_unbind_filter(tp, &f->res);
>> + __fl_put(f);
>
> and a return 0; here
Agree, this function looks better when structured in the way you
suggest. Will change it in V2.
>
>> + } else {
>> + err = -ENOENT;
>> + }
>>
>> - return last;
>> + return err;
>> }
>>
>> [...]
>>
>> @@ -1520,14 +1541,14 @@ static int fl_delete(struct tcf_proto *tp, void
>> *arg, bool *last,
>> {
>> struct cls_fl_head *head = fl_head_dereference(tp);
>> struct cls_fl_filter *f = arg;
>> + bool last_on_mask;
>
> This is unused in this series, maybe change __fl_delete() to optionally
> take NULL as 'bool *last' argument?
It was implemented like that originally, but on internal review with
Jiri we decided that having unused variable here is better than
complicating __fl_delete() with support for "last" being NULL without
good reason.
>
>> + int err = 0;
>
> Nit: no need to initialise this.
Yes, but I always regret having uninitialized variables in my functions
later on :(
>
>> - rhashtable_remove_fast(&f->mask->ht, &f->ht_node,
>> - f->mask->filter_ht_params);
>> - __fl_delete(tp, f, extack);
>> + err = __fl_delete(tp, f, &last_on_mask, extack);
>> *last = list_empty(&head->masks);
>> __fl_put(f);
>>
>> - return 0;
>> + return err;
>> }
>>
>> static void fl_walk(struct tcf_proto *tp, struct tcf_walker *arg,
