[PATCH 0/5] Fixes for nv command handling

[PATCH 0/5] Fixes for nv command handling

[Enrico Jorns]

This series adds a couple of fixes and enhancement in option parsing and error
handling for the 'nv' command.

Enrico Jorns (5):
  commands: nv: argc cannot be < 1
  commands: nv: call nvvar_print() only if no argument is given
  commands: nv: assure error code will be returned when an error
    occurred
  commands: nv: fail if required var arguments are missing
  common: globvar: let nvvar_remove() report non-existing variable

 commands/nv.c      | 31 ++++++++++++++++++++++---------
 common/globalvar.c |  4 +++-
 2 files changed, 25 insertions(+), 10 deletions(-)

-- 
2.11.0


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

[PATCH 1/5] commands: nv: argc cannot be < 1

[Enrico Jorns]

argc == 1 will only contain the name of the program itself which must be
present when invoking nv command code.

Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
---
 commands/nv.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/commands/nv.c b/commands/nv.c
index 37cdb96647..eb396bfd9b 100644
--- a/commands/nv.c
+++ b/commands/nv.c
@@ -55,9 +55,6 @@ static int do_nv(int argc, char *argv[])
 	argc -= optind;
 	argv += optind;
 
-	if (argc < 1)
-		return COMMAND_ERROR_USAGE;
-
 	for (i = 0; i < argc; i++) {
 		value = strchr(argv[0], '=');
 		if (value) {
-- 
2.11.0


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

Re: [PATCH 1/5] commands: nv: argc cannot be < 1

[Sascha Hauer]

On Wed, Sep 13, 2017 at 02:23:28PM +0200, Enrico Jorns wrote:
> argc == 1 will only contain the name of the program itself which must be
> present when invoking nv command code.

This reasoning is not quite right. You miss the argc -= optind above the
argc < 1 test. In fact in patch 4/5 you re-add the same test again
(written as argc == 0 this time).

Shouldn't this patch simply be merged with 4/5?

Sascha

> 
> Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
> ---
>  commands/nv.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/commands/nv.c b/commands/nv.c
> index 37cdb96647..eb396bfd9b 100644
> --- a/commands/nv.c
> +++ b/commands/nv.c
> @@ -55,9 +55,6 @@ static int do_nv(int argc, char *argv[])
>  	argc -= optind;
>  	argv += optind;
>  
> -	if (argc < 1)
> -		return COMMAND_ERROR_USAGE;
> -
>  	for (i = 0; i < argc; i++) {
>  		value = strchr(argv[0], '=');
>  		if (value) {
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

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

Re: [PATCH 1/5] commands: nv: argc cannot be < 1

[Enrico Joerns]

On 09/14/2017 08:27 AM, Sascha Hauer wrote:
> On Wed, Sep 13, 2017 at 02:23:28PM +0200, Enrico Jorns wrote:
>> argc == 1 will only contain the name of the program itself which must be
>> present when invoking nv command code.
> 
> This reasoning is not quite right. You miss the argc -= optind above the
> argc < 1 test. In fact in patch 4/5 you re-add the same test again
> (written as argc == 0 this time).
> 
> Shouldn't this patch simply be merged with 4/5?

In fact, you're right. Seems as if I splitted up changes a bit too much 
and confused myself. Merging it with 4/5 sound more reasonable (or 
simply drop both of them).

Enrico

-- 
Pengutronix e.K.                           | Enrico Jörns                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5080 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |


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

[PATCH 2/5] commands: nv: call nvvar_print() only if no argument is given

[Enrico Jorns]

Printing the nv variables (before performing any modification!) is useless
and irritating.

Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
---
 commands/nv.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/commands/nv.c b/commands/nv.c
index eb396bfd9b..b141198a93 100644
--- a/commands/nv.c
+++ b/commands/nv.c
@@ -44,14 +44,15 @@ static int do_nv(int argc, char *argv[])
 		}
 	}
 
-	if (do_save)
-		return nvvar_save();
-
-	if (argc == optind) {
+	if (argc == 1) {
 		nvvar_print();
 		return 0;
 	}
 
+	if (do_save) {
+		return nvvar_save();
+	}
+
 	argc -= optind;
 	argv += optind;
 
-- 
2.11.0


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

[PATCH 3/5] commands: nv: assure error code will be returned when an error occurred

[Enrico Jorns]

Due to iteration over possibly multiple variables, the return value of
an individual call to nvvar_remove() / nvvar_add() gets lost.

This will result in do_nv() returning success (0) if any variable
processing failed as long as processing of the last variable succeeded.

Processing will not be aborted on individual errors, such as the 'cp'
handles it for invalid files.

Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
---
 commands/nv.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/commands/nv.c b/commands/nv.c
index b141198a93..d391f9e6b4 100644
--- a/commands/nv.c
+++ b/commands/nv.c
@@ -28,7 +28,7 @@ static int do_nv(int argc, char *argv[])
 {
 	int opt;
 	int do_remove = 0, do_save = 0;
-	int ret, i;
+	int failed = 0, i;
 	char *value;
 
 	while ((opt = getopt(argc, argv, "rs")) > 0) {
@@ -57,19 +57,29 @@ static int do_nv(int argc, char *argv[])
 	argv += optind;
 
 	for (i = 0; i < argc; i++) {
+		int ret;
 		value = strchr(argv[0], '=');
 		if (value) {
 			*value = 0;
 			value++;
 		}
 
-		if (do_remove)
+		if (do_remove) {
 			ret = nvvar_remove(argv[i]);
-		else
+			if (ret) {
+				printf("Failed removing %s: %s\n", argv[i], strerror(-ret));
+				failed = 1;
+			}
+		} else {
 			ret = nvvar_add(argv[i], value);
+			if (ret) {
+				printf("Failed adding %s: %s\n", argv[i], strerror(-ret));
+				failed = 1;
+			}
+		}
 	}
 
-	return ret;
+	return failed;
 }
 
 BAREBOX_CMD_HELP_START(nv)
-- 
2.11.0


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

[PATCH 4/5] commands: nv: fail if required var arguments are missing

[Enrico Jorns]

Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
---
 commands/nv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/commands/nv.c b/commands/nv.c
index d391f9e6b4..bb13ed1db0 100644
--- a/commands/nv.c
+++ b/commands/nv.c
@@ -56,6 +56,11 @@ static int do_nv(int argc, char *argv[])
 	argc -= optind;
 	argv += optind;
 
+	if (argc == 0) {
+		printf("Invalid usage: Missing argument");
+		return COMMAND_ERROR_USAGE;
+	}
+
 	for (i = 0; i < argc; i++) {
 		int ret;
 		value = strchr(argv[0], '=');
-- 
2.11.0


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

[PATCH 5/5] common: globvar: let nvvar_remove() report non-existing variable

[Enrico Jorns]

The former implementation did not allow to detect whether the call to
nvvar_remove() succeeded or failed and always returned 0.

This changes the implementation to return 0 only if a variable with the
given name was found and return ENOENT otherwise.

Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
---
 common/globalvar.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/common/globalvar.c b/common/globalvar.c
index fdfaf76fae..ee756e5140 100644
--- a/common/globalvar.c
+++ b/common/globalvar.c
@@ -261,9 +261,11 @@ int nvvar_remove(const char *name)
 
 		unlink(fname);
 		free(fname);
+
+		return 0;
 	}
 
-	return 0;
+	return -ENOENT;
 }
 
 int nvvar_load(void)
-- 
2.11.0


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

[PATCH] commands: nv: adjust command help

[Enrico Jorns]

Help did not point out that -r option requires n arguments and did not
mention support for removing variables in help text.

Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
---

Noticed this change would also be required to round out the prior changes for
option parsing.

 commands/nv.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/commands/nv.c b/commands/nv.c
index bb13ed1db0..e3ca9879bd 100644
--- a/commands/nv.c
+++ b/commands/nv.c
@@ -89,18 +89,18 @@ static int do_nv(int argc, char *argv[])
 
 BAREBOX_CMD_HELP_START(nv)
 BAREBOX_CMD_HELP_TEXT("Add a new non volatile variable named VAR, optionally set to VALUE.")
-BAREBOX_CMD_HELP_TEXT("non volatile variables are persistent variables that overwrite the")
+BAREBOX_CMD_HELP_TEXT("Non volatile variables are persistent variables that overwrite the")
 BAREBOX_CMD_HELP_TEXT("global variables of the same name. Their value is saved implicitly with")
 BAREBOX_CMD_HELP_TEXT("'saveenv' or explicitly with 'nv -s'")
 BAREBOX_CMD_HELP_TEXT("")
 BAREBOX_CMD_HELP_TEXT("Options:")
-BAREBOX_CMD_HELP_OPT("-r", "remove non volatile variables")
-BAREBOX_CMD_HELP_OPT("-s", "Save NV variables")
+BAREBOX_CMD_HELP_OPT("-r VAR1 ...", "remove non volatile variable(s)")
+BAREBOX_CMD_HELP_OPT("-s\t", "save NV variables")
 BAREBOX_CMD_HELP_END
 
 BAREBOX_CMD_START(nv)
 	.cmd		= do_nv,
-	BAREBOX_CMD_DESC("create or set non volatile variables")
+	BAREBOX_CMD_DESC("create, set or remove non volatile variables")
 	BAREBOX_CMD_OPTS("[-r] VAR[=VALUE] ...")
 	BAREBOX_CMD_GROUP(CMD_GRP_ENV)
 	BAREBOX_CMD_HELP(cmd_nv_help)
-- 
2.11.0


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

[PATCH v2 0/5] Fixes for nv command handling

[Enrico Jorns]

This series adds a couple of fixes and enhancement in option parsing and error
handling for the 'nv' command.

CHANGES:

* Merged 1/5 and 4/5 of old series into 1/5
* added patch: 'common: globvar: let nvvar_remove() report non-existing variable'

Enrico Jorns (5):
  commands: nv: fail with verbose message if invoked without args
  commands: nv: call nvvar_print() only if no argument is given
  commands: nv: assure error code will be returned when an error
    occurred
  common: globvar: let nvvar_remove() report non-existing variable
  commands: nv: adjust command help

 commands/nv.c      | 36 ++++++++++++++++++++++++------------
 common/globalvar.c |  4 +++-
 2 files changed, 27 insertions(+), 13 deletions(-)

-- 
2.11.0


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

[PATCH 3/5] commands: nv: assure error code will be returned when an error occurred

[Enrico Jorns]

Due to iteration over possibly multiple variables, the return value of
an individual call to nvvar_remove() / nvvar_add() gets lost.

This will result in do_nv() returning success (0) if any variable
processing failed as long as processing of the last variable succeeded.

Processing will not be aborted on individual errors, such as the 'cp'
handles it for invalid files.

Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
---
 commands/nv.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/commands/nv.c b/commands/nv.c
index 2ee18187e0..2229adde17 100644
--- a/commands/nv.c
+++ b/commands/nv.c
@@ -28,7 +28,7 @@ static int do_nv(int argc, char *argv[])
 {
 	int opt;
 	int do_remove = 0, do_save = 0;
-	int ret, i;
+	int failed = 0, i;
 	char *value;
 
 	while ((opt = getopt(argc, argv, "rs")) > 0) {
@@ -61,19 +61,29 @@ static int do_nv(int argc, char *argv[])
 		return COMMAND_ERROR_USAGE;
 
 	for (i = 0; i < argc; i++) {
+		int ret;
 		value = strchr(argv[0], '=');
 		if (value) {
 			*value = 0;
 			value++;
 		}
 
-		if (do_remove)
+		if (do_remove) {
 			ret = nvvar_remove(argv[i]);
-		else
+			if (ret) {
+				printf("Failed removing %s: %s\n", argv[i], strerror(-ret));
+				failed = 1;
+			}
+		} else {
 			ret = nvvar_add(argv[i], value);
+			if (ret) {
+				printf("Failed adding %s: %s\n", argv[i], strerror(-ret));
+				failed = 1;
+			}
+		}
 	}
 
-	return ret;
+	return failed;
 }
 
 BAREBOX_CMD_HELP_START(nv)
-- 
2.11.0


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