DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 55FEDC0587D7
Date: Fri, 18 Aug 2017 14:04:47 -0500
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Joe Lawrence <joe.lawrence@redhat.com>, live-patching@vger.kernel.org,
        linux-kernel@vger.kernel.org, Jessica Yu <jeyu@redhat.com>,
        Jiri Kosina <jikos@kernel.org>, Miroslav Benes <mbenes@suse.cz>
Subject: Re: [PATCH v4] livepatch: introduce shadow variable API
Message-ID: <20170818190447.jyflvqdteg64ddij@treble>
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>
 <cdb91d50-5292-0479-1dfe-52d46f089e50@redhat.com>
 <20170818094250.GA25223@pathway.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20170818094250.GA25223@pathway.suse.cz>
User-Agent: Mutt/1.6.0.1 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1754
Lines: 45

On Fri, Aug 18, 2017 at 11:42:50AM +0200, Petr Mladek wrote:
> 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 <obj, id>
> > >> + * @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.

It's an easy mistake to make.  hash_for_each_possible() is not
intuitive, IMO.  Maybe some brave soul should fix it.

-- 
Josh