| Message ID | 1316703972-8417-2-git-send-email-gerlando.falauto@keymile.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Gerlando Falauto, In message <1316703972-8417-2-git-send-email-gerlando.falauto@keymile.com> you wrote: > implement command "env default <vars>..." for resetting individual > variables to their default values. > > "env default -f" will always keep resetting the whole environment > to default. I don't think this is a good idea. The "-f" should be used to allow resetting write-protected variables like serial# or ethaddr. With no variable names given, it will therefore always be needed (assuming you have such protected variables in your environment). But it should also work when arguments are given, i. e. env default ethaddr is supposed to fail, because ethaddr is write protected. To reset this, you should use env default -f ethaddr > +#ifdef CONFIG_CMD_DEFAULTENV_VARS > +/* > + * import individual variables from an external environment > + * (e.g. default environment). > + * Most of this code comes straight from himport_r(). Indeed. This is way too much code copied. Please factor this out into a separate function to avoid such duplication (which is always a maintenance nightmare). ... > + /* deal with "name" and "name=" entries (delete var) */ > + if (*dp == '\0' || *(dp + 1) == '\0' || > + *dp == sep || *(dp + 1) == sep) { > + if (*dp == '=') > + *dp++ = '\0'; > + *dp++ = '\0'; /* terminate name */ Here the comment (about the "delete var") is even wrong. > + /* > + * size check needed for text without '\0' termination > + * (e.g. default environment) > + */ What makes you think the default environment was NOT '\0' terminated? Best regards, Wolfgang Denk
On 09/22/2011 09:51 PM, Wolfgang Denk wrote: > > "env default -f" will always keep resetting the whole environment > > to default. > > I don't think this is a good idea. > > The "-f" should be used to allow resetting write-protected variables > like serial# or ethaddr. I see what you mean, it's the same rationale behind env set [-f] var right? Point is, has "-f" ever been implemented in "env set"? => env set -f ethaddr => printenv -f=ethaddr ... > With no variable names given, it will therefore always be needed > (assuming you have such protected variables in your environment). OK, so "env default -f" should reset all variables, including protected ones. Should there also be some way to reset *ALL BUT* protected variables? Like "env default" or "env default all" as suggested in the wiki page? Or such operation doesn't make any sense indeed (i.e., all variables means ALL OF THEM)? >> +#ifdef CONFIG_CMD_DEFAULTENV_VARS >> +/* >> + * import individual variables from an external environment >> + * (e.g. default environment). >> + * Most of this code comes straight from himport_r(). > > Indeed. This is way too much code copied. Please factor this out > into a separate function to avoid such duplication (which is always a > maintenance nightmare). While we're on this subject, do you think it would make any sense to *import* individual variables? If yes, what could the syntax be? env import [flags] addr [size] <vars...> would make the optional argument "size" ambiguous (unless we take for granted that variable names cannot start with a digit...). > ... >> + /* deal with "name" and "name=" entries (delete var) */ >> + if (*dp == '\0' || *(dp + 1) == '\0' || >> + *dp == sep || *(dp + 1) == sep) { >> + if (*dp == '=') >> + *dp++ = '\0'; >> + *dp++ = '\0'; /* terminate name */ > > Here the comment (about the "delete var") is even wrong. What do you mean? It's what himport_r() does (but please see also my final comments). > >> + /* >> + * size check needed for text without '\0' termination >> + * (e.g. default environment) >> + */ > > What makes you think the default environment was NOT '\0' terminated? Sorry, my mistake. Anyway, the whole thing was based on the assumption that it could make some sense at some point to import individual variables from a downloaded file. If that's the case, code should be factored together with himport_r() as you suggested. If not, most of this code (the part checking for comments, tabs, '\0'...) can be safely deleted (and I don't think factoring the rest would be necessary). Thank you, Gerlando Falauto
Dear Gerlando Falauto, In message <4E7C35E6.5030808@keymile.com> you wrote: > > I see what you mean, it's the same rationale behind > > env set [-f] var > > right? Point is, has "-f" ever been implemented in "env set"? No, it has not (and there is also no such thing as "setenv -f ..."). Not yet, that is :-) > OK, so "env default -f" should reset all variables, including protected > ones. Should there also be some way to reset *ALL BUT* protected variables? > Like "env default" or "env default all" as suggested in the wiki page? Yes, that would be nice. With a warning about the not changed ones. > While we're on this subject, do you think it would make any sense to > *import* individual variables? Tough question. In theory yes, it would make perfect sense. On the other hand, this is a boot loader, and we should not try to be feature-complete. So far I haven't seen a use case for this. > If yes, what could the syntax be? > > env import [flags] addr [size] <vars...> env import -n name[,..] [other_flags] addr [size] ? > would make the optional argument "size" ambiguous (unless we take for > granted that variable names cannot start with a digit...). But they can, at least until now. > > ... > >> + /* deal with "name" and "name=" entries (delete var) */ > >> + if (*dp == '\0' || *(dp + 1) == '\0' || > >> + *dp == sep || *(dp + 1) == sep) { > >> + if (*dp == '=') > >> + *dp++ = '\0'; > >> + *dp++ = '\0'; /* terminate name */ > > > > Here the comment (about the "delete var") is even wrong. > > What do you mean? It's what himport_r() does (but please see also my > final comments). Yes, but himport_r() actually does the delete: ... if (hdelete_r(name, htab) == 0) debug("DELETE ERROR ##############################\n"); ... You removed this part, so the comment became wrong. > Anyway, the whole thing was based on the assumption that it could make > some sense at some point to import individual variables from a > downloaded file. If that's the case, code should be factored together > with himport_r() as you suggested. If not, most of this code (the part > checking for comments, tabs, '\0'...) can be safely deleted (and I don't > think factoring the rest would be necessary). I agree that it makes sense to generalize and clean up this interface. It makes sense to select individual variables, and it makes sense to unify the "-f" handling to enforce actions on protected variables (while without "-f" only actions on the "normal" variables should be done). I can even imagine introducing a new variable that contains the name of the write-protected variables (and probably other properties, like being excluded from saveenv, etc.) - this has been discussed a number of times before, now we have the code base in place to actually implement it. All we need to do is extend the struct entry (in "include/search.h") by an "int flags"), and we can there register properties like read-only, don't-save etc. In a first step this could be added transparently - so we could remove all the special handling of "ethaddr", "serial#" etc. in common/cmd_nvedit.c; then we could unify this to include "eth1addr" etc as well; then we could extend it to read the names of such variables and their properties from a variable, etc. ... there is plenty of ideas someone could pick up... Best regards, Wolfgang Denk
On 09/23/2011 11:55 AM, Wolfgang Denk wrote: [...] >> While we're on this subject, do you think it would make any sense to >> *import* individual variables? > > Tough question. In theory yes, it would make perfect sense. On the > other hand, this is a boot loader, and we should not try to be > feature-complete. So far I haven't seen a use case for this. Well, at Keymile we are essentially using "env import" as a way to have "default" variables stored in an external file (e.g., because they are only needed for development and not for production, and that not only reduces the site of the environments, but also makes updates easier). To this extent, "env import" and "env default" sort of have the same purpose... >> If yes, what could the syntax be? >> >> env import [flags] addr [size]<vars...> > > env import -n name[,..] [other_flags] addr [size] > > ? Uhm, wouldn't that make the syntax completely unrelated to all other commands, leading to confusion? How about something like: env import [-f] [flags] addr [size] [-n name[ ...]] env default [-f] -a|-n name[ ...] env set [-f] name [val ...] ? Where: -a in "env default" would be the way to prevent the inadverent user from wrecking the environment by mistake. -f forces overwriting of read-only or write-once variables -n allows to be selective about which variables should be restored from default/external environment >> would make the optional argument "size" ambiguous (unless we take for >> granted that variable names cannot start with a digit...). > > But they can, at least until now. Slightly off-topic: how about variables starting with "-"? [...] >> Anyway, the whole thing was based on the assumption that it could make >> some sense at some point to import individual variables from a >> downloaded file. If that's the case, code should be factored together >> with himport_r() as you suggested. If not, most of this code (the part >> checking for comments, tabs, '\0'...) can be safely deleted (and I don't >> think factoring the rest would be necessary). > > I agree that it makes sense to generalize and clean up this interface. > It makes sense to select individual variables, and it makes sense to > unify the "-f" handling to enforce actions on protected variables > (while without "-f" only actions on the "normal" variables should be > done). > > I can even imagine introducing a new variable that contains the name > of the write-protected variables (and probably other properties, like > being excluded from saveenv, etc.) - this has been discussed a number > of times before, now we have the code base in place to actually > implement it. > > All we need to do is extend the struct entry (in "include/search.h") > by an "int flags"), and we can there register properties like > read-only, don't-save etc. In a first step this could be added > transparently - so we could remove all the special handling of > "ethaddr", "serial#" etc. in common/cmd_nvedit.c; then we could unify > this to include "eth1addr" etc as well; then we could extend it to > read the names of such variables and their properties from a variable, > etc. > > ... there is plenty of ideas someone could pick up... > OK, one step at a time. But it's definitely worth knowing your thoughts before starting. Thank you for sharing them! Best, Gerlando Falauto
Dear Gerlando Falauto, In message <4E82E6CA.9030802@keymile.com> you wrote: > > > env import -n name[,..] [other_flags] addr [size] > > > > ? > > Uhm, wouldn't that make the syntax completely unrelated to all other > commands, leading to confusion? > > How about something like: > > env import [-f] [flags] addr [size] [-n name[ ...]] > env default [-f] -a|-n name[ ...] > env set [-f] name [val ...] No. That's even worse. If you don't like the comma separation,we could as well accept multiple -n args: env import -n name [-n name1 ... ] [other_flags] addr [size] > Where: > -a in "env default" would be the way to prevent the inadverent user > from wrecking the environment by mistake. What does -a stand for? "all" ? > Slightly off-topic: how about variables starting with "-"? Should be no problem in general, except for the pathological cases like variable names "-f" etc. But there has to be a penalty for such stupid names :-) Best regards, Wolfgang Denk
On 09/28/2011 11:08 PM, Wolfgang Denk wrote: > Dear Gerlando Falauto, > > In message<4E82E6CA.9030802@keymile.com> you wrote: >> >>> env import -n name[,..] [other_flags] addr [size] >>> >>> ? >> >> Uhm, wouldn't that make the syntax completely unrelated to all other >> commands, leading to confusion? >> >> How about something like: >> >> env import [-f] [flags] addr [size] [-n name[ ...]] >> env default [-f] -a|-n name[ ...] >> env set [-f] name [val ...] > > No. That's even worse. You mean that options should always precede the main argument? > If you don't like the comma separation,we > could as well accept multiple -n args: > > env import -n name [-n name1 ... ] [other_flags] addr [size] It's not that I don't like comma separation. It's just that maybe the syntax should be consistent between commands... that's all. But since it's a very unusual (and possibly unused) command, I am probably worrying too much. >> Where: >> -a in "env default" would be the way to prevent the inadverent user >> from wrecking the environment by mistake. > > What does -a stand for? "all" ? Yes. Any comments on this? How should we implement "env default" for the whole env? 1) "env default" 2) "env deafult -a" 3) "env default all" >> Slightly off-topic: how about variables starting with "-"? > > Should be no problem in general, except for the pathological cases > like variable names "-f" etc. But there has to be a penalty for such > stupid names :-) I can't think of any shell or programming language allowing for variables starting with "-" or digits... Thasnks, Gerlando Falauto
diff --git a/README b/README index a43da97..4b28854 100644 --- a/README +++ b/README @@ -705,6 +705,8 @@ The following options need to be configured: CONFIG_CMD_CONSOLE coninfo CONFIG_CMD_CRC32 * crc32 CONFIG_CMD_DATE * support for RTC, date/time... + CONFIG_CMD_DEFAULTENV_VARS + * Reset individual variables to default CONFIG_CMD_DHCP * DHCP support CONFIG_CMD_DIAG * Diagnostics CONFIG_CMD_DS4510 * ds4510 I2C gpio commands diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c index e8b116d..ea1db22 100644 --- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c @@ -581,11 +581,20 @@ int envmatch(uchar *s1, int i2) static int do_env_default(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { - if ((argc != 2) || (strcmp(argv[1], "-f") != 0)) - return cmd_usage(cmdtp); - - set_default_env("## Resetting to default environment\n"); - return 0; + if ((argc == 2) && (strcmp(argv[1], "-f")) == 0) { + /* Reset the whole environment */ + set_default_env("## Resetting to default environment\n"); + return 0; + } +#ifdef CONFIG_CMD_DEFAULTENV_VARS + /* Check that we have at least one argument */ + else if (argc >= 2) { + /* Reset individual variables */ + env_default_vars(argc-1, argv+1); + return 0; + } +#endif + return cmd_usage(cmdtp); } static int do_env_delete(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) @@ -911,6 +920,9 @@ U_BOOT_CMD( "ask name [message] [size] - ask for environment variable\nenv " #endif "default -f - reset default environment\n" +#if defined(CONFIG_CMD_DEFAULTENV_VARS) + "env default var [...] - reset variable(s) to their default value\n" +#endif #if defined(CONFIG_CMD_EDITENV) "env edit name - edit environment variable\n" #endif diff --git a/common/env_common.c b/common/env_common.c index 19149b5..77cc439 100644 --- a/common/env_common.c +++ b/common/env_common.c @@ -196,6 +196,115 @@ void set_default_env(const char *s) gd->flags |= GD_FLG_ENV_READY; } +#ifdef CONFIG_CMD_DEFAULTENV_VARS +/* + * import individual variables from an external environment + * (e.g. default environment). + * Most of this code comes straight from himport_r(). + */ +static int env_import_vars(const char *env, const size_t size, const char sep, + int nvars, char * const vars[]) +{ + char *data, *sp, *dp, *name, *value, *thisvalue; + int i; + + /* we allocate new space to make sure we can write to the array */ + data = malloc(size); + if (data == NULL) { + debug("env_import_vars: can't malloc %d bytes\n", size); + __set_errno(ENOMEM); + return 0; + } + + /* Loop through all passed variables */ + for (i = 0; i < nvars; i++) { + debug("looking for a default value for %s\n", vars[i]); + + memcpy(data, env, size); + dp = data; + + /* + * Unless proven otherwise, this variable + * does not exist in the default env + */ + thisvalue = ""; + /* Parse environment; allow for '\0' and 'sep' as separators */ + do { + /* skip leading white space */ + while ((*dp == ' ') || (*dp == '\t')) + ++dp; + + /* skip comment lines */ + if (*dp == '#') { + while (*dp && (*dp != sep)) + ++dp; + ++dp; + continue; + } + + /* parse name */ + for (name = dp; *dp != '=' && *dp && *dp != sep; ++dp) + ; + + /* deal with "name" and "name=" entries (delete var) */ + if (*dp == '\0' || *(dp + 1) == '\0' || + *dp == sep || *(dp + 1) == sep) { + if (*dp == '=') + *dp++ = '\0'; + *dp++ = '\0'; /* terminate name */ + + /* default is none */ + value = ""; + } else { + *dp++ = '\0'; /* terminate name */ + value = dp; /* value starts here */ + /* parse value; deal with escapes */ + for (sp = dp; *dp && (*dp != sep); ++dp) { + if ((*dp == '\\') && *(dp + 1)) + ++dp; + *sp++ = *dp; + } + *sp++ = '\0'; /* terminate value */ + } + + if (strcmp(name, vars[i]) == 0) { + debug("found variable: %s\n" + "default value: %s\n", name, value); + thisvalue = value; + /* exit from loop parsing the default env */ + break; + } + ++dp; + + } while ((dp < data + size) && *dp); + /* + * size check needed for text without '\0' termination + * (e.g. default environment) + */ + + debug("setting default value: %s=%s\n", vars[i], thisvalue); + setenv(vars[i], thisvalue); + } /* for-loop over i */ + + debug("env_import_vars: free(data = %p)\n", data); + free(data); + debug("env_import_vars: done\n"); + return 1; /* everything OK */ + +} + +/* [re]set individual variables to their value in the default environment */ +int env_default_vars(int nvars, char * const vars[]) +{ + /* Special use-case: import from default environment + (and use \0 as a separator) */ + return env_import_vars((const char *)default_environment, + sizeof(default_environment), '\0', + nvars, vars); +} + +#endif /* CONFIG_CMD_DEFAULTENV_VARS */ + /* * Check if CRC is valid and (if yes) import the environment. * Note that "buf" may or may not be aligned. diff --git a/include/config_cmd_all.h b/include/config_cmd_all.h index 9716f9c..e728eae 100644 --- a/include/config_cmd_all.h +++ b/include/config_cmd_all.h @@ -25,6 +25,7 @@ #define CONFIG_CMD_CDP /* Cisco Discovery Protocol */ #define CONFIG_CMD_CONSOLE /* coninfo */ #define CONFIG_CMD_DATE /* support for RTC, date/time...*/ +#define CONFIG_CMD_DEFAULTENV_VARS /* default individ variables */ #define CONFIG_CMD_DHCP /* DHCP Support */ #define CONFIG_CMD_DIAG /* Diagnostics */ #define CONFIG_CMD_DISPLAY /* Display support */ diff --git a/include/environment.h b/include/environment.h index 6394a96..0be92b6 100644 --- a/include/environment.h +++ b/include/environment.h @@ -171,6 +171,11 @@ void env_crc_update (void); /* [re]set to the default environment */ void set_default_env(const char *s); +#ifdef CONFIG_CMD_DEFAULTENV_VARS +/* [re]set individual variables to their value in the default environment */ +int env_default_vars(int nvars, char * const vars[]); +#endif + /* Import from binary representation into hash table */ int env_import(const char *buf, int check);