s390-ccw: print carriage return with new lines

Commit Message

Christian Borntraeger Oct. 20, 2017, 11:02 a.m. UTC

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?

Comments

Alexander Graf Oct. 20, 2017, 11:09 a.m. UTC | #1

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

Christian Borntraeger Oct. 20, 2017, 11:18 a.m. UTC | #2

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

Thomas Huth Oct. 20, 2017, 11:28 a.m. UTC | #3

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

Farhan Ali Oct. 20, 2017, 1:48 p.m. UTC | #4

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?

Christian Borntraeger Oct. 25, 2017, 5:55 a.m. UTC | #5

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?

Patch

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