Quoting Janosch Frank (2023-10-10 10:57:24)
> On 10/10/23 10:35, Nico Boehr wrote:
> > Quoting Janosch Frank (2023-10-10 09:38:54)
> > [...]
> >> diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c
> >> index 19c74e46..313be1e4 100644
> >> --- a/lib/s390x/sclp-console.c
> >> +++ b/lib/s390x/sclp-console.c
> > [...]
> >> +static bool lpar_ascii_compat;
> >
> > This only toggles adding \r. So why not name it accordingly?
>
> Because it also toggles clearing the screen
OK
[...]
> >> @@ -97,14 +100,27 @@ static void sclp_print_ascii(const char *str)
> >> {
> >> int len = strlen(str);
> >> WriteEventData *sccb = (void *)_sccb;
> >> + char *str_dest = (char *)&sccb->msg;
> >> + int i = 0;
> >>
> >> sclp_mark_busy();
> >> memset(sccb, 0, sizeof(*sccb));
> >> +
> >> + for (; i < len; i++) {
> >> + *str_dest = str[i];
> >> + str_dest++;
> >> + /* Add a \r to the \n for HMC ASCII console */
> >> + if (str[i] == '\n' && lpar_ascii_compat) {
> >> + *str_dest = '\r';
> >> + str_dest++;
> >> + }
> >> + }
> >
> > Please don't hide the check inside the loop.
> > Do:
> > if (lpar_ascii_compat)
> > // your loop
> > else
> > memcpy()
>
>
> I'd rather have a loop than to nest it inside an if.
I disagree, but it's not worth discussing too much over this.
> > Also, please add protection against overflowing sccb->msg (max 4088 bytes
> > if I looked it up right).
>
> I considered this but we already have a 2k length check before that
...which is not sufficient...
> and I'd like to see someone print ~2k in a single call.
>
> The question is if we want the complexity for something that we'll very
> likely never hit.
IMO we want it since it can lead to random memory corruption which can be
very hard to debug.
And I don't think it's complex since in the simplest case you could just go
with a strnlen() here. Better just truncate the string than to corrupt
random memory.
