| Message ID | 1509114613-10601-1-git-send-email-walling@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/27/2017 04:30 PM, Collin L. Walling wrote: > 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> FWIW, please remove my signed-off-by when resending.
On 10/27/2017 04:30 PM, Collin L. Walling wrote: > 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/sclp.c | 24 +++++++++++++++++++++--- > 1 file changed, 21 insertions(+), 3 deletions(-) > > diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c > index 486fce1..a57006e 100644 > --- a/pc-bios/s390-ccw/sclp.c > +++ b/pc-bios/s390-ccw/sclp.c > @@ -68,17 +68,35 @@ void sclp_setup(void) > long write(int fd, const void *str, size_t len) > { > WriteEventData *sccb = (void *)_sccb; > + const char *p = str; > + size_t data_len = 0; > + size_t i; > > if (fd != 1 && fd != 2) { > return -EIO; > } > > - sccb->h.length = sizeof(WriteEventData) + len; > + for (i = len; i > 0; i--) > + if (data_len + 1 >= SCCB_DATA_LEN) { > + /* We would overflow the sccb buffer, abort early */ > + len = i; This is not correct. Write is supposed to return the number of bytes written. In this case the number of the bytes of the original string which have been processed (so the client code can resume at that place, using the nuber of bytes transferred via sclp_service_call would be wrong if at least one \n was processed). Here you return the number of the bytes remaining, as i goes from len to 0 and not the other way around. By the way Alex's version was correct. > + break; > + } > + > + if (*p == '\n') { > + /* Terminal emulators might need \r\n, so generate it */ > + sccb->data[data_len++] = '\r'; > + } > + > + sccb->data[data_len++] = *p; > + 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); > I would have wrote the loop part like this: - sccb->h.length = sizeof(WriteEventData) + len; + for (i = 0; i < len; ++i) { + if (data_len + 1 >= SCCB_DATA_LEN) { + /* We would overflow the sccb buffer, abort early */ + len = i; + break; + } + + if (str[i] == '\n') { + /* Terminal emulators might need \r\n, so generate it */ + sccb->data[data_len++] = '\r'; + } + + sccb->data[data_len++] = str[i]; + } + + sccb->h.length = sizeof(WriteEventData) + data_len; This way you don't need p, and the loop steps trough the str indexed by i while sccb->data is indexed by data_len. data_len is incremented each time after we have written to the buffer (that's why postfix ++) and i is incremented on each iteration (that's why normal prefix ++). Btw I would also rename i to str_i and data_len to data_i to better reflect this. The cosmetics aren't important though, so it's probably better to go with what we have already discussed as Alex's version. Regards, Halil
On 10/27/2017 11:39 AM, Halil Pasic wrote: > > On 10/27/2017 04:30 PM, Collin L. Walling wrote: >> 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/sclp.c | 24 +++++++++++++++++++++--- >> 1 file changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c >> index 486fce1..a57006e 100644 >> --- a/pc-bios/s390-ccw/sclp.c >> +++ b/pc-bios/s390-ccw/sclp.c >> @@ -68,17 +68,35 @@ void sclp_setup(void) >> long write(int fd, const void *str, size_t len) >> { >> WriteEventData *sccb = (void *)_sccb; >> + const char *p = str; >> + size_t data_len = 0; >> + size_t i; >> >> if (fd != 1 && fd != 2) { >> return -EIO; >> } >> >> - sccb->h.length = sizeof(WriteEventData) + len; >> + for (i = len; i > 0; i--) > + if (data_len + 1 >= SCCB_DATA_LEN) { >> + /* We would overflow the sccb buffer, abort early */ >> + len = i; > This is not correct. Write is supposed to return the number > of bytes written. In this case the number of the bytes of the > original string which have been processed (so the client > code can resume at that place, using the nuber of bytes > transferred via sclp_service_call would be wrong if at least > one \n was processed). > > Here you return the number of the bytes remaining, as i goes > from len to 0 and not the other way around. By the way Alex's > version was correct. Yikes -- thank you for catching this. I blindly copied. Will fix. > >> + break; >> + } >> + >> + if (*p == '\n') { >> + /* Terminal emulators might need \r\n, so generate it */ >> + sccb->data[data_len++] = '\r'; >> + } >> + >> + sccb->data[data_len++] = *p; >> + 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); >> > I would have wrote the loop part like this: > - sccb->h.length = sizeof(WriteEventData) + len; > + for (i = 0; i < len; ++i) { > + if (data_len + 1 >= SCCB_DATA_LEN) { > + /* We would overflow the sccb buffer, abort early */ > + len = i; > + break; > + } > + > + if (str[i] == '\n') { > + /* Terminal emulators might need \r\n, so generate it */ > + sccb->data[data_len++] = '\r'; > + } > + > + sccb->data[data_len++] = str[i]; > + } > + > + sccb->h.length = sizeof(WriteEventData) + data_len; > > This way you don't need p, and the loop steps trough the str indexed by > i while sccb->data is indexed by data_len. data_len is incremented each > time after we have written to the buffer (that's why postfix ++) > and i is incremented on each iteration (that's why normal prefix ++). > > Btw I would also rename i to str_i and data_len to data_i to better > reflect this. > > The cosmetics aren't important though, so it's probably better to go > with what we have already discussed as Alex's version. > > Regards, > Halil > > > I wanted to do the same, however str is void can cannot be indexed directly. We'd have to do some nasty casting first, so I figured having *p made things look cleaner. I'll post v4 with fixups immediately, but with more care this time.
On 10/27/2017 05:47 PM, Collin L. Walling wrote: >> > > I wanted to do the same, however str is void can cannot be indexed directly. > We'd have to do some nasty casting first, so I figured having *p made things > look cleaner. I did not consider that, sorry. (One could rename p to str AFAIR to express that we are actually scanning the parameter and that the local is just for avoiding the cast). But I really think we should go with Alex's proposal. Halil
diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c index 486fce1..a57006e 100644 --- a/pc-bios/s390-ccw/sclp.c +++ b/pc-bios/s390-ccw/sclp.c @@ -68,17 +68,35 @@ void sclp_setup(void) long write(int fd, const void *str, size_t len) { WriteEventData *sccb = (void *)_sccb; + const char *p = str; + size_t data_len = 0; + size_t i; if (fd != 1 && fd != 2) { return -EIO; } - sccb->h.length = sizeof(WriteEventData) + len; + for (i = len; i > 0; i--) { + if (data_len + 1 >= SCCB_DATA_LEN) { + /* We would overflow the sccb buffer, abort early */ + len = i; + break; + } + + if (*p == '\n') { + /* Terminal emulators might need \r\n, so generate it */ + sccb->data[data_len++] = '\r'; + } + + sccb->data[data_len++] = *p; + 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);
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/sclp.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)