Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

[Thomas Huth <thuth@redhat.com>]

On 04/11/2019 12.19, Claudio Imbrenda wrote:
> On Mon, 4 Nov 2019 10:45:07 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
[...]
>>> +static void test_toolong(void)
>>> +{
>>> +	uint32_t cmd = SCLP_CMD_WRITE_EVENT_DATA;
>>> +	uint16_t res = SCLP_RC_SCCB_BOUNDARY_VIOLATION;
>>
>> Why use variables for constants that are never touched?
> 
> readability mostly. the names of the constants are rather long.
> the compiler will notice it and do the Right Thing™

I'd like to suggest to add the "const" keyword to both variables in that 
case, then it's clear that they are not used to be modified.

>>> +		h->length = 4096;
>>> +
>>> +		valid_code = commands[i];
>>> +		cc = sclp_service_call(commands[i], h);
>>> +		if (cc)
>>> +			break;
>>> +		if (h->response_code ==
>>> SCLP_RC_NORMAL_READ_COMPLETION)
>>> +			return;
>>> +		if (h->response_code !=
>>> SCLP_RC_INVALID_SCLP_COMMAND)
>>> +			break;
>>
>> Depending on line length you could add that to the cc check.
>> Maybe you could also group the error conditions before the success
>> conditions or the other way around.
> 
> yeah it woud fit, but I'm not sure it would be more readable:
> 
> if (cc || (h->response_code != SCLP_RC_INVALID_SCLP_COMMAND))
>                          break;

In case you go with that solution, please drop the innermost parentheses.

  Thomas