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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew

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