Re: [RFC PATCH 6/9] livepatch: create per-task consistency model

[Miroslav Benes <mbenes@suse.cz>]

On Tue, 17 Feb 2015, Josh Poimboeuf wrote:

> On Mon, Feb 16, 2015 at 03:19:10PM +0100, Miroslav Benes wrote:
> > On Mon, 9 Feb 2015, Josh Poimboeuf wrote:
> > 

[...]

> > > +
> > > +void klp_unpatch_objects(struct klp_patch *patch)
> > > +{
> > > +	struct klp_object *obj;
> > > +
> > > +	for (obj = patch->objs; obj->funcs; obj++)
> > > +		if (obj->patched)
> > > +			klp_unpatch_object(obj);
> > > +}
> > 
> > Maybe we should introduce for_each_* macros which could be used in the 
> > code and avoid such functions. I do not have strong opinion about it.
> 
> Yeah, but each such loop seems to differ a little bit, so I'm not quite
> sure how to structure the macros such that they'd be useful.  Maybe for
> a future patch.

Yes, that is correct. The code in the caller of klp_unpatch_objects would 
look something like this

klp_for_each_object(obj, patch->objs)
	if (obj->patched)
		klp_unpatch_object(obj)

So there is in fact no change (compared to opencoding of 
klp_unpatch_objects), but IMO it is more legible. The upside is 
that we wouldn't introduce functions with similar names which could be 
confusing in the future AND we could use such macros throughout the code.

One step more could be macro klp_for_each_patched_object which would 
include the check.

However it is a nitpick, matter of taste and it is up to you.

> 
> > > diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
> > > index bb34bd3..1648259 100644
> > > --- a/kernel/livepatch/patch.h
> > > +++ b/kernel/livepatch/patch.h
> > > @@ -23,3 +23,4 @@ struct klp_ops *klp_find_ops(unsigned long old_addr);
> > >  
> > >  extern int klp_patch_object(struct klp_object *obj);
> > >  extern void klp_unpatch_object(struct klp_object *obj);
> > > +extern void klp_unpatch_objects(struct klp_patch *patch);
> > 
> > [...]
> > 
> > > diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
> > > new file mode 100644
> > > index 0000000..ba9a55c
> > > --- /dev/null
> > > +++ b/kernel/livepatch/transition.h
> > > @@ -0,0 +1,16 @@
> > > +#include <linux/livepatch.h>
> > > +
> > > +enum {
> > > +	KLP_UNIVERSE_UNDEFINED = -1,
> > > +	KLP_UNIVERSE_OLD,
> > > +	KLP_UNIVERSE_NEW,
> > > +};
> > > +
> > > +extern struct mutex klp_mutex;
> > > +extern struct klp_patch *klp_transition_patch;
> > > +
> > > +extern void klp_init_transition(struct klp_patch *patch, int universe);
> > > +extern void klp_start_transition(int universe);
> > > +extern void klp_reverse_transition(void);
> > > +extern void klp_try_complete_transition(void);
> > > +extern void klp_complete_transition(void);
> > 
> > Double inclusion protection is missing
> 
> Ok.
> 
> > and externs for functions are redundant.
> 
> I agree, but it seems to be the norm in Linux.  I have no idea why.  I'm
> just following the existing convention.

Yes, I know. It seems that each author does it differently. You can find 
both forms even in one header file in the kernel. There is no functional 
difference AFAIK (it is not the case for variables of course). So as long 
as we are consistent I do not care. And since we have externs already in 
livepatch.h... you can scratch this remark if you want to :)

Miroslav