* [PATCH] edit: Return error when save_file failed
@ 2015-11-03 16:19 Enrico Jorns
2015-11-04 11:03 ` [PATCH v2] " Enrico Jorns
0 siblings, 1 reply; 6+ messages in thread
From: Enrico Jorns @ 2015-11-03 16:19 UTC (permalink / raw)
To: barebox; +Cc: Enrico Jorns
When writing a file failed (e.g. due to a read-only file system), no
error was reported by the 'edit' tool. To be valid (and to not confuse
the poor user) at least '1' should be returned to indicate an error.
Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
---
commands/edit.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/commands/edit.c b/commands/edit.c
index b28e2b9..2d9c8b5 100644
--- a/commands/edit.c
+++ b/commands/edit.c
@@ -533,7 +533,8 @@ static int do_edit(int argc, char *argv[])
}
break;
case 4:
- save_file(argv[1]);
+ if (save_file(argv[1]))
+ return 1;
goto out;
case 3:
goto out;
--
2.6.1
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] edit: Return error when save_file failed
2015-11-03 16:19 [PATCH] edit: Return error when save_file failed Enrico Jorns
@ 2015-11-04 11:03 ` Enrico Jorns
2015-11-04 12:23 ` Antony Pavlov
0 siblings, 1 reply; 6+ messages in thread
From: Enrico Jorns @ 2015-11-04 11:03 UTC (permalink / raw)
To: barebox; +Cc: Enrico Jorns
When writing a file failed (e.g. due to a read-only file system), no
error was reported by the 'edit' tool. To be valid (and to not confuse
the poor user) at least '1' should be returned to indicate an error.
Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
---
commands/edit.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/commands/edit.c b/commands/edit.c
index b28e2b9..3b653b7 100644
--- a/commands/edit.c
+++ b/commands/edit.c
@@ -375,6 +375,7 @@ static int do_edit(int argc, char *argv[])
int i;
int linepos;
int c;
+ int ret = 0;
if (argc != 2)
return COMMAND_ERROR_USAGE;
@@ -533,7 +534,7 @@ static int do_edit(int argc, char *argv[])
}
break;
case 4:
- save_file(argv[1]);
+ ret = save_file(argv[1]);
goto out;
case 3:
goto out;
@@ -546,7 +547,7 @@ out:
free_buffer();
printf("%c[2J%c[r", 27, 27);
printf("\n");
- return 0;
+ return ret;
}
static const char *edit_aliases[] = { "sedit", NULL};
--
2.6.1
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] edit: Return error when save_file failed
2015-11-04 11:03 ` [PATCH v2] " Enrico Jorns
@ 2015-11-04 12:23 ` Antony Pavlov
2015-11-04 13:30 ` Sascha Hauer
0 siblings, 1 reply; 6+ messages in thread
From: Antony Pavlov @ 2015-11-04 12:23 UTC (permalink / raw)
To: Enrico Jorns; +Cc: barebox
On Wed, 4 Nov 2015 12:03:24 +0100
Enrico Jorns <ejo@pengutronix.de> wrote:
> When writing a file failed (e.g. due to a read-only file system), no
> error was reported by the 'edit' tool. To be valid (and to not confuse
> the poor user) at least '1' should be returned to indicate an error.
>
> Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
> ---
> commands/edit.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/commands/edit.c b/commands/edit.c
> index b28e2b9..3b653b7 100644
> --- a/commands/edit.c
> +++ b/commands/edit.c
> @@ -375,6 +375,7 @@ static int do_edit(int argc, char *argv[])
> int i;
> int linepos;
> int c;
> + int ret = 0;
Can we use COMMAND_SUCCESS insted of 0?
>
> if (argc != 2)
> return COMMAND_ERROR_USAGE;
> @@ -533,7 +534,7 @@ static int do_edit(int argc, char *argv[])
> }
> break;
> case 4:
> - save_file(argv[1]);
> + ret = save_file(argv[1]);
Actually save_file() returns open()'s error. We have to convert it to COMMAND_ERROR, e.g.:
if (save_file(argv[1]) != 0)
ret = COMMAND_ERROR;
Also save_file() does not check write()'s error.
> goto out;
> case 3:
> goto out;
> @@ -546,7 +547,7 @@ out:
> free_buffer();
> printf("%c[2J%c[r", 27, 27);
> printf("\n");
> - return 0;
> + return ret;
> }
>
> static const char *edit_aliases[] = { "sedit", NULL};
> --
> 2.6.1
>
>
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
--
--
Best regards,
Antony Pavlov
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] edit: Return error when save_file failed
2015-11-04 12:23 ` Antony Pavlov
@ 2015-11-04 13:30 ` Sascha Hauer
2015-11-04 14:48 ` Antony Pavlov
0 siblings, 1 reply; 6+ messages in thread
From: Sascha Hauer @ 2015-11-04 13:30 UTC (permalink / raw)
To: Antony Pavlov; +Cc: barebox, Enrico Jorns
On Wed, Nov 04, 2015 at 03:23:21PM +0300, Antony Pavlov wrote:
> On Wed, 4 Nov 2015 12:03:24 +0100
> Enrico Jorns <ejo@pengutronix.de> wrote:
>
> > When writing a file failed (e.g. due to a read-only file system), no
> > error was reported by the 'edit' tool. To be valid (and to not confuse
> > the poor user) at least '1' should be returned to indicate an error.
> >
> > Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
> > ---
> > commands/edit.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/commands/edit.c b/commands/edit.c
> > index b28e2b9..3b653b7 100644
> > --- a/commands/edit.c
> > +++ b/commands/edit.c
> > @@ -375,6 +375,7 @@ static int do_edit(int argc, char *argv[])
> > int i;
> > int linepos;
> > int c;
> > + int ret = 0;
>
> Can we use COMMAND_SUCCESS insted of 0?
>
>
> >
> > if (argc != 2)
> > return COMMAND_ERROR_USAGE;
> > @@ -533,7 +534,7 @@ static int do_edit(int argc, char *argv[])
> > }
> > break;
> > case 4:
> > - save_file(argv[1]);
> > + ret = save_file(argv[1]);
>
> Actually save_file() returns open()'s error. We have to convert it to COMMAND_ERROR, e.g.:
If we return an error code from a command then the caller will print the
corresponding error string to the console which might be what we want
here.
>
> if (save_file(argv[1]) != 0)
> ret = COMMAND_ERROR;
>
> Also save_file() does not check write()'s error.
Indeed, this could be fixed while touching this code.
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] 6+ messages in thread
* Re: [PATCH v2] edit: Return error when save_file failed
2015-11-04 13:30 ` Sascha Hauer
@ 2015-11-04 14:48 ` Antony Pavlov
2015-11-05 8:17 ` Sascha Hauer
0 siblings, 1 reply; 6+ messages in thread
From: Antony Pavlov @ 2015-11-04 14:48 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox, Enrico Jorns
On Wed, 4 Nov 2015 14:30:53 +0100
Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, Nov 04, 2015 at 03:23:21PM +0300, Antony Pavlov wrote:
> > On Wed, 4 Nov 2015 12:03:24 +0100
> > Enrico Jorns <ejo@pengutronix.de> wrote:
> >
> > > When writing a file failed (e.g. due to a read-only file system), no
> > > error was reported by the 'edit' tool. To be valid (and to not confuse
> > > the poor user) at least '1' should be returned to indicate an error.
> > >
> > > Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
> > > ---
> > > commands/edit.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/commands/edit.c b/commands/edit.c
> > > index b28e2b9..3b653b7 100644
> > > --- a/commands/edit.c
> > > +++ b/commands/edit.c
> > > @@ -375,6 +375,7 @@ static int do_edit(int argc, char *argv[])
> > > int i;
> > > int linepos;
> > > int c;
> > > + int ret = 0;
> >
> > Can we use COMMAND_SUCCESS insted of 0?
> >
> >
> > >
> > > if (argc != 2)
> > > return COMMAND_ERROR_USAGE;
> > > @@ -533,7 +534,7 @@ static int do_edit(int argc, char *argv[])
> > > }
> > > break;
> > > case 4:
> > > - save_file(argv[1]);
> > > + ret = save_file(argv[1]);
> >
> > Actually save_file() returns open()'s error. We have to convert it to COMMAND_ERROR, e.g.:
>
> If we return an error code from a command then the caller will print the
> corresponding error string to the console which might be what we want
> here.
>
> >
> > if (save_file(argv[1]) != 0)
> > ret = COMMAND_ERROR;
> >
At the moment the only cmd() caller is inside of execute_command():
ret = cmdtp->cmd(argc, argv);
if (ret == COMMAND_ERROR_USAGE) {
barebox_cmd_usage(cmdtp);
ret = COMMAND_ERROR;
}
...
return ret;
But we brop execute_command() result in run_command():
if (execute_command(argc, argv) != COMMAND_SUCCESS)
rc = -1;
We have a chance to see error message only if we call execute_command()
from run_pipe_real():
ret = execute_binfmt(globbuf.gl_pathc, globbuf.gl_pathv);
if (ret < 0) {
printf("%s: %s\n", globbuf.gl_pathv[0], strerror(-ret));
ret = 127;
}
So, yes, you are right we have no reason to convert open()'s error into COMMAND_ERROR,
but for consistency we have toadd strerror() print to run_command() code.
> > Also save_file() does not check write()'s error.
> Indeed, this could be fixed while touching this code.
>
> 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 |
--
--
Best regards,
Antony Pavlov
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] edit: Return error when save_file failed
2015-11-04 14:48 ` Antony Pavlov
@ 2015-11-05 8:17 ` Sascha Hauer
0 siblings, 0 replies; 6+ messages in thread
From: Sascha Hauer @ 2015-11-05 8:17 UTC (permalink / raw)
To: Antony Pavlov; +Cc: barebox, Enrico Jorns
On Wed, Nov 04, 2015 at 05:48:10PM +0300, Antony Pavlov wrote:
> On Wed, 4 Nov 2015 14:30:53 +0100
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> > On Wed, Nov 04, 2015 at 03:23:21PM +0300, Antony Pavlov wrote:
> > > On Wed, 4 Nov 2015 12:03:24 +0100
> > > Enrico Jorns <ejo@pengutronix.de> wrote:
> > >
> > > > When writing a file failed (e.g. due to a read-only file system), no
> > > > error was reported by the 'edit' tool. To be valid (and to not confuse
> > > > the poor user) at least '1' should be returned to indicate an error.
> > > >
> > > > Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
> > > > ---
> > > > commands/edit.c | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/commands/edit.c b/commands/edit.c
> > > > index b28e2b9..3b653b7 100644
> > > > --- a/commands/edit.c
> > > > +++ b/commands/edit.c
> > > > @@ -375,6 +375,7 @@ static int do_edit(int argc, char *argv[])
> > > > int i;
> > > > int linepos;
> > > > int c;
> > > > + int ret = 0;
> > >
> > > Can we use COMMAND_SUCCESS insted of 0?
> > >
> > >
> > > >
> > > > if (argc != 2)
> > > > return COMMAND_ERROR_USAGE;
> > > > @@ -533,7 +534,7 @@ static int do_edit(int argc, char *argv[])
> > > > }
> > > > break;
> > > > case 4:
> > > > - save_file(argv[1]);
> > > > + ret = save_file(argv[1]);
> > >
> > > Actually save_file() returns open()'s error. We have to convert it to COMMAND_ERROR, e.g.:
> >
> > If we return an error code from a command then the caller will print the
> > corresponding error string to the console which might be what we want
> > here.
> >
> > >
> > > if (save_file(argv[1]) != 0)
> > > ret = COMMAND_ERROR;
> > >
>
> At the moment the only cmd() caller is inside of execute_command():
>
> ret = cmdtp->cmd(argc, argv);
> if (ret == COMMAND_ERROR_USAGE) {
> barebox_cmd_usage(cmdtp);
> ret = COMMAND_ERROR;
> }
> ...
> return ret;
>
>
> But we brop execute_command() result in run_command():
> if (execute_command(argc, argv) != COMMAND_SUCCESS)
> rc = -1;
>
> We have a chance to see error message only if we call execute_command()
> from run_pipe_real():
>
> ret = execute_binfmt(globbuf.gl_pathc, globbuf.gl_pathv);
> if (ret < 0) {
> printf("%s: %s\n", globbuf.gl_pathv[0], strerror(-ret));
> ret = 127;
> }
>
> So, yes, you are right we have no reason to convert open()'s error into COMMAND_ERROR,
> but for consistency we have toadd strerror() print to run_command() code.
Yes, right.
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] 6+ messages in thread
end of thread, other threads:[~2015-11-05 8:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-03 16:19 [PATCH] edit: Return error when save_file failed Enrico Jorns
2015-11-04 11:03 ` [PATCH v2] " Enrico Jorns
2015-11-04 12:23 ` Antony Pavlov
2015-11-04 13:30 ` Sascha Hauer
2015-11-04 14:48 ` Antony Pavlov
2015-11-05 8:17 ` Sascha Hauer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox