Re: [Xen-devel] [PATCH 4/6] x86/xstate: Fix latent bugs in expand_xsave_states()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/09/16 13:43, Jan Beulich wrote:
>>>> On 12.09.16 at 14:29, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 12/09/16 12:41, Jan Beulich wrote:
>>>>>> On 12.09.16 at 11:51, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> @@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, 
>>>> unsigned int size)
>>>>  
>>>>          if ( src )
>>>>          {
>>>> -            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
>>>> +            BUG_ON((xstate_offsets[index] + xstate_sizes[index]) <= size);
>>> Surely converting an ASSERT() to BUG_ON() means inverting the
>>> relational operator used?
>> Very true.  It is unfortunate that all of this is dead code, and
>> impossible to test.  I also had half a mind to explicitly #if 0 it out
>> to leave people in no illusion that it ever might have been tested.
> So with this correct, the patch is then
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

The discussion on this patch has shown that the "if ( src )" is
unconditionally true, and as such, I would like to remove it.  Would
your R-b stand with this hunk altered similarly to the final hunk in
patch 6 (with the BUG_ON() logic adjustment, and updated wording) ?

~Andrew

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