[U-Boot] [PATCH 1/2] env: set individual variables to default

[Wolfgang Denk]

Dear Gerlando Falauto,

In message <4E7C35E6.5030808 at keymile.com> you wrote:
>
> I see what you mean, it's the same rationale behind
> 
>    env set [-f] var
> 
> right? Point is, has "-f" ever been implemented in "env set"?

No, it has not (and there is also no such thing as "setenv -f ...").
Not yet, that is :-)

> OK, so "env default -f" should reset all variables, including protected 
> ones. Should there also be some way to reset *ALL BUT* protected variables?
> Like "env default" or "env default all" as suggested in the wiki page?

Yes, that would be nice. With a warning about the not changed ones.

> While we're on this subject, do you think it would make any sense to 
> *import* individual variables?

Tough question.  In theory yes, it would make perfect sense. On the
other hand, this is a boot loader, and we should not try to be
feature-complete.  So far I haven't seen a use case for this.

> If yes, what could the syntax be?
> 
>    env import [flags] addr [size] <vars...>

env import -n name[,..] [other_flags] addr [size]

?

> would make the optional argument "size" ambiguous (unless we take for 
> granted that variable names cannot start with a digit...).

But they can, at least until now.

> > ...
> >> +			/* deal with "name" and "name=" entries (delete var) */
> >> +			if (*dp == '\0' || *(dp + 1) == '\0' ||
> >> +				*dp == sep || *(dp + 1) == sep) {
> >> +				if (*dp == '=')
> >> +					*dp++ = '\0';
> >> +				*dp++ = '\0';	/* terminate name */
> >
> > Here the comment (about the "delete var") is even wrong.
> 
> What do you mean? It's what himport_r() does (but please see also my 
> final comments).

Yes, but himport_r() actually does the delete:

		...
		if (hdelete_r(name, htab) == 0)
			debug("DELETE ERROR ##############################\n");
		...

You removed this part, so the comment became wrong.

> Anyway, the whole thing was based on the assumption that it could make 
> some sense at some point to import individual variables from a 
> downloaded file. If that's the case, code should be factored together 
> with himport_r() as you suggested. If not, most of this code (the part 
> checking for comments, tabs, '\0'...) can be safely deleted (and I don't 
> think factoring the rest would be necessary).

I agree that it makes sense to generalize and clean up this interface.
It makes sense to select individual variables, and it makes sense to
unify the "-f" handling to enforce actions on protected variables
(while without "-f" only actions on the "normal" variables should be
done).

I can even imagine introducing a new variable that contains the name
of the write-protected variables (and probably other properties, like
being excluded from saveenv, etc.) - this has been discussed a number
of times before, now we have the code base in place to actually
implement it.

All we need to do is extend the struct entry (in "include/search.h")
by an "int flags"), and we can there register properties like
read-only, don't-save etc.  In a first step this could be added
transparently - so we could remove all the special handling of
"ethaddr", "serial#" etc. in common/cmd_nvedit.c; then we could unify
this to include "eth1addr" etc as well; then  we could extend it to
read the names of such variables and their properties from a variable,
etc.

... there is plenty of ideas someone could pick up...

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 bad reputation UNIX has gotten is totally undeserved, laid on by
people who don't understand, who have not gotten in there  and  tried
anything."          -- Jim Joyce, owner of Jim Joyce's UNIX Bookstore