Re: [kvm-unit-tests PATCH v3 1/4] lib: s390x: add support for SCLP console read

[Janosch Frank <frankja@linux.ibm.com>]

On 4/22/22 09:50, Nico Boehr wrote:
> On Thu, 2022-04-21 at 16:29 +0200, Janosch Frank wrote:
>>
> [...]
>>> diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c
>>> index fa36a6a42381..8c4bf68cbbab 100644
>>> --- a/lib/s390x/sclp-console.c
>>> +++ b/lib/s390x/sclp-console.c
> [...]
>>> +       read_buf_end = sccb->ebh.length -
>>> event_buffer_ascii_recv_header_len;
>>
>> Isn't this more like a length of the current read buffer contents?
> 
> Right, thanks, length is a much better name.
> 
> [...]
>>> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
>>> index fead007a6037..e48a5a3df20b 100644
>>> --- a/lib/s390x/sclp.h
>>> +++ b/lib/s390x/sclp.h
>>> @@ -313,6 +313,14 @@ typedef struct ReadEventData {
>>>          uint32_t mask;
>>>    } __attribute__((packed)) ReadEventData;
>>>    
>>> +#define SCLP_EVENT_ASCII_TYPE_DATA_STREAM_FOLLOWS 0
>>
>> Hrm, I'm not completely happy with the naming here since I confused
>> it
>> to the ebh->type when looking up the constants. But now I understand
>> why
>> you chose it.
> 
> Yeah, it sure is confusing.
> 
> Maybe it is better if we leave out the "type" entirely, but this might
> make it harder to understand where it's coming from:
> SCLP_ASCII_RECEIVE_DATA_STREAM_FOLLOWS
> 
> Another alternative I thought about is using enums, it won't fix the
> naming, but at least it might be clearer to which type it belongs.
> 
> Let me know what you think.

It should be fine as is. I don't expect that we have to touch this file 
very often.