Re: [Qemu-devel] [PATCH v3] s390-ccw: print carriage return with new lines

[Halil Pasic <pasic@linux.vnet.ibm.com>]

On 10/27/2017 04:30 PM, Collin L. Walling wrote:
> The sclp console in the s390 bios writes raw data,
> leading console emulators (such as virsh console) to
> treat a new line ('\n') as just a new line instead
> of as a Unix line feed. Because of this, output
> appears in a "stair case" pattern.
> 
> Let's print \r\n on every occurrence of a new line
> in the string passed to write to amend this issue.
> 
> This is in sync with the guest Linux code in
> drivers/s390/char/sclp_vt220.c which also does a line feed
> conversion  in the console part of the driver. 
> 
> This fixes the s390-ccw and s390-netboot output like
> $ virsh start test --console
> Domain test started
> Connected to domain test
> Escape character is ^]
> Network boot starting...
>                           Using MAC address: 02:01:02:03:04:05
>                                                                 Requesting information via DHCP:  010
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  pc-bios/s390-ccw/sclp.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
> index 486fce1..a57006e 100644
> --- a/pc-bios/s390-ccw/sclp.c
> +++ b/pc-bios/s390-ccw/sclp.c
> @@ -68,17 +68,35 @@ void sclp_setup(void)
>  long write(int fd, const void *str, size_t len)
>  {
>      WriteEventData *sccb = (void *)_sccb;
> +    const char *p = str;
> +    size_t data_len = 0;
> +    size_t i;
> 
>      if (fd != 1 && fd != 2) {
>          return -EIO;
>      }
> 
> -    sccb->h.length = sizeof(WriteEventData) + len;
> +    for (i = len; i > 0; i--) > +        if (data_len + 1 >= SCCB_DATA_LEN) {
> +            /* We would overflow the sccb buffer, abort early */
> +            len = i;

This is not correct. Write is supposed to return the number
of bytes written. In this case the number of the bytes of the
original string which have been processed (so the client
code can resume at that place, using the nuber of bytes
transferred via sclp_service_call would be wrong if at least
one \n was processed).

Here you return the number of the bytes remaining, as i goes
from len to 0 and not the other way around. By the way Alex's
version was correct.

> +            break;
> +        }
> +
> +        if (*p == '\n') {
> +            /* Terminal emulators might need \r\n, so generate it */
> +            sccb->data[data_len++] = '\r';
> +        }
> +
> +        sccb->data[data_len++] = *p;
> +        p++;
> +    }
> +
> +    sccb->h.length = sizeof(WriteEventData) + data_len;
>      sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
> -    sccb->ebh.length = sizeof(EventBufferHeader) + len;
> +    sccb->ebh.length = sizeof(EventBufferHeader) + data_len;
>      sccb->ebh.type = SCLP_EVENT_ASCII_CONSOLE_DATA;
>      sccb->ebh.flags = 0;
> -    memcpy(sccb->data, str, len);
> 
>      sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb);
> 
I would have wrote the loop part like this:
-    sccb->h.length = sizeof(WriteEventData) + len;
+    for (i = 0; i < len; ++i) {
+        if (data_len + 1 >= SCCB_DATA_LEN) {
+            /* We would overflow the sccb buffer, abort early */
+            len = i;
+            break;
+        }
+
+        if (str[i] == '\n') {
+            /* Terminal emulators might need \r\n, so generate it */
+            sccb->data[data_len++] = '\r';
+        }
+
+        sccb->data[data_len++] = str[i];
+    }
+
+    sccb->h.length = sizeof(WriteEventData) + data_len;

This way you don't need p, and the loop steps trough the str indexed by
i while sccb->data is indexed by data_len. data_len is incremented each
time after we have written to the buffer (that's why postfix ++)
and i is incremented on each iteration (that's why normal prefix ++).

Btw I would also rename i to str_i and data_len to data_i to better
reflect this.

The cosmetics aren't important though, so it's probably better to go
with what we have already discussed as Alex's version.

Regards,
Halil