Re: [PATCH v2 2/2] livepatch: add shadow variable sample programs

[Petr Mladek]

On Tue 2017-07-18 15:15:00, Joe Lawrence wrote:
> On Tue, Jul 18, 2017 at 04:47:45PM +0200, Petr Mladek wrote:
> > Date:   Tue, 18 Jul 2017 16:47:45 +0200
> > From: Petr Mladek <pmladek@xxxxxxxx>
> > To: Joe Lawrence <joe.lawrence@xxxxxxxxxx>
> > Cc: live-patching@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, Josh
> >  Poimboeuf <jpoimboe@xxxxxxxxxx>, Jessica Yu <jeyu@xxxxxxxxxx>, Jiri Kosina
> >  <jikos@xxxxxxxxxx>, Miroslav Benes <mbenes@xxxxxxx>
> > Subject: Re: [PATCH v2 2/2] livepatch: add shadow variable sample programs
> > User-Agent: Mutt/1.5.21 (2010-09-15)
> > 
> > On Wed 2017-06-28 11:37:27, Joe Lawrence wrote:
> > > diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c
> > > new file mode 100644
> > > index 000000000000..423f4b7b0adb
> > > --- /dev/null
> > > +++ b/samples/livepatch/livepatch-shadow-mod.c
> > > + * Usage
> > > + * -----
> > > + *
> > > + * Fix the memory leak
> > > + * -------------------
> > > + *
> > > + * Extend functionality
> > > + * --------------------
> > 
> > When I made first quick look, I had troubles to understand that
> > these two sections were still part of the Usage section.
> 
> I could double underline "Usage" and keep the single underlines for "Fix
> the memory leak' and "Extend functionality" sections if that helps.

I am not sure that it would make any big difference.


> > the commands how to enable the modules were hidden deep in the
> > expected output analyze.
> 
> Yeah, v2's examples were hard to summarize and describe through
> comments.  The code is pretty simple, but the devil is in the timing
> details and log output seemed the best way to illustrate the fixes.
> 
> Do you think the log entries are worth copying into these comments?  I
> could simply describe the effect and let the reader generate their own
> if they so desired.

The logs are fine but I would put them into separate sections/files.
I mean that I would redude the "Usage" section to

 * Load the buggy demonstration module:
 * $> insmod samples/livepatch/livepatch-shadow-mod.ko
 *
 * After xx seconds/minutes watch log messages
 * $> dmesg
 * And load the livepatch fix1:
 * $> insmod samples/livepatch/livepatch-shadow-fix1.ko
 ...

Then I would describe the expected effect and output in separate section.

> > I wonder if it would make sense to move most of the information
> > into the respective module sources. 
> 
> Are you suggesting that it would be clearer to distribute the
> log + comments to each individual livepatch-shadow-*.c module file?

Exactly. I am not 100% sure that it will be better but my guess
is that it might help.

> >                                     I actually missed some
> > more info in that modules. The few lines were hard to spot
> > after the long copyright/license blob.
> 
> Would a heading like this help separate the legalese from the patch
> description?

> /* [ ... GPL, copyrights, etc ... ]
>  *
>  * Module Description
>  * ==================
>  * [...]

This won't be needed if the description is longer and the expected
logs are part of it. IMHO, people want to compare the code and
the output anyway.

Anyway, this is a minor issue. I was not only able to descibe
it shortly. Do not worry much about it.

>  
> > Otherwise I like the examples.
> 
> Funny that I had anticipated head-scratching over the convoluted example
> instead of my workqueue usage :)

Heh, I can't think of anything better. I like that that it shows
usage for both klp_shadow_get() and klp_shadow_get_or_attach().

Best Regards,
Petr