Dear John, In message <1257352452-11748-1-git-send-email-jschmol...@xes-inc.com> you wrote: > This patch groups environment variables using a non-invasive protocol. > Grouping is achieved by setting a "grouping" variable to a string of > variables, and setting the master grouping variable, "env_groups" to > the list of these grouping variables. > > For instance, > setenv net ipaddr netmask gatewayip serverip > setenv boot bootcmd bootdelay bootargs > setenv env_groups net boot > > would print 4 variables grouped under net, 3 variables grouped under > boot, and the rest of the variables grouped under "other". If env_groups > is not defined, print behaves normally.
First of all: thanks for the patch. > I'm interesetd in seeing peoples opinions of this implementation of grouping > environment variables. My major concerns about this implementation are I have to admit that I did not actually test the code yet. I just read it a bit... > 1) Using parse_line() requires placing several potentially large char array > (CONFIG_SYS_CBSIZE in size) on the stack. Parse_line() does seem to be the > right tool for the job, though. I am not sure about this. Keep in mind that parse_line() processes a maximum of CONFIG_SYS_MAXARGS arguments only, which on most boards is only 16 (and on some boards even less). > 2) Trying to figure out which enviroment variables have already been printed > in groups is less than elegant. Currently, it's a brute-force approach of > looking through every entry until a variable is found in a group or not. > Suggestions for cleaner algorithms here would be appreciated. The repeated scanning and comparing doesn't make much sense to me. Probably it makes more sense to use a more suitable data structure here. How about performing a linear scan of the environment only once and convert it into a more easily processable data structure, say a hash table or a binary tree or whatever, and then operate on this. Here you can easily add additional flags like a pointer which group the variable belongs to (if any). Also, this would make it easier for example to print a sorted list. Eventually we should _always_ do that, i. e. replace the standard copy operation as done in env_relocate*() by a function that not only copies the environment, but converts it into a new internal representation. This might be beneficial to accelerate access to variables, too. => print only a list of groups > If env_groups is defined, none of the grouping variables will be printed. > This seemed to clutter up the printenv output. I don't think this is a wise decision. Having "magic" variables which cannot be seen an idea I dislike. I think the standard "printenv" (without args) should print the grouping variables as first block. Also, it would be nice if "prontenv" now would allow to print a group, i. e. in your example something as "printenv net pci" should be supported. > Grouping environment variables will almost certainly lead to a reqirement for > bumping up CONFIG_SYS_MAXARGS. ...which raises the question why there is such a static limit in the first place. Yes, it was trivial to implement, but maybe this can be improved? Artificial limits on line lengths and numbers of arguments are a nice thing - to remove :-) 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: w...@denx.de "The question of whether a computer can think is no more interesting than the question of whether a submarine can swim" - Edsgar W. Dijkstra _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot