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