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