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 9:44 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 15.02.16 at 17:28, <czuzu@xxxxxxxxxxxxxxx> wrote:
> On 2/15/2016 4:08 PM, Jan Beulich wrote:
>>
>>> 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.
>
> Weirdo C, didn't know that!
> I've just read http://www.danielvik.com/2010/05/c-language-quirks.html .
> That's crazy, I can't believe such 'quirks' exist and that I never knew
> of them.
> So then, would this do:
>
> /* sanity check - avoid '<<' operator undefined behavior */
> if ( unlikely(mop->event > 31) )
>Â Â Â return -EINVAL;
> if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) )
>Â Â Â return -EOPNOTSUPP;

I'd say -EOPNOTSUPP in both cases, but if the maintainers like
-EINVAL better I wouldn't insist on my preference.

The best approach of course would be if we had __MAX values defined for such enums to check against but that doesn't seem to be part of Xen's coding practice. So in this case I would say leave it as -EINVAL as it's more descriptive of the problem and may even signal to the caller some inherent bug on their side, not just that the requested option is not supported.

Thanks,

Tamas

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