From mboxrd@z Thu Jan 1 00:00:00 1970 From: Corneliu ZUZU 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 Message-ID: <56C203D8.9040704@bitdefender.com> References: <1455518118-414-1-git-send-email-czuzu@bitdefender.com> <1455518254-507-1-git-send-email-czuzu@bitdefender.com> <56C1C79002000078000D20B8@prv-mh.provo.novell.com> <56C1C135.9070707@bitdefender.com> <56C1D62C02000078000D21B3@prv-mh.provo.novell.com> <56C1D2B3.8050604@bitdefender.com> <56C1E9FC02000078000D225E@prv-mh.provo.novell.com> <56C1FCBB.5070004@bitdefender.com> <56C20E9502000078000D24B6@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5585707095671194244==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel , Jan Beulich Cc: Keir Fraser , Ian Campbell , Razvan Cojocaru , Andrew Cooper , Xen-devel , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --===============5585707095671194244== Content-Type: multipart/alternative; boundary="------------090200000300060803060405" This is a multi-part message in MIME format. --------------090200000300060803060405 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 2/15/2016 6:51 PM, Tamas K Lengyel wrote: > > > On Mon, Feb 15, 2016 at 9:44 AM, Jan Beulich > wrote: > > >>> On 15.02.16 at 17:28, > 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. --------------090200000300060803060405 Content-Type: text/html; charset=utf-8 Content-Length: 3872 Content-Transfer-Encoding: quoted-printable
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> wrote:
>>> On 15.02.16 at 17:28, <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) =3D 0 or >=3D (0x1 + 0xFFFFFFFF) (=3F)
>> 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) )
>=C2=A0 =C2=A0 =C2=A0 return -EINVAL;
> if ( unlikely(!(arch_monitor_get_capabilities(d) & (1U << mop->event))) )
>=C2=A0 =C2=A0 =C2=A0 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.
--------------090200000300060803060405-- --===============5585707095671194244== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============5585707095671194244==--