[U-Boot] [RFC] env: Group environment variables

[Wolfgang Denk]

Dear John,

In message <1257352452-11748-1-git-send-email-jschmoller at 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: wd at 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