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