[U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes

Simon Glass
2011-12-07T22:02:06Z

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