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

[Nico Boehr <nrb@linux.ibm.com>]

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.