Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752316AbbBQPsr (ORCPT ); Tue, 17 Feb 2015 10:48:47 -0500 Received: from cantor2.suse.de ([195.135.220.15]:51220 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750863AbbBQPsp (ORCPT ); Tue, 17 Feb 2015 10:48:45 -0500 Date: Tue, 17 Feb 2015 16:48:39 +0100 (CET) From: Miroslav Benes To: Josh Poimboeuf cc: Seth Jennings , Jiri Kosina , Vojtech Pavlik , Masami Hiramatsu , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 6/9] livepatch: create per-task consistency model In-Reply-To: <20150217151048.GF11861@treble.redhat.com> Message-ID: References: <2c3d1e685dae5cccc2dfdb1b24c241b2f1c89348.1423499826.git.jpoimboe@redhat.com> <20150217151048.GF11861@treble.redhat.com> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3429 Lines: 99 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 > > > + > > > +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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/