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

From: Corneliu ZUZU <czuzu@bitdefender.com>
To: Tamas K Lengyel <tamas@tklengyel.com>, Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>,
	Ian Campbell <ian.campbell@citrix.com>,
	Razvan Cojocaru <rcojocaru@bitdefender.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Xen-devel <xen-devel@lists.xen.org>,
	Stefano Stabellini <stefano.stabellini@citrix.com>
Subject: Re: [PATCH v3 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.
Date: Mon, 15 Feb 2016 18:59:04 +0200	[thread overview]
Message-ID: <56C203D8.9040704@bitdefender.com> (raw)
In-Reply-To: <CABfawhnh54v0Ww4=6vYg_4Lqa+QyYLVtV_vvOTCtOJDvnth_hQ@mail.gmail.com>

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

On 2/15/2016 6:51 PM, Tamas K Lengyel wrote:
>
>
> On Mon, Feb 15, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com 
> <mailto:JBeulich@suse.com>> wrote:
>
>     >>> On 15.02.16 at 17:28, <czuzu@bitdefender.com <mailto:czuzu@bitdefender.com>> 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
>

Good points, noted.

Corneliu.

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

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

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