| Message ID | 20190717082525.891-4-takahiro.akashi@linaro.org |
|---|---|
| State | RFC |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: non-volatile variables support | expand |
Dear Takahiro, In message <20190717082525.891-4-takahiro.akashi@linaro.org> you wrote: > The following interfaces are extended to allow for accepting an additional > argument, env_context. > env_get() -> env_get_ext() > env_set() -> env_get_ext() Please don't, see previous comments. > Relevant env commands are synced with this change to maintain the semantics > of existing U-Boot environment. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > cmd/nvedit.c | 82 ++++++++++++++++++++++++++++++++++------------- > include/exports.h | 3 ++ > 2 files changed, 62 insertions(+), 23 deletions(-) > > diff --git a/cmd/nvedit.c b/cmd/nvedit.c > index 49d3b5bdf466..cc80ba712767 100644 > --- a/cmd/nvedit.c > +++ b/cmd/nvedit.c > @@ -87,7 +87,7 @@ int get_env_id(void) > * > * Returns 0 in case of error, or length of printed string > */ > -static int env_print(char *name, int flag) > +static int env_print_ext(enum env_context ctx, char *name, int flag) > { > char *res = NULL; > ssize_t len; > @@ -96,6 +96,7 @@ static int env_print(char *name, int flag) > ENTRY e, *ep; > > e.key = name; > + e.context = ctx; > e.data = NULL; > hsearch_r(e, FIND, &ep, &env_htab, flag); > if (ep == NULL) > -#if defined(CONFIG_CMD_NVEDIT_EFI) > - if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') > - return do_env_print_efi(cmdtp, flag, --argc, ++argv); > -#endif > + if (ctx == ENVCTX_UEFI) > + return do_env_print_efi(cmdtp, flag, argc, argv); I don't like this. It doesn't scale. ENVCTX_UEFI is just one context among others; it should not need a "if" here to handle. > +{ > +#if defined(CONFIG_CMD_NVEDIT_EFI) > + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') Please use proper argument handling, options can be combined, so the 'e' might not be the second character. Also, this should probably changed to support a generic "context" specific handling, though "-c context" or such, with U-Boot being the default. This again allows to get rid of all these "if"s > -#if CONFIG_IS_ENABLED(CMD_NVEDIT_EFI) > - if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') > - return do_env_set_efi(NULL, flag, --argc, ++argv); > -#endif > + if (ctx == ENVCTX_UEFI) > + return do_env_set_efi(NULL, flag, argc, argv); Ditto here. > +#if CONFIG_IS_ENABLED(CMD_NVEDIT_EFI) > + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') > + return _do_env_set(flag, --argc, ++argv, H_INTERACTIVE, > + ENVCTX_UEFI); > + else > +#endif And here. > const char * const _argv[3] = { "setenv", argv[1], NULL }; > > - return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE); > + return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE, > + ENVCTX_UBOOT); > } else { > const char * const _argv[4] = { "setenv", argv[1], buffer, > NULL }; > > - return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE); > + return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE, > + ENVCTX_UBOOT); Also here. ENVCTX_UBOOT is not a special context and should not need special handling. Best regards, Wolfgang Denk
On Fri, Jul 19, 2019 at 10:09:19AM +0200, Wolfgang Denk wrote: > Dear Takahiro, > > In message <20190717082525.891-4-takahiro.akashi@linaro.org> you wrote: > > The following interfaces are extended to allow for accepting an additional > > argument, env_context. > > env_get() -> env_get_ext() > > env_set() -> env_get_ext() > > Please don't, see previous comments. NAK again:) > > Relevant env commands are synced with this change to maintain the semantics > > of existing U-Boot environment. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > cmd/nvedit.c | 82 ++++++++++++++++++++++++++++++++++------------- > > include/exports.h | 3 ++ > > 2 files changed, 62 insertions(+), 23 deletions(-) > > > > diff --git a/cmd/nvedit.c b/cmd/nvedit.c > > index 49d3b5bdf466..cc80ba712767 100644 > > --- a/cmd/nvedit.c > > +++ b/cmd/nvedit.c > > @@ -87,7 +87,7 @@ int get_env_id(void) > > * > > * Returns 0 in case of error, or length of printed string > > */ > > -static int env_print(char *name, int flag) > > +static int env_print_ext(enum env_context ctx, char *name, int flag) > > { > > char *res = NULL; > > ssize_t len; > > @@ -96,6 +96,7 @@ static int env_print(char *name, int flag) > > ENTRY e, *ep; > > > > e.key = name; > > + e.context = ctx; > > e.data = NULL; > > hsearch_r(e, FIND, &ep, &env_htab, flag); > > if (ep == NULL) > > -#if defined(CONFIG_CMD_NVEDIT_EFI) > > - if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') > > - return do_env_print_efi(cmdtp, flag, --argc, ++argv); > > -#endif > > + if (ctx == ENVCTX_UEFI) > > + return do_env_print_efi(cmdtp, flag, argc, argv); > > I don't like this. It doesn't scale. ENVCTX_UEFI is just one > context among others; it should not need a "if" here to handle. Unfortunately, this is necessary. As you know, we want to allow different implementations of UEFI variables while we want to use "env" commands in the same way. do_env_print_efi(), for example, is implemented using *UEFI interfaces*, neither env_*() nor h*_r(). > > +{ > > +#if defined(CONFIG_CMD_NVEDIT_EFI) > > + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') > > Please use proper argument handling, options can be combined, so the > 'e' might not be the second character. I think there are bunch of code like this in U-Boot. No? > Also, this should probably changed to support a generic "context" > specific handling, though "-c context" or such, with U-Boot being > the default. Yes, but please note that this option, -e, is already merged in the upstream. > This again allows to get rid of all these "if"s I agree, but only when yet another context be introduced. Thanks, -Takahiro Akashi > > -#if CONFIG_IS_ENABLED(CMD_NVEDIT_EFI) > > - if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') > > - return do_env_set_efi(NULL, flag, --argc, ++argv); > > -#endif > > + if (ctx == ENVCTX_UEFI) > > + return do_env_set_efi(NULL, flag, argc, argv); > > Ditto here. > > > +#if CONFIG_IS_ENABLED(CMD_NVEDIT_EFI) > > + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') > > + return _do_env_set(flag, --argc, ++argv, H_INTERACTIVE, > > + ENVCTX_UEFI); > > + else > > +#endif > > And here. > > > const char * const _argv[3] = { "setenv", argv[1], NULL }; > > > > - return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE); > > + return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE, > > + ENVCTX_UBOOT); > > } else { > > const char * const _argv[4] = { "setenv", argv[1], buffer, > > NULL }; > > > > - return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE); > > + return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE, > > + ENVCTX_UBOOT); > > Also here. ENVCTX_UBOOT is not a special context and should not need > special handling. > > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > I've got to get something inside me. Some coffee or something. And > then the world will somehow be better. > - Terry Pratchett, _Men at Arms_
Dear Takahiro, In message <20190719082504.GS21948@linaro.org> you wrote: > > > > + if (ctx == ENVCTX_UEFI) > > > + return do_env_print_efi(cmdtp, flag, argc, argv); > > > > I don't like this. It doesn't scale. ENVCTX_UEFI is just one > > context among others; it should not need a "if" here to handle. > > Unfortunately, this is necessary. > As you know, we want to allow different implementations of UEFI > variables while we want to use "env" commands in the same way. > do_env_print_efi(), for example, is implemented using *UEFI interfaces*, > neither env_*() nor h*_r(). It is certainly not necessary to add conditional code here for each and every potential context. It should be able to provide a generic function, and each context can either use the default code, or implement his own. Then all you have to do here is to call the context's specific function pointer. No need for conditional code. > > > +#if defined(CONFIG_CMD_NVEDIT_EFI) > > > + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') > > > > Please use proper argument handling, options can be combined, so the > > 'e' might not be the second character. > > I think there are bunch of code like this in U-Boot. No? Maybe, but this does not make it better / correct. > > Also, this should probably changed to support a generic "context" > > specific handling, though "-c context" or such, with U-Boot being > > the default. > > Yes, but please note that this option, -e, is already merged > in the upstream. This can be fixed, then? > > This again allows to get rid of all these "if"s > > I agree, but only when yet another context be introduced. No, we should do it right from the beginning. It is not friendly to implement quick and dirty code and let the next who wants to use it do the cleanup. Best regards, Wolfgang Denk
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 49d3b5bdf466..cc80ba712767 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -87,7 +87,7 @@ int get_env_id(void) * * Returns 0 in case of error, or length of printed string */ -static int env_print(char *name, int flag) +static int env_print_ext(enum env_context ctx, char *name, int flag) { char *res = NULL; ssize_t len; @@ -96,6 +96,7 @@ static int env_print(char *name, int flag) ENTRY e, *ep; e.key = name; + e.context = ctx; e.data = NULL; hsearch_r(e, FIND, &ep, &env_htab, flag); if (ep == NULL) @@ -105,7 +106,7 @@ static int env_print(char *name, int flag) } /* print whole list */ - len = hexport_r(&env_htab, '\n', flag, &res, 0, 0, NULL); + len = hexport_ext(&env_htab, ctx, '\n', flag, &res, 0, 0, NULL); if (len > 0) { puts(res); @@ -118,17 +119,15 @@ static int env_print(char *name, int flag) return 0; } -static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc, - char * const argv[]) +static int do_env_print_ext(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[], enum env_context ctx) { int i; int rcode = 0; int env_flag = H_HIDE_DOT; -#if defined(CONFIG_CMD_NVEDIT_EFI) - if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') - return do_env_print_efi(cmdtp, flag, --argc, ++argv); -#endif + if (ctx == ENVCTX_UEFI) + return do_env_print_efi(cmdtp, flag, argc, argv); if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'a') { argc--; @@ -138,7 +137,7 @@ static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc, if (argc == 1) { /* print all env vars */ - rcode = env_print(NULL, env_flag); + rcode = env_print_ext(ctx, NULL, env_flag); if (!rcode) return 1; printf("\nEnvironment size: %d/%ld bytes\n", @@ -149,7 +148,7 @@ static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc, /* print selected env vars */ env_flag &= ~H_HIDE_DOT; for (i = 1; i < argc; ++i) { - int rc = env_print(argv[i], env_flag); + int rc = env_print_ext(ctx, argv[i], env_flag); if (!rc) { printf("## Error: \"%s\" not defined\n", argv[i]); ++rcode; @@ -159,6 +158,19 @@ static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc, return rcode; } +static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ +#if defined(CONFIG_CMD_NVEDIT_EFI) + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') + return do_env_print_ext(cmdtp, flag, --argc, ++argv, + ENVCTX_UEFI); + else +#endif + + return do_env_print_ext(cmdtp, flag, argc, argv, ENVCTX_UBOOT); +} + #ifdef CONFIG_CMD_GREPENV static int do_env_grep(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) @@ -220,7 +232,8 @@ DONE: * Set a new environment variable, * or replace or delete an existing one. */ -static int _do_env_set(int flag, int argc, char * const argv[], int env_flag) +static int _do_env_set(int flag, int argc, char * const argv[], int env_flag, + enum env_context ctx) { int i, len; char *name, *value, *s; @@ -228,10 +241,8 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag) debug("Initial value for argc=%d\n", argc); -#if CONFIG_IS_ENABLED(CMD_NVEDIT_EFI) - if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') - return do_env_set_efi(NULL, flag, --argc, ++argv); -#endif + if (ctx == ENVCTX_UEFI) + return do_env_set_efi(NULL, flag, argc, argv); while (argc > 1 && **(argv + 1) == '-') { char *arg = *++argv; @@ -286,6 +297,7 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag) *--s = '\0'; e.key = name; + e.context = ctx; e.data = value; hsearch_r(e, ENTER, &ep, &env_htab, env_flag); free(value); @@ -298,7 +310,8 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag) return 0; } -int env_set(const char *varname, const char *varvalue) +int env_set_ext(const enum env_context ctx, + const char *varname, const char *varvalue) { const char * const argv[4] = { "setenv", varname, varvalue, NULL }; @@ -307,9 +320,16 @@ int env_set(const char *varname, const char *varvalue) return 1; if (varvalue == NULL || varvalue[0] == '\0') - return _do_env_set(0, 2, (char * const *)argv, H_PROGRAMMATIC); + return _do_env_set(0, 2, (char * const *)argv, H_PROGRAMMATIC, + ctx); else - return _do_env_set(0, 3, (char * const *)argv, H_PROGRAMMATIC); + return _do_env_set(0, 3, (char * const *)argv, H_PROGRAMMATIC, + ctx); +} + +int env_set(const char *varname, const char *varvalue) +{ + return env_set_ext(ENVCTX_UBOOT, varname, varvalue); } /** @@ -393,7 +413,14 @@ static int do_env_set(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE; - return _do_env_set(flag, argc, argv, H_INTERACTIVE); +#if CONFIG_IS_ENABLED(CMD_NVEDIT_EFI) + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e') + return _do_env_set(flag, --argc, ++argv, H_INTERACTIVE, + ENVCTX_UEFI); + else +#endif + + return _do_env_set(flag, argc, argv, H_INTERACTIVE, ENVCTX_UBOOT); } /* @@ -471,7 +498,7 @@ int do_env_ask(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } /* Continue calling setenv code */ - return _do_env_set(flag, len, local_args, H_INTERACTIVE); + return _do_env_set(flag, len, local_args, H_INTERACTIVE, ENVCTX_UBOOT); } #endif @@ -654,12 +681,14 @@ static int do_env_edit(cmd_tbl_t *cmdtp, int flag, int argc, if (buffer[0] == '\0') { const char * const _argv[3] = { "setenv", argv[1], NULL }; - return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE); + return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE, + ENVCTX_UBOOT); } else { const char * const _argv[4] = { "setenv", argv[1], buffer, NULL }; - return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE); + return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE, + ENVCTX_UBOOT); } } #endif /* CONFIG_CMD_EDITENV */ @@ -670,7 +699,7 @@ static int do_env_edit(cmd_tbl_t *cmdtp, int flag, int argc, * return address of storage for that variable, * or NULL if not found */ -char *env_get(const char *name) +char *env_get_ext(const enum env_context ctx, const char *name) { if (gd->flags & GD_FLG_ENV_READY) { /* after import into hashtable */ ENTRY e, *ep; @@ -678,6 +707,7 @@ char *env_get(const char *name) WATCHDOG_RESET(); e.key = name; + e.context = ctx; e.data = NULL; hsearch_r(e, FIND, &ep, &env_htab, 0); @@ -691,6 +721,11 @@ char *env_get(const char *name) return NULL; } +char *env_get(const char *name) +{ + return env_get_ext(ENVCTX_UBOOT, name); +} + /* * Look up variable from environment for restricted C runtime env. */ @@ -1173,6 +1208,7 @@ static int do_env_exists(cmd_tbl_t *cmdtp, int flag, int argc, return CMD_RET_USAGE; e.key = argv[1]; + e.context = ENVCTX_UBOOT; e.data = NULL; hsearch_r(e, FIND, &ep, &env_htab, 0); diff --git a/include/exports.h b/include/exports.h index a4b862f19178..0c39d9f16f3d 100644 --- a/include/exports.h +++ b/include/exports.h @@ -26,8 +26,11 @@ unsigned long get_timer(unsigned long); int vprintf(const char *, va_list); unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base); int strict_strtoul(const char *cp, unsigned int base, unsigned long *res); +enum env_context; /* defined in environment.h */ char *env_get(const char *name); +char *env_get_ext(enum env_context, const char *name); int env_set(const char *varname, const char *value); +int env_set_ext(enum env_context, const char *varname, const char *value); long simple_strtol(const char *cp, char **endp, unsigned int base); int strcmp(const char *cs, const char *ct); unsigned long ustrtoul(const char *cp, char **endp, unsigned int base);
The following interfaces are extended to allow for accepting an additional argument, env_context. env_get() -> env_get_ext() env_set() -> env_get_ext() Relevant env commands are synced with this change to maintain the semantics of existing U-Boot environment. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- cmd/nvedit.c | 82 ++++++++++++++++++++++++++++++++++------------- include/exports.h | 3 ++ 2 files changed, 62 insertions(+), 23 deletions(-)