| Message ID | 1455289189-32425-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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. > 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. > and introduce an ASSERT() as requested on one thread of the original patch. Whereas this part is again appreciated. Jan
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
diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c index 8839f8d..234d2ba 100644 --- a/xen/arch/x86/cpu/mtrr/generic.c +++ b/xen/arch/x86/cpu/mtrr/generic.c @@ -98,7 +98,7 @@ static const char *__init mtrr_attrib_to_str(mtrr_type x) [MTRR_TYPE_WRBACK] = "write-back", }; - return x < MTRR_NUM_TYPES ? (strings[x] ?: "?") : "?"; + return (x < ARRAY_SIZE(strings) && strings[x][0]) ? strings[x] : "?"; } static unsigned int __initdata last_fixed_start; diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index 316e3f3..3cb6868 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -1204,7 +1204,7 @@ void ept_p2m_uninit(struct p2m_domain *p2m) static const char *memory_type_to_str(unsigned int x) { - static const char memory_types[8][2] = { + static const char memory_types[8][3] = { [MTRR_TYPE_UNCACHABLE] = "UC", [MTRR_TYPE_WRCOMB] = "WC", [MTRR_TYPE_WRTHROUGH] = "WT", @@ -1213,7 +1213,8 @@ static const char *memory_type_to_str(unsigned int x) [MTRR_NUM_TYPES] = "??" }; - return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?"; + ASSERT(x < ARRAY_SIZE(memory_types)); + return memory_types[x][0] ? memory_types[x] : "?"; } static void ept_dump_p2m_table(unsigned char key)
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(). There was also a further error; the strings were not NULL terminated, which made the return type of memory_type_to_str() erronious. Bump the 2D array to 3 characters, so the strings retain their NUL characters, and introduce an ASSERT() as requested on one thread of the original patch. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: George Dunlap <george.dunlap@eu.citrix.com> --- xen/arch/x86/cpu/mtrr/generic.c | 2 +- xen/arch/x86/mm/p2m-ept.c | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-)