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

[David Hildenbrand <david@redhat.com>]

[...]

>>> +    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.

> 
>>> +    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.

> 
>> 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.

-- 
Thanks,

David / dhildenb