From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] xen/x86: Fix errors arising from c/s dab76ff Date: Fri, 12 Feb 2016 15:23:54 +0000 Message-ID: <56BDF90A.1040500@citrix.com> References: <1455289189-32425-1-git-send-email-andrew.cooper3@citrix.com> <56BE049C02000078000D1771@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56BE049C02000078000D1771@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: George Dunlap , Xen-devel List-Id: xen-devel@lists.xenproject.org On 12/02/16 15:13, Jan Beulich wrote: >>>> On 12.02.16 at 15:59, wrote: >> Coverity correctly identifies that the changes in mtrr_attrib_to_str() >> introduce dead code. strings[] is a 2d array, rather than an array of >> strings, which means that strings[x] will never be a NULL pointer. >> >> Adjust the check to compenstate, by looking for a NUL in strings[x][0] >> instead. >> >> Curiously, Coverity did not notice the same error with memory_type_to_str(). > I agree up to here. > >> There was also a further error; the strings were not NULL terminated, which >> made the return type of memory_type_to_str() erronious. > What's erroneous here? I don't think there's any requirement > for a function returning char * to always return NUL-terminated > strings. The name of the function very clearly indicates that it is returning a string. > >> Bump the 2D array to 3 characters, so the strings retain their NUL >> characters, > I.e. I don't agree with this part of the change, even if the addition > of these few bytes doesn't make a whole lot of a difference. They > end up being "dead data" now, and if Coverity is smart it should > even be able to notice. It will produce something wrong if someone introduces a new path doing something like printk("%s", memory_type_to_str()). 8 extra bytes is a very small price to pay to make this work properly. The alternative, const char (*memory_type_to_str(unsigned int x))[2] is unrecognisable to most C programmers, and can't be used with printk(). ~Andrew > >> and introduce an ASSERT() as requested on one thread of the original patch. > Whereas this part is again appreciated. > > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel