Re: [Xen-devel] [PATCH 2/3] x86/hvm: Rearange check_segment() to use a switch statement

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.07.17 at 15:58, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 03/07/17 14:33, Jan Beulich wrote:
>>>>> On 03.07.17 at 15:15, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 03/07/17 13:34, Jan Beulich wrote:
>>>>>>> On 30.06.17 at 17:04, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> +    case x86_seg_ds:
>>>>> +    case x86_seg_es:
>>>>> +        if ( (reg->attr.fields.type & 0x8) && !(reg->attr.fields.type & 
>>>>> 0x2) )
>>>>> +        {
>>>>> +            gprintk(XENLOG_ERR, "Non-readable segment provided for DS or 
>>> ES\n");
>>>>> +            return -EINVAL;
>>>>> +        }
>>>>> +        break;
>>>>> +
>>>>> +    default: /* -Werror=switch */
>>>>> +        break;
>>>>>      }
>>>> Perhaps better to have
>>>>
>>>>     default:
>>>>         ASSERT_UNREACHABLE();
>>>>     case x86_seg_tr:
>>>>         break;
>>>>
>>>> to make more visible that it is not an oversight that especially FS
>>>> and GS aren't being handled here? Either way
>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> The x86_seg_tr case exits check_segment() rather earlier.
>> I don't think it does - there are just two specific error paths there.
>>
>>>  How about
>>>
>>> default:
>>>     ASSERT_UNREACHABLE();
>>>     return -EINVAL;
>>>
>>> ?
>> Indeed I would have suggested this if I had been able to convince
>> myself that x86_seg_tr can't come here.
> 
> You are quite right.  I was mistaken.  I will go with this:
> 
>     case x86_seg_tr:
>         break;
> 
>     default:
>         ASSERT_UNREACHABLE();
>         return -EINVAL;
>     }
> 
> which fails slightly safer than your suggestion in release builds.

Ah, yes, agreed.

Jan


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