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

[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 1/5] commands: nv: fail with verbose message if invoked without args

[Enrico Jorns]

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

diff --git a/commands/nv.c b/commands/nv.c
index 37cdb96647..1c5514dd32 100644
--- a/commands/nv.c
+++ b/commands/nv.c
@@ -56,6 +56,7 @@ static int do_nv(int argc, char *argv[])
 	argv += optind;
 
 	if (argc < 1)
+		printf("Invalid usage: Missing argument");
 		return COMMAND_ERROR_USAGE;
 
 	for (i = 0; i < argc; i++) {
-- 
2.11.0


_______________________________________________
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 1c5514dd32..2ee18187e0 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 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

[PATCH 4/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 5/5] 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>
---
 commands/nv.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/commands/nv.c b/commands/nv.c
index 2229adde17..f15b75fe69 100644
--- a/commands/nv.c
+++ b/commands/nv.c
@@ -88,18 +88,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

Re: [PATCH 5/5] commands: nv: adjust command help

[Sam Ravnborg]

Hi enrico

On Mon, Oct 30, 2017 at 11:34:21AM +0100, Enrico Jorns wrote:
> 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>
> ---
...

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

It looks inconsistent that we in one line uses "non volatile" and the next line uses "NV".
I know it was like this before but since touching this line consider changing this
detail too.

I skimmed the other pacthes - looked good to me.

	Sam

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

Re: [PATCH 1/5] commands: nv: fail with verbose message if invoked without args

[Stefan Lengfeld]

Hi Jorns,

On Mon, Oct 30, 2017 at 11:34:17AM +0100, Enrico Jorns wrote:
> Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
> ---
>  commands/nv.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/commands/nv.c b/commands/nv.c
> index 37cdb96647..1c5514dd32 100644
> --- a/commands/nv.c
> +++ b/commands/nv.c
> @@ -56,6 +56,7 @@ static int do_nv(int argc, char *argv[])
>       argv += optind;
>
>       if (argc < 1)
> +             printf("Invalid usage: Missing argument");
>               return COMMAND_ERROR_USAGE;

Missing curly brackets for if clause. Indentation does not matter in C
as it does in for example python. Now the function returns always
'COMMAND_ERROR_USAGE'.

Kind regards
    Stefan

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

Re: [PATCH 1/5] commands: nv: fail with verbose message if invoked without args

[Enrico Jörns]

Hi Lengfeld,

Am 31.10.2017 um 18:51 schrieb Stefan Lengfeld:
> Hi Jorns,
> 
> On Mon, Oct 30, 2017 at 11:34:17AM +0100, Enrico Jorns wrote:
>> Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
>> ---
>>  commands/nv.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/commands/nv.c b/commands/nv.c
>> index 37cdb96647..1c5514dd32 100644
>> --- a/commands/nv.c
>> +++ b/commands/nv.c
>> @@ -56,6 +56,7 @@ static int do_nv(int argc, char *argv[])
>>       argv += optind;
>>
>>       if (argc < 1)
>> +             printf("Invalid usage: Missing argument");
>>               return COMMAND_ERROR_USAGE;
> 
> Missing curly brackets for if clause. Indentation does not matter in C
> as it does in for example python. Now the function returns always
> 'COMMAND_ERROR_USAGE'.

ouh, you are right, must have lost them while merging the patches too
quickly.
But I tried my best to hide it, didn't I?

Best regards, 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

Re: [PATCH 5/5] commands: nv: adjust command help

[Enrico Jörns]

Hi Sam,

Am 30.10.2017 um 17:51 schrieb Sam Ravnborg:
> Hi enrico
> 
> On Mon, Oct 30, 2017 at 11:34:21AM +0100, Enrico Jorns wrote:
>> 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>
>> ---
> ...
> 
>> -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")
> 
> It looks inconsistent that we in one line uses "non volatile" and the next line uses "NV".
> I know it was like this before but since touching this line consider changing this
> detail too.

yes, this is a good point. Indeed it looks a bit inconsistent and should
be touched by this series, too. I will turn a new round.

Best regards, 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

Re: [PATCH 1/5] commands: nv: fail with verbose message if invoked without args

[Stefan Lengfeld]

Hi Enrico,

On Tue, Oct 31, 2017 at 10:47:43PM +0100, Enrico Jörns wrote:
> Hi Lengfeld,
> 
> Am 31.10.2017 um 18:51 schrieb Stefan Lengfeld:
> > Hi Jorns,
> > 
> > On Mon, Oct 30, 2017 at 11:34:17AM +0100, Enrico Jorns wrote:
> >> Signed-off-by: Enrico Jorns <ejo@pengutronix.de>
> >> ---
> >>  commands/nv.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/commands/nv.c b/commands/nv.c
> >> index 37cdb96647..1c5514dd32 100644
> >> --- a/commands/nv.c
> >> +++ b/commands/nv.c
> >> @@ -56,6 +56,7 @@ static int do_nv(int argc, char *argv[])
> >>       argv += optind;
> >>
> >>       if (argc < 1)
> >> +             printf("Invalid usage: Missing argument");
> >>               return COMMAND_ERROR_USAGE;
> > 
> > Missing curly brackets for if clause. Indentation does not matter in C
> > as it does in for example python. Now the function returns always
> > 'COMMAND_ERROR_USAGE'.
> 
> ouh, you are right, must have lost them while merging the patches too
> quickly.
> But I tried my best to hide it, didn't I?

<joke>
Yeah. Maybe we should start a working group: How to hide non-obvious
functionality in obvious looking patches and invite the 'goto fail;'
guys from apple.
</joke>

Recents version of gcc (>=6) have a compiler warning
'-Wmisleading-indentation' to catch theses errors. [1] 

Kind regards
	Stefan

[1] https://www.phoronix.com/scan.php?page=news_item&px=GCC6-Misleading-Indentation

_______________________________________________
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