On Mon, Feb 15, 2016 at 7:08 AM, Jan Beulich <jbeul...@suse.com> wrote:
> >>> On 15.02.16 at 14:29, <cz...@bitdefender.com> 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@lists.xen.org http://lists.xen.org/xen-devel