* [PATCH v2 1/3] console: don't count newlines twice in bytes written @ 2019-07-31 10:21 Ahmad Fatoum 2019-07-31 10:21 ` [PATCH v2 2/3] ratp: return 0 bytes written from puts if busy Ahmad Fatoum ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Ahmad Fatoum @ 2019-07-31 10:21 UTC (permalink / raw) To: barebox; +Cc: Ahmad Fatoum Both the PBL and simple console only return number of input bytes, not number of bytes actually written out. These differ, because each LF is converted to CRLF pairs. The behavior of not counting actual written out characters is more sensible, because otherwise callers interested in finding out if all bytes have been written (e.g. to avoid incomplete writes with ratp) would need to keep count of all line feeds in the string. Therefore change the normal console to behave like its less featureful brethren. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- Changes in v2: New commit. --- common/console.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/common/console.c b/common/console.c index 47ccf2e54de0..52162df23b04 100644 --- a/common/console.c +++ b/common/console.c @@ -258,10 +258,9 @@ static int __console_puts(struct console_device *cdev, const char *s) int n = 0; while (*s) { - if (*s == '\n') { + if (*s == '\n') cdev->putc(cdev, '\r'); - n++; - } + cdev->putc(cdev, *s); n++; s++; -- 2.20.1 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] ratp: return 0 bytes written from puts if busy 2019-07-31 10:21 [PATCH v2 1/3] console: don't count newlines twice in bytes written Ahmad Fatoum @ 2019-07-31 10:21 ` Ahmad Fatoum 2019-08-05 8:59 ` Sascha Hauer 2019-07-31 10:21 ` [PATCH v2 3/3] console: fix out-of-bounds read in dputc(/dev/*, ...) Ahmad Fatoum 2019-08-05 9:11 ` [PATCH v2 1/3] console: don't count newlines twice in bytes written Sascha Hauer 2 siblings, 1 reply; 11+ messages in thread From: Ahmad Fatoum @ 2019-07-31 10:21 UTC (permalink / raw) To: barebox; +Cc: Ahmad Fatoum Prior behavior was to wrongly report all bytes written if enqueueing wasn't possible at the time. Instead we should either return 0 or an error code if users need to retry. write(2) returns 0 in such cases. Follow suit. As no current users run puts in a loop, this has no effect for now. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- Changes in v2: New commit. --- common/ratp/ratp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c index 9aea1786d684..8ac7dc98b6f8 100644 --- a/common/ratp/ratp.c +++ b/common/ratp/ratp.c @@ -267,7 +267,7 @@ static int ratp_console_puts(struct console_device *cdev, const char *s) len = strlen(s); if (ratp_busy(&ctx->ratp)) - return len; + return 0; kfifo_put(ctx->console_transmit_fifo, s, len); -- 2.20.1 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] ratp: return 0 bytes written from puts if busy 2019-07-31 10:21 ` [PATCH v2 2/3] ratp: return 0 bytes written from puts if busy Ahmad Fatoum @ 2019-08-05 8:59 ` Sascha Hauer 2019-08-22 6:57 ` Ahmad Fatoum 0 siblings, 1 reply; 11+ messages in thread From: Sascha Hauer @ 2019-08-05 8:59 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox On Wed, Jul 31, 2019 at 12:21:42PM +0200, Ahmad Fatoum wrote: > Prior behavior was to wrongly report all bytes written if enqueueing wasn't > possible at the time. Instead we should either return 0 or an error code if > users need to retry. write(2) returns 0 in such cases. Follow suit. > > As no current users run puts in a loop, this has no effect for now. > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > --- > Changes in v2: > New commit. > --- > common/ratp/ratp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c > index 9aea1786d684..8ac7dc98b6f8 100644 > --- a/common/ratp/ratp.c > +++ b/common/ratp/ratp.c > @@ -267,7 +267,7 @@ static int ratp_console_puts(struct console_device *cdev, const char *s) > len = strlen(s); > > if (ratp_busy(&ctx->ratp)) > - return len; > + return 0; I'm not sure if this return value is ever used for something useful, not sure how relevant this is. ratp_busy() however returns true when it's called from inside the ratp code. This is necessary so that we don't get stuck in an endless loop. If we start returning 0 for "no characters sent" how should code evaluating this return value react? Retrying it until all characters are sent obviously is not an option. I think the current behaviour of just returning 'len' is correct. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] ratp: return 0 bytes written from puts if busy 2019-08-05 8:59 ` Sascha Hauer @ 2019-08-22 6:57 ` Ahmad Fatoum 0 siblings, 0 replies; 11+ messages in thread From: Ahmad Fatoum @ 2019-08-22 6:57 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox On 8/5/19 10:59 AM, Sascha Hauer wrote: > On Wed, Jul 31, 2019 at 12:21:42PM +0200, Ahmad Fatoum wrote: >> Prior behavior was to wrongly report all bytes written if enqueueing wasn't >> possible at the time. Instead we should either return 0 or an error code if >> users need to retry. write(2) returns 0 in such cases. Follow suit. >> >> As no current users run puts in a loop, this has no effect for now. >> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> >> --- >> Changes in v2: >> New commit. >> --- >> common/ratp/ratp.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c >> index 9aea1786d684..8ac7dc98b6f8 100644 >> --- a/common/ratp/ratp.c >> +++ b/common/ratp/ratp.c >> @@ -267,7 +267,7 @@ static int ratp_console_puts(struct console_device *cdev, const char *s) >> len = strlen(s); >> >> if (ratp_busy(&ctx->ratp)) >> - return len; >> + return 0; > > I'm not sure if this return value is ever used for something useful, > not sure how relevant this is. ratp_busy() however returns true when > it's called from inside the ratp code. This is necessary so that we > don't get stuck in an endless loop. If we start returning 0 for > "no characters sent" how should code evaluating this return value > react? Retrying it until all characters are sent obviously is not an > option. > > I think the current behaviour of just returning 'len' is correct. I see. I will drop this patch then. > > Sascha > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] console: fix out-of-bounds read in dputc(/dev/*, ...) 2019-07-31 10:21 [PATCH v2 1/3] console: don't count newlines twice in bytes written Ahmad Fatoum 2019-07-31 10:21 ` [PATCH v2 2/3] ratp: return 0 bytes written from puts if busy Ahmad Fatoum @ 2019-07-31 10:21 ` Ahmad Fatoum 2019-08-22 7:06 ` Ahmad Fatoum 2019-08-05 9:11 ` [PATCH v2 1/3] console: don't count newlines twice in bytes written Sascha Hauer 2 siblings, 1 reply; 11+ messages in thread From: Ahmad Fatoum @ 2019-07-31 10:21 UTC (permalink / raw) To: barebox; +Cc: Ahmad Fatoum, Bastian Krause Trying to output a single character via echo -a /dev/serial0-1 currently results in garbage output after the newline, because console.c's fops_write discards the buffer length and passes the buffer to (struct cdev)::puts which only handles NUL-terminated strings. Fix this by amending (struct cdev)::puts with a new nbytes parameter, which is correctly propagated. Fixes: b4f55fcf35 ("console: expose consoles in devfs") Cc: Bastian Krause <bst@pengutronix.de> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- Changes in v2: - fixed commit hash in Fixes:, by Bastian - fixed possible incomplete writes in __console_puts - rebased on top of two potential bug fixes --- common/console.c | 14 +++++++------- common/ratp/ratp.c | 10 ++++------ drivers/serial/efi-stdio.c | 5 +++-- drivers/serial/serial_efi.c | 5 +++-- fs/pstore/platform.c | 7 ++++--- include/console.h | 2 +- 6 files changed, 22 insertions(+), 21 deletions(-) diff --git a/common/console.c b/common/console.c index 52162df23b04..ce4122fa76f1 100644 --- a/common/console.c +++ b/common/console.c @@ -253,19 +253,19 @@ static void console_set_stdoutpath(struct console_device *cdev) free(str); } -static int __console_puts(struct console_device *cdev, const char *s) +static int __console_puts(struct console_device *cdev, const char *s, + size_t nbytes) { - int n = 0; + size_t i; - while (*s) { + for (i = 0; i < nbytes; i++) { if (*s == '\n') cdev->putc(cdev, '\r'); cdev->putc(cdev, *s); - n++; s++; } - return n; + return i; } static int fops_open(struct cdev *cdev, unsigned long flags) @@ -297,7 +297,7 @@ static ssize_t fops_write(struct cdev* dev, const void* buf, size_t count, { struct console_device *priv = dev->priv; - priv->puts(priv, buf); + priv->puts(priv, buf, count); return 0; } @@ -544,7 +544,7 @@ int console_puts(unsigned int ch, const char *str) if (initialized == CONSOLE_INIT_FULL) { for_each_console(cdev) { if (cdev->f_active & ch) { - n = cdev->puts(cdev, str); + n = cdev->puts(cdev, str, strlen(str)); } } return n; diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c index 8ac7dc98b6f8..8f989515e485 100644 --- a/common/ratp/ratp.c +++ b/common/ratp/ratp.c @@ -259,19 +259,17 @@ static int ratp_console_tstc(struct console_device *cdev) return kfifo_len(ctx->console_recv_fifo) ? 1 : 0; } -static int ratp_console_puts(struct console_device *cdev, const char *s) +static int ratp_console_puts(struct console_device *cdev, const char *s, + size_t nbytes) { struct ratp_ctx *ctx = container_of(cdev, struct ratp_ctx, ratp_console); - int len = 0; - - len = strlen(s); if (ratp_busy(&ctx->ratp)) return 0; - kfifo_put(ctx->console_transmit_fifo, s, len); + kfifo_put(ctx->console_transmit_fifo, s, nbytes); - return len; + return nbytes; } static void ratp_console_putc(struct console_device *cdev, char c) diff --git a/drivers/serial/efi-stdio.c b/drivers/serial/efi-stdio.c index 0703f727e766..2ca89fa4f861 100644 --- a/drivers/serial/efi-stdio.c +++ b/drivers/serial/efi-stdio.c @@ -243,12 +243,13 @@ static int efi_process_key(struct efi_console_priv *priv, const char *inp) return 1; } -static int efi_console_puts(struct console_device *cdev, const char *s) +static int efi_console_puts(struct console_device *cdev, const char *s, + size_t nbytes) { struct efi_console_priv *priv = to_efi(cdev); int n = 0; - while (*s) { + while (nbytes--) { if (*s == 27) { priv->efi_console_buffer[n] = 0; priv->out->output_string(priv->out, diff --git a/drivers/serial/serial_efi.c b/drivers/serial/serial_efi.c index f0a2b22c2bc2..667d51f622ec 100644 --- a/drivers/serial/serial_efi.c +++ b/drivers/serial/serial_efi.c @@ -130,13 +130,14 @@ static void efi_serial_putc(struct console_device *cdev, char c) serial->write(serial, &buffersize, &c); } -static int efi_serial_puts(struct console_device *cdev, const char *s) +static int efi_serial_puts(struct console_device *cdev, const char *s, + size_t nbytes) { struct efi_serial_port *uart = to_efi_serial_port(cdev); struct efi_serial_io_protocol *serial = uart->serial; uint32_t control; efi_status_t efiret; - unsigned long buffersize = strlen(s) * sizeof(char); + unsigned long buffersize = nbytes; do { efiret = serial->getcontrol(serial, &control); diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index 755363c30999..601bfca6b025 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -67,10 +67,11 @@ static void pstore_console_write(const char *s, unsigned c) } } -static int pstore_console_puts(struct console_device *cdev, const char *s) +static int pstore_console_puts(struct console_device *cdev, const char *s, + size_t nbytes) { - pstore_console_write(s, strlen(s)); - return strlen(s); + pstore_console_write(s, nbytes); + return nbytes; } static void pstore_console_putc(struct console_device *cdev, char c) diff --git a/include/console.h b/include/console.h index 673921331db7..1d86a584a7f2 100644 --- a/include/console.h +++ b/include/console.h @@ -42,7 +42,7 @@ struct console_device { int (*tstc)(struct console_device *cdev); void (*putc)(struct console_device *cdev, char c); - int (*puts)(struct console_device *cdev, const char *s); + int (*puts)(struct console_device *cdev, const char *s, size_t nbytes); int (*getc)(struct console_device *cdev); int (*setbrg)(struct console_device *cdev, int baudrate); void (*flush)(struct console_device *cdev); -- 2.20.1 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] console: fix out-of-bounds read in dputc(/dev/*, ...) 2019-07-31 10:21 ` [PATCH v2 3/3] console: fix out-of-bounds read in dputc(/dev/*, ...) Ahmad Fatoum @ 2019-08-22 7:06 ` Ahmad Fatoum 2019-08-23 7:07 ` Sascha Hauer 0 siblings, 1 reply; 11+ messages in thread From: Ahmad Fatoum @ 2019-08-22 7:06 UTC (permalink / raw) To: barebox; +Cc: Bastian Krause Hello Sascha, On 7/31/19 12:21 PM, Ahmad Fatoum wrote: > Trying to output a single character via > echo -a /dev/serial0-1 > currently results in garbage output after the newline, because console.c's > fops_write discards the buffer length and passes the buffer to > (struct cdev)::puts which only handles NUL-terminated strings. > > Fix this by amending (struct cdev)::puts with a new nbytes parameter, > which is correctly propagated. Can this one be merged? It doesn;t depend on the other two patches. > > Fixes: b4f55fcf35 ("console: expose consoles in devfs") > Cc: Bastian Krause <bst@pengutronix.de> > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > --- > Changes in v2: > - fixed commit hash in Fixes:, by Bastian > - fixed possible incomplete writes in __console_puts > - rebased on top of two potential bug fixes > --- > common/console.c | 14 +++++++------- > common/ratp/ratp.c | 10 ++++------ > drivers/serial/efi-stdio.c | 5 +++-- > drivers/serial/serial_efi.c | 5 +++-- > fs/pstore/platform.c | 7 ++++--- > include/console.h | 2 +- > 6 files changed, 22 insertions(+), 21 deletions(-) > > diff --git a/common/console.c b/common/console.c > index 52162df23b04..ce4122fa76f1 100644 > --- a/common/console.c > +++ b/common/console.c > @@ -253,19 +253,19 @@ static void console_set_stdoutpath(struct console_device *cdev) > free(str); > } > > -static int __console_puts(struct console_device *cdev, const char *s) > +static int __console_puts(struct console_device *cdev, const char *s, > + size_t nbytes) > { > - int n = 0; > + size_t i; > > - while (*s) { > + for (i = 0; i < nbytes; i++) { > if (*s == '\n') > cdev->putc(cdev, '\r'); > > cdev->putc(cdev, *s); > - n++; > s++; > } > - return n; > + return i; > } > > static int fops_open(struct cdev *cdev, unsigned long flags) > @@ -297,7 +297,7 @@ static ssize_t fops_write(struct cdev* dev, const void* buf, size_t count, > { > struct console_device *priv = dev->priv; > > - priv->puts(priv, buf); > + priv->puts(priv, buf, count); > > return 0; > } > @@ -544,7 +544,7 @@ int console_puts(unsigned int ch, const char *str) > if (initialized == CONSOLE_INIT_FULL) { > for_each_console(cdev) { > if (cdev->f_active & ch) { > - n = cdev->puts(cdev, str); > + n = cdev->puts(cdev, str, strlen(str)); > } > } > return n; > diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c > index 8ac7dc98b6f8..8f989515e485 100644 > --- a/common/ratp/ratp.c > +++ b/common/ratp/ratp.c > @@ -259,19 +259,17 @@ static int ratp_console_tstc(struct console_device *cdev) > return kfifo_len(ctx->console_recv_fifo) ? 1 : 0; > } > > -static int ratp_console_puts(struct console_device *cdev, const char *s) > +static int ratp_console_puts(struct console_device *cdev, const char *s, > + size_t nbytes) > { > struct ratp_ctx *ctx = container_of(cdev, struct ratp_ctx, ratp_console); > - int len = 0; > - > - len = strlen(s); > > if (ratp_busy(&ctx->ratp)) > return 0; > > - kfifo_put(ctx->console_transmit_fifo, s, len); > + kfifo_put(ctx->console_transmit_fifo, s, nbytes); > > - return len; > + return nbytes; > } > > static void ratp_console_putc(struct console_device *cdev, char c) > diff --git a/drivers/serial/efi-stdio.c b/drivers/serial/efi-stdio.c > index 0703f727e766..2ca89fa4f861 100644 > --- a/drivers/serial/efi-stdio.c > +++ b/drivers/serial/efi-stdio.c > @@ -243,12 +243,13 @@ static int efi_process_key(struct efi_console_priv *priv, const char *inp) > return 1; > } > > -static int efi_console_puts(struct console_device *cdev, const char *s) > +static int efi_console_puts(struct console_device *cdev, const char *s, > + size_t nbytes) > { > struct efi_console_priv *priv = to_efi(cdev); > int n = 0; > > - while (*s) { > + while (nbytes--) { > if (*s == 27) { > priv->efi_console_buffer[n] = 0; > priv->out->output_string(priv->out, > diff --git a/drivers/serial/serial_efi.c b/drivers/serial/serial_efi.c > index f0a2b22c2bc2..667d51f622ec 100644 > --- a/drivers/serial/serial_efi.c > +++ b/drivers/serial/serial_efi.c > @@ -130,13 +130,14 @@ static void efi_serial_putc(struct console_device *cdev, char c) > serial->write(serial, &buffersize, &c); > } > > -static int efi_serial_puts(struct console_device *cdev, const char *s) > +static int efi_serial_puts(struct console_device *cdev, const char *s, > + size_t nbytes) > { > struct efi_serial_port *uart = to_efi_serial_port(cdev); > struct efi_serial_io_protocol *serial = uart->serial; > uint32_t control; > efi_status_t efiret; > - unsigned long buffersize = strlen(s) * sizeof(char); > + unsigned long buffersize = nbytes; > > do { > efiret = serial->getcontrol(serial, &control); > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c > index 755363c30999..601bfca6b025 100644 > --- a/fs/pstore/platform.c > +++ b/fs/pstore/platform.c > @@ -67,10 +67,11 @@ static void pstore_console_write(const char *s, unsigned c) > } > } > > -static int pstore_console_puts(struct console_device *cdev, const char *s) > +static int pstore_console_puts(struct console_device *cdev, const char *s, > + size_t nbytes) > { > - pstore_console_write(s, strlen(s)); > - return strlen(s); > + pstore_console_write(s, nbytes); > + return nbytes; > } > > static void pstore_console_putc(struct console_device *cdev, char c) > diff --git a/include/console.h b/include/console.h > index 673921331db7..1d86a584a7f2 100644 > --- a/include/console.h > +++ b/include/console.h > @@ -42,7 +42,7 @@ struct console_device { > > int (*tstc)(struct console_device *cdev); > void (*putc)(struct console_device *cdev, char c); > - int (*puts)(struct console_device *cdev, const char *s); > + int (*puts)(struct console_device *cdev, const char *s, size_t nbytes); > int (*getc)(struct console_device *cdev); > int (*setbrg)(struct console_device *cdev, int baudrate); > void (*flush)(struct console_device *cdev); > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] console: fix out-of-bounds read in dputc(/dev/*, ...) 2019-08-22 7:06 ` Ahmad Fatoum @ 2019-08-23 7:07 ` Sascha Hauer 2019-08-23 9:28 ` Ahmad Fatoum 0 siblings, 1 reply; 11+ messages in thread From: Sascha Hauer @ 2019-08-23 7:07 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox, Bastian Krause On Thu, Aug 22, 2019 at 09:06:09AM +0200, Ahmad Fatoum wrote: > Hello Sascha, > > On 7/31/19 12:21 PM, Ahmad Fatoum wrote: > > Trying to output a single character via > > echo -a /dev/serial0-1 > > currently results in garbage output after the newline, because console.c's > > fops_write discards the buffer length and passes the buffer to > > (struct cdev)::puts which only handles NUL-terminated strings. > > > > Fix this by amending (struct cdev)::puts with a new nbytes parameter, > > which is correctly propagated. > > Can this one be merged? It doesn;t depend on the other two patches. Ok. Can you rebase it? Without the others it doesn't seem to apply. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] console: fix out-of-bounds read in dputc(/dev/*, ...) 2019-08-23 7:07 ` Sascha Hauer @ 2019-08-23 9:28 ` Ahmad Fatoum 0 siblings, 0 replies; 11+ messages in thread From: Ahmad Fatoum @ 2019-08-23 9:28 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox, Bastian Krause Hello Sascha, On 8/23/19 9:07 AM, Sascha Hauer wrote: > On Thu, Aug 22, 2019 at 09:06:09AM +0200, Ahmad Fatoum wrote: >> Hello Sascha, >> >> On 7/31/19 12:21 PM, Ahmad Fatoum wrote: >>> Trying to output a single character via >>> echo -a /dev/serial0-1 >>> currently results in garbage output after the newline, because console.c's >>> fops_write discards the buffer length and passes the buffer to >>> (struct cdev)::puts which only handles NUL-terminated strings. >>> >>> Fix this by amending (struct cdev)::puts with a new nbytes parameter, >>> which is correctly propagated. >> >> Can this one be merged? It doesn;t depend on the other two patches. > > Ok. Can you rebase it? Without the others it doesn't seem to apply. done. I dropped the RATP one, but squashed the other. > > Sascha > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] console: don't count newlines twice in bytes written 2019-07-31 10:21 [PATCH v2 1/3] console: don't count newlines twice in bytes written Ahmad Fatoum 2019-07-31 10:21 ` [PATCH v2 2/3] ratp: return 0 bytes written from puts if busy Ahmad Fatoum 2019-07-31 10:21 ` [PATCH v2 3/3] console: fix out-of-bounds read in dputc(/dev/*, ...) Ahmad Fatoum @ 2019-08-05 9:11 ` Sascha Hauer 2019-08-22 7:04 ` Ahmad Fatoum 2 siblings, 1 reply; 11+ messages in thread From: Sascha Hauer @ 2019-08-05 9:11 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox On Wed, Jul 31, 2019 at 12:21:41PM +0200, Ahmad Fatoum wrote: > Both the PBL and simple console only return number of input bytes, not > number of bytes actually written out. These differ, because each LF is > converted to CRLF pairs. > > The behavior of not counting actual written out characters is more sensible, > because otherwise callers interested in finding out if all bytes have been > written (e.g. to avoid incomplete writes with ratp) would need to keep count > of all line feeds in the string. > Therefore change the normal console to behave like its less featureful > brethren. According to "man puts" puts() returns a non negative number on success. I can't find a place where it's claimed that we have to return the number of characters. I also can't find a place where the return value of any puts like function in barebox is ever evaluated. Given that it might make more sense to just return zero in the absense of an error. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] console: don't count newlines twice in bytes written 2019-08-05 9:11 ` [PATCH v2 1/3] console: don't count newlines twice in bytes written Sascha Hauer @ 2019-08-22 7:04 ` Ahmad Fatoum 2019-08-22 8:16 ` Ahmad Fatoum 0 siblings, 1 reply; 11+ messages in thread From: Ahmad Fatoum @ 2019-08-22 7:04 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox On 8/5/19 11:11 AM, Sascha Hauer wrote: > On Wed, Jul 31, 2019 at 12:21:41PM +0200, Ahmad Fatoum wrote: >> Both the PBL and simple console only return number of input bytes, not >> number of bytes actually written out. These differ, because each LF is >> converted to CRLF pairs. >> >> The behavior of not counting actual written out characters is more sensible, >> because otherwise callers interested in finding out if all bytes have been >> written (e.g. to avoid incomplete writes with ratp) would need to keep count >> of all line feeds in the string. >> Therefore change the normal console to behave like its less featureful >> brethren. > > According to "man puts" puts() returns a non negative number on success. > I can't find a place where it's claimed that we have to return the > number of characters. console_puts in common/console_simple.c and pbl/console.c already return the number of characters. Same for all the console_device puts members in the drivers. > I also can't find a place where the return value > of any puts like function in barebox is ever evaluated. Given that it > might make more sense to just return zero in the absense of an error. I too think there isn't, but it would be less surprising if common/console.c would behave like other puts's in barebox. > > Sascha > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] console: don't count newlines twice in bytes written 2019-08-22 7:04 ` Ahmad Fatoum @ 2019-08-22 8:16 ` Ahmad Fatoum 0 siblings, 0 replies; 11+ messages in thread From: Ahmad Fatoum @ 2019-08-22 8:16 UTC (permalink / raw) To: barebox Hello, On 8/22/19 9:04 AM, Ahmad Fatoum wrote: > > > On 8/5/19 11:11 AM, Sascha Hauer wrote: >> On Wed, Jul 31, 2019 at 12:21:41PM +0200, Ahmad Fatoum wrote: >>> Both the PBL and simple console only return number of input bytes, not >>> number of bytes actually written out. These differ, because each LF is >>> converted to CRLF pairs. >>> >>> The behavior of not counting actual written out characters is more sensible, >>> because otherwise callers interested in finding out if all bytes have been >>> written (e.g. to avoid incomplete writes with ratp) would need to keep count >>> of all line feeds in the string. >>> Therefore change the normal console to behave like its less featureful >>> brethren. >> >> According to "man puts" puts() returns a non negative number on success. >> I can't find a place where it's claimed that we have to return the >> number of characters. > > console_puts in common/console_simple.c and pbl/console.c already return the > number of characters. Same for all the console_device puts members in the > drivers. > >> I also can't find a place where the return value >> of any puts like function in barebox is ever evaluated. Given that it >> might make more sense to just return zero in the absense of an error. > > I too think there isn't, but it would be less surprising if common/console.c > would behave like other puts's in barebox. Sorry, I mixed it up. Ye, it's arguable whether they should return number of input bytes written or zero. But no other puts in barebox counts new lines twice, so I think the patch has its merit, even if a follow up patch in future has puts functions return 0 instead. > >> >> Sascha >> > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-08-23 9:28 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-31 10:21 [PATCH v2 1/3] console: don't count newlines twice in bytes written Ahmad Fatoum 2019-07-31 10:21 ` [PATCH v2 2/3] ratp: return 0 bytes written from puts if busy Ahmad Fatoum 2019-08-05 8:59 ` Sascha Hauer 2019-08-22 6:57 ` Ahmad Fatoum 2019-07-31 10:21 ` [PATCH v2 3/3] console: fix out-of-bounds read in dputc(/dev/*, ...) Ahmad Fatoum 2019-08-22 7:06 ` Ahmad Fatoum 2019-08-23 7:07 ` Sascha Hauer 2019-08-23 9:28 ` Ahmad Fatoum 2019-08-05 9:11 ` [PATCH v2 1/3] console: don't count newlines twice in bytes written Sascha Hauer 2019-08-22 7:04 ` Ahmad Fatoum 2019-08-22 8:16 ` Ahmad Fatoum
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox