linux-kernel - Re: [PATCH v2] staging: zcache: support multiple clients, prep for KVM and RAMster

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 5 Jul 2011 20:30:12 -0700
From:	Greg KH <greg@...ah.com>
To:	Dan Carpenter <error27@...il.com>
Cc:	Dan Magenheimer <dan.magenheimer@...cle.com>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Marcus Klemm <marcus.klemm@...glemail.com>,
	kvm@...r.kernel.org, Konrad Wilk <konrad.wilk@...cle.com>,
	linux-kernel@...r.kernel.org, linux-mm <linux-mm@...ck.org>,
	Seth Jennings <sjenning@...ux.vnet.ibm.com>,
	devel@...uxdriverproject.org
Subject: Re: [PATCH v2] staging: zcache: support multiple clients, prep for
 KVM and RAMster

On Fri, Jul 01, 2011 at 07:58:45PM +0300, Dan Carpenter wrote:
> On Fri, Jul 01, 2011 at 07:31:54AM -0700, Dan Magenheimer wrote:
> > > Off by one errors are kind of insidious.  People cut and paste them
> > > and they spread.  If someone adds a new list of chunks then there
> > > are now two examples that are correct and two which have an extra
> > > element, so it's 50/50 that he'll copy the right one.
> > 
> > True, but these are NOT off-by-one errors... they are
> > correct-but-slightly-ugly code snippets.  (To clarify, I said
> > the *ugliness* arose when debugging an off-by-one error.)
> > 
> 
> What I meant was the new arrays are *one* element too large.
> 
> > Patches always welcome, and I agree that these should be
> > fixed eventually, assuming the code doesn't go away completely
> > first.. I'm simply stating the position
> > that going through another test/submit cycling to fix
> > correct-but-slightly-ugly code which is present only to
> > surface information for experiments is not high on my priority
> > list right now... unless GregKH says he won't accept the patch.
> >  
> > > Btw, looking at it again, this seems like maybe a similar issue in
> > > zbud_evict_zbpg():
> > > 
> > >    516          for (i = 0; i < MAX_CHUNK; i++) {
> > >    517  retry_unbud_list_i:
> > > 
> > > 
> > > MAX_CHUNKS is NCHUNKS - 1.  Shouldn't that be i < NCHUNKS so that we
> > > reach the last element in the list?
> > 
> > No, the last element in that list is unused.  There is a comment
> > to that effect someplace in the code.  (These lists are keeping
> > track of pages with "chunks" of available space and the last
> > entry would have no available space so is always empty.)
> 
> The comment says that the first element isn't used.  Perhaps the
> comment is out of date and now it's the last element that isn't
> used.  To me, it makes sense to have an unused first element, but it
> doesn't make sense to have an unused last element.  Why not just
> make the array smaller?
> 
> Also if the last element of the original arrays isn't used, then
> does that mean the last *two* elements of the new arrays aren't
> used?
> 
> Getting array sizes wrong is not a "correct-but-slightly-ugly"
> thing.  *grumble* *grumble* *grumble*.  But it doesn't crash the
> system so I'm fine with it going in as is...

I'm not.  Please fix this up.  I'll not accept it until it is.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/