* [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