Re: [Xen-devel] [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Feb 15, 2016 at 7:08 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 15.02.16 at 14:29, <czuzu@xxxxxxxxxxxxxxx> wrote:
> On 2/15/2016 2:44 PM, Jan Beulich wrote:
>>
>>>Â Â Â Âswitch ( mop->op )
>>>Â Â Â Â{
>>>Â Â Â Âcase XEN_DOMCTL_MONITOR_OP_ENABLE:
>>>Â Â Â Âcase XEN_DOMCTL_MONITOR_OP_DISABLE:
>>>Â Â Â Â Â Â/* Check if event type is available. */
>>>Â Â Â Â Â Âif ( unlikely(!(arch_monitor_get_capabilities(d) & (1 << mop->event))) )
>>>Â Â Â Â Â Â Â Âreturn -EOPNOTSUPP;
>>>Â Â Â Â Â Â/* Arch-side handles enable/disable ops. */
>>>Â Â Â Â Â Âreturn arch_monitor_domctl_event(d, mop);
>> Ah, I see now that I've mis-read the default: code further below,
>> which actually calls arch_monitor_domctl_op(), not ..._event().
>> However, there's an "undefined behavior" issue with the code
>> above then when mop->event >= 31 - I think you want to left
>> shift 1U instead of plain 1, and you need to range check
>> mop->event first.
>>
> Never looked @ that part before, used it the way it was.
> I suppose that's because "according to the C specification, the result
> of a bit shift
> operation on a signed argument gives implementation-defined results, so
> in/theory/|1U << i|is
> more portable than|1 << i|" (taken from a stackoverflow post).

Yes.

> After changing 1 to 1U though, I don't understand why we should also
> range-check mop->event.
> I'm imagining when (mop->event > 31):
> * (1U << mop->event) = 0 or >= (0x1 + 0xFFFFFFFF) (?)

No, it's plain undefined.

> * in both cases arch_monitor_get_capabilities(d) & (1U << mop->event)
> would be = 0
> * in which case we would return -EOPNOTSUPP
> , no?

And even that would be true only today, and would break once
bit 31 gets a meaning. Whenever possible we should avoid
introducing such latent issues.

Ah yes, good catch, +1 for doing this extra check.

Thanks,
Tamas

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