[U-Boot] [PATCH 10/12] env: acl: Add environment variable access control list
wd at denx.de
Thu Sep 13 22:13:58 CEST 2012
Dear Joe Hershberger,
In message <1345236586-19076-11-git-send-email-joe.hershberger at ni.com> you wrote:
> Currently just validates variable types as decimal, hexidecimal,
> boolean, ip address, and mac address. Call
> env_acl_validate_setenv_params() from setenv() in both cmd_nvedit.c
> and fw_env.c.
> If the entry is not found in the env ACL, then look in the static one.
> This allows the env to override the static definitions, but prevents
> the need to have every definition in the environment distracting you.
I have to admid that I haven't tested your code at all, but here are
a few comments anyway:
- It appears you will store all access related information in the ACL
variable (or a copy of it), i. e. in linearized form. Are you not
concerned that this will make variable access slow?
- Encoding type information in a variable named "acl" doesn;t look
really elegant to me. Of course, chosing another name is a trivial
thing to do.
- There have been discussions that it would be nice to have a
"volatile" variable type, for example for variables like "filesize"
or "loadaddr" - such variables would not be stored to the persistent
storage by the "saveenv" command. Thi sis something that should be
fairly wasy to add to your code.
- It would be nice if we could group variables; for example, assume we
could define a group "network" which includes things like ethaddr,
ipaddr, netmask, hostname etc. - and we could then do something like
"env print -g network" to print all variables of such a group.
- We have the situation that certain variables need specific handling
when they get assigned to - baudrate, ethaddr etc. come to mind.
But the code to implement this is scattered all over the place, nd
doesn't really scale.
I've been thinking about similar features for some time, but the
implementation I had in mind was way different. This is what I had in
- Like you, I wanted to use plain environment variables for
persistent storage of all such information. The format you chose
looks fine for me, too.
- I would like to flag these variables as special by using special
names for them. All such special vailables would start with a dot,
i. e. instead of "acl" I had planned to call this ".flags".
Normally "env print" etc. would not show such specal variables, but
"env print -a" would.
Similarly, grous of variables could be defined in a ".groups"
- Instead of parsing the ".flags" variable again and again when
accessing variables, my idea was to extend the hash table: so far,
the struct entry (see "include/search.h") holds only pointers to the
key (variable name) and data (variable value). I suggest to
extend this struct:
* By adding a field "flags" (u32) we could easily encode access
permission type information (like read-only, write-once,
volatile, ...), and of course also type information (hex, dec,
string, IP addr, MAC addr, ...).
* By adding a field "callbacks" (int foo(const char *)) we could
register pointers to a function which gets called whenever the
respective variable value gets changed.
We would still need a compile time mapping from strings to
pointers, but at least this would allow to generalize and unify
the existing code. For example, setting up a mapping of string
"ethaddr" to function env_set_ethaddr() would allow to create
something like a special variable
This way we could even enale user specific callbacks without need
to modify any common code.
And any "env set" would just have to check if the callback
function pointer is non-NULL ...
What do you think?
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Too many people are ready to carry the stool when the piano needs to
More information about the U-Boot