From mboxrd@z Thu Jan  1 00:00:00 1970
From: Gerlando Falauto <gerlando.falauto@keymile.com>
Date: Fri, 23 Sep 2011 09:31:50 +0200
Subject: [U-Boot] [PATCH 1/2] env: set individual variables to default
In-Reply-To: <20110922195134.543F21407979@gemini.denx.de>
References: <1316703972-8417-1-git-send-email-gerlando.falauto@keymile.com>
	<1316703972-8417-2-git-send-email-gerlando.falauto@keymile.com>
	<20110922195134.543F21407979@gemini.denx.de>
Message-ID: <4E7C35E6.5030808@keymile.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 09/22/2011 09:51 PM, Wolfgang Denk wrote:
> > "env default -f" will always keep resetting the whole environment
> > to default.
>
> I don't think this is a good idea.
>
> The "-f" should be used to allow resetting write-protected variables
> like serial# or ethaddr.

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"?

=> env set -f ethaddr
=> printenv
-f=ethaddr
...

> With no variable names given, it will therefore always be needed
> (assuming you have such protected variables in your environment).

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?
Or such operation doesn't make any sense indeed (i.e., all variables 
means ALL OF THEM)?

>> +#ifdef CONFIG_CMD_DEFAULTENV_VARS
>> +/*
>> + * import individual variables from an external environment
>> + * (e.g. default environment).
>> + * Most of this code comes straight from himport_r().
>
> Indeed.  This is way too much code copied.  Please factor this out
> into a separate function to avoid such duplication (which is always a
> maintenance nightmare).

While we're on this subject, do you think it would make any sense to 
*import* individual variables?
If yes, what could the syntax be?

   env import [flags] addr [size] <vars...>

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


> ...
>> +			/* 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).

>
>> +		/*
>> +		 * size check needed for text without '\0' termination
>> +		 * (e.g. default environment)
>> +		 */
>
> What makes you think the default environment was NOT '\0' terminated?

Sorry, my mistake.

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).

Thank you,
Gerlando Falauto