Hi Wolfgang, I guess you didn't see this the last time I sent it to you off list...
On Thu, Sep 13, 2012 at 3:13 PM, Wolfgang Denk <w...@denx.de> wrote: > Dear Joe Hershberger, > > In message <1345236586-19076-11-git-send-email-joe.hershber...@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? At least in my uses so far, there are not so many variables that it is noticeably slower. > - 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. For sure. I accept that this is only one of the things it does and it's not really access control in the typical sense (with users and what not). > - 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. I was one of the proponents of this when we last discussed this some years ago. I left it out because you convinced me I should just use other variables and overwrite the special ones in a script when I need to. I still think supporting a volatile variable would be cleaner than that. I'm glad you changed your mind. > - 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. I agree that would be nice. > - 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 agree wholeheartedly. > 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". OK... I like the prepended '.'. I'm not sure that "flags" is a very good name for something so specific from a user point of view, but I see why you chose it based on how you plan to store it in the hash. > Normally "env print" etc. would not show such specal variables, but > "env print -a" would. That makes sense. > Similarly, grous of variables could be defined in a ".groups" > variable, etc. OK. > - 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: This seems like a good idea for the most part. Are you concerned that the hast table will take twice the space? Should this be optional? > * 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;... One of the things I've run into with baudrate and silent specifically is wanting to control if the special callback is made on set and/or on env relocation, and/or on initial load, and/or on import. I think there should be a field in the flags for when to check the callback. > This way we could even enale user specific callbacks without need > to modify any common code. Would the user be able to define other callbacks somehow? > And any "env set" would just have to check if the callback > function pointer is non-NULL ... > > > What do you think? I think is sounds very nice. And it's not too far from what I've done. I'll look into implementing some of it. Cheers, -Joe _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot