| Message ID | 1323264605-13541-2-git-send-email-gerlando.falauto@keymile.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Wolfgang Denk |
| Headers | show |
Hi, On Wed, Dec 7, 2011 at 5:30 AM, Gerlando Falauto <gerlando.falauto@keymile.com> wrote: > The logic of checking special parameters (e.g. baudrate, stdin, stdout, > for a valid value and/or whether can be overwritten) and applying the > new value to the running system is now all within a single function > env_check_apply() which can be called whenever changes are made > to the environment, no matter if by set, default or import. Thanks for the rebase - I was able to try this out. > > With this patch env_check_apply() is only called by "env set", > retaining previous behavior. > > Also allow for selectively importing/resetting variables. > > So add 3 new arguments to himport_r(): > o "nvars", "vars":, number and list of variables to take into account > (0 means ALL) > > o "apply" callback function to check whether a variable can be > overwritten, and possibly immediately apply the changes; > when NULL, no check is performed. > > Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com> Tested-by: Simon Glass <sjg@chromium.org> > --- > common/cmd_nvedit.c | 174 +++++++++++++++++++++++++++++++------------------ > common/env_common.c | 6 +- > include/environment.h | 7 ++ > include/search.h | 17 +++++- > lib/hashtable.c | 43 ++++++++++++- > 5 files changed, 180 insertions(+), 67 deletions(-) > > diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c > index 5995354..a31d413 100644 > --- a/common/cmd_nvedit.c > +++ b/common/cmd_nvedit.c > @@ -197,32 +197,23 @@ static int do_env_grep(cmd_tbl_t *cmdtp, int flag, > #endif > > /* > - * Set a new environment variable, > - * or replace or delete an existing one. > + * Performs consistency checking before setting, replacing, > + * or deleting an environment variable, then (if successful) > + * apply the changes to internals so to make them effective. > + * Code for this function was taken out of _do_env_set(), > + * which now calls it. > + * Also called as a callback function by himport_r(). > + * Returns 0 in case of success, 1 in case of failure. > + * When (flag & H_FORCE) is set, force overwriting of > + * write-once variables. can you word-wrap that to 72 columns perhaps? > diff --git a/include/environment.h b/include/environment.h > index 3c145af..3a3e6b8 100644 > --- a/include/environment.h > +++ b/include/environment.h > @@ -193,6 +193,13 @@ void set_default_env(const char *s); > /* Import from binary representation into hash table */ > int env_import(const char *buf, int check); > > +/* > + * Check if variable "name" can be changed from oldval to newval, > + * and if so, apply the changes (e.g. baudrate) Can you document flag here also please? > @@ -47,6 +47,13 @@ typedef struct entry { > struct _ENTRY; > > /* > + * Callback function to be called for checking whether the given change may > + * be applied or not. Must return 0 for approval, 1 for denial. > + */ > +typedef int (*apply_cb)(const char *name, const char *oldval, > + const char *newval, int flag); can you document args also? > + > +/* > * Family of hash table handling functions. The functions also > * have reentrant counterparts ending with _r. The non-reentrant > * functions all work on a signle internal hashing table. > @@ -94,11 +101,19 @@ extern ssize_t hexport_r(struct hsearch_data *__htab, > const char __sep, char **__resp, size_t __size, > int argc, char * const argv[]); > > +/* > + * nvars, vars: variables to import (nvars == 0 means all) > + * apply_cb: callback function to check validity of the new argument, > + * and possibly apply changes (NULL means accept everything) > + */ What is vars? Is it a NULL terminated list of string pointers? Please document. But actually you already have function comments in the C file so I think you should either add your comments there or (better in my view but I may be alone) move the comments to the header file. > extern int himport_r(struct hsearch_data *__htab, > const char *__env, size_t __size, const char __sep, > - int __flag); > + int __flag, > + int nvars, char * const vars[], > + apply_cb apply); > diff --git a/lib/hashtable.c b/lib/hashtable.c > index abd61c8..75b9b07 100644 > --- a/lib/hashtable.c > +++ b/lib/hashtable.c > @@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, > * himport() > */ > > +/* Check whether variable name is amongst vars[] */ > +static int process_var(const char *name, int nvars, char * const vars[]) > +{ > + int i = 0; blank line here. Can part of this function be #ifdefed to reduce code size when the feature is not required? > + /* No variables specified means process all of them */ > + if (nvars == 0) > + return 1; > + > + for (i = 0; i < nvars; i++) { > + if (!strcmp(name, vars[i])) > + return 1; > + } > + debug("Skipping non-listed variable %s\n", name); and here I think according to Mike > + return 0; > +} > + > /* > * Import linearized data into hash table. > * > @@ -743,10 +763,31 @@ int himport_r(struct hsearch_data *htab, > *sp++ = '\0'; /* terminate value */ > ++dp; > > + /* Skip variables which are not supposed to be treated */ s/treated/processed/ ? + if (!process_var(name, nvars, vars)) + continue; if (hdelete_r(name, htab) == 0) debug("DELETE ERROR ##############################\n"); perhaps: if (process_var(name, nvars, vars) && hdelete_r(name, htab) == 0) debug("DELETE ERROR ##############################\n"); himport_r() is getting a bit overloaded, and it's a shame but I can't think of an easy way to refactor it to reduce the number of args. In a way you are adding a processing option and so you could put the three extra args in a structure and pass a pointer to it (or NULL to skip this feature). Not sure though... Also, for me this patch adds 500 bytes. I wonder if more of the code could made optional? Regards, Simon
On Wednesday 07 December 2011 17:02:06 Simon Glass wrote: > Also, for me this patch adds 500 bytes. I wonder if more of the code > could made optional? yeah, it seems like these new params are only used when CONFIG_CMD_DEFAULTENV_VARS is defined. which means for most people i think, this is unused bloat :(. i wonder if we can do better somehow without having to resort to putting these behind #ifdef's. -mike
On 12/07/2011 11:02 PM, Simon Glass wrote: [...] >> /* >> - * Set a new environment variable, >> - * or replace or delete an existing one. >> + * Performs consistency checking before setting, replacing, >> + * or deleting an environment variable, then (if successful) >> + * apply the changes to internals so to make them effective. >> + * Code for this function was taken out of _do_env_set(), >> + * which now calls it. >> + * Also called as a callback function by himport_r(). >> + * Returns 0 in case of success, 1 in case of failure. >> + * When (flag& H_FORCE) is set, force overwriting of >> + * write-once variables. > > can you word-wrap that to 72 columns perhaps? Sorry, I am little confused now. What is the maximum line length? [...] >> +/* Check whether variable name is amongst vars[] */ >> +static int process_var(const char *name, int nvars, char * const vars[]) >> +{ >> + int i = 0; > > blank line here. Thanks, I didn't know about this. > Can part of this function be #ifdefed to reduce code > size when the feature is not required? Uhm, I don't think so. This would be a common feature for selectively importing & setting back to default. Unless we want to #ifdef both of them. Personally, I would #ifdef neither. [...] > > + if (!process_var(name, nvars, vars)) > + continue; > > if (hdelete_r(name, htab) == 0) > debug("DELETE ERROR > ##############################\n"); > > perhaps: > > if (process_var(name, nvars, vars)&& > hdelete_r(name, htab) == 0) > debug("DELETE ERROR ##############################\n"); I think it's easier to read it the original way, and it should not make any difference as far as code size is concerned. > himport_r() is getting a bit overloaded, Actually, I believe it makes no longer sense to have it called "_r", as it was the original reference to the function being recursively calleable (i.e. reentrant) as opposed to other versions which were not. > and it's a shame but I can't > think of an easy way to refactor it to reduce the number of args. In a > way you are adding a processing option and so you could put the three > extra args in a structure and pass a pointer to it (or NULL to skip > this feature). Not sure though... Can't think of any other way either, except maybe renaming it and re-implementing the shorter version as a wrapper around the newly named function. I had already done that, but there would be very few places where the old syntax would be kept, so it just didn't make much sense. > Also, for me this patch adds 500 bytes. I wonder if more of the code > could made optional? Frankly, I'm surprised to hear this adds that much overhead. Actually, I can't see this increase in code size as you mention. What architecture are you referring to? In my workspace (ppc_6xx) u-boot.bin and a stripped u-boot ELF file are surprisingly unchanged in size, even with debug #defined! Only place I can experience such growth is within unstripped u-boot ELF, which I believe shouldn't really matter... or should it? Best, Gerlando
Dear Gerlando Falauto, In message <4EE5CA38.6090807@keymile.com> you wrote: > > > if (process_var(name, nvars, vars)&& > > hdelete_r(name, htab) == 0) > > debug("DELETE ERROR ##############################\n"); This is incorrect indentation. > I think it's easier to read it the original way, and it should not make > any difference as far as code size is concerned. The Coding Style makes an explicit exception regarding the line length for user visible strings: 83 Statements longer than 80 columns will be broken into sensible chunks, unless 84 exceeding 80 columns significantly increases readability and does not hide 85 information. Descendants are always substantially shorter than the parent and 86 are placed substantially to the right. The same applies to function headers 87 with a long argument list. However, never break user-visible strings such as 88 printk messages, because that breaks the ability to grep for them. > > himport_r() is getting a bit overloaded, > > Actually, I believe it makes no longer sense to have it called "_r", as > it was the original reference to the function being recursively > calleable (i.e. reentrant) as opposed to other versions which were not. Has this changed? > > Also, for me this patch adds 500 bytes. I wonder if more of the code > > could made optional? > > Frankly, I'm surprised to hear this adds that much overhead. > Actually, I can't see this increase in code size as you mention. > What architecture are you referring to? > In my workspace (ppc_6xx) u-boot.bin and a stripped u-boot ELF file are > surprisingly unchanged in size, even with debug #defined! Don't look at file size. Use the "size" command and compare code / data / bss sizes. Best regards, Wolfgang Denk
On 12/12/2011 01:18 PM, Wolfgang Denk wrote: > Dear Gerlando Falauto, > >> I think it's easier to read it the original way, and it should not make >> any difference as far as code size is concerned. > > The Coding Style makes an explicit exception regarding the line length > for user visible strings: > > 83 Statements longer than 80 columns will be broken into sensible chunks, unless > 84 exceeding 80 columns significantly increases readability and does not hide > 85 information. Descendants are always substantially shorter than the parent and > 86 are placed substantially to the right. The same applies to function headers > 87 with a long argument list. However, never break user-visible strings such as > 88 printk messages, because that breaks the ability to grep for them. I don't understand: why are you quoting this here and now? The chunk we are referring to doesn't change a bit about the printable string. >>> himport_r() is getting a bit overloaded, >> >> Actually, I believe it makes no longer sense to have it called "_r", as >> it was the original reference to the function being recursively >> calleable (i.e. reentrant) as opposed to other versions which were not. > > Has this changed? Of course it hasn't. But I don't see any point in keeping this notation anyway. >>> Also, for me this patch adds 500 bytes. I wonder if more of the code >>> could made optional? >> >> Frankly, I'm surprised to hear this adds that much overhead. >> Actually, I can't see this increase in code size as you mention. >> What architecture are you referring to? >> In my workspace (ppc_6xx) u-boot.bin and a stripped u-boot ELF file are >> surprisingly unchanged in size, even with debug #defined! > > Don't look at file size. Use the "size" command and compare code / > data / bss sizes. Yep, sorry, I'll look into that to see what part is mostly reponsible for this. Best, Gerlando
Dear Gerlando Falauto, In message <4EE603D0.5010109@keymile.com> you wrote: > > I don't understand: why are you quoting this here and now? The chunk we > are referring to doesn't change a bit about the printable string. Please use proper indentation for it, even if it exceeds the line length then - although I wonder if the string actually makes any sense or is needed / useful as is. Best regards, Wolfgang Denk
Hi Gerlando, On Mon, Dec 12, 2011 at 1:32 AM, Gerlando Falauto <gerlando.falauto@keymile.com> wrote: > On 12/07/2011 11:02 PM, Simon Glass wrote: > > [...] >>> >>> /* >>> - * Set a new environment variable, >>> - * or replace or delete an existing one. >>> + * Performs consistency checking before setting, replacing, >>> + * or deleting an environment variable, then (if successful) >>> + * apply the changes to internals so to make them effective. >>> + * Code for this function was taken out of _do_env_set(), >>> + * which now calls it. >>> + * Also called as a callback function by himport_r(). >>> + * Returns 0 in case of success, 1 in case of failure. >>> + * When (flag& H_FORCE) is set, force overwriting of >>> + * write-once variables. >> >> >> can you word-wrap that to 72 columns perhaps? > > > Sorry, I am little confused now. What is the maximum line length? What I meant was that it seems like it you could put more characters on each line, and then would take up less vertical space. You could also use the doxygen @param formal to make it quicker for people to read. > > [...] > > >>> +/* Check whether variable name is amongst vars[] */ >>> +static int process_var(const char *name, int nvars, char * const vars[]) >>> +{ >>> + int i = 0; >> >> >> blank line here. > > > Thanks, I didn't know about this. > > >> Can part of this function be #ifdefed to reduce code >> size when the feature is not required? > > > Uhm, I don't think so. This would be a common feature for selectively > importing & setting back to default. Unless we want to #ifdef both of them. > Personally, I would #ifdef neither. OK, just a thought, it is up to you. > > [...] > >> >> + if (!process_var(name, nvars, vars)) >> + continue; >> >> if (hdelete_r(name, htab) == 0) >> debug("DELETE ERROR >> ##############################\n"); >> >> perhaps: >> >> if (process_var(name, nvars, vars)&& >> hdelete_r(name, htab) == 0) >> debug("DELETE ERROR ##############################\n"); > > > > I think it's easier to read it the original way, and it should not make any > difference as far as code size is concerned. OK > > >> himport_r() is getting a bit overloaded, > > > Actually, I believe it makes no longer sense to have it called "_r", as it > was the original reference to the function being recursively calleable (i.e. > reentrant) as opposed to other versions which were not. OK I'm not sure about that. > > >> and it's a shame but I can't >> think of an easy way to refactor it to reduce the number of args. In a >> way you are adding a processing option and so you could put the three >> extra args in a structure and pass a pointer to it (or NULL to skip >> this feature). Not sure though... > > > Can't think of any other way either, except maybe renaming it and > re-implementing the shorter version as a wrapper around the newly named > function. I had already done that, but there would be very few places where > the old syntax would be kept, so it just didn't make much sense. Packaging up a lot of zero arguments to a function does chew up code space. > > >> Also, for me this patch adds 500 bytes. I wonder if more of the code >> could made optional? > > > Frankly, I'm surprised to hear this adds that much overhead. > Actually, I can't see this increase in code size as you mention. > What architecture are you referring to? ARMv7. Ideally if your feature is optional it shouldn't add much size when it is turned off. > In my workspace (ppc_6xx) u-boot.bin and a stripped u-boot ELF file are > surprisingly unchanged in size, even with debug #defined! > Only place I can experience such growth is within unstripped u-boot ELF, > which I believe shouldn't really matter... or should it? As Wolfgang says, use your tool chain's size tool for this. Regards, Simon > > Best, > Gerlando
Dear Gerlando Falauto, WD prodded me too long to review this patchset ;-D > The logic of checking special parameters (e.g. baudrate, stdin, stdout, > for a valid value and/or whether can be overwritten) and applying the > new value to the running system is now all within a single function > env_check_apply() which can be called whenever changes are made > to the environment, no matter if by set, default or import. > > With this patch env_check_apply() is only called by "env set", > retaining previous behavior. > > Also allow for selectively importing/resetting variables. > > So add 3 new arguments to himport_r(): > o "nvars", "vars":, number and list of variables to take into account > (0 means ALL) > > o "apply" callback function to check whether a variable can be > overwritten, and possibly immediately apply the changes; > when NULL, no check is performed. > > Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com> [...] > + } > +#if defined(CONFIG_CMD_NET) > + else if (strcmp(name, "bootfile") == 0) { > + copy_filename(BootFile, newval, sizeof(BootFile)); Can you remove the camel-case here please? > + return 0; > + } > +#endif > + return 0; > +} > + > +/* > + * Set a new environment variable, > + * or replace or delete an existing one. > +*/ > +int _do_env_set(int flag, int argc, char * const argv[]) > +{ > + int i, len; > + char *name, *value, *s; > + ENTRY e, *ep; > + > + name = argv[1]; > + value = argv[2]; > + > + if (strchr(name, '=')) { > + printf("## Error: illegal character '='" > + "in variable name \"%s\"\n", name); > + return 1; > + } > + > + env_id++; > + /* > + * search if variable with this name already exists > + */ > + e.key = name; > + e.data = NULL; > + hsearch_r(e, FIND, &ep, &env_htab); > + > + /* > + * Perform requested checks. Notice how since we are overwriting > + * a single variable, we need to set H_NOCLEAR > + */ > + if (env_check_apply(name, ep ? ep->data : NULL, value, H_NOCLEAR)) { > + debug("check function did not approve, refusing\n"); > + return 1; > + } > + > /* Delete only ? */ > if (argc < 3 || argv[2] == NULL) { > int rc = hdelete_r(name, &env_htab); > @@ -337,34 +412,6 @@ int _do_env_set(int flag, int argc, char * const > argv[]) return 1; > } > > - /* > - * Some variables should be updated when the corresponding > - * entry in the environment is changed > - */ > - if (strcmp(name, "ipaddr") == 0) { > - char *s = argv[2]; /* always use only one arg */ > - char *e; > - unsigned long addr; > - bd->bi_ip_addr = 0; > - for (addr = 0, i = 0; i < 4; ++i) { > - ulong val = s ? simple_strtoul(s, &e, 10) : 0; > - addr <<= 8; > - addr |= val & 0xFF; > - if (s) > - s = *e ? e + 1 : e; > - } > - bd->bi_ip_addr = htonl(addr); > - return 0; > - } else if (strcmp(argv[1], "loadaddr") == 0) { > - load_addr = simple_strtoul(argv[2], NULL, 16); > - return 0; > - } > -#if defined(CONFIG_CMD_NET) > - else if (strcmp(argv[1], "bootfile") == 0) { > - copy_filename(BootFile, argv[2], sizeof(BootFile)); > - return 0; > - } > -#endif > return 0; > } > > @@ -886,7 +933,8 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, > addr = (char *)ep->data; > } > > - if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR) == 0) { > + if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR, > + 0, NULL, NULL) == 0) { > error("Environment import failed: errno = %d\n", errno); > return 1; > } > diff --git a/common/env_common.c b/common/env_common.c > index 8a71096..7e2bb2f 100644 > --- a/common/env_common.c > +++ b/common/env_common.c > @@ -175,7 +175,8 @@ void set_default_env(const char *s) > } > > if (himport_r(&env_htab, (char *)default_environment, > - sizeof(default_environment), '\0', 0) == 0) > + sizeof(default_environment), '\0', 0, > + 0, NULL, NULL) == 0) > error("Environment import failed: errno = %d\n", errno); > > gd->flags |= GD_FLG_ENV_READY; > @@ -200,7 +201,8 @@ int env_import(const char *buf, int check) > } > } > > - if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0)) { > + if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, > + 0, NULL, NULL)) { > gd->flags |= GD_FLG_ENV_READY; > return 1; > } > diff --git a/include/environment.h b/include/environment.h > index 3c145af..3a3e6b8 100644 > --- a/include/environment.h > +++ b/include/environment.h > @@ -193,6 +193,13 @@ void set_default_env(const char *s); > /* Import from binary representation into hash table */ > int env_import(const char *buf, int check); > > +/* > + * Check if variable "name" can be changed from oldval to newval, > + * and if so, apply the changes (e.g. baudrate) > + */ > +int env_check_apply(const char *name, const char *oldval, > + const char *newval, int flag); > + > #endif /* DO_DEPS_ONLY */ > > #endif /* _ENVIRONMENT_H_ */ > diff --git a/include/search.h b/include/search.h > index ef53edb..2a59e03 100644 > --- a/include/search.h > +++ b/include/search.h > @@ -47,6 +47,13 @@ typedef struct entry { > struct _ENTRY; > > /* > + * Callback function to be called for checking whether the given change > may + * be applied or not. Must return 0 for approval, 1 for denial. > + */ > +typedef int (*apply_cb)(const char *name, const char *oldval, > + const char *newval, int flag); Is the typedef really necessary ? > + > +/* > * Family of hash table handling functions. The functions also > * have reentrant counterparts ending with _r. The non-reentrant > * functions all work on a signle internal hashing table. > @@ -94,11 +101,19 @@ extern ssize_t hexport_r(struct hsearch_data *__htab, > const char __sep, char **__resp, size_t __size, > int argc, char * const argv[]); > > +/* > + * nvars, vars: variables to import (nvars == 0 means all) > + * apply_cb: callback function to check validity of the new argument, > + * and possibly apply changes (NULL means accept everything) > + */ > extern int himport_r(struct hsearch_data *__htab, > const char *__env, size_t __size, const char __sep, > - int __flag); > + int __flag, > + int nvars, char * const vars[], > + apply_cb apply); > > /* Flags for himport_r() */ > #define H_NOCLEAR 1 /* do not clear hash table before importing */ > +#define H_FORCE 2 /* overwrite read-only/write-once variables */ Make this 1 << x please. > > #endif /* search.h */ > diff --git a/lib/hashtable.c b/lib/hashtable.c > index abd61c8..75b9b07 100644 > --- a/lib/hashtable.c > +++ b/lib/hashtable.c > @@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab, const > char sep, * himport() > */ > > +/* Check whether variable name is amongst vars[] */ > +static int process_var(const char *name, int nvars, char * const vars[]) You mean check_var()? > +{ > + int i = 0; > + /* No variables specified means process all of them */ > + if (nvars == 0) > + return 1; > + > + for (i = 0; i < nvars; i++) { > + if (!strcmp(name, vars[i])) > + return 1; > + } > + debug("Skipping non-listed variable %s\n", name); > + return 0; > +} > + > /* > * Import linearized data into hash table. > * > @@ -639,7 +655,9 @@ ssize_t hexport_r(struct hsearch_data *htab, const char > sep, */ > > int himport_r(struct hsearch_data *htab, > - const char *env, size_t size, const char sep, int flag) > + const char *env, size_t size, const char sep, int flag, > + int nvars, char * const vars[], > + apply_cb apply) > { > char *data, *sp, *dp, *name, *value; > > @@ -726,6 +744,8 @@ int himport_r(struct hsearch_data *htab, > *dp++ = '\0'; /* terminate name */ > > debug("DELETE CANDIDATE: \"%s\"\n", name); > + if (!process_var(name, nvars, vars)) > + continue; > > if (hdelete_r(name, htab) == 0) > debug("DELETE ERROR ##############################\n"); > @@ -743,10 +763,31 @@ int himport_r(struct hsearch_data *htab, > *sp++ = '\0'; /* terminate value */ > ++dp; > > + /* Skip variables which are not supposed to be treated */ > + if (!process_var(name, nvars, vars)) > + continue; > + > /* enter into hash table */ > e.key = name; > e.data = value; Do you need to do this if you eventually later figure out you have no apply() callback and you did this for no reason? > > + /* if there is an apply function, check what it has to say */ > + if (apply != NULL) { > + debug("searching before calling cb function" > + " for %s\n", name); > + /* > + * Search for variable in existing env, so to pass > + * its previous value to the apply callback > + */ > + hsearch_r(e, FIND, &rv, htab); > + debug("previous value was %s\n", rv ? rv->data : ""); > + if (apply(name, rv ? rv->data : NULL, value, flag)) { > + debug("callback function refused to set" > + " variable %s, skipping it!\n", name); > + continue; > + } > + } > + > hsearch_r(e, ENTER, &rv, htab); > if (rv == NULL) { > printf("himport_r: can't insert \"%s=%s\" into hash table\n",
On 03/29/2012 10:19 PM, Marek Vasut wrote: > Dear Gerlando Falauto, > > WD prodded me too long to review this patchset ;-D Well, better late than never! ;-) [...] >> +#if defined(CONFIG_CMD_NET) >> + else if (strcmp(name, "bootfile") == 0) { >> + copy_filename(BootFile, newval, sizeof(BootFile)); > > Can you remove the camel-case here please? > That's code I just moved around... Will do, sir. >> + return 0; >> + } >> +#endif >> + return 0; >> +} >> + [...] >> --- a/include/search.h >> +++ b/include/search.h >> @@ -47,6 +47,13 @@ typedef struct entry { >> struct _ENTRY; >> >> /* >> + * Callback function to be called for checking whether the given change >> may + * be applied or not. Must return 0 for approval, 1 for denial. >> + */ >> +typedef int (*apply_cb)(const char *name, const char *oldval, >> + const char *newval, int flag); > > Is the typedef really necessary ? >[From your other email] > > I have to admit I'm not much of a fan of how you use this apply() > callback, is it really necessary? > Why ask, if you already know the answer? :-) I'm not a big fan either, seemed like the easiest approach at the time. The idea was to keep the hastable (struct hsearch_data) as decoupled as possible from the environment (env_htab which is, in fact, the only instance of struct hsearch_data). What if the function pointer was stored within the hastable itself? Sort of a virtual method. This way we get rid of the typedef and the function pointer as a parameter altogether. The callback parameter then just becomes a boolean value (meaning, do/don't call the callback function stored within the hashtable itself). I like that much better. What do you think? [...] >> >> /* Flags for himport_r() */ >> #define H_NOCLEAR 1 /* do not clear hash table before > importing */ >> +#define H_FORCE 2 /* overwrite read-only/write-once > variables */ > > Make this 1<< x please. OK. > >> >> #endif /* search.h */ >> diff --git a/lib/hashtable.c b/lib/hashtable.c >> index abd61c8..75b9b07 100644 >> --- a/lib/hashtable.c >> +++ b/lib/hashtable.c >> @@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab, const >> char sep, * himport() >> */ >> >> +/* Check whether variable name is amongst vars[] */ >> +static int process_var(const char *name, int nvars, char * const vars[]) > > You mean check_var()? I mean is_var_in_set_or_is_set_empty(). Sorry, I'm very, very bad at picking function names. Any suggestion? >> +{ >> + int i = 0; >> + /* No variables specified means process all of them */ >> + if (nvars == 0) >> + return 1; >> + >> + for (i = 0; i< nvars; i++) { >> + if (!strcmp(name, vars[i])) >> + return 1; >> + } >> + debug("Skipping non-listed variable %s\n", name); >> + return 0; >> +} >> + >> /* >> * Import linearized data into hash table. >> * >> @@ -639,7 +655,9 @@ ssize_t hexport_r(struct hsearch_data *htab, const char >> sep, */ >> >> int himport_r(struct hsearch_data *htab, >> - const char *env, size_t size, const char sep, int flag) >> + const char *env, size_t size, const char sep, int flag, >> + int nvars, char * const vars[], >> + apply_cb apply) >> { >> char *data, *sp, *dp, *name, *value; >> >> @@ -726,6 +744,8 @@ int himport_r(struct hsearch_data *htab, >> *dp++ = '\0'; /* terminate name */ >> >> debug("DELETE CANDIDATE: \"%s\"\n", name); >> + if (!process_var(name, nvars, vars)) >> + continue; >> >> if (hdelete_r(name, htab) == 0) >> debug("DELETE ERROR > ##############################\n"); >> @@ -743,10 +763,31 @@ int himport_r(struct hsearch_data *htab, >> *sp++ = '\0'; /* terminate value */ >> ++dp; >> >> + /* Skip variables which are not supposed to be treated */ >> + if (!process_var(name, nvars, vars)) >> + continue; >> + >> /* enter into hash table */ >> e.key = name; >> e.data = value; > > Do you need to do this if you eventually later figure out you have no apply() > callback and you did this for no reason? You mean calling process_var()? It's two separate things. One, filter out the variables that were not asked to be processed, if there were any (call to process_var()) Two, check whether the new value is acceptable and/or apply it (call apply()) You could have none, either, or both. Or else, if you mean moving the e.key = name, e.data = value assignments, you're right, they should be moved down (but in that case it would be because the apply function fails, not because it's not present -- default case is always successful). >> >> + /* if there is an apply function, check what it has to say */ >> + if (apply != NULL) { >> + debug("searching before calling cb function" >> + " for %s\n", name); >> + /* >> + * Search for variable in existing env, so to pass >> + * its previous value to the apply callback >> + */ >> + hsearch_r(e, FIND,&rv, htab); >> + debug("previous value was %s\n", rv ? rv->data : ""); >> + if (apply(name, rv ? rv->data : NULL, value, flag)) { >> + debug("callback function refused to set" >> + " variable %s, skipping it!\n", name); >> + continue; >> + } >> + } >> + >> hsearch_r(e, ENTER,&rv, htab); >> if (rv == NULL) { >> printf("himport_r: can't insert \"%s=%s\" into hash > table\n", Thank you, Gerlando
Dear Gerlando Falauto, > On 03/29/2012 10:19 PM, Marek Vasut wrote: > > Dear Gerlando Falauto, > > > > WD prodded me too long to review this patchset ;-D > > Well, better late than never! ;-) > > [...] > > >> +#if defined(CONFIG_CMD_NET) > >> + else if (strcmp(name, "bootfile") == 0) { > >> + copy_filename(BootFile, newval, sizeof(BootFile)); > > > > Can you remove the camel-case here please? > > That's code I just moved around... Will do, sir. Don't call me that way, makes me feel old :D > >> + return 0; > >> + } > >> +#endif > >> + return 0; > >> +} > >> + > > [...] > > >> --- a/include/search.h > >> +++ b/include/search.h > >> @@ -47,6 +47,13 @@ typedef struct entry { > >> > >> struct _ENTRY; > >> > >> /* > >> > >> + * Callback function to be called for checking whether the given change > >> may + * be applied or not. Must return 0 for approval, 1 for denial. > >> + */ > >> +typedef int (*apply_cb)(const char *name, const char *oldval, > >> + const char *newval, int flag); > > > > Is the typedef really necessary ? > > > >[From your other email] > > > > I have to admit I'm not much of a fan of how you use this apply() > > callback, is it really necessary? > > Why ask, if you already know the answer? :-) > > I'm not a big fan either, seemed like the easiest approach at the time. > The idea was to keep the hastable (struct hsearch_data) as decoupled as > possible from the environment (env_htab which is, in fact, the only > instance of struct hsearch_data). > > What if the function pointer was stored within the hastable itself? > Sort of a virtual method. > This way we get rid of the typedef and the function pointer as a > parameter altogether. > The callback parameter then just becomes a boolean value (meaning, > do/don't call the callback function stored within the hashtable itself). > I like that much better. What do you think? Don't we always use only one (this callback) function? > > [...] > > >> /* Flags for himport_r() */ > >> #define H_NOCLEAR 1 /* do not clear hash table before > > > > importing */ > > > >> +#define H_FORCE 2 /* overwrite read-only/write-once > > > > variables */ > > > > Make this 1<< x please. > > OK. > > >> #endif /* search.h */ > >> > >> diff --git a/lib/hashtable.c b/lib/hashtable.c > >> index abd61c8..75b9b07 100644 > >> --- a/lib/hashtable.c > >> +++ b/lib/hashtable.c > >> @@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab, const > >> char sep, * himport() > >> > >> */ > >> > >> +/* Check whether variable name is amongst vars[] */ > >> +static int process_var(const char *name, int nvars, char * const > >> vars[]) > > > > You mean check_var()? > > I mean is_var_in_set_or_is_set_empty(). Nice name :) > Sorry, I'm very, very bad at picking function names. > Any suggestion? The above is quite descriptive ... maybe is_var_in_set() ? And hey, don't be sorry, you're doing very good job! > > >> +{ > >> + int i = 0; > >> + /* No variables specified means process all of them */ > >> + if (nvars == 0) > >> + return 1; > >> + > >> + for (i = 0; i< nvars; i++) { > >> + if (!strcmp(name, vars[i])) > >> + return 1; > >> + } > >> + debug("Skipping non-listed variable %s\n", name); > >> + return 0; > >> +} > >> + > >> > >> /* > >> > >> * Import linearized data into hash table. > >> * > >> > >> @@ -639,7 +655,9 @@ ssize_t hexport_r(struct hsearch_data *htab, const > >> char sep, */ > >> > >> int himport_r(struct hsearch_data *htab, > >> > >> - const char *env, size_t size, const char sep, int flag) > >> + const char *env, size_t size, const char sep, int flag, > >> + int nvars, char * const vars[], > >> + apply_cb apply) > >> > >> { > >> > >> char *data, *sp, *dp, *name, *value; > >> > >> @@ -726,6 +744,8 @@ int himport_r(struct hsearch_data *htab, > >> > >> *dp++ = '\0'; /* terminate name */ > >> > >> debug("DELETE CANDIDATE: \"%s\"\n", name); > >> > >> + if (!process_var(name, nvars, vars)) > >> + continue; > >> > >> if (hdelete_r(name, htab) == 0) > >> > >> debug("DELETE ERROR > > > > ##############################\n"); > > > >> @@ -743,10 +763,31 @@ int himport_r(struct hsearch_data *htab, > >> > >> *sp++ = '\0'; /* terminate value */ > >> ++dp; > >> > >> + /* Skip variables which are not supposed to be treated */ > >> + if (!process_var(name, nvars, vars)) > >> + continue; > >> + > >> > >> /* enter into hash table */ > >> e.key = name; > >> e.data = value; > > > > Do you need to do this if you eventually later figure out you have no > > apply() callback and you did this for no reason? > > You mean calling process_var()? It's two separate things. > > One, filter out the variables that were not asked to be processed, if > there were any (call to process_var()) > Two, check whether the new value is acceptable and/or apply it (call > apply()) > You could have none, either, or both. > > Or else, if you mean moving the e.key = name, e.data = value > assignments, you're right, they should be moved down (but in that case > it would be because the apply function fails, not because it's not > present -- default case is always successful). Yep, that's what I meant. OK > > >> + /* if there is an apply function, check what it has to say */ > >> + if (apply != NULL) { > >> + debug("searching before calling cb function" > >> + " for %s\n", name); > >> + /* > >> + * Search for variable in existing env, so to pass > >> + * its previous value to the apply callback > >> + */ > >> + hsearch_r(e, FIND,&rv, htab); > >> + debug("previous value was %s\n", rv ? rv->data : ""); > >> + if (apply(name, rv ? rv->data : NULL, value, flag)) { > >> + debug("callback function refused to set" > >> + " variable %s, skipping it!\n", name); > >> + continue; > >> + } > >> + } > >> + > >> > >> hsearch_r(e, ENTER,&rv, htab); > >> if (rv == NULL) { > >> > >> printf("himport_r: can't insert \"%s=%s\" into hash > > > > table\n", > > Thank you, > Gerlando
On 03/30/2012 03:08 PM, Marek Vasut wrote: > Dear Gerlando Falauto, > >> On 03/29/2012 10:19 PM, Marek Vasut wrote: >>> Dear Gerlando Falauto, >>> >>> WD prodded me too long to review this patchset ;-D >> >> Well, better late than never! ;-) >> >> [...] >> >>>> +#if defined(CONFIG_CMD_NET) >>>> + else if (strcmp(name, "bootfile") == 0) { >>>> + copy_filename(BootFile, newval, sizeof(BootFile)); >>> >>> Can you remove the camel-case here please? >> >> That's code I just moved around... Will do, sir. > > Don't call me that way, makes me feel old :D Was more of a way to remark authority than age. :-) > >>>> + return 0; >>>> + } >>>> +#endif >>>> + return 0; >>>> +} >>>> + >> >> [...] >> >>>> --- a/include/search.h >>>> +++ b/include/search.h >>>> @@ -47,6 +47,13 @@ typedef struct entry { >>>> >>>> struct _ENTRY; >>>> >>>> /* >>>> >>>> + * Callback function to be called for checking whether the given change >>>> may + * be applied or not. Must return 0 for approval, 1 for denial. >>>> + */ >>>> +typedef int (*apply_cb)(const char *name, const char *oldval, >>>> + const char *newval, int flag); >>> >>> Is the typedef really necessary ? >>> >> >[From your other email] >> > >> > I have to admit I'm not much of a fan of how you use this apply() >> > callback, is it really necessary? >> >> Why ask, if you already know the answer? :-) >> >> I'm not a big fan either, seemed like the easiest approach at the time. >> The idea was to keep the hastable (struct hsearch_data) as decoupled as >> possible from the environment (env_htab which is, in fact, the only >> instance of struct hsearch_data). >> >> What if the function pointer was stored within the hastable itself? >> Sort of a virtual method. >> This way we get rid of the typedef and the function pointer as a >> parameter altogether. >> The callback parameter then just becomes a boolean value (meaning, >> do/don't call the callback function stored within the hashtable itself). >> I like that much better. What do you think? > > Don't we always use only one (this callback) function? Yes, but only because env is the only hashtable present. Is that a yes or a no, then? > >> >> [...] >> >>>> /* Flags for himport_r() */ >>>> #define H_NOCLEAR 1 /* do not clear hash table before >>> >>> importing */ >>> >>>> +#define H_FORCE 2 /* overwrite read-only/write-once >>> >>> variables */ >>> >>> Make this 1<< x please. >> >> OK. >> >>>> #endif /* search.h */ >>>> >>>> diff --git a/lib/hashtable.c b/lib/hashtable.c >>>> index abd61c8..75b9b07 100644 >>>> --- a/lib/hashtable.c >>>> +++ b/lib/hashtable.c >>>> @@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab, const >>>> char sep, * himport() >>>> >>>> */ >>>> >>>> +/* Check whether variable name is amongst vars[] */ >>>> +static int process_var(const char *name, int nvars, char * const >>>> vars[]) >>> >>> You mean check_var()? >> >> I mean is_var_in_set_or_is_set_empty(). > > Nice name :) > >> Sorry, I'm very, very bad at picking function names. >> Any suggestion? > > The above is quite descriptive ... maybe is_var_in_set() ? And hey, don't be > sorry, you're doing very good job! I like is_var_in_set(). Thanks, Gerlando
Dear Gerlando Falauto, > On 03/30/2012 03:08 PM, Marek Vasut wrote: > > Dear Gerlando Falauto, > > > >> On 03/29/2012 10:19 PM, Marek Vasut wrote: > >>> Dear Gerlando Falauto, > >>> > >>> WD prodded me too long to review this patchset ;-D > >> > >> Well, better late than never! ;-) > >> > >> [...] > >> > >>>> +#if defined(CONFIG_CMD_NET) > >>>> + else if (strcmp(name, "bootfile") == 0) { > >>>> + copy_filename(BootFile, newval, sizeof(BootFile)); > >>> > >>> Can you remove the camel-case here please? > >> > >> That's code I just moved around... Will do, sir. > > > > Don't call me that way, makes me feel old :D > > Was more of a way to remark authority than age. :-) ;-) > >>>> + return 0; > >>>> + } > >>>> +#endif > >>>> + return 0; > >>>> +} > >>>> + > >> > >> [...] > >> > >>>> --- a/include/search.h > >>>> +++ b/include/search.h > >>>> @@ -47,6 +47,13 @@ typedef struct entry { > >>>> > >>>> struct _ENTRY; > >>>> > >>>> /* > >>>> > >>>> + * Callback function to be called for checking whether the given > >>>> change may + * be applied or not. Must return 0 for approval, 1 for > >>>> denial. + */ > >>>> +typedef int (*apply_cb)(const char *name, const char *oldval, > >>>> + const char *newval, int flag); > >>> > >>> Is the typedef really necessary ? > >>> > >> >[From your other email] > >> > > >> > I have to admit I'm not much of a fan of how you use this apply() > >> > callback, is it really necessary? > >> > >> Why ask, if you already know the answer? :-) > >> > >> I'm not a big fan either, seemed like the easiest approach at the time. > >> The idea was to keep the hastable (struct hsearch_data) as decoupled as > >> possible from the environment (env_htab which is, in fact, the only > >> instance of struct hsearch_data). > >> > >> What if the function pointer was stored within the hastable itself? > >> Sort of a virtual method. > >> This way we get rid of the typedef and the function pointer as a > >> parameter altogether. > >> The callback parameter then just becomes a boolean value (meaning, > >> do/don't call the callback function stored within the hashtable itself). > >> I like that much better. What do you think? > > > > Don't we always use only one (this callback) function? > > Yes, but only because env is the only hashtable present. > Is that a yes or a no, then? Do we expect any more hashtables in the near future ? > >> [...] > >> > >>>> /* Flags for himport_r() */ > >>>> #define H_NOCLEAR 1 /* do not clear hash table before > >>> > >>> importing */ > >>> > >>>> +#define H_FORCE 2 /* overwrite read-only/write-once > >>> > >>> variables */ > >>> > >>> Make this 1<< x please. > >> > >> OK. > >> > >>>> #endif /* search.h */ > >>>> > >>>> diff --git a/lib/hashtable.c b/lib/hashtable.c > >>>> index abd61c8..75b9b07 100644 > >>>> --- a/lib/hashtable.c > >>>> +++ b/lib/hashtable.c > >>>> @@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab, > >>>> const char sep, * himport() > >>>> > >>>> */ > >>>> > >>>> +/* Check whether variable name is amongst vars[] */ > >>>> +static int process_var(const char *name, int nvars, char * const > >>>> vars[]) > >>> > >>> You mean check_var()? > >> > >> I mean is_var_in_set_or_is_set_empty(). > > > > Nice name :) > > > >> Sorry, I'm very, very bad at picking function names. > >> Any suggestion? > > > > The above is quite descriptive ... maybe is_var_in_set() ? And hey, don't > > be sorry, you're doing very good job! > > I like is_var_in_set(). So be it then ;-) Thanks! > Thanks, > Gerlando
On 03/30/2012 03:55 PM, Marek Vasut wrote: > Dear Gerlando Falauto, > >> On 03/30/2012 03:08 PM, Marek Vasut wrote: >>> Dear Gerlando Falauto, >>> >>>> On 03/29/2012 10:19 PM, Marek Vasut wrote: [...] >>>>>> + return 0; >>>>>> + } >>>>>> +#endif >>>>>> + return 0; >>>>>> +} >>>>>> + >>>> >>>> [...] >>>> >>>>>> --- a/include/search.h >>>>>> +++ b/include/search.h >>>>>> @@ -47,6 +47,13 @@ typedef struct entry { >>>>>> >>>>>> struct _ENTRY; >>>>>> >>>>>> /* >>>>>> >>>>>> + * Callback function to be called for checking whether the given >>>>>> change may + * be applied or not. Must return 0 for approval, 1 for >>>>>> denial. + */ >>>>>> +typedef int (*apply_cb)(const char *name, const char *oldval, >>>>>> + const char *newval, int flag); >>>>> >>>>> Is the typedef really necessary ? >>>>> >>>> >[From your other email] >>>> > >>>> > I have to admit I'm not much of a fan of how you use this apply() >>>> > callback, is it really necessary? >>>> >>>> Why ask, if you already know the answer? :-) >>>> >>>> I'm not a big fan either, seemed like the easiest approach at the time. >>>> The idea was to keep the hastable (struct hsearch_data) as decoupled as >>>> possible from the environment (env_htab which is, in fact, the only >>>> instance of struct hsearch_data). >>>> >>>> What if the function pointer was stored within the hastable itself? >>>> Sort of a virtual method. >>>> This way we get rid of the typedef and the function pointer as a >>>> parameter altogether. >>>> The callback parameter then just becomes a boolean value (meaning, >>>> do/don't call the callback function stored within the hashtable itself). >>>> I like that much better. What do you think? >>> >>> Don't we always use only one (this callback) function? >> >> Yes, but only because env is the only hashtable present. >> Is that a yes or a no, then? > > Do we expect any more hashtables in the near future ? I don't think so. Anyway I would rather avoid calling functions belonging to the environment domain from the hastable domain directly. For that matter, we have a single "struct hsearch_data" instance in the whole project, but we keep passing it around as an argument to the hashtable functions. Best, Gerlando
Dear Gerlando Falauto, > On 03/30/2012 03:55 PM, Marek Vasut wrote: > > Dear Gerlando Falauto, > > > >> On 03/30/2012 03:08 PM, Marek Vasut wrote: > >>> Dear Gerlando Falauto, > >>> > >>>> On 03/29/2012 10:19 PM, Marek Vasut wrote: > [...] > > >>>>>> + return 0; > >>>>>> + } > >>>>>> +#endif > >>>>>> + return 0; > >>>>>> +} > >>>>>> + > >>>> > >>>> [...] > >>>> > >>>>>> --- a/include/search.h > >>>>>> +++ b/include/search.h > >>>>>> @@ -47,6 +47,13 @@ typedef struct entry { > >>>>>> > >>>>>> struct _ENTRY; > >>>>>> > >>>>>> /* > >>>>>> > >>>>>> + * Callback function to be called for checking whether the given > >>>>>> change may + * be applied or not. Must return 0 for approval, 1 for > >>>>>> denial. + */ > >>>>>> +typedef int (*apply_cb)(const char *name, const char *oldval, > >>>>>> + const char *newval, int flag); > >>>>> > >>>>> Is the typedef really necessary ? > >>>>> > >>>> >[From your other email] > >>>> > > >>>> > I have to admit I'm not much of a fan of how you use this > >>>> > apply() callback, is it really necessary? > >>>> > >>>> Why ask, if you already know the answer? :-) > >>>> > >>>> I'm not a big fan either, seemed like the easiest approach at the > >>>> time. The idea was to keep the hastable (struct hsearch_data) as > >>>> decoupled as possible from the environment (env_htab which is, in > >>>> fact, the only instance of struct hsearch_data). > >>>> > >>>> What if the function pointer was stored within the hastable itself? > >>>> Sort of a virtual method. > >>>> This way we get rid of the typedef and the function pointer as a > >>>> parameter altogether. > >>>> The callback parameter then just becomes a boolean value (meaning, > >>>> do/don't call the callback function stored within the hashtable > >>>> itself). I like that much better. What do you think? > >>> > >>> Don't we always use only one (this callback) function? > >> > >> Yes, but only because env is the only hashtable present. > >> Is that a yes or a no, then? > > > > Do we expect any more hashtables in the near future ? > > I don't think so. Anyway I would rather avoid calling functions > belonging to the environment domain from the hastable domain directly. > For that matter, we have a single "struct hsearch_data" instance in the > whole project, but we keep passing it around as an argument to the > hashtable functions. Ah, it just came to me. On the other hand, I have this strange feeling lib/hashtable.c is so modified for uboot it can't be used (in a generic way) for any other storage than env ... or am I wrong? WD, can you comment please? > Best, > Gerlando Best regards, Marek Vasut
On 03/30/2012 03:00 PM, Gerlando Falauto wrote: > On 03/29/2012 10:19 PM, Marek Vasut wrote: [...] >>> +#if defined(CONFIG_CMD_NET) >>> + else if (strcmp(name, "bootfile") == 0) { >>> + copy_filename(BootFile, newval, sizeof(BootFile)); >> >> Can you remove the camel-case here please? >> > > That's code I just moved around... Will do, sir. Can't do that, sorry. The global symbol "BootFile" has been defined somewhere else and it's used all over the place. [...] >>> diff --git a/lib/hashtable.c b/lib/hashtable.c >>> index abd61c8..75b9b07 100644 >>> --- a/lib/hashtable.c >>> +++ b/lib/hashtable.c [...] >>> @@ -726,6 +744,8 @@ int himport_r(struct hsearch_data *htab, >>> *dp++ = '\0'; /* terminate name */ >>> >>> debug("DELETE CANDIDATE: \"%s\"\n", name); >>> + if (!process_var(name, nvars, vars)) >>> + continue; >>> >>> if (hdelete_r(name, htab) == 0) >>> debug("DELETE ERROR >> ##############################\n"); >>> @@ -743,10 +763,31 @@ int himport_r(struct hsearch_data *htab, >>> *sp++ = '\0'; /* terminate value */ >>> ++dp; >>> >>> + /* Skip variables which are not supposed to be treated */ >>> + if (!process_var(name, nvars, vars)) >>> + continue; >>> + >>> /* enter into hash table */ >>> e.key = name; >>> e.data = value; >> >> Do you need to do this if you eventually later figure out you have no >> apply() >> callback and you did this for no reason? > > You mean calling process_var()? It's two separate things. > > One, filter out the variables that were not asked to be processed, if > there were any (call to process_var()) > Two, check whether the new value is acceptable and/or apply it (call > apply()) > You could have none, either, or both. > > Or else, if you mean moving the e.key = name, e.data = value > assignments, you're right, they should be moved down (but in that case > it would be because the apply function fails, not because it's not > present -- default case is always successful). Hhmm... sorry, the assignments need to stay where they are. Values in e.key and e.data are used by hsearch_r() within the if statement. > >>> >>> + /* if there is an apply function, check what it has to say */ >>> + if (apply != NULL) { >>> + debug("searching before calling cb function" >>> + " for %s\n", name); >>> + /* >>> + * Search for variable in existing env, so to pass >>> + * its previous value to the apply callback >>> + */ >>> + hsearch_r(e, FIND,&rv, htab); >>> + debug("previous value was %s\n", rv ? rv->data : ""); >>> + if (apply(name, rv ? rv->data : NULL, value, flag)) { >>> + debug("callback function refused to set" >>> + " variable %s, skipping it!\n", name); >>> + continue; >>> + } >>> + } >>> + >>> hsearch_r(e, ENTER,&rv, htab); >>> if (rv == NULL) { >>> printf("himport_r: can't insert \"%s=%s\" into hash >> table\n", > > Thank you, > Gerlando Gerlando
diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c index 5995354..a31d413 100644 --- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c @@ -197,32 +197,23 @@ static int do_env_grep(cmd_tbl_t *cmdtp, int flag, #endif /* - * Set a new environment variable, - * or replace or delete an existing one. + * Performs consistency checking before setting, replacing, + * or deleting an environment variable, then (if successful) + * apply the changes to internals so to make them effective. + * Code for this function was taken out of _do_env_set(), + * which now calls it. + * Also called as a callback function by himport_r(). + * Returns 0 in case of success, 1 in case of failure. + * When (flag & H_FORCE) is set, force overwriting of + * write-once variables. */ -int _do_env_set(int flag, int argc, char * const argv[]) + +int env_check_apply(const char *name, const char *oldval, + const char *newval, int flag) { bd_t *bd = gd->bd; - int i, len; + int i; int console = -1; - char *name, *value, *s; - ENTRY e, *ep; - - name = argv[1]; - - if (strchr(name, '=')) { - printf("## Error: illegal character '=' in variable name" - "\"%s\"\n", name); - return 1; - } - - env_id++; - /* - * search if variable with this name already exists - */ - e.key = name; - e.data = NULL; - hsearch_r(e, FIND, &ep, &env_htab); /* Check for console redirection */ if (strcmp(name, "stdin") == 0) @@ -233,22 +224,24 @@ int _do_env_set(int flag, int argc, char * const argv[]) console = stderr; if (console != -1) { - if (argc < 3) { /* Cannot delete it! */ - printf("Can't delete \"%s\"\n", name); + if ((newval == NULL) || (*newval == '\0')) { + /* We cannot delete stdin/stdout/stderr */ + if ((flag & H_FORCE) == 0) + printf("Can't delete \"%s\"\n", name); return 1; } #ifdef CONFIG_CONSOLE_MUX - i = iomux_doenv(console, argv[2]); + i = iomux_doenv(console, newval); if (i) return i; #else /* Try assigning specified device */ - if (console_assign(console, argv[2]) < 0) + if (console_assign(console, newval) < 0) return 1; #ifdef CONFIG_SERIAL_MULTI - if (serial_assign(argv[2]) < 0) + if (serial_assign(newval) < 0) return 1; #endif #endif /* CONFIG_CONSOLE_MUX */ @@ -256,37 +249,52 @@ int _do_env_set(int flag, int argc, char * const argv[]) /* * Some variables like "ethaddr" and "serial#" can be set only - * once and cannot be deleted; also, "ver" is readonly. + * once and cannot be deleted, unless CONFIG_ENV_OVERWRITE + * is defined. */ - if (ep) { /* variable exists */ #ifndef CONFIG_ENV_OVERWRITE + if (oldval != NULL && /* variable exists */ + (flag & H_FORCE) == 0) { /* and we are not forced */ if (strcmp(name, "serial#") == 0 || (strcmp(name, "ethaddr") == 0 #if defined(CONFIG_OVERWRITE_ETHADDR_ONCE) && defined(CONFIG_ETHADDR) - && strcmp(ep->data, MK_STR(CONFIG_ETHADDR)) != 0 + && strcmp(oldval, MK_STR(CONFIG_ETHADDR)) != 0 #endif /* CONFIG_OVERWRITE_ETHADDR_ONCE && CONFIG_ETHADDR */ )) { printf("Can't overwrite \"%s\"\n", name); return 1; } + } #endif + /* + * When we change baudrate, or we are doing an env default -a + * (which will erase all variables prior to calling this), + * we want the baudrate to actually change - for real. + */ + if (oldval != NULL || /* variable exists */ + (flag & H_NOCLEAR) == 0) { /* or env is clear */ /* * Switch to new baudrate if new baudrate is supported */ if (strcmp(name, "baudrate") == 0) { - int baudrate = simple_strtoul(argv[2], NULL, 10); + int baudrate = simple_strtoul(newval, NULL, 10); int i; for (i = 0; i < N_BAUDRATES; ++i) { if (baudrate == baudrate_table[i]) break; } if (i == N_BAUDRATES) { - printf("## Baudrate %d bps not supported\n", - baudrate); + if ((flag & H_FORCE) == 0) + printf("## Baudrate %d bps not " + "supported\n", baudrate); return 1; } + if (gd->baudrate == baudrate) { + /* If unchanged, we just say it's OK */ + return 0; + } printf("## Switch baudrate to %d bps and" - "press ENTER ...\n", baudrate); + "press ENTER ...\n", baudrate); udelay(50000); gd->baudrate = baudrate; #if defined(CONFIG_PPC) || defined(CONFIG_MCF52x2) @@ -300,6 +308,73 @@ int _do_env_set(int flag, int argc, char * const argv[]) } } + /* + * Some variables should be updated when the corresponding + * entry in the environment is changed + */ + if (strcmp(name, "ipaddr") == 0) { + const char *s = newval; + char *e; + unsigned long addr; + bd->bi_ip_addr = 0; + for (addr = 0, i = 0; i < 4; ++i) { + ulong val = s ? simple_strtoul(s, &e, 10) : 0; + addr <<= 8; + addr |= val & 0xFF; + if (s) + s = *e ? e + 1 : e; + } + bd->bi_ip_addr = htonl(addr); + return 0; + } else if (strcmp(name, "loadaddr") == 0) { + load_addr = simple_strtoul(newval, NULL, 16); + return 0; + } +#if defined(CONFIG_CMD_NET) + else if (strcmp(name, "bootfile") == 0) { + copy_filename(BootFile, newval, sizeof(BootFile)); + return 0; + } +#endif + return 0; +} + +/* + * Set a new environment variable, + * or replace or delete an existing one. +*/ +int _do_env_set(int flag, int argc, char * const argv[]) +{ + int i, len; + char *name, *value, *s; + ENTRY e, *ep; + + name = argv[1]; + value = argv[2]; + + if (strchr(name, '=')) { + printf("## Error: illegal character '='" + "in variable name \"%s\"\n", name); + return 1; + } + + env_id++; + /* + * search if variable with this name already exists + */ + e.key = name; + e.data = NULL; + hsearch_r(e, FIND, &ep, &env_htab); + + /* + * Perform requested checks. Notice how since we are overwriting + * a single variable, we need to set H_NOCLEAR + */ + if (env_check_apply(name, ep ? ep->data : NULL, value, H_NOCLEAR)) { + debug("check function did not approve, refusing\n"); + return 1; + } + /* Delete only ? */ if (argc < 3 || argv[2] == NULL) { int rc = hdelete_r(name, &env_htab); @@ -337,34 +412,6 @@ int _do_env_set(int flag, int argc, char * const argv[]) return 1; } - /* - * Some variables should be updated when the corresponding - * entry in the environment is changed - */ - if (strcmp(name, "ipaddr") == 0) { - char *s = argv[2]; /* always use only one arg */ - char *e; - unsigned long addr; - bd->bi_ip_addr = 0; - for (addr = 0, i = 0; i < 4; ++i) { - ulong val = s ? simple_strtoul(s, &e, 10) : 0; - addr <<= 8; - addr |= val & 0xFF; - if (s) - s = *e ? e + 1 : e; - } - bd->bi_ip_addr = htonl(addr); - return 0; - } else if (strcmp(argv[1], "loadaddr") == 0) { - load_addr = simple_strtoul(argv[2], NULL, 16); - return 0; - } -#if defined(CONFIG_CMD_NET) - else if (strcmp(argv[1], "bootfile") == 0) { - copy_filename(BootFile, argv[2], sizeof(BootFile)); - return 0; - } -#endif return 0; } @@ -886,7 +933,8 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, addr = (char *)ep->data; } - if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR) == 0) { + if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR, + 0, NULL, NULL) == 0) { error("Environment import failed: errno = %d\n", errno); return 1; } diff --git a/common/env_common.c b/common/env_common.c index 8a71096..7e2bb2f 100644 --- a/common/env_common.c +++ b/common/env_common.c @@ -175,7 +175,8 @@ void set_default_env(const char *s) } if (himport_r(&env_htab, (char *)default_environment, - sizeof(default_environment), '\0', 0) == 0) + sizeof(default_environment), '\0', 0, + 0, NULL, NULL) == 0) error("Environment import failed: errno = %d\n", errno); gd->flags |= GD_FLG_ENV_READY; @@ -200,7 +201,8 @@ int env_import(const char *buf, int check) } } - if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0)) { + if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, + 0, NULL, NULL)) { gd->flags |= GD_FLG_ENV_READY; return 1; } diff --git a/include/environment.h b/include/environment.h index 3c145af..3a3e6b8 100644 --- a/include/environment.h +++ b/include/environment.h @@ -193,6 +193,13 @@ void set_default_env(const char *s); /* Import from binary representation into hash table */ int env_import(const char *buf, int check); +/* + * Check if variable "name" can be changed from oldval to newval, + * and if so, apply the changes (e.g. baudrate) + */ +int env_check_apply(const char *name, const char *oldval, + const char *newval, int flag); + #endif /* DO_DEPS_ONLY */ #endif /* _ENVIRONMENT_H_ */ diff --git a/include/search.h b/include/search.h index ef53edb..2a59e03 100644 --- a/include/search.h +++ b/include/search.h @@ -47,6 +47,13 @@ typedef struct entry { struct _ENTRY; /* + * Callback function to be called for checking whether the given change may + * be applied or not. Must return 0 for approval, 1 for denial. + */ +typedef int (*apply_cb)(const char *name, const char *oldval, + const char *newval, int flag); + +/* * Family of hash table handling functions. The functions also * have reentrant counterparts ending with _r. The non-reentrant * functions all work on a signle internal hashing table. @@ -94,11 +101,19 @@ extern ssize_t hexport_r(struct hsearch_data *__htab, const char __sep, char **__resp, size_t __size, int argc, char * const argv[]); +/* + * nvars, vars: variables to import (nvars == 0 means all) + * apply_cb: callback function to check validity of the new argument, + * and possibly apply changes (NULL means accept everything) + */ extern int himport_r(struct hsearch_data *__htab, const char *__env, size_t __size, const char __sep, - int __flag); + int __flag, + int nvars, char * const vars[], + apply_cb apply); /* Flags for himport_r() */ #define H_NOCLEAR 1 /* do not clear hash table before importing */ +#define H_FORCE 2 /* overwrite read-only/write-once variables */ #endif /* search.h */ diff --git a/lib/hashtable.c b/lib/hashtable.c index abd61c8..75b9b07 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, * himport() */ +/* Check whether variable name is amongst vars[] */ +static int process_var(const char *name, int nvars, char * const vars[]) +{ + int i = 0; + /* No variables specified means process all of them */ + if (nvars == 0) + return 1; + + for (i = 0; i < nvars; i++) { + if (!strcmp(name, vars[i])) + return 1; + } + debug("Skipping non-listed variable %s\n", name); + return 0; +} + /* * Import linearized data into hash table. * @@ -639,7 +655,9 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, */ int himport_r(struct hsearch_data *htab, - const char *env, size_t size, const char sep, int flag) + const char *env, size_t size, const char sep, int flag, + int nvars, char * const vars[], + apply_cb apply) { char *data, *sp, *dp, *name, *value; @@ -726,6 +744,8 @@ int himport_r(struct hsearch_data *htab, *dp++ = '\0'; /* terminate name */ debug("DELETE CANDIDATE: \"%s\"\n", name); + if (!process_var(name, nvars, vars)) + continue; if (hdelete_r(name, htab) == 0) debug("DELETE ERROR ##############################\n"); @@ -743,10 +763,31 @@ int himport_r(struct hsearch_data *htab, *sp++ = '\0'; /* terminate value */ ++dp; + /* Skip variables which are not supposed to be treated */ + if (!process_var(name, nvars, vars)) + continue; + /* enter into hash table */ e.key = name; e.data = value; + /* if there is an apply function, check what it has to say */ + if (apply != NULL) { + debug("searching before calling cb function" + " for %s\n", name); + /* + * Search for variable in existing env, so to pass + * its previous value to the apply callback + */ + hsearch_r(e, FIND, &rv, htab); + debug("previous value was %s\n", rv ? rv->data : ""); + if (apply(name, rv ? rv->data : NULL, value, flag)) { + debug("callback function refused to set" + " variable %s, skipping it!\n", name); + continue; + } + } + hsearch_r(e, ENTER, &rv, htab); if (rv == NULL) { printf("himport_r: can't insert \"%s=%s\" into hash table\n",
The logic of checking special parameters (e.g. baudrate, stdin, stdout, for a valid value and/or whether can be overwritten) and applying the new value to the running system is now all within a single function env_check_apply() which can be called whenever changes are made to the environment, no matter if by set, default or import. With this patch env_check_apply() is only called by "env set", retaining previous behavior. Also allow for selectively importing/resetting variables. So add 3 new arguments to himport_r(): o "nvars", "vars":, number and list of variables to take into account (0 means ALL) o "apply" callback function to check whether a variable can be overwritten, and possibly immediately apply the changes; when NULL, no check is performed. Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com> --- common/cmd_nvedit.c | 174 +++++++++++++++++++++++++++++++------------------ common/env_common.c | 6 +- include/environment.h | 7 ++ include/search.h | 17 +++++- lib/hashtable.c | 43 ++++++++++++- 5 files changed, 180 insertions(+), 67 deletions(-)