Re: [PATCH] xen/x86: Fix errors arising from c/s dab76ff

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/02/16 15:13, Jan Beulich wrote:
>>>> On 12.02.16 at 15:59, <andrew.cooper3@citrix.com> 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