Messages in this thread Patch in this message | | | Date | Thu, 7 Sep 2017 14:34:52 +0200 | From | Petr Mladek <> | Subject | Re: [PATCH v2 1/3] livepatch: Add dynamic klp_object and klp_func iterators |
| |
On Wed 2017-08-30 17:38:43, Jason Baron wrote: > In preparation to introducing atomic replace, introduce iterators for klp_func
> and klp_object, such that objects and functions can be dynmically allocated > (needed for atomic replace). This patch is intended to effectively be a no-op
./scripts/checkpatch.pl reports that these lines are too long. It prefers a maximum 75 chars per line in the commit message ;-)
> until atomic replace is introduced. > > --- a/include/linux/livepatch.h > +++ b/include/linux/livepatch.h > @@ -24,6 +24,7 @@ > #include <linux/module.h> > #include <linux/ftrace.h> > #include <linux/completion.h> > +#include <linux/list.h> > > #if IS_ENABLED(CONFIG_LIVEPATCH) > > @@ -44,6 +45,7 @@ > * @old_addr: the address of the function being patched > * @kobj: kobject for sysfs resources > * @stack_node: list node for klp_ops func_stack list > + * @func_entry: used to link struct klp_func to struct klp_object
Please, make it clear that only dynamically allocated structures are linked. Same for the other entries.
It might look superfluous when you read this patch. But it will help a lot when you read the code one year from now.
> * @old_size: size of the old function > * @new_size: size of the new function > * @patched: the func has been added to the klp_ops list
[...]
> @@ -126,17 +134,95 @@ struct klp_patch { > /* internal */ > struct list_head list; > struct kobject kobj; > + struct list_head obj_list; > bool enabled; > struct completion finish; > }; > > +static inline struct klp_object *obj_iter_next(struct klp_patch *patch, > + struct klp_object *obj)
The function is far from trivial. I wonder if it is still a good candidate for inlining.
Also it should get prefixed by klp_ because it is in a header that can be included anywhere.
Next I still think that it will be easier to understand when we make it more clear that only the dymanically allocated objects are in the list. I mean renaming:
obj_entry -> dyn_obj_entry obj_list -> dyn_obj_list
> +{ > + struct klp_object *next_obj = NULL; > +
The code quite tricky. IMHO, it would deserve a comment.
/* * Statically defined objects are in NULL-ended array. * Only dynamic ones are in the obj_list. */ > + if (list_empty(&obj->obj_entry)) { > + next_obj = obj + 1; > + if (next_obj->funcs || next_obj->name) > + goto out; > + else > + next_obj = NULL;
Please, add an empty line here to make it better readable.
> + if (!list_empty(&patch->obj_list)) > + next_obj = container_of(patch->obj_list.next, > + struct klp_object, > + obj_entry); > + goto out; > + }
Also here an empty line.
> + if (obj->obj_entry.next != &patch->obj_list) > + next_obj = container_of(obj->obj_entry.next, > + struct klp_object, > + obj_entry); > +out: > + return next_obj; > +}
> +static inline struct klp_object *obj_iter_init(struct klp_patch *patch) > +{ > + if (patch->objs->funcs || patch->objs->name)
I would do something like
#define klp_is_null_obj(obj) (!obj->funcs && !obj->name)
Then it can be used here an in klp_obj_iter_next(). This will be even more useful in the func iterator where the check is even more complicated.
> + return patch->objs; > + else > + return NULL; > +} > + > #define klp_for_each_object(patch, obj) \ > - for (obj = patch->objs; obj->funcs || obj->name; obj++) > + for (obj = obj_iter_init(patch); obj; obj = obj_iter_next(patch, obj)) > + > +static inline struct klp_func *func_iter_next(struct klp_object *obj, > + struct klp_func *func) > +{ > + struct klp_func *next_func = NULL; > + > + if (list_empty(&func->func_entry)) { > + next_func = func + 1; > + if (next_func->old_name || next_func->new_func || > + next_func->old_sympos) > + goto out; > + else > + next_func = NULL; > + if (!list_empty(&obj->func_list)) > + next_func = container_of(obj->func_list.next, > + struct klp_func, > + func_entry);
I have just realized that a practice is to use list_entry() instead of container_of() for list entries. It probably makes the code better readable for a trained eye.
> + goto out; > + } > + if (func->func_entry.next != &obj->func_list) > + next_func = container_of(func->func_entry.next, > + struct klp_func, > + func_entry); > +out: > + return next_func; > +}
[...]
> #define klp_for_each_func(obj, func) \ > - for (func = obj->funcs; \ > - func->old_name || func->new_func || func->old_sympos; \ > - func++) > + for (func = func_iter_init(obj); func; \ > + func = func_iter_next(obj, func))
Otherwise, I have basically the same comments about iter_func like for iter_obj.
> int klp_register_patch(struct klp_patch *); > int klp_unregister_patch(struct klp_patch *); > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index b9628e4..6004be3 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -729,7 +730,10 @@ static int klp_init_patch(struct klp_patch *patch) > return ret; > } > > + INIT_LIST_HEAD(&patch->obj_list);
Please, do this together with the other trivial initizalizations. I mean to do it in the same place like in the other init functions:
patch->enabled = false; patch->replaced = false; + INIT_LIST_HEAD(&patch->obj_list);
> klp_for_each_object(patch, obj) { > + INIT_LIST_HEAD(&obj->obj_entry); > + INIT_LIST_HEAD(&obj->func_list);
These two should be done in klp_init_object(). You move it there in the second patch anyway.
> ret = klp_init_object(patch, obj); > if (ret) > goto free;
Best Regards, Petr
| |