Re: [PATCH v2][4.17?] core-parking: fix build with gcc12 and NR_CPUS=1

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.10.2022 12:03, Julien Grall wrote:
> Hi Jan,
> 
> On 24/10/2022 08:26, Jan Beulich wrote:
>> On 22.10.2022 17:30, Julien Grall wrote:
>>> Is this intended for 4.17?
>>
>> Well, yes, it was meant to be - it has been ...
>>
>>> On 09/09/2022 15:30, Jan Beulich wrote:
>>
>> ... well over a month since it was sent.
>>
>>>> --- a/xen/arch/x86/sysctl.c
>>>> +++ b/xen/arch/x86/sysctl.c
>>>> @@ -157,7 +157,7 @@ long arch_do_sysctl(
>>>>            long (*fn)(void *);
>>>>            void *hcpu;
>>>>    
>>>> -        switch ( op )
>>>> +        switch ( op | -(CONFIG_NR_CPUS == 1) )
>>> This code is quite confusing to read and potentially risky as you are
>>> are relying the top bit of 'op' to never be 1. While I am expecting this
>>> will ever be the case, this will be a "fun" issue to debug if this ever
>>> happen. So I would suggest to check CONFIG_NR_CPUS == 1 separately.
>>
>> You're aware that we use this pattern in a few other places already (I
>> guess in my local tree I have one or two which aren't upstream yet)? Just
>> grep for "switch[^_].*[|]" to see them.
> 
> I could only spot two upstream in arch/x86/hvm/svm/svm.c and 
> arch/x86/hvm/vmx/vmx.c.
> 
> But I am not convinced this is a good enough reason to continue to use 
> this approach. There are a few bad code examples in Xen and we have been 
> pushing against continue to spread certain construct.

Sure. But these were introduced consciously and deliberately, iirc.

>> Also note that it's not just the
>> top bit of "op" - we merely assume "op" will never be ~0.
> Yes, I misread the code.
> 
>> Personally I
>> prefer this way of coding the logic, but if at least one of the other x86
>> maintainers agreed with you, I'd be okay to switch to using a separate
>> if().
> 
> I am curious why, is it just a matter of taste?

I think so. It's not written down anywhere that such constructs should
not be used.

> If you really want to go down this route, then I think you should add 
> "ASSERT(op != ~0U);" to catch someone introducing a value match that one 
> we exclude.

No, such an assertion would be checking user input; a caller might
deliberately pass this (invalid) value.

Jan