[U-Boot] [PATCH 10/12] env: acl: Add environment variable access control list

[Wolfgang Denk]

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
mind:

- 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"
  variable, etc.

- 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

    .callbacks=ethaddr:ethaddr,eth1addr,ath2addr;baudrate:baudrate;...

    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?

Best regards,

Wolfgang Denk

-- 
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
be moved.