Re: [PATCH 1/2] readline_simple: return -1 if getc fails

[Gaël PORTAY]

On Tue, Aug 08, 2017 at 05:36:14PM +0200, Lucas Stach wrote:
> Am Dienstag, den 08.08.2017, 11:20 -0400 schrieb Gaël PORTAY:
> > Hi Lucas,
> > 
> > On Tue, Aug 08, 2017 at 09:51:54AM +0200, Lucas Stach wrote:
> > > > diff --git a/lib/readline_simple.c b/lib/readline_simple.c
> > > > index c4d3d240e..1283c9602 100644
> > > > --- a/lib/readline_simple.c
> > > > +++ b/lib/readline_simple.c
> > > > @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
> > > >  
> > > > for (;;) {
> > > > c = getchar();
> > > > +   if (c < 0)
> > > > +   return (-1);
> > > 
> > > I don't like made up error codes. Is there any reason why we couldn't
> > > just pass through the negative error code from getchar?
> > > 
> > 
> > The thing here is that getchar() may return an error, and that error is not
> > tested. This causes readline to print the character 0xea (-EINVAL) which is 
> > not
> > printable.
> 
> So why wouldn't the following fix the issue?
> 
> signed char c;
> 
> if (c < 0)
>   return c;
> 

Okay. I do prefer your solution.

I returned -1 mainly because the function comment says it returns -1 when it
breaks; and because parser.c and hush.c test readline function against -1 and
not against a negative value.

Also readline returns -1 if character is 0x03. Maybe it should return -EINTR;
-1 is EPERM: Operation not permitted.

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] readline_simple: return -1 if getc fails

[Ian Abbott]

On 08/08/17 16:36, Lucas Stach wrote:

Am Dienstag, den 08.08.2017, 11:20 -0400 schrieb Gaël PORTAY:

Hi Lucas,

On Tue, Aug 08, 2017 at 09:51:54AM +0200, Lucas Stach wrote:

Am Montag, den 07.08.2017, 18:10 -0400 schrieb Gaël PORTAY:

The getc function may return an errno code if an error happens.

This patch prevents readline from printing a non printable character and
from looping to infinity and beyong.

Signed-off-by: Gaël PORTAY 
---
  lib/readline_simple.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/lib/readline_simple.c b/lib/readline_simple.c
index c4d3d240e..1283c9602 100644
--- a/lib/readline_simple.c
+++ b/lib/readline_simple.c
@@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
  
  	for (;;) {

c = getchar();
+   if (c < 0)
+   return (-1);


I don't like made up error codes. Is there any reason why we couldn't
just pass through the negative error code from getchar?



The thing here is that getchar() may return an error, and that error is not
tested. This causes readline to print the character 0xea (-EINVAL) which is not
printable.


So why wouldn't the following fix the issue?

signed char c;


`int` would be better to allow non-ASCII characters.



if (c < 0)
return c;


There are places where the return value is checked for `-1` for example 
in get_user_input() ("common/hush.c"), and in run_shell() 
("common/parser.c").


I think Gaël's patch is reasonable, although perhaps it should also set 
`line[0] = '\0';` before returning.


Off topic: there is another oddity in the the "simple" version of 
readline(). It ignores the `len` parameter and uses `CONFIG_CBSIZE` instead.


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] readline_simple: return -1 if getc fails

[Lucas Stach]

Am Dienstag, den 08.08.2017, 11:20 -0400 schrieb Gaël PORTAY:
> Hi Lucas,
> 
> On Tue, Aug 08, 2017 at 09:51:54AM +0200, Lucas Stach wrote:
> > Am Montag, den 07.08.2017, 18:10 -0400 schrieb Gaël PORTAY:
> > > The getc function may return an errno code if an error happens.
> > > 
> > > This patch prevents readline from printing a non printable character and
> > > from looping to infinity and beyong.
> > > 
> > > Signed-off-by: Gaël PORTAY 
> > > ---
> > >  lib/readline_simple.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/lib/readline_simple.c b/lib/readline_simple.c
> > > index c4d3d240e..1283c9602 100644
> > > --- a/lib/readline_simple.c
> > > +++ b/lib/readline_simple.c
> > > @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
> > >  
> > >   for (;;) {
> > >   c = getchar();
> > > + if (c < 0)
> > > + return (-1);
> > 
> > I don't like made up error codes. Is there any reason why we couldn't
> > just pass through the negative error code from getchar?
> > 
> 
> The thing here is that getchar() may return an error, and that error is not
> tested. This causes readline to print the character 0xea (-EINVAL) which is 
> not
> printable.

So why wouldn't the following fix the issue?

signed char c;

if (c < 0)
return c;

Regards,
Lucas



___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] readline_simple: return -1 if getc fails

[Gaël PORTAY]

Hi Ian,

On Tue, Aug 08, 2017 at 11:07:54AM +0100, Ian Abbott wrote:
> On 08/08/17 08:51, Lucas Stach wrote:
> > Am Montag, den 07.08.2017, 18:10 -0400 schrieb Gaël PORTAY:
> > > The getc function may return an errno code if an error happens.
> > > 
> > > This patch prevents readline from printing a non printable character and
> > > from looping to infinity and beyong.
> > > 
> > > Signed-off-by: Gaël PORTAY 
> > > ---
> > >   lib/readline_simple.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/lib/readline_simple.c b/lib/readline_simple.c
> > > index c4d3d240e..1283c9602 100644
> > > --- a/lib/readline_simple.c
> > > +++ b/lib/readline_simple.c
> > > @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
> > >   for (;;) {
> > >   c = getchar();
> > > + if (c < 0)
> > > + return (-1);
> > 
> > I don't like made up error codes. Is there any reason why we couldn't
> > just pass through the negative error code from getchar?
> 
> And also change the type of the 'c' variable, as it is currently a plain
> 'char'?
> 

Indeed :)

Regards,
Gaël

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] readline_simple: return -1 if getc fails

[Gaël PORTAY]

Hi Lucas,

On Tue, Aug 08, 2017 at 09:51:54AM +0200, Lucas Stach wrote:
> Am Montag, den 07.08.2017, 18:10 -0400 schrieb Gaël PORTAY:
> > The getc function may return an errno code if an error happens.
> > 
> > This patch prevents readline from printing a non printable character and
> > from looping to infinity and beyong.
> > 
> > Signed-off-by: Gaël PORTAY 
> > ---
> >  lib/readline_simple.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/lib/readline_simple.c b/lib/readline_simple.c
> > index c4d3d240e..1283c9602 100644
> > --- a/lib/readline_simple.c
> > +++ b/lib/readline_simple.c
> > @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
> >  
> > for (;;) {
> > c = getchar();
> > +   if (c < 0)
> > +   return (-1);
> 
> I don't like made up error codes. Is there any reason why we couldn't
> just pass through the negative error code from getchar?
> 

The thing here is that getchar() may return an error, and that error is not
tested. This causes readline to print the character 0xea (-EINVAL) which is not
printable.

Maybe another solutions would be to print the errno string and continue; or to
return the number of characters already read.

Regards,
Gaël

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] readline_simple: return -1 if getc fails

[Ian Abbott]

On 08/08/17 08:51, Lucas Stach wrote:

Am Montag, den 07.08.2017, 18:10 -0400 schrieb Gaël PORTAY:

The getc function may return an errno code if an error happens.

This patch prevents readline from printing a non printable character and
from looping to infinity and beyong.

Signed-off-by: Gaël PORTAY 
---
  lib/readline_simple.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/lib/readline_simple.c b/lib/readline_simple.c
index c4d3d240e..1283c9602 100644
--- a/lib/readline_simple.c
+++ b/lib/readline_simple.c
@@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
  
  	for (;;) {

c = getchar();
+   if (c < 0)
+   return (-1);


I don't like made up error codes. Is there any reason why we couldn't
just pass through the negative error code from getchar?


And also change the type of the 'c' variable, as it is currently a plain 
'char'?



Regards,
Lucas


--
-=( Ian Abbott @ MEV Ltd.E-mail:  )=-
-=(  Web: http://www.mev.co.uk/  )=-

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] readline_simple: return -1 if getc fails

[Lucas Stach]

Am Montag, den 07.08.2017, 18:10 -0400 schrieb Gaël PORTAY:
> The getc function may return an errno code if an error happens.
> 
> This patch prevents readline from printing a non printable character and
> from looping to infinity and beyong.
> 
> Signed-off-by: Gaël PORTAY 
> ---
>  lib/readline_simple.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/readline_simple.c b/lib/readline_simple.c
> index c4d3d240e..1283c9602 100644
> --- a/lib/readline_simple.c
> +++ b/lib/readline_simple.c
> @@ -58,6 +58,8 @@ int readline (const char *prompt, char *line, int len)
>  
>   for (;;) {
>   c = getchar();
> + if (c < 0)
> + return (-1);

I don't like made up error codes. Is there any reason why we couldn't
just pass through the negative error code from getchar?

Regards,
Lucas


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox