* [PATCH] xen/x86: Fix errors arising from c/s dab76ff @ 2016-02-12 14:59 Andrew Cooper 2016-02-12 15:13 ` Jan Beulich 2016-02-15 15:15 ` George Dunlap 0 siblings, 2 replies; 5+ messages in thread From: Andrew Cooper @ 2016-02-12 14:59 UTC (permalink / raw) To: Xen-devel; +Cc: George Dunlap, Andrew Cooper, Jan Beulich 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(-) 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) -- 2.1.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/x86: Fix errors arising from c/s dab76ff 2016-02-12 14:59 [PATCH] xen/x86: Fix errors arising from c/s dab76ff Andrew Cooper @ 2016-02-12 15:13 ` Jan Beulich 2016-02-12 15:23 ` Andrew Cooper 2016-02-15 15:15 ` George Dunlap 1 sibling, 1 reply; 5+ messages in thread From: Jan Beulich @ 2016-02-12 15:13 UTC (permalink / raw) To: Andrew Cooper; +Cc: George Dunlap, Xen-devel >>> 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/x86: Fix errors arising from c/s dab76ff 2016-02-12 15:13 ` Jan Beulich @ 2016-02-12 15:23 ` Andrew Cooper 0 siblings, 0 replies; 5+ messages in thread From: Andrew Cooper @ 2016-02-12 15:23 UTC (permalink / raw) To: Jan Beulich; +Cc: George Dunlap, Xen-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/x86: Fix errors arising from c/s dab76ff 2016-02-12 14:59 [PATCH] xen/x86: Fix errors arising from c/s dab76ff Andrew Cooper 2016-02-12 15:13 ` Jan Beulich @ 2016-02-15 15:15 ` George Dunlap 2016-02-15 16:52 ` Jan Beulich 1 sibling, 1 reply; 5+ messages in thread From: George Dunlap @ 2016-02-15 15:15 UTC (permalink / raw) To: Andrew Cooper, Xen-devel; +Cc: George Dunlap, Jan Beulich [-- Attachment #1: Type: text/plain, Size: 2971 bytes --] On 12/02/16 14:59, Andrew Cooper 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(). > 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> What about something like this instead? (Ported to be on top of this patch, since it's already been committed.) -George [PATCH] xen/p2m: Make dump table printing less clever Rather than detecting whether to print out the numerical value of the memory type based on whether the second byte of the stringified value is a null character, just always print out both. Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Tim Deegan <tim@xen.org> --- xen/arch/x86/mm/p2m-ept.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index 3cb6868..be528e7 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][3] = { + static const char *memory_types[8] = { [MTRR_TYPE_UNCACHABLE] = "UC", [MTRR_TYPE_WRCOMB] = "WC", [MTRR_TYPE_WRTHROUGH] = "WT", @@ -1262,14 +1262,13 @@ static void ept_dump_p2m_table(unsigned char key) if ( ept_entry->sa_p2mt == p2m_populate_on_demand ) printk("gfn: %13lx order: %2d PoD\n", gfn, order); else - printk("gfn: %13lx order: %2d mfn: %13lx %c%c%c %c%c%c\n", + printk("gfn: %13lx order: %2d mfn: %13lx %c%c%c %s(%d)%c\n", gfn, order, ept_entry->mfn + 0UL, ept_entry->r ? 'r' : ' ', ept_entry->w ? 'w' : ' ', ept_entry->x ? 'x' : ' ', - memory_type_to_str(ept_entry->emt)[0], - memory_type_to_str(ept_entry->emt)[1] - ?: ept_entry->emt + '0', + memory_type_to_str(ept_entry->emt), + ept_entry->emt, c ?: ept_entry->ipat ? '!' : ' '); if ( !(record_counter++ % 100) ) -- 2.1.4 [-- Attachment #2: 0001-xen-p2m-Make-dump-table-printing-less-clever.patch --] [-- Type: text/x-patch, Size: 2232 bytes --] >From b9d1d77c001507c4c414b83666a3f59e327364c3 Mon Sep 17 00:00:00 2001 From: George Dunlap <george.dunlap@eu.citrix.com> Date: Mon, 15 Feb 2016 14:58:29 +0000 Subject: [PATCH] xen/p2m: Make dump table printing less clever Rather than detecting whether to print out the numerical value of the memory type based on whether the second byte of the stringified value is a null character, just always print out both. Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Tim Deegan <tim@xen.org> --- xen/arch/x86/mm/p2m-ept.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index 3cb6868..be528e7 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][3] = { + static const char *memory_types[8] = { [MTRR_TYPE_UNCACHABLE] = "UC", [MTRR_TYPE_WRCOMB] = "WC", [MTRR_TYPE_WRTHROUGH] = "WT", @@ -1262,14 +1262,13 @@ static void ept_dump_p2m_table(unsigned char key) if ( ept_entry->sa_p2mt == p2m_populate_on_demand ) printk("gfn: %13lx order: %2d PoD\n", gfn, order); else - printk("gfn: %13lx order: %2d mfn: %13lx %c%c%c %c%c%c\n", + printk("gfn: %13lx order: %2d mfn: %13lx %c%c%c %s(%d)%c\n", gfn, order, ept_entry->mfn + 0UL, ept_entry->r ? 'r' : ' ', ept_entry->w ? 'w' : ' ', ept_entry->x ? 'x' : ' ', - memory_type_to_str(ept_entry->emt)[0], - memory_type_to_str(ept_entry->emt)[1] - ?: ept_entry->emt + '0', + memory_type_to_str(ept_entry->emt), + ept_entry->emt, c ?: ept_entry->ipat ? '!' : ' '); if ( !(record_counter++ % 100) ) -- 2.1.4 [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/x86: Fix errors arising from c/s dab76ff 2016-02-15 15:15 ` George Dunlap @ 2016-02-15 16:52 ` Jan Beulich 0 siblings, 0 replies; 5+ messages in thread From: Jan Beulich @ 2016-02-15 16:52 UTC (permalink / raw) To: George Dunlap; +Cc: George Dunlap, Andrew Cooper, Xen-devel >>> On 15.02.16 at 16:15, <george.dunlap@citrix.com> wrote: > What about something like this instead? (Ported to be on top of this > patch, since it's already been committed.) > > -George > > [PATCH] xen/p2m: Make dump table printing less clever > > Rather than detecting whether to print out the numerical value of the > memory type based on whether > the second byte of the stringified value is a null character, just > always print out both. Generally a good idea, but ... > @@ -1262,14 +1262,13 @@ static void ept_dump_p2m_table(unsigned char key) > if ( ept_entry->sa_p2mt == p2m_populate_on_demand ) > printk("gfn: %13lx order: %2d PoD\n", gfn, order); > else > - printk("gfn: %13lx order: %2d mfn: %13lx %c%c%c%c%c%c\n", > + printk("gfn: %13lx order: %2d mfn: %13lx %c%c%c%s(%d)%c\n", > gfn, order, ept_entry->mfn + 0UL, > ept_entry->r ? 'r' : ' ', > ept_entry->w ? 'w' : ' ', > ept_entry->x ? 'x' : ' ', > - memory_type_to_str(ept_entry->emt)[0], > - memory_type_to_str(ept_entry->emt)[1] > - ?: ept_entry->emt + '0', > + memory_type_to_str(ept_entry->emt), > + ept_entry->emt, > c ?: ept_entry->ipat ? '!' : ' '); ... this will further increase the amount of data to be pushed out, and the debug key being handled here is already putting quite a bit of load on the serial console. It was the goal to save every byte we can which drove me to the solution currently in place. Jan ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-02-15 16:52 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-12 14:59 [PATCH] xen/x86: Fix errors arising from c/s dab76ff Andrew Cooper 2016-02-12 15:13 ` Jan Beulich 2016-02-12 15:23 ` Andrew Cooper 2016-02-15 15:15 ` George Dunlap 2016-02-15 16:52 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).