On Fri 2022-07-01 16:48:14, Marcos Paulo de Souza wrote:
> From: Petr Mladek <pmladek@xxxxxxxx>
>
> Separate code that is used in klp_shadow_get_or_alloc() under klp_mutex.
> It splits a long spaghetti function into two. Also it unifies the error
> handling. The old used a mix of duplicated code, direct returns,
> and goto. The new code has only one unlock, free, and return calls.
>
> Background: The change was needed by an earlier variant of the code adding
> garbage collection of shadow variables. It is not needed in
> the end. But the change still looks like an useful clean up.
Maybe the above paragraph is superfluous. I would remove it after all.
> It is code refactoring without any functional changes.
>
> Signed-off-by: Petr Mladek <pmladek@xxxxxxxx>
(It is fun to review my own RFC patch).
> ---
> kernel/livepatch/shadow.c | 77 ++++++++++++++++++++++-----------------
> 1 file changed, 43 insertions(+), 34 deletions(-)
>
> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> index c2e724d97ddf..67c1313f6831 100644
> --- a/kernel/livepatch/shadow.c
> +++ b/kernel/livepatch/shadow.c
> @@ -101,41 +101,19 @@ void *klp_shadow_get(void *obj, unsigned long id)
> }
> EXPORT_SYMBOL_GPL(klp_shadow_get);
>
> -static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
> - size_t size, gfp_t gfp_flags,
> - klp_shadow_ctor_t ctor, void *ctor_data,
> - bool warn_on_exist)
> +static void *__klp_shadow_get_or_use(void *obj, unsigned long id,
> + struct klp_shadow *new_shadow,
> + klp_shadow_ctor_t ctor, void *ctor_data,
> + bool *exist)
I have to admit that the name is a bit misleading. The real meaning
is "get existing" or "use newly allocated one".
I think that the following might better describe the real meaning
in the context where it is used:
__klp_shadow_get_or_alloc_locked()
__klp_shadow_get_or_add()
Anyway, it would deserve some comment above the function definition.
For example:
/*
* Check if the variable already exists. Otherwise, add
* the pre-allocated one.
*/
> {
> - struct klp_shadow *new_shadow;
> void *shadow_data;
> - unsigned long flags;
It would deserve:
lockdep_assert_held(&klp_shadow_lock);
> -
> - /* Check if the shadow variable already exists */
> - shadow_data = klp_shadow_get(obj, id);
> - if (shadow_data)
> - goto exists;
> -
Best Regards,
Petr
