* [PATCH 1/5] commands: nv: fail with verbose message if invoked without args
2017-10-30 10:34 [PATCH v2 0/5] Fixes for nv command handling Enrico Jorns
@ 2017-10-30 10:34 ` Enrico Jorns
2017-10-31 17:51 ` Stefan Lengfeld
2017-10-30 10:34 ` [PATCH 2/5] commands: nv: call nvvar_print() only if no argument is given Enrico Jorns
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Enrico Jorns @ 2017-10-30 10:34 UTC (permalink / raw)
To: barebox; +Cc: 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] commands: nv: fail with verbose message if invoked without args
2017-10-30 10:34 ` [PATCH 1/5] commands: nv: fail with verbose message if invoked without args Enrico Jorns
@ 2017-10-31 17:51 ` Stefan Lengfeld
2017-10-31 21:47 ` Enrico Jörns
0 siblings, 1 reply; 11+ messages in thread
From: Stefan Lengfeld @ 2017-10-31 17:51 UTC (permalink / raw)
To: barebox; +Cc: ejo
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] commands: nv: fail with verbose message if invoked without args
2017-10-31 17:51 ` Stefan Lengfeld
@ 2017-10-31 21:47 ` Enrico Jörns
2017-11-01 19:43 ` Stefan Lengfeld
0 siblings, 1 reply; 11+ messages in thread
From: Enrico Jörns @ 2017-10-31 21:47 UTC (permalink / raw)
To: Stefan Lengfeld, barebox
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] commands: nv: fail with verbose message if invoked without args
2017-10-31 21:47 ` Enrico Jörns
@ 2017-11-01 19:43 ` Stefan Lengfeld
0 siblings, 0 replies; 11+ messages in thread
From: Stefan Lengfeld @ 2017-11-01 19:43 UTC (permalink / raw)
To: Enrico Jörns; +Cc: barebox
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/5] commands: nv: call nvvar_print() only if no argument is given
2017-10-30 10:34 [PATCH v2 0/5] Fixes for nv command handling Enrico Jorns
2017-10-30 10:34 ` [PATCH 1/5] commands: nv: fail with verbose message if invoked without args Enrico Jorns
@ 2017-10-30 10:34 ` Enrico Jorns
2017-10-30 10:34 ` [PATCH 3/5] commands: nv: assure error code will be returned when an error occurred Enrico Jorns
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Enrico Jorns @ 2017-10-30 10:34 UTC (permalink / raw)
To: barebox; +Cc: 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/5] commands: nv: assure error code will be returned when an error occurred
2017-10-30 10:34 [PATCH v2 0/5] Fixes for nv command handling Enrico Jorns
2017-10-30 10:34 ` [PATCH 1/5] commands: nv: fail with verbose message if invoked without args Enrico Jorns
2017-10-30 10:34 ` [PATCH 2/5] commands: nv: call nvvar_print() only if no argument is given Enrico Jorns
@ 2017-10-30 10:34 ` Enrico Jorns
2017-10-30 10:34 ` [PATCH 4/5] common: globvar: let nvvar_remove() report non-existing variable Enrico Jorns
2017-10-30 10:34 ` [PATCH 5/5] commands: nv: adjust command help Enrico Jorns
4 siblings, 0 replies; 11+ messages in thread
From: Enrico Jorns @ 2017-10-30 10:34 UTC (permalink / raw)
To: barebox; +Cc: 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/5] common: globvar: let nvvar_remove() report non-existing variable
2017-10-30 10:34 [PATCH v2 0/5] Fixes for nv command handling Enrico Jorns
` (2 preceding siblings ...)
2017-10-30 10:34 ` [PATCH 3/5] commands: nv: assure error code will be returned when an error occurred Enrico Jorns
@ 2017-10-30 10:34 ` Enrico Jorns
2017-10-30 10:34 ` [PATCH 5/5] commands: nv: adjust command help Enrico Jorns
4 siblings, 0 replies; 11+ messages in thread
From: Enrico Jorns @ 2017-10-30 10:34 UTC (permalink / raw)
To: barebox; +Cc: 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 5/5] commands: nv: adjust command help
2017-10-30 10:34 [PATCH v2 0/5] Fixes for nv command handling Enrico Jorns
` (3 preceding siblings ...)
2017-10-30 10:34 ` [PATCH 4/5] common: globvar: let nvvar_remove() report non-existing variable Enrico Jorns
@ 2017-10-30 10:34 ` Enrico Jorns
2017-10-30 16:51 ` Sam Ravnborg
4 siblings, 1 reply; 11+ messages in thread
From: Enrico Jorns @ 2017-10-30 10:34 UTC (permalink / raw)
To: barebox; +Cc: 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 5/5] commands: nv: adjust command help
2017-10-30 10:34 ` [PATCH 5/5] commands: nv: adjust command help Enrico Jorns
@ 2017-10-30 16:51 ` Sam Ravnborg
2017-10-31 21:52 ` Enrico Jörns
0 siblings, 1 reply; 11+ messages in thread
From: Sam Ravnborg @ 2017-10-30 16:51 UTC (permalink / raw)
To: Enrico Jorns; +Cc: barebox
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 5/5] commands: nv: adjust command help
2017-10-30 16:51 ` Sam Ravnborg
@ 2017-10-31 21:52 ` Enrico Jörns
0 siblings, 0 replies; 11+ messages in thread
From: Enrico Jörns @ 2017-10-31 21:52 UTC (permalink / raw)
To: Sam Ravnborg; +Cc: barebox
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
^ permalink raw reply [flat|nested] 11+ messages in thread