Re: [Xen-devel] [PATCH 6/8] xen/x86: Avoid overriding initialisers in arrays

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.02.16 at 21:01, <andrew.cooper3@xxxxxxxxxx> wrote:
> Clang objects to having multiple initialisers when creating an array.
> 
> As this warning is useful for spotting obscure bugs, disabling it is
> unhelpful.  Instead, fix our two deliberate usecases.

Ugly again, but - well ...

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m)
>      free_cpumask_var(ept->invalidate);
>  }
>  
> +static const char *memory_type_to_str(unsigned int x)
> +{
> +    static const char memory_types[8][2] = {
> +        [MTRR_TYPE_UNCACHABLE]     = "UC",
> +        [MTRR_TYPE_WRCOMB]         = "WC",
> +        [MTRR_TYPE_WRTHROUGH]      = "WT",
> +        [MTRR_TYPE_WRPROT]         = "WP",
> +        [MTRR_TYPE_WRBACK]         = "WB",
> +        [MTRR_NUM_TYPES]           = "??"
> +    };
> +
> +    return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?";

I think this should really ASSERT() the first condition.

> @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key)
>      unsigned long record_counter = 0;
>      struct p2m_domain *p2m;
>      struct ept_data *ept;
> -    static const char memory_types[8][2] = {
> -        [0 ... 7] = "?",
> -        [MTRR_TYPE_UNCACHABLE]     = "UC",
> -        [MTRR_TYPE_WRCOMB]         = "WC",
> -        [MTRR_TYPE_WRTHROUGH]      = "WT",
> -        [MTRR_TYPE_WRPROT]         = "WP",
> -        [MTRR_TYPE_WRBACK]         = "WB",
> -        [MTRR_NUM_TYPES]           = "??"
> -    };
>  
>      for_each_domain(d)
>      {
> @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key)
>                             ept_entry->r ? 'r' : ' ',
>                             ept_entry->w ? 'w' : ' ',
>                             ept_entry->x ? 'x' : ' ',
> -                           memory_types[ept_entry->emt][0],
> -                           memory_types[ept_entry->emt][1]
> +                           memory_type_to_str(ept_entry->emt)[0],
> +                           memory_type_to_str(ept_entry->emt)[1]
>                             ?: ept_entry->emt + '0',
>                             c ?: ept_entry->ipat ? '!' : ' ');

There's actually a bug here, which I think is worth fixing at once:
The default initializer was a string of length 1, resulting in a
premature NUL character to get placed into the fully expanded
string, causing - afaict - truncation of the intended message. I
therefore think the default string should be e.g. "? ".

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel