[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: David Hildenbrand
Subject: Re: [PATCH v4 3/8] s390/sclp: rework sclp boundary and length checks
Date: Tue, 21 Jul 2020 10:41:47 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0


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


David / dhildenb

reply via email to

[Prev in Thread] Current Thread [Next in Thread]