| Message ID | 20171020102517.28385-1-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20.10.17 12:25, Christian Borntraeger wrote: > From: "Collin L. Walling" <walling@linux.vnet.ibm.com> > > 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/s390-ccw.h | 3 +++ > pc-bios/s390-ccw/sclp.c | 17 ++++++++++++++--- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h > index 25d4d21..a8bd204 100644 > --- a/pc-bios/s390-ccw/s390-ccw.h > +++ b/pc-bios/s390-ccw/s390-ccw.h > @@ -33,6 +33,9 @@ typedef unsigned long long __u64; > #ifndef EBUSY > #define EBUSY 2 > #endif > +#ifndef EFBIG > +#define EFBIG 3 > +#endif > #ifndef NULL > #define NULL 0 > #endif > diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c > index b1fc8ff..4795259 100644 > --- a/pc-bios/s390-ccw/sclp.c > +++ b/pc-bios/s390-ccw/sclp.c > @@ -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? Alex > 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/20/2017 12:30 PM, Alexander Graf wrote: > > > On 20.10.17 12:25, Christian Borntraeger wrote: >> From: "Collin L. Walling" <walling@linux.vnet.ibm.com> >> >> 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/s390-ccw.h | 3 +++ >> pc-bios/s390-ccw/sclp.c | 17 ++++++++++++++--- >> 2 files changed, 17 insertions(+), 3 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h >> index 25d4d21..a8bd204 100644 >> --- a/pc-bios/s390-ccw/s390-ccw.h >> +++ b/pc-bios/s390-ccw/s390-ccw.h >> @@ -33,6 +33,9 @@ typedef unsigned long long __u64; >> #ifndef EBUSY >> #define EBUSY 2 >> #endif >> +#ifndef EFBIG >> +#define EFBIG 3 >> +#endif >> #ifndef NULL >> #define NULL 0 >> #endif >> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c >> index b1fc8ff..4795259 100644 >> --- a/pc-bios/s390-ccw/sclp.c >> +++ b/pc-bios/s390-ccw/sclp.c >> @@ -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. > > > Alex > >> 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 Fri, 20 Oct 2017 12:25:17 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > From: "Collin L. Walling" <walling@linux.vnet.ibm.com> > > 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/s390-ccw.h | 3 +++ > pc-bios/s390-ccw/sclp.c | 17 ++++++++++++++--- > 2 files changed, 17 insertions(+), 3 deletions(-) I'd defer that to a second pullreq before softfreeze where we can collect the stragglers (currently building a pullreq). And I'd also like to delegate that second pullreq to you, as I'll be busy/offline after KVM Forum.
On 10/20/2017 12:25 PM, Christian Borntraeger wrote: > From: "Collin L. Walling" <walling@linux.vnet.ibm.com> > > 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 > The check for buffer overflow was not there previously, or? Maybe integrate that in the commit message too. > Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > pc-bios/s390-ccw/s390-ccw.h | 3 +++ > pc-bios/s390-ccw/sclp.c | 17 ++++++++++++++--- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h > index 25d4d21..a8bd204 100644 > --- a/pc-bios/s390-ccw/s390-ccw.h > +++ b/pc-bios/s390-ccw/s390-ccw.h > @@ -33,6 +33,9 @@ typedef unsigned long long __u64; > #ifndef EBUSY > #define EBUSY 2 > #endif > +#ifndef EFBIG > +#define EFBIG 3 > +#endif > #ifndef NULL > #define NULL 0 > #endif > diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c > index b1fc8ff..4795259 100644 > --- a/pc-bios/s390-ccw/sclp.c > +++ b/pc-bios/s390-ccw/sclp.c > @@ -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; > + } We could also do a partial write or do more that one sclp_service_call calls. I don't think EFBIG is entirely correct here. From the man page: """ EFBIG An attempt was made to write a file that exceeds the implementa- tion-defined maximum file size or the process’s file size limit, or to write at a position past the maximum allowed offset. """ That's not what we have here IMHO. > + if (*p == '\n') { > + sccb->data[data_len++] = '\r'; > + } > + sccb->data[data_len++] = *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); > Otherwise LGTM Halil
On 10/20/2017 01:31 PM, Cornelia Huck wrote: > On Fri, 20 Oct 2017 12:25:17 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> From: "Collin L. Walling" <walling@linux.vnet.ibm.com> >> >> 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/s390-ccw.h | 3 +++ >> pc-bios/s390-ccw/sclp.c | 17 ++++++++++++++--- >> 2 files changed, 17 insertions(+), 3 deletions(-) > > I'd defer that to a second pullreq before softfreeze where we can > collect the stragglers (currently building a pullreq). And I'd also > like to delegate that second pullreq to you, as I'll be busy/offline > after KVM Forum. Sure I can do that in a 2nd pull req after KVM forum.
On Fri, 20 Oct 2017 14:27:12 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 10/20/2017 01:31 PM, Cornelia Huck wrote: > > On Fri, 20 Oct 2017 12:25:17 +0200 > > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > >> From: "Collin L. Walling" <walling@linux.vnet.ibm.com> > >> > >> 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/s390-ccw.h | 3 +++ > >> pc-bios/s390-ccw/sclp.c | 17 ++++++++++++++--- > >> 2 files changed, 17 insertions(+), 3 deletions(-) > > > > I'd defer that to a second pullreq before softfreeze where we can > > collect the stragglers (currently building a pullreq). And I'd also > > like to delegate that second pullreq to you, as I'll be busy/offline > > after KVM Forum. > > Sure I can do that in a 2nd pull req after KVM forum. > Cool, thx.
On 10/20/2017 07:37 AM, Halil Pasic wrote: > > On 10/20/2017 12:25 PM, Christian Borntraeger wrote: >> From: "Collin L. Walling" <walling@linux.vnet.ibm.com> >> >> 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 >> > The check for buffer overflow was not there previously, or? > > Maybe integrate that in the commit message too. Good point. > >> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> pc-bios/s390-ccw/s390-ccw.h | 3 +++ >> pc-bios/s390-ccw/sclp.c | 17 ++++++++++++++--- >> 2 files changed, 17 insertions(+), 3 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h >> index 25d4d21..a8bd204 100644 >> --- a/pc-bios/s390-ccw/s390-ccw.h >> +++ b/pc-bios/s390-ccw/s390-ccw.h >> @@ -33,6 +33,9 @@ typedef unsigned long long __u64; >> #ifndef EBUSY >> #define EBUSY 2 >> #endif >> +#ifndef EFBIG >> +#define EFBIG 3 >> +#endif >> #ifndef NULL >> #define NULL 0 >> #endif >> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c >> index b1fc8ff..4795259 100644 >> --- a/pc-bios/s390-ccw/sclp.c >> +++ b/pc-bios/s390-ccw/sclp.c >> @@ -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; >> + } > We could also do a partial write or do more that one > sclp_service_call calls. > > I don't think EFBIG is entirely correct here. From the man page: > """ > EFBIG An attempt was made to write a file that exceeds the implementa- > tion-defined maximum file size or the process’s file size limit, > or to write at a position past the maximum allowed offset. > """ > > That's not what we have here IMHO. From my perspective, the error code was a tie between EFBIG (consider max sccb size as the maximum allowed offset) and ENOSPC: """ ENOSPC The device containing the file referred to by /fd/has no room for the data. """ (consider "the file" as the sccb data buffer) However, I extremely doubt we'll ever encounter an overflow from printing during the bios (why would we print something that large?) ... perhaps the check is redundant? > >> + if (*p == '\n') { >> + sccb->data[data_len++] = '\r'; >> + } >> + sccb->data[data_len++] = *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); >> > Otherwise LGTM > > Halil > >
On 10/25/2017 09:49 PM, Collin L. Walling wrote: >>> - sccb->h.length = sizeof(WriteEventData) + len; >>> + for (p = str; *p; ++p) { >>> + if (data_len > SCCB_DATA_LEN - 1) { >>> + return -EFBIG; >>> + } >> We could also do a partial write or do more that one >> sclp_service_call calls. >> >> I don't think EFBIG is entirely correct here. From the man page: >> """ >> EFBIG An attempt was made to write a file that exceeds the implementa- >> tion-defined maximum file size or the process’s file size limit, >> or to write at a position past the maximum allowed offset. >> """ >> >> That's not what we have here IMHO. > > > From my perspective, the error code was a tie between EFBIG (consider > max sccb size as the maximum allowed offset) and ENOSPC: > > > """ > ENOSPC The device containing the file referred to by /fd/has no room > for the data. > """ > From where I stand, the file behind the fd is the VT220 terminal you talk to via sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb) -- you can check the AR). In the end, the target scope (in sense of expected client code) of this function very limited, and AFAIU does not necessarily care about error codes (see sclp_print). So basically anything is fine with me, because it does not really matter -- but just because it does not really matter. Halil > > (consider "the file" as the sccb data buffer) > > However, I extremely doubt we'll ever encounter an overflow from > printing during the bios (why would we print something that large?) > ... perhaps the check is redundant?
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h index 25d4d21..a8bd204 100644 --- a/pc-bios/s390-ccw/s390-ccw.h +++ b/pc-bios/s390-ccw/s390-ccw.h @@ -33,6 +33,9 @@ typedef unsigned long long __u64; #ifndef EBUSY #define EBUSY 2 #endif +#ifndef EFBIG +#define EFBIG 3 +#endif #ifndef NULL #define NULL 0 #endif diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c index b1fc8ff..4795259 100644 --- a/pc-bios/s390-ccw/sclp.c +++ b/pc-bios/s390-ccw/sclp.c @@ -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; 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);