Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1686996 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Josh Poimboeuf <jpoimboe@redhat.com> - 2017-07-14 02:50 +0200
    Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Miroslav Benes <mbenes@suse.cz> - 2017-07-17 17:40 +0200

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

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@redhat.com>

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

[toc] | [next] | [standalone]

#1689179

On Thu, 13 Jul 2017, Josh Poimboeuf wrote:

> On Wed, Jun 28, 2017 at 11:37:26AM -0400, Joe Lawrence wrote:
> 
> > +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.

I agree that "original data" does not sound right. However, we use "parent 
object" for vmlinux or a module in our code. But I don't have a better 
name and "parent object" sounds good.
 
> > +    - 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.

I agree with Josh on all of this.

Miroslav

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web