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

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

[Christian Borntraeger]

From: "Collin L. Walling" <walling@linux.vnet.ibm.com>

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/s390-ccw.h |  3 +++
 pc-bios/s390-ccw/sclp.c     | 17 ++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 25d4d21..a8bd204 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -33,6 +33,9 @@ typedef unsigned long long __u64;
 #ifndef EBUSY
 #define EBUSY   2
 #endif
+#ifndef EFBIG
+#define EFBIG   3
+#endif
 #ifndef NULL
 #define NULL    0
 #endif
diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
index b1fc8ff..4795259 100644
--- a/pc-bios/s390-ccw/sclp.c
+++ b/pc-bios/s390-ccw/sclp.c
@@ -76,17 +76,28 @@ static int _strlen(const char *str)
 long write(int fd, const void *str, size_t len)
 {
     WriteEventData *sccb = (void *)_sccb;
+    const char *p;
+    size_t data_len = 0;
 
     if (fd != 1 && fd != 2) {
         return -EIO;
     }
 
-    sccb->h.length = sizeof(WriteEventData) + len;
+    for (p = str; *p; ++p) {
+        if (data_len > SCCB_DATA_LEN - 1) {
+            return -EFBIG;
+        }
+        if (*p == '\n') {
+            sccb->data[data_len++] = '\r';
+        }
+        sccb->data[data_len++] = *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);
 
-- 
2.9.4

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

[Alexander Graf]

On 20.10.17 12:25, Christian Borntraeger wrote:
> From: "Collin L. Walling" <walling@linux.vnet.ibm.com>
> 
> 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/s390-ccw.h |  3 +++
>  pc-bios/s390-ccw/sclp.c     | 17 ++++++++++++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 25d4d21..a8bd204 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -33,6 +33,9 @@ typedef unsigned long long __u64;
>  #ifndef EBUSY
>  #define EBUSY   2
>  #endif
> +#ifndef EFBIG
> +#define EFBIG   3
> +#endif
>  #ifndef NULL
>  #define NULL    0
>  #endif
> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
> index b1fc8ff..4795259 100644
> --- a/pc-bios/s390-ccw/sclp.c
> +++ b/pc-bios/s390-ccw/sclp.c
> @@ -76,17 +76,28 @@ static int _strlen(const char *str)
>  long write(int fd, const void *str, size_t len)
>  {
>      WriteEventData *sccb = (void *)_sccb;
> +    const char *p;
> +    size_t data_len = 0;
>  
>      if (fd != 1 && fd != 2) {
>          return -EIO;
>      }
>  
> -    sccb->h.length = sizeof(WriteEventData) + len;
> +    for (p = str; *p; ++p) {
> +        if (data_len > SCCB_DATA_LEN - 1) {
> +            return -EFBIG;
> +        }
> +        if (*p == '\n') {
> +            sccb->data[data_len++] = '\r';
> +        }
> +        sccb->data[data_len++] = *p;
> +    }
> +
> +    sccb->h.length = sizeof(WriteEventData) + data_len;

This subtly changes the semantics of the write() function from an
explicitly passed in "len" argument to NULL termination determined
sizing, no?

In that case, wouldn't it make sense to either remove the len argument
altogether or keep respecting it?


Alex

>      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);
>  
> 

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

[Christian Borntraeger]

On 10/20/2017 12:30 PM, Alexander Graf wrote:
> 
> 
> On 20.10.17 12:25, Christian Borntraeger wrote:
>> From: "Collin L. Walling" <walling@linux.vnet.ibm.com>
>>
>> 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/s390-ccw.h |  3 +++
>>  pc-bios/s390-ccw/sclp.c     | 17 ++++++++++++++---
>>  2 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index 25d4d21..a8bd204 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -33,6 +33,9 @@ typedef unsigned long long __u64;
>>  #ifndef EBUSY
>>  #define EBUSY   2
>>  #endif
>> +#ifndef EFBIG
>> +#define EFBIG   3
>> +#endif
>>  #ifndef NULL
>>  #define NULL    0
>>  #endif
>> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
>> index b1fc8ff..4795259 100644
>> --- a/pc-bios/s390-ccw/sclp.c
>> +++ b/pc-bios/s390-ccw/sclp.c
>> @@ -76,17 +76,28 @@ static int _strlen(const char *str)
>>  long write(int fd, const void *str, size_t len)
>>  {
>>      WriteEventData *sccb = (void *)_sccb;
>> +    const char *p;
>> +    size_t data_len = 0;
>>  
>>      if (fd != 1 && fd != 2) {
>>          return -EIO;
>>      }
>>  
>> -    sccb->h.length = sizeof(WriteEventData) + len;
>> +    for (p = str; *p; ++p) {
>> +        if (data_len > SCCB_DATA_LEN - 1) {
>> +            return -EFBIG;
>> +        }
>> +        if (*p == '\n') {
>> +            sccb->data[data_len++] = '\r';
>> +        }
>> +        sccb->data[data_len++] = *p;
>> +    }
>> +
>> +    sccb->h.length = sizeof(WriteEventData) + data_len;
> 
> This subtly changes the semantics of the write() function from an
> explicitly passed in "len" argument to NULL termination determined
> sizing, no?
> 
> In that case, wouldn't it make sense to either remove the len argument
> altogether or keep respecting it?

Yes, well spotted.
The write function is used in other code (SLOF related network boot),
so we should change it to respect the length, I think.

> 
> 
> Alex
> 
>>      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);
>>  
>>
> 

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

[Christian Borntraeger]

On 10/20/2017 12:41 PM, Christian Borntraeger wrote:
[...]
>>> @@ -76,17 +76,28 @@ static int _strlen(const char *str)
>>>  long write(int fd, const void *str, size_t len)
>>>  {
>>>      WriteEventData *sccb = (void *)_sccb;
>>> +    const char *p;
>>> +    size_t data_len = 0;
>>>  
>>>      if (fd != 1 && fd != 2) {
>>>          return -EIO;
>>>      }
>>>  
>>> -    sccb->h.length = sizeof(WriteEventData) + len;
>>> +    for (p = str; *p; ++p) {
>>> +        if (data_len > SCCB_DATA_LEN - 1) {
>>> +            return -EFBIG;
>>> +        }
>>> +        if (*p == '\n') {
>>> +            sccb->data[data_len++] = '\r';
>>> +        }
>>> +        sccb->data[data_len++] = *p;
>>> +    }
>>> +
>>> +    sccb->h.length = sizeof(WriteEventData) + data_len;
>>
>> This subtly changes the semantics of the write() function from an
>> explicitly passed in "len" argument to NULL termination determined
>> sizing, no?
>>
>> In that case, wouldn't it make sense to either remove the len argument
>> altogether or keep respecting it?
> 
> Yes, well spotted.
> The write function is used in other code (SLOF related network boot),
> so we should change it to respect the length, I think.

Something like this on top?

--- a/pc-bios/s390-ccw/sclp.c
+++ b/pc-bios/s390-ccw/sclp.c
@@ -83,7 +83,7 @@ long write(int fd, const void *str, size_t len)
         return -EIO;
     }
 
-    for (p = str; *p; ++p) {
+    for (p = str; len ; ++p, len--) {
         if (data_len > SCCB_DATA_LEN - 1) {
             return -EFBIG;
         }

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

[Alexander Graf]

On 20.10.17 13:02, Christian Borntraeger wrote:
> 
> 
> On 10/20/2017 12:41 PM, Christian Borntraeger wrote:
> [...]
>>>> @@ -76,17 +76,28 @@ static int _strlen(const char *str)
>>>>  long write(int fd, const void *str, size_t len)
>>>>  {
>>>>      WriteEventData *sccb = (void *)_sccb;
>>>> +    const char *p;
>>>> +    size_t data_len = 0;
>>>>  
>>>>      if (fd != 1 && fd != 2) {
>>>>          return -EIO;
>>>>      }
>>>>  
>>>> -    sccb->h.length = sizeof(WriteEventData) + len;
>>>> +    for (p = str; *p; ++p) {
>>>> +        if (data_len > SCCB_DATA_LEN - 1) {
>>>> +            return -EFBIG;
>>>> +        }
>>>> +        if (*p == '\n') {
>>>> +            sccb->data[data_len++] = '\r';
>>>> +        }
>>>> +        sccb->data[data_len++] = *p;
>>>> +    }
>>>> +
>>>> +    sccb->h.length = sizeof(WriteEventData) + data_len;
>>>
>>> This subtly changes the semantics of the write() function from an
>>> explicitly passed in "len" argument to NULL termination determined
>>> sizing, no?
>>>
>>> In that case, wouldn't it make sense to either remove the len argument
>>> altogether or keep respecting it?
>>
>> Yes, well spotted.
>> The write function is used in other code (SLOF related network boot),
>> so we should change it to respect the length, I think.
> 
> Something like this on top?
> 

I think that basically gets you back to the original semantics. I'm not
terribly thrilled about the readability of the function though, but
that's your call :)


Alex

> --- a/pc-bios/s390-ccw/sclp.c
> +++ b/pc-bios/s390-ccw/sclp.c
> @@ -83,7 +83,7 @@ long write(int fd, const void *str, size_t len)
>          return -EIO;
>      }
>  
> -    for (p = str; *p; ++p) {
> +    for (p = str; len ; ++p, len--) {
>          if (data_len > SCCB_DATA_LEN - 1) {
>              return -EFBIG;
>          }
> 
> 

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

[Christian Borntraeger]

On 10/20/2017 01:09 PM, Alexander Graf wrote:
> 
> 
> On 20.10.17 13:02, Christian Borntraeger wrote:
>>
>>
>> On 10/20/2017 12:41 PM, Christian Borntraeger wrote:
>> [...]
>>>>> @@ -76,17 +76,28 @@ static int _strlen(const char *str)
>>>>>  long write(int fd, const void *str, size_t len)
>>>>>  {
>>>>>      WriteEventData *sccb = (void *)_sccb;
>>>>> +    const char *p;
>>>>> +    size_t data_len = 0;
>>>>>  
>>>>>      if (fd != 1 && fd != 2) {
>>>>>          return -EIO;
>>>>>      }
>>>>>  
>>>>> -    sccb->h.length = sizeof(WriteEventData) + len;
>>>>> +    for (p = str; *p; ++p) {
>>>>> +        if (data_len > SCCB_DATA_LEN - 1) {
>>>>> +            return -EFBIG;
>>>>> +        }
>>>>> +        if (*p == '\n') {
>>>>> +            sccb->data[data_len++] = '\r';
>>>>> +        }
>>>>> +        sccb->data[data_len++] = *p;
>>>>> +    }
>>>>> +
>>>>> +    sccb->h.length = sizeof(WriteEventData) + data_len;
>>>>
>>>> This subtly changes the semantics of the write() function from an
>>>> explicitly passed in "len" argument to NULL termination determined
>>>> sizing, no?
>>>>
>>>> In that case, wouldn't it make sense to either remove the len argument
>>>> altogether or keep respecting it?
>>>
>>> Yes, well spotted.
>>> The write function is used in other code (SLOF related network boot),
>>> so we should change it to respect the length, I think.
>>
>> Something like this on top?
>>
> 
> I think that basically gets you back to the original semantics. I'm not
> terribly thrilled about the readability of the function though, but
> that's your call :)

In the end I want to refactor the whole thing. we have write and sclp_print.
So there is certainly room for improvement. With softfreeze approaching
this seems like the minimal fix. I will respin if Conny is ok with this
approach.
> 
> 
> Alex
> 
>> --- a/pc-bios/s390-ccw/sclp.c
>> +++ b/pc-bios/s390-ccw/sclp.c
>> @@ -83,7 +83,7 @@ long write(int fd, const void *str, size_t len)
>>          return -EIO;
>>      }
>>  
>> -    for (p = str; *p; ++p) {
>> +    for (p = str; len ; ++p, len--) {
>>          if (data_len > SCCB_DATA_LEN - 1) {
>>              return -EFBIG;
>>          }
>>
>>
> 

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

[Thomas Huth]

On 20.10.2017 13:02, Christian Borntraeger wrote:
> 
> 
> On 10/20/2017 12:41 PM, Christian Borntraeger wrote:
> [...]
>>>> @@ -76,17 +76,28 @@ static int _strlen(const char *str)
>>>>  long write(int fd, const void *str, size_t len)
>>>>  {
>>>>      WriteEventData *sccb = (void *)_sccb;
>>>> +    const char *p;
>>>> +    size_t data_len = 0;
>>>>  
>>>>      if (fd != 1 && fd != 2) {
>>>>          return -EIO;
>>>>      }
>>>>  
>>>> -    sccb->h.length = sizeof(WriteEventData) + len;
>>>> +    for (p = str; *p; ++p) {
>>>> +        if (data_len > SCCB_DATA_LEN - 1) {
>>>> +            return -EFBIG;
>>>> +        }
>>>> +        if (*p == '\n') {
>>>> +            sccb->data[data_len++] = '\r';
>>>> +        }
>>>> +        sccb->data[data_len++] = *p;
>>>> +    }
>>>> +
>>>> +    sccb->h.length = sizeof(WriteEventData) + data_len;
>>>
>>> This subtly changes the semantics of the write() function from an
>>> explicitly passed in "len" argument to NULL termination determined
>>> sizing, no?
>>>
>>> In that case, wouldn't it make sense to either remove the len argument
>>> altogether or keep respecting it?
>>
>> Yes, well spotted.
>> The write function is used in other code (SLOF related network boot),
>> so we should change it to respect the length, I think.
> 
> Something like this on top?
> 
> --- a/pc-bios/s390-ccw/sclp.c
> +++ b/pc-bios/s390-ccw/sclp.c
> @@ -83,7 +83,7 @@ long write(int fd, const void *str, size_t len)
>          return -EIO;
>      }
>  
> -    for (p = str; *p; ++p) {
> +    for (p = str; len ; ++p, len--) {

I'd maybe rather use "len > 0" as end condition, but that's just
cosmetics. Anyway, patch looks fine to me with one of these changes.
And for what it's worth, SLOF is doing a similar '\n' -> '\r\n'
convertion in its write function (see e.g.
https://github.com/aik/SLOF/blob/master/llfw/clib/iolib.c), so I think
this is the right way to go here, too.

 Thomas

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

[Cornelia Huck]

On Fri, 20 Oct 2017 12:25:17 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: "Collin L. Walling" <walling@linux.vnet.ibm.com>
> 
> 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/s390-ccw.h |  3 +++
>  pc-bios/s390-ccw/sclp.c     | 17 ++++++++++++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)

I'd defer that to a second pullreq before softfreeze where we can
collect the stragglers (currently building a pullreq). And I'd also
like to delegate that second pullreq to you, as I'll be busy/offline
after KVM Forum.

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

[Halil Pasic]

On 10/20/2017 12:25 PM, Christian Borntraeger wrote:
> From: "Collin L. Walling" <walling@linux.vnet.ibm.com>
> 
> 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
> 

The check for buffer overflow was not there previously, or?

Maybe integrate that in the commit message too.

> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  pc-bios/s390-ccw/s390-ccw.h |  3 +++
>  pc-bios/s390-ccw/sclp.c     | 17 ++++++++++++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 25d4d21..a8bd204 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -33,6 +33,9 @@ typedef unsigned long long __u64;
>  #ifndef EBUSY
>  #define EBUSY   2
>  #endif
> +#ifndef EFBIG
> +#define EFBIG   3
> +#endif
>  #ifndef NULL
>  #define NULL    0
>  #endif
> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
> index b1fc8ff..4795259 100644
> --- a/pc-bios/s390-ccw/sclp.c
> +++ b/pc-bios/s390-ccw/sclp.c
> @@ -76,17 +76,28 @@ static int _strlen(const char *str)
>  long write(int fd, const void *str, size_t len)
>  {
>      WriteEventData *sccb = (void *)_sccb;
> +    const char *p;
> +    size_t data_len = 0;
> 
>      if (fd != 1 && fd != 2) {
>          return -EIO;
>      }
> 
> -    sccb->h.length = sizeof(WriteEventData) + len;
> +    for (p = str; *p; ++p) {
> +        if (data_len > SCCB_DATA_LEN - 1) {
> +            return -EFBIG;
> +        }

We could also do a partial write or do more that one
sclp_service_call calls.

I don't think EFBIG is entirely correct here. From the man page:
"""
       EFBIG  An attempt was made to write a file that exceeds the implementa-
              tion-defined maximum file size or the process’s file size limit,
              or to write at a position past the maximum allowed offset.
"""

That's not what we have here IMHO.

> +        if (*p == '\n') {
> +            sccb->data[data_len++] = '\r';
> +        }
> +        sccb->data[data_len++] = *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);
> 

Otherwise LGTM

Halil

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

[Christian Borntraeger]

On 10/20/2017 01:31 PM, Cornelia Huck wrote:
> On Fri, 20 Oct 2017 12:25:17 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> From: "Collin L. Walling" <walling@linux.vnet.ibm.com>
>>
>> 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/s390-ccw.h |  3 +++
>>  pc-bios/s390-ccw/sclp.c     | 17 ++++++++++++++---
>>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> I'd defer that to a second pullreq before softfreeze where we can
> collect the stragglers (currently building a pullreq). And I'd also
> like to delegate that second pullreq to you, as I'll be busy/offline
> after KVM Forum.

Sure I can do that in a 2nd pull req after KVM forum.

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

[Cornelia Huck]

On Fri, 20 Oct 2017 14:27:12 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 10/20/2017 01:31 PM, Cornelia Huck wrote:
> > On Fri, 20 Oct 2017 12:25:17 +0200
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> From: "Collin L. Walling" <walling@linux.vnet.ibm.com>
> >>
> >> 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/s390-ccw.h |  3 +++
> >>  pc-bios/s390-ccw/sclp.c     | 17 ++++++++++++++---
> >>  2 files changed, 17 insertions(+), 3 deletions(-)  
> > 
> > I'd defer that to a second pullreq before softfreeze where we can
> > collect the stragglers (currently building a pullreq). And I'd also
> > like to delegate that second pullreq to you, as I'll be busy/offline
> > after KVM Forum.  
> 
> Sure I can do that in a 2nd pull req after KVM forum.
> 

Cool, thx.

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

[Farhan Ali]

On 10/20/2017 07:02 AM, Christian Borntraeger wrote:
> --- a/pc-bios/s390-ccw/sclp.c
> +++ b/pc-bios/s390-ccw/sclp.c
> @@ -83,7 +83,7 @@ long write(int fd, const void *str, size_t len)
>          return -EIO;
>      }
>
> -    for (p = str; *p; ++p) {
> +    for (p = str; len ; ++p, len--) {
>          if (data_len > SCCB_DATA_LEN - 1) {
>              return -EFBIG;
>          }
>
>
>
The write function returns len, wouldn't this change make write return 0?

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

[Christian Borntraeger]

Collin,

can you take care of the comments and send out a new version?

On 10/20/2017 03:48 PM, Farhan Ali wrote:
> 
> 
> On 10/20/2017 07:02 AM, Christian Borntraeger wrote:
>> --- a/pc-bios/s390-ccw/sclp.c
>> +++ b/pc-bios/s390-ccw/sclp.c
>> @@ -83,7 +83,7 @@ long write(int fd, const void *str, size_t len)
>>          return -EIO;
>>      }
>>
>> -    for (p = str; *p; ++p) {
>> +    for (p = str; len ; ++p, len--) {
>>          if (data_len > SCCB_DATA_LEN - 1) {
>>              return -EFBIG;
>>          }
>>
>>
>>
> The write function returns len, wouldn't this change make write return 0?

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

[Collin L. Walling]

On 10/20/2017 07:37 AM, Halil Pasic wrote:
>
> On 10/20/2017 12:25 PM, Christian Borntraeger wrote:
>> From: "Collin L. Walling" <walling@linux.vnet.ibm.com>
>>
>> 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
>>
> The check for buffer overflow was not there previously, or?
>
> Maybe integrate that in the commit message too.


Good point.


>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>   pc-bios/s390-ccw/s390-ccw.h |  3 +++
>>   pc-bios/s390-ccw/sclp.c     | 17 ++++++++++++++---
>>   2 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index 25d4d21..a8bd204 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -33,6 +33,9 @@ typedef unsigned long long __u64;
>>   #ifndef EBUSY
>>   #define EBUSY   2
>>   #endif
>> +#ifndef EFBIG
>> +#define EFBIG   3
>> +#endif
>>   #ifndef NULL
>>   #define NULL    0
>>   #endif
>> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
>> index b1fc8ff..4795259 100644
>> --- a/pc-bios/s390-ccw/sclp.c
>> +++ b/pc-bios/s390-ccw/sclp.c
>> @@ -76,17 +76,28 @@ static int _strlen(const char *str)
>>   long write(int fd, const void *str, size_t len)
>>   {
>>       WriteEventData *sccb = (void *)_sccb;
>> +    const char *p;
>> +    size_t data_len = 0;
>>
>>       if (fd != 1 && fd != 2) {
>>           return -EIO;
>>       }
>>
>> -    sccb->h.length = sizeof(WriteEventData) + len;
>> +    for (p = str; *p; ++p) {
>> +        if (data_len > SCCB_DATA_LEN - 1) {
>> +            return -EFBIG;
>> +        }
> We could also do a partial write or do more that one
> sclp_service_call calls.
>
> I don't think EFBIG is entirely correct here. From the man page:
> """
>         EFBIG  An attempt was made to write a file that exceeds the implementa-
>                tion-defined maximum file size or the process’s file size limit,
>                or to write at a position past the maximum allowed offset.
> """
>
> That's not what we have here IMHO.


 From my perspective, the error code was a tie between EFBIG (consider
max sccb size as the maximum allowed offset) and ENOSPC:


"""
ENOSPC The device containing the file referred to by /fd/has no room
for the data.
"""


(consider "the file" as the sccb data buffer)

However, I extremely doubt we'll ever encounter an overflow from
printing during the bios (why would we print something that large?)
... perhaps the check is redundant?


>
>> +        if (*p == '\n') {
>> +            sccb->data[data_len++] = '\r';
>> +        }
>> +        sccb->data[data_len++] = *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);
>>
> Otherwise LGTM
>
> Halil
>
>

-- 
- Collin L Walling

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

[Halil Pasic]

On 10/25/2017 09:49 PM, Collin L. Walling wrote:
>>> -    sccb->h.length = sizeof(WriteEventData) + len;
>>> +    for (p = str; *p; ++p) {
>>> +        if (data_len > SCCB_DATA_LEN - 1) {
>>> +            return -EFBIG;
>>> +        }
>> We could also do a partial write or do more that one
>> sclp_service_call calls.
>>
>> I don't think EFBIG is entirely correct here. From the man page:
>> """
>>         EFBIG  An attempt was made to write a file that exceeds the implementa-
>>                tion-defined maximum file size or the process’s file size limit,
>>                or to write at a position past the maximum allowed offset.
>> """
>>
>> That's not what we have here IMHO.
> 
> 
> From my perspective, the error code was a tie between EFBIG (consider
> max sccb size as the maximum allowed offset) and ENOSPC:
> 
> 
> """
> ENOSPC The device containing the file referred to by /fd/has no room
> for the data.
> """
> 

From where I stand, the file behind the fd is the VT220 terminal you
talk to via sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb) -- you
can check the AR).

In the end, the target scope (in sense of expected client
code) of this function very limited, and AFAIU does not necessarily
care about error codes (see sclp_print). So basically anything is fine
with me, because it does not really matter -- but just because it does
not really matter.

Halil
> 
> (consider "the file" as the sccb data buffer)
> 
> However, I extremely doubt we'll ever encounter an overflow from
> printing during the bios (why would we print something that large?)
> ... perhaps the check is redundant?