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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 10.02.16 at 14:50, <andrew.cooper3@citrix.com> wrote:
> 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).

I know.

> 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.

Oh, right - this is the operand to a ?:, not by itself passed to
printk(). Line breaks like this (to aid people with old editors) are
really undesirable in places like this...

Sorry for the noise,

Jan