| Message ID | 1509043965-5852-1-git-send-email-walling@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26.10.17 20:52, 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 | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c > index 486fce1..f8ad5ae 100644 > --- a/pc-bios/s390-ccw/sclp.c > +++ b/pc-bios/s390-ccw/sclp.c > @@ -68,17 +68,27 @@ 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--) { Where did the bounds check go? If you write(max) before, you were writing max bytes. If you do it now, you end up writing max + n bytes and potentially overflow the array, no? Alex > + if (*p == '\n') { > + 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); > >
On 10/26/2017 04:25 PM, Alexander Graf wrote: > > On 26.10.17 20:52, 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 | 16 +++++++++++++--- >> 1 file changed, 13 insertions(+), 3 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c >> index 486fce1..f8ad5ae 100644 >> --- a/pc-bios/s390-ccw/sclp.c >> +++ b/pc-bios/s390-ccw/sclp.c >> @@ -68,17 +68,27 @@ 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--) { > Where did the bounds check go? If you write(max) before, you were > writing max bytes. If you do it now, you end up writing max + n bytes > and potentially overflow the array, no? > > > Alex I wasn't a fan of the code aesthetics and being that the SCCB write buffer allows about 4k bytes of data to be written to it, I felt it was safe to remove it. It's unlikely we'd be writing that much data in the bios, plus that check did not exist prior to this fixup. Though, reading that out loud, it probably isn't the best idea to sacrifice code robustness for code aesthetics. for (i = len; i > 0; i--) { if (data_len > SCCB_DATA_LEN - 1) { return -SOME_ERROR } if (*p == '\n') { sccb->data[data_len++] = '\r'; } sccb->data[data_len++] = *p; p++; } What do you think? - Collin > >> + if (*p == '\n') { >> + 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); >> >>
On 26.10.17 22:37, Collin L. Walling wrote: > On 10/26/2017 04:25 PM, Alexander Graf wrote: >> >> On 26.10.17 20:52, 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 | 16 +++++++++++++--- >>> 1 file changed, 13 insertions(+), 3 deletions(-) >>> >>> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c >>> index 486fce1..f8ad5ae 100644 >>> --- a/pc-bios/s390-ccw/sclp.c >>> +++ b/pc-bios/s390-ccw/sclp.c >>> @@ -68,17 +68,27 @@ 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--) { >> Where did the bounds check go? If you write(max) before, you were >> writing max bytes. If you do it now, you end up writing max + n bytes >> and potentially overflow the array, no? >> >> >> Alex > > I wasn't a fan of the code aesthetics and being that the SCCB write buffer > allows about 4k bytes of data to be written to it, I felt it was safe to > remove it. It's unlikely we'd be writing that much data in the bios, plus > that check did not exist prior to this fixup. > > Though, reading that out loud, it probably isn't the best idea to sacrifice > code robustness for code aesthetics. > > for (i = len; i > 0; i--) { > if (data_len > SCCB_DATA_LEN - 1) { > return -SOME_ERROR > } > if (*p == '\n') { > sccb->data[data_len++] = '\r'; > } > sccb->data[data_len++] = *p; > p++; > } > > What do you think? Normally write() would just write less bytes than it was requested to write and tell you that in the return value. So how about 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 (*p == '\n') { /* Terminal emulators might need \r\n, so generate it */ sccb->data[data_len++] = '\r'; } sccb->data[data_len++] = *p; p++; } Alex
On 10/26/2017 04:48 PM, Alexander Graf wrote: > > On 26.10.17 22:37, Collin L. Walling wrote: >> On 10/26/2017 04:25 PM, Alexander Graf wrote: >>> On 26.10.17 20:52, 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 | 16 +++++++++++++--- >>>> 1 file changed, 13 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c >>>> index 486fce1..f8ad5ae 100644 >>>> --- a/pc-bios/s390-ccw/sclp.c >>>> +++ b/pc-bios/s390-ccw/sclp.c >>>> @@ -68,17 +68,27 @@ 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--) { >>> Where did the bounds check go? If you write(max) before, you were >>> writing max bytes. If you do it now, you end up writing max + n bytes >>> and potentially overflow the array, no? >>> >>> >>> Alex >> I wasn't a fan of the code aesthetics and being that the SCCB write buffer >> allows about 4k bytes of data to be written to it, I felt it was safe to >> remove it. It's unlikely we'd be writing that much data in the bios, plus >> that check did not exist prior to this fixup. >> >> Though, reading that out loud, it probably isn't the best idea to sacrifice >> code robustness for code aesthetics. >> >> for (i = len; i > 0; i--) { >> if (data_len > SCCB_DATA_LEN - 1) { >> return -SOME_ERROR >> } >> if (*p == '\n') { >> sccb->data[data_len++] = '\r'; >> } >> sccb->data[data_len++] = *p; >> p++; >> } >> >> What do you think? > Normally write() would just write less bytes than it was requested to > write and tell you that in the return value. So how about > > 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 (*p == '\n') { > /* Terminal emulators might need \r\n, so generate it */ > sccb->data[data_len++] = '\r'; > } > > sccb->data[data_len++] = *p; > p++; > } > > > Alex > Makes sense to me. I'll let this patch sit on the list for a little while longer before fixing up for v3 in case Imight have missed something else :) Thanks for your time, Alex.
On Thu, 26 Oct 2017 14:52:45 -0400 "Collin L. Walling" <walling@linux.vnet.ibm.com> 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> I'm a bit confused about that s-o-b chain... where does Christian come in here? [Nothing further from me about the actual code change.] > --- > pc-bios/s390-ccw/sclp.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-)
On 10/26/2017 10:54 PM, Collin L. Walling wrote: > On 10/26/2017 04:48 PM, Alexander Graf wrote: >> >> On 26.10.17 22:37, Collin L. Walling wrote: >>> On 10/26/2017 04:25 PM, Alexander Graf wrote: >>>> On 26.10.17 20:52, 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 | 16 +++++++++++++--- >>>>> 1 file changed, 13 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c >>>>> index 486fce1..f8ad5ae 100644 >>>>> --- a/pc-bios/s390-ccw/sclp.c >>>>> +++ b/pc-bios/s390-ccw/sclp.c >>>>> @@ -68,17 +68,27 @@ 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--) { >>>> Where did the bounds check go? If you write(max) before, you were >>>> writing max bytes. If you do it now, you end up writing max + n bytes >>>> and potentially overflow the array, no? >>>> >>>> >>>> Alex >>> I wasn't a fan of the code aesthetics and being that the SCCB write buffer >>> allows about 4k bytes of data to be written to it, I felt it was safe to >>> remove it. It's unlikely we'd be writing that much data in the bios, plus >>> that check did not exist prior to this fixup. >>> >>> Though, reading that out loud, it probably isn't the best idea to sacrifice >>> code robustness for code aesthetics. >>> >>> for (i = len; i > 0; i--) { >>> if (data_len > SCCB_DATA_LEN - 1) { >>> return -SOME_ERROR >>> } >>> if (*p == '\n') { >>> sccb->data[data_len++] = '\r'; >>> } >>> sccb->data[data_len++] = *p; >>> p++; >>> } >>> >>> What do you think? >> Normally write() would just write less bytes than it was requested to >> write and tell you that in the return value. So how about >> >> 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 (*p == '\n') { >> /* Terminal emulators might need \r\n, so generate it */ >> sccb->data[data_len++] = '\r'; >> } >> >> sccb->data[data_len++] = *p; >> p++; >> } >> >> >> Alex >> > Makes sense to me. I'll let this patch sit on the list for a little > while longer before fixing up for v3 in case Imight have missed > something else :) Alex version looks sane. Can you post the patch today? soft freeze is approaching soon.
diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c index 486fce1..f8ad5ae 100644 --- a/pc-bios/s390-ccw/sclp.c +++ b/pc-bios/s390-ccw/sclp.c @@ -68,17 +68,27 @@ 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 (*p == '\n') { + 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 | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)