Dear Joe Hershberger,
In message 1345236586-19076-11-git-send-email-joe.hershberger@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