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
