Messages in this thread | | | Date | Mon, 13 Feb 2017 05:43:26 +0000 | From | Al Viro <> | Subject | Re: [PATCH] llist: Don't reinvent the wheel but use existing llist API |
| |
On Mon, Feb 13, 2017 at 01:10:13PM +0900, Byungchul Park wrote: > Although llist provides proper APIs, they are not used. Make them used.
> @@ -231,12 +231,10 @@ static void __fput(struct file *file) > static void delayed_fput(struct work_struct *unused) > { > struct llist_node *node = llist_del_all(&delayed_fput_list); > - struct llist_node *next; > + struct file *f; > > - for (; node; node = next) { > - next = llist_next(node); > - __fput(llist_entry(node, struct file, f_u.fu_llist)); > - } > + llist_for_each_entry(f, node, f_u.fu_llist) > + __fput(f); > }
#define llist_for_each_entry(pos, node, member) \ for ((pos) = llist_entry((node), typeof(*(pos)), member); \ &(pos)->member != NULL; \ (pos) = llist_entry((pos)->member.next, typeof(*(pos)), member))
Now, think what happens after __fput() frees the damn thing. In the step of that loop, that is.
That kind of pattern (find next, do something with the current, proceed to the next we'd found before) is a strong hint that this "do something" might remove the thing from the list, or outright destroy it. Both file_table.c and namespace.c chunks are breaking exactly that kind of places.
Please, don't do this kind of conversions blindly. There _is_ another iterating primitive for such places, but figuring out which one is right is not something you can do without understanding what the code is doing. And no, blind replacement of all such loops with llist_for_each_entry_safe, just in case, is not a good idea either.
| |