Re: [PATCH v2 1/2] livepatch: introduce shadow variable API

[Josh Poimboeuf]

On Wed, Jun 28, 2017 at 11:37:26AM -0400, Joe Lawrence wrote:
> Add exported API for livepatch modules:
> 
>   klp_shadow_get()
>   klp_shadow_attach()
>   klp_shadow_get_or_attach()
>   klp_shadow_detach()
>   klp_shadow_detach_all()
> 
> that implement "shadow" variables, which allow callers to associate new
> shadow fields to existing data structures.  This is intended to be used
> by livepatch modules seeking to emulate additions to data structure
> definitions.
> 
> See Documentation/livepatch/shadow-vars.txt for a summary of the new
> shadow variable API, including a few common use cases.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@xxxxxxxxxx>

The API, docs, and code all look really good.

A few comments below about some of the variable naming and descriptions.

> diff --git a/Documentation/livepatch/shadow-vars.txt b/Documentation/livepatch/shadow-vars.txt
> new file mode 100644
> index 000000000000..7f28982e6b1c
> --- /dev/null
> +++ b/Documentation/livepatch/shadow-vars.txt
> @@ -0,0 +1,156 @@
> +Shadow Variables
> +================
> +
> +Shadow variables are a simple way for livepatch modules to associate new
> +"shadow" data to existing data structures.  Original data structures
> +(both their definition and storage) are left unmodified and "new" data
> +is allocated separately.  A shadow variable hashtable associates a
> +string key, enumeration pair with a pointer to the new data.

s/string key/numeric key/

> +Brief API summary
> +-----------------
> +
> +See the full API usage docbook notes in the livepatch/shadow.c
> +implementation.
> +
> +An in-kernel hashtable references all of the shadow variables.  These
> +references are stored/retrieved through a <obj, num> key pair.

"num" is rather vague, how about "key"?

(And note, this and some of the other comments also apply to the code as
well)

> +* The klp_shadow variable data structure encapsulates both tracking
> +meta-data and shadow-data:
> +  - meta-data
> +    - obj - pointer to original data

Instead of "original data", how about calling it the "parent object"?
That describes it better to me at least.  "Original data" sounds like
some of the data might be replaced.

> +    - num - numerical description of new data

"numerical description of new data" sounds a little confusing, how about
"unique identifier for new data"?

I'm also not sure about the phrase "new data".  Maybe something like
"new data field" would be more descriptive?  Or just "new field"?  I
view it kind of like adding a field to a struct.  Not a big deal either
way.

> +void *klp_shadow_attach(void *obj, unsigned long num, void *new_data,
> +			size_t new_size, gfp_t gfp_flags);

It could be just me, but the "new_" prefixes threw me off a little bit.
The new is implied anyway.  How about just "data" and "size"?

And the same comment for the klp_shadow struct.

-- 
Josh