Re: [kvm-unit-tests PATCH v3 3/6] s390x: Add linemode buffer to fix newline on every print

[Janosch Frank <frankja@linux.ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4552 bytes --]

On 9/20/19 10:24 AM, David Hildenbrand wrote:
> On 20.09.19 10:03, Janosch Frank wrote:
>> Linemode seems to add a newline for each sent message which makes
>> reading rather hard. Hence we add a small buffer and only print if
>> it's full or a newline is encountered. Except for when the string is
>> longer than the buffer, then we flush the buffer and print directly.
> 
> I think that last sentence is not correct anymore.
> 
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Acked-by: David Hildenbrand <david@redhat.com>
>> ---
>>  lib/s390x/sclp-console.c | 45 ++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 41 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c
>> index 19416b5..e1b9000 100644
>> --- a/lib/s390x/sclp-console.c
>> +++ b/lib/s390x/sclp-console.c
>> @@ -13,6 +13,7 @@
>>  #include <asm/page.h>
>>  #include <asm/arch_def.h>
>>  #include <asm/io.h>
>> +#include <asm/spinlock.h>
>>  #include "sclp.h"
>>  
>>  /*
>> @@ -87,6 +88,10 @@ static uint8_t _ascebc[256] = {
>>       0x90, 0x3F, 0x3F, 0x3F, 0x3F, 0xEA, 0x3F, 0xFF
>>  };
>>  
>> +static char lm_buff[120];
>> +static unsigned char lm_buff_off;
>> +static struct spinlock lm_buff_lock;
>> +
>>  static void sclp_print_ascii(const char *str)
>>  {
>>  	int len = strlen(str);
>> @@ -103,10 +108,10 @@ static void sclp_print_ascii(const char *str)
>>  	sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb);
>>  }
>>  
>> -static void sclp_print_lm(const char *str)
>> +static void lm_print(const char *buff, int len)
>>  {
>>  	unsigned char *ptr, *end, ch;
>> -	unsigned int count, offset, len;
>> +	unsigned int count, offset;
>>  	struct WriteEventData *sccb;
>>  	struct mdb *mdb;
>>  	struct mto *mto;
>> @@ -117,11 +122,10 @@ static void sclp_print_lm(const char *str)
>>  	end = (unsigned char *) sccb + 4096 - 1;
>>  	memset(sccb, 0, sizeof(*sccb));
>>  	ptr = (unsigned char *) &sccb->msg.mdb.mto;
>> -	len = strlen(str);
>>  	offset = 0;
>>  	do {
>>  		for (count = sizeof(*mto); offset < len; count++) {
>> -			ch = str[offset++];
>> +			ch = buff[offset++];
>>  			if (ch == 0x0a || ptr + count > end)
>>  				break;
>>  			ptr[count] = _ascebc[ch];
>> @@ -148,6 +152,39 @@ static void sclp_print_lm(const char *str)
>>  	sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb);
>>  }
>>  
>> +
>> +/*
>> + * In contrast to the ascii console, linemode produces a new
>> + * line with every write of data. The report() function uses
>> + * several printf() calls to generate a line of data which
>> + * would all end up on different lines.
>> + *
>> + * Hence we buffer here until we encounter a \n or the buffer
>> + * is full. That means that linemode output can look a bit
>> + * different from ascii and that it takes a bit longer for
>> + * lines to appear.
>> + */
>> +static void sclp_print_lm(const char *str)
>> +{
>> +	int len = strlen(str), i = 0;
>> +
>> +	spin_lock(&lm_buff_lock);
>> +
>> +	while (len) {
> 
> for (i = 0; i < len; i++)
> 
> Then make len const and drop "len--" and "i++" from the body.

Sure, the loop was a first draft anyway.
I'm asking myself how I ended up writing something so complicated in the
first place :-)

> 
>> +		lm_buff[lm_buff_off] = str[i];
>> +		lm_buff_off++;
> 
> lm_buff[lm_buff_off++] = str[i];
> 
> if you feel like saving one LOC :)
> 
>> +		len--;
>> +		/* Buffer full or newline? */
>> +		if (str[i] == '\n' || lm_buff_off == (sizeof(lm_buff) - 1)) {
> 
> I still prefer ARRAY_SIZE(), but this is also fine.
> 
>> +			lm_print(lm_buff, lm_buff_off);
>> +			memset(lm_buff, 0 , sizeof(lm_buff));
> 
> Is the memset really necessary? (I would have assumed it would only
> print until the last "\n" ?)

It's not, I just like clearing things

> 
>> +			lm_buff_off = 0;
>> +		}
>> +		i++;
>> +	}
> 
> Guess we don't care about performance, so the simple byte-based approach
> should be just fine.

;-)

> 
>> +	spin_unlock(&lm_buff_lock);
>> +}
>> +
>>  /*
>>   * SCLP needs to be initialized by setting a send and receive mask,
>>   * indicating which messages the control program (we) want(s) to
>>
> 
> 
> I wonder if we have to implement some kind of fflush(), so we will flush
> the buffer on any abort ... but I assume we will always end printing
> with a "\n", so we don't really care.

I haven't encountered incomplete output up to now.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]