* [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines @ 2017-10-20 10:25 Christian Borntraeger 2017-10-20 10:30 ` Alexander Graf ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Christian Borntraeger @ 2017-10-20 10:25 UTC (permalink / raw) To: Cornelia Huck Cc: qemu-devel, Alexander Graf, Thomas Huth, David Hildenbrand, Richard Henderson, Bjoern Walk, Pierre Morel, Halil Pasic, Jason J . Herne, Collin L. Walling, Christian Borntraeger 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; 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); -- 2.9.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines 2017-10-20 10:25 [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines Christian Borntraeger @ 2017-10-20 10:30 ` Alexander Graf 2017-10-20 10:41 ` Christian Borntraeger 2017-10-20 11:31 ` Cornelia Huck 2017-10-20 11:37 ` Halil Pasic 2 siblings, 1 reply; 15+ messages in thread From: Alexander Graf @ 2017-10-20 10:30 UTC (permalink / raw) To: Christian Borntraeger, Cornelia Huck Cc: qemu-devel, Thomas Huth, David Hildenbrand, Richard Henderson, Bjoern Walk, Pierre Morel, Halil Pasic, Jason J . Herne, Collin L. Walling 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); > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines 2017-10-20 10:30 ` Alexander Graf @ 2017-10-20 10:41 ` Christian Borntraeger 2017-10-20 11:02 ` Christian Borntraeger 0 siblings, 1 reply; 15+ messages in thread From: Christian Borntraeger @ 2017-10-20 10:41 UTC (permalink / raw) To: Alexander Graf, Cornelia Huck Cc: qemu-devel, Thomas Huth, David Hildenbrand, Richard Henderson, Bjoern Walk, Pierre Morel, Halil Pasic, Jason J . Herne, Collin L. Walling 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); >> >> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines 2017-10-20 10:41 ` Christian Borntraeger @ 2017-10-20 11:02 ` Christian Borntraeger 2017-10-20 11:09 ` Alexander Graf ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Christian Borntraeger @ 2017-10-20 11:02 UTC (permalink / raw) To: Alexander Graf, Cornelia Huck Cc: qemu-devel, Thomas Huth, David Hildenbrand, Richard Henderson, Bjoern Walk, Pierre Morel, Halil Pasic, Jason J . Herne, Collin L. Walling 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--) { if (data_len > SCCB_DATA_LEN - 1) { return -EFBIG; } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines 2017-10-20 11:02 ` Christian Borntraeger @ 2017-10-20 11:09 ` Alexander Graf 2017-10-20 11:18 ` Christian Borntraeger 2017-10-20 11:28 ` Thomas Huth 2017-10-20 13:48 ` Farhan Ali 2 siblings, 1 reply; 15+ messages in thread From: Alexander Graf @ 2017-10-20 11:09 UTC (permalink / raw) To: Christian Borntraeger, Cornelia Huck Cc: qemu-devel, Thomas Huth, David Hildenbrand, Richard Henderson, Bjoern Walk, Pierre Morel, Halil Pasic, Jason J . Herne, Collin L. Walling 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; > } > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines 2017-10-20 11:09 ` Alexander Graf @ 2017-10-20 11:18 ` Christian Borntraeger 0 siblings, 0 replies; 15+ messages in thread From: Christian Borntraeger @ 2017-10-20 11:18 UTC (permalink / raw) To: Alexander Graf, Cornelia Huck Cc: qemu-devel, Thomas Huth, David Hildenbrand, Richard Henderson, Bjoern Walk, Pierre Morel, Halil Pasic, Jason J . Herne, Collin L. Walling 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; >> } >> >> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines 2017-10-20 11:02 ` Christian Borntraeger 2017-10-20 11:09 ` Alexander Graf @ 2017-10-20 11:28 ` Thomas Huth 2017-10-20 13:48 ` Farhan Ali 2 siblings, 0 replies; 15+ messages in thread From: Thomas Huth @ 2017-10-20 11:28 UTC (permalink / raw) To: Christian Borntraeger, Alexander Graf, Cornelia Huck Cc: qemu-devel, David Hildenbrand, Richard Henderson, Bjoern Walk, Pierre Morel, Halil Pasic, Jason J . Herne, Collin L. Walling 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines 2017-10-20 11:02 ` Christian Borntraeger 2017-10-20 11:09 ` Alexander Graf 2017-10-20 11:28 ` Thomas Huth @ 2017-10-20 13:48 ` Farhan Ali 2017-10-25 5:55 ` Christian Borntraeger 2 siblings, 1 reply; 15+ messages in thread From: Farhan Ali @ 2017-10-20 13:48 UTC (permalink / raw) To: Christian Borntraeger, Alexander Graf, Cornelia Huck Cc: Thomas Huth, Bjoern Walk, David Hildenbrand, Pierre Morel, qemu-devel, Halil Pasic, Jason J . Herne, Collin L. Walling, Richard Henderson 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? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines 2017-10-20 13:48 ` Farhan Ali @ 2017-10-25 5:55 ` Christian Borntraeger 0 siblings, 0 replies; 15+ messages in thread From: Christian Borntraeger @ 2017-10-25 5:55 UTC (permalink / raw) To: Farhan Ali, Alexander Graf, Cornelia Huck Cc: Thomas Huth, Bjoern Walk, David Hildenbrand, Pierre Morel, qemu-devel, Halil Pasic, Jason J . Herne, Collin L. Walling, Richard Henderson 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? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines 2017-10-20 10:25 [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines Christian Borntraeger 2017-10-20 10:30 ` Alexander Graf @ 2017-10-20 11:31 ` Cornelia Huck 2017-10-20 12:27 ` Christian Borntraeger 2017-10-20 11:37 ` Halil Pasic 2 siblings, 1 reply; 15+ messages in thread From: Cornelia Huck @ 2017-10-20 11:31 UTC (permalink / raw) To: Christian Borntraeger Cc: qemu-devel, Alexander Graf, Thomas Huth, David Hildenbrand, Richard Henderson, Bjoern Walk, Pierre Morel, Halil Pasic, Jason J . Herne, Collin L. Walling 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines 2017-10-20 11:31 ` Cornelia Huck @ 2017-10-20 12:27 ` Christian Borntraeger 2017-10-20 12:41 ` Cornelia Huck 0 siblings, 1 reply; 15+ messages in thread From: Christian Borntraeger @ 2017-10-20 12:27 UTC (permalink / raw) To: Cornelia Huck Cc: qemu-devel, Alexander Graf, Thomas Huth, David Hildenbrand, Richard Henderson, Bjoern Walk, Pierre Morel, Halil Pasic, Jason J . Herne, Collin L. Walling 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines 2017-10-20 12:27 ` Christian Borntraeger @ 2017-10-20 12:41 ` Cornelia Huck 0 siblings, 0 replies; 15+ messages in thread From: Cornelia Huck @ 2017-10-20 12:41 UTC (permalink / raw) To: Christian Borntraeger Cc: qemu-devel, Alexander Graf, Thomas Huth, David Hildenbrand, Richard Henderson, Bjoern Walk, Pierre Morel, Halil Pasic, Jason J . Herne, Collin L. Walling 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines 2017-10-20 10:25 [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines Christian Borntraeger 2017-10-20 10:30 ` Alexander Graf 2017-10-20 11:31 ` Cornelia Huck @ 2017-10-20 11:37 ` Halil Pasic 2017-10-25 19:49 ` Collin L. Walling 2 siblings, 1 reply; 15+ messages in thread From: Halil Pasic @ 2017-10-20 11:37 UTC (permalink / raw) To: Christian Borntraeger, Cornelia Huck Cc: Thomas Huth, Bjoern Walk, David Hildenbrand, Pierre Morel, qemu-devel, Alexander Graf, Jason J . Herne, Collin L. Walling, Richard Henderson 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines 2017-10-20 11:37 ` Halil Pasic @ 2017-10-25 19:49 ` Collin L. Walling 2017-10-25 22:24 ` Halil Pasic 0 siblings, 1 reply; 15+ messages in thread From: Collin L. Walling @ 2017-10-25 19:49 UTC (permalink / raw) To: Halil Pasic, Christian Borntraeger, Cornelia Huck Cc: Thomas Huth, Bjoern Walk, David Hildenbrand, Pierre Morel, qemu-devel, Alexander Graf, Jason J . Herne, Richard Henderson 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 > > -- - Collin L Walling ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines 2017-10-25 19:49 ` Collin L. Walling @ 2017-10-25 22:24 ` Halil Pasic 0 siblings, 0 replies; 15+ messages in thread From: Halil Pasic @ 2017-10-25 22:24 UTC (permalink / raw) To: Collin L. Walling, Christian Borntraeger, Cornelia Huck Cc: Thomas Huth, Bjoern Walk, David Hildenbrand, Pierre Morel, qemu-devel, Alexander Graf, Jason J . Herne, Richard Henderson 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? ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-10-25 22:24 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-20 10:25 [Qemu-devel] [PATCH] s390-ccw: print carriage return with new lines Christian Borntraeger 2017-10-20 10:30 ` Alexander Graf 2017-10-20 10:41 ` Christian Borntraeger 2017-10-20 11:02 ` Christian Borntraeger 2017-10-20 11:09 ` Alexander Graf 2017-10-20 11:18 ` Christian Borntraeger 2017-10-20 11:28 ` Thomas Huth 2017-10-20 13:48 ` Farhan Ali 2017-10-25 5:55 ` Christian Borntraeger 2017-10-20 11:31 ` Cornelia Huck 2017-10-20 12:27 ` Christian Borntraeger 2017-10-20 12:41 ` Cornelia Huck 2017-10-20 11:37 ` Halil Pasic 2017-10-25 19:49 ` Collin L. Walling 2017-10-25 22:24 ` Halil Pasic
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.