Re: [PATCH v4 3/8] s390/sclp: rework sclp boundary and length checks

[Collin Walling]

On 7/21/20 4:41 AM, David Hildenbrand wrote:
> [...]
> 
>>>> +    switch (code & SCLP_CMD_CODE_MASK) {
>>>> +    default:
>>>> +        if (sccb_max_addr < sccb_boundary) {
>>>> +            return true;
>>>> +        }
>>>> +    }
>>>
>>> ^ what is that?
>>>
>>>     if ((code & SCLP_CMD_CODE_MASK) && sccb_max_addr < sccb_boundary) {
>>>         return true;
>>>     }
> 
> Oh, my tired eyes missed that it's actually only
> 
> if (sccb_max_addr < sccb_boundary) :)
> 
>>>
>>
>> I agree it looks pointless in this patch, but it makes more sense in
>> patch #6 where we introduce cases for the SCLP commands that bypass
>> these checks if the extended-length sccb feature is enabled.
> 
> I am really a friend of introducing stuff where needed. Just use a
> simple "if" here and convert to the switch in patch #6.
> 

I can understand that. This follows the whole "each patch should be
sufficient on its own" way of thinking. A switch with no cases and a
default _does_ look silly.

>>
>>>> +    header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>>>> +    return false;
>>>
>>> So we return "false" on success? At least I consider that weird when
>>> returning the bool type. Maybe make it clearer what the function indicates
>>>
>>
>> Hmmm... I figured since there were more paths that can lead to success
>> (i.e. when I introduce the feat check in a later patch), then it made
>> more sense to to return false at the end. sclp_command_code_valid has
>> similar logic.
>>
>> But if boolean functions traditionally return true as the last return
>> value, I can rework it to align to coding preferences / standards.
>>
>>> "sccb_boundary_is_invalid"
>>>
>>
>> Unless it's simply the name that is confusing?
> 
> The options I would support are
> 
> 1. "sccb_boundary_is_valid" which returns "true" if valid
> 2. "sccb_boundary_is_invalid" which returns "true" if invalid
> 3. "sccb_boundary_validate" which returns "0" if valid and -EINVAL if not.
> 
> Which makes reading this code a bit easier.
> 

Sounds good. I'll takes this into consideration for the next round. (I
may wait just a little longer for that to allow more reviews to come in
from whoever has the time, if that's okay.)

>>
>>> or leave it named as is and switch from return value "bool" to "int",
>>> using "0" on success and "-EINVAL" on error.
>>>
>>
>> Is the switch statement an overkill? I thought of it as a cleaner way to
>> later show which commands have a special conditions (introduced in patch
>> 6 for the ELS stuff) instead of a nasty long if statement.
> 
> I think the switch make sense in patch #6.
> 


-- 
Regards,
Collin

Stay safe and stay healthy