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