On Thu 14 Feb 2019 at 20:49, Stefano Brivio [off-list ref] wrote:

On Thu, 14 Feb 2019 09:47:04 +0200
Vlad Buslov [off-list ref] wrote:

quoted

+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):

quoted

 {
 	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

quoted

+
+	(*last) = false;

with *last = false;

quoted

+
+	if (!f->deleted) {

with:
	if (f->deleted)
		return -ENOENT;

quoted

+		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);

quoted

+		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.

quoted

+	} 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.

quoted

+	int err = 0;

Nit: no need to initialise this.

Yes, but I always regret having uninitialized variables in my functions
later on :(

quoted

-	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,