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
