Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751076AbdHRJmy (ORCPT ); Fri, 18 Aug 2017 05:42:54 -0400 Received: from mx2.suse.de ([195.135.220.15]:38071 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750954AbdHRJmx (ORCPT ); Fri, 18 Aug 2017 05:42:53 -0400 Date: Fri, 18 Aug 2017 11:42:50 +0200 From: Petr Mladek To: Joe Lawrence Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Josh Poimboeuf , Jessica Yu , Jiri Kosina , Miroslav Benes Subject: Re: [PATCH v4] livepatch: introduce shadow variable API Message-ID: <20170818094250.GA25223@pathway.suse.cz> References: <1502740963-31310-1-git-send-email-joe.lawrence@redhat.com> <1502740963-31310-2-git-send-email-joe.lawrence@redhat.com> <20170817140545.GF601@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1620 Lines: 44 On Thu 2017-08-17 12:01:33, Joe Lawrence wrote: > On 08/17/2017 10:05 AM, Petr Mladek wrote: > >> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c > >> new file mode 100644 > >> index 000000000000..0ebd4b635e4f > >> --- /dev/null > >> +++ b/kernel/livepatch/shadow.c > >> +/** > >> + * klp_shadow_match() - verify a shadow variable matches given > >> + * @shadow: shadow variable to match > >> + * @obj: pointer to parent object > >> + * @id: data identifier > >> + * > >> + * Return: true if the shadow variable matches. > >> + * > >> + * Callers should hold the klp_shadow_lock. > >> + */ > >> +static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj, > >> + unsigned long id) > >> +{ > >> + return shadow->obj == obj && shadow->id == id; > >> +} > > > > Do we really need this function? It is called only in situations > > where shadow->obj == obj is always true. Especially the use in > > klp_shadow_detach_all() is funny because we pass shadow->obj as > > the shadow parameter. > > Personal preference. Abstracting out all of the routines that operated > on the shadow variables (setting up, comparison) did save some code > lines and centralized these common bits. I take this back. We actually need to check obj because different objects might have the same hash. I think that I did the same mistake also the last time. I hope that I will be able to fix this in my mind faster than "never" vs. "newer" typo that I do for years. Also I forgot to say that you did great work. Each version of the patch is much better than the previous one. Best Regards, Petr