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

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

On 10/02/16 13:22, Jan Beulich wrote:
>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> 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. "? ".

The code is very opaque.  However, that appears to be precisely how it
is intended to work.  (Having said that - it is your code from c/s
90e9c95f).

The following line will only format the raw emt value as a number if
there is a NUL character returned from memory_type_to_str().  Putting a
space in instead would break this.

~Andrew