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

[Corneliu ZUZU <czuzu@bitdefender.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1441 bytes --]

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

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

Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 5984 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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