[U-Boot] [PATCH] Add support for setting environment variable from RAM.

[U-Boot] [PATCH] Add support for setting environment variable from RAM.

[Eric Nelson]

This is useful for allowing scripts to read environment variables from
file, among other things.

This is a slightly modified version of what Alessandro submitted to the
mailing list last July:
	http://www.mail-archive.com/u-boot-users at lists.sourceforge.net/msg07932.html

I changed the name from 'setenvram' to 'ramenv' to prevent breakage of scripts
that use the abbreviation 'set' (which my handss have the habit of doing).

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
 common/cmd_nvedit.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 1fcb4c9..bcb6d9f 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -494,6 +494,44 @@ int do_askenv ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 #endif
 
 /************************************************************************
+ * Set a new environment variable from RAM.
+ * Requires three arguments: the variable name, a memory address and a length.
+ *
+ * Deletes the environment variable if the length is zero.
+ */
+int do_ramenv(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
+{
+	unsigned long len, i;
+	char *addr;
+
+	if (argc != 4) {
+		cmd_usage(cmdtp);
+		return 1;
+	}
+	addr = (char *)simple_strtol(argv[2], NULL, 16);
+	len = simple_strtol(argv[3], NULL, 16);
+	if (!addr || !len) {
+		cmd_usage(cmdtp);
+		return 1;
+	}
+	addr[len] = '\0';
+	for (i = 0; i < len; i++) {
+		/* turn newlines into semicolon */
+		if (addr[i] == '\n')
+			addr[i] = ';'; /* ignore dos-style newlines */
+		if (addr[i] == '\r')
+			addr[i] = ' '; /* accept sh-comments and discard them */
+		if (addr[i] == '#') {
+			while (addr[i] && addr[i] != '\n')
+				addr[i++] = ' ';
+			i--;
+		}
+	}
+	setenv(argv[1], addr);
+	return 0;
+}
+
+/************************************************************************
  * Look up variable from environment,
  * return address of storage for that variable,
  * or NULL if not found
@@ -605,6 +643,14 @@ U_BOOT_CMD(
 	"    - delete environment variable 'name'\n"
 );
 
+U_BOOT_CMD(
+	ramenv, 4, 0, do_ramenv,
+	"ramenv  - get environment variable from ram\n",
+	"name addr maxlen\n"
+	"    - set environment variable 'name' from addr 'addr'\n"
+	"    - delete environment variable if maxlen is 0\n"
+);
+
 #if defined(CONFIG_CMD_ASKENV)
 
 U_BOOT_CMD(
-- 
1.5.5.1.382.g182fb

[U-Boot] [PATCH] Add support for setting environment variable from RAM.

[Eric Nelson]

This is useful for allowing scripts to read environment variables from
file, among other things.

This is a slightly modified version of what Alessandro submitted to the
mailing list last July:
	http://www.mail-archive.com/u-boot-users at lists.sourceforge.net/msg07932.html

I changed the name from 'setenvram' to 'ramenv' to prevent breakage of scripts
that use the abbreviation 'set' (which my handss have the habit of doing).

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
 common/cmd_nvedit.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 1fcb4c9..bcb6d9f 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -494,6 +494,44 @@ int do_askenv ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 #endif
 
 /************************************************************************
+ * Set a new environment variable from RAM.
+ * Requires three arguments: the variable name, a memory address and a length.
+ *
+ * Deletes the environment variable if the length is zero.
+ */
+int do_ramenv(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
+{
+	unsigned long len, i;
+	char *addr;
+
+	if (argc != 4) {
+		cmd_usage(cmdtp);
+		return 1;
+	}
+	addr = (char *)simple_strtol(argv[2], NULL, 16);
+	len = simple_strtol(argv[3], NULL, 16);
+	if (!addr || !len) {
+		cmd_usage(cmdtp);
+		return 1;
+	}
+	addr[len] = '\0';
+	for (i = 0; i < len; i++) {
+		/* turn newlines into semicolon */
+		if (addr[i] == '\n')
+			addr[i] = ';'; /* ignore dos-style newlines */
+		if (addr[i] == '\r')
+			addr[i] = ' '; /* accept sh-comments and discard them */
+		if (addr[i] == '#') {
+			while (addr[i] && addr[i] != '\n')
+				addr[i++] = ' ';
+			i--;
+		}
+	}
+	setenv(argv[1], addr);
+	return 0;
+}
+
+/************************************************************************
  * Look up variable from environment,
  * return address of storage for that variable,
  * or NULL if not found
@@ -605,6 +643,14 @@ U_BOOT_CMD(
 	"    - delete environment variable 'name'\n"
 );
 
+U_BOOT_CMD(
+	ramenv, 4, 0, do_ramenv,
+	"ramenv  - get environment variable from ram\n",
+	"name addr maxlen\n"
+	"    - set environment variable 'name' from addr 'addr'\n"
+	"    - delete environment variable if maxlen is 0\n"
+);
+
 #if defined(CONFIG_CMD_ASKENV)
 
 U_BOOT_CMD(
-- 
1.5.5.1.382.g182fb

[U-Boot] [PATCH] Add support for setting environment variable from RAM.

[Peter Tyser]

Hi Eric,

On Tue, 2009-02-03 at 08:28 -0700, Eric Nelson (Boundary Devices) wrote:
>  /************************************************************************
> + * Set a new environment variable from RAM.
> + * Requires three arguments: the variable name, a memory address and a length.
> + *
> + * Deletes the environment variable if the length is zero.
> + */
> +int do_ramenv(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> +	unsigned long len, i;
> +	char *addr;
> +
> +	if (argc != 4) {
> +		cmd_usage(cmdtp);
> +		return 1;
> +	}
> +	addr = (char *)simple_strtol(argv[2], NULL, 16);
> +	len = simple_strtol(argv[3], NULL, 16);
> +	if (!addr || !len) {
> +		cmd_usage(cmdtp);
> +		return 1;
> +	}
> +	addr[len] = '\0';
> +	for (i = 0; i < len; i++) {
> +		/* turn newlines into semicolon */
> +		if (addr[i] == '\n')
> +			addr[i] = ';'; /* ignore dos-style newlines */
> +		if (addr[i] == '\r')
> +			addr[i] = ' '; /* accept sh-comments and discard them */
> +		if (addr[i] == '#') {
> +			while (addr[i] && addr[i] != '\n')
> +				addr[i++] = ' ';
> +			i--;
> +		}
> +	}
> +	setenv(argv[1], addr);
> +	return 0;
> +}
> +
> +/************************************************************************
>   * Look up variable from environment,
>   * return address of storage for that variable,
>   * or NULL if not found
> @@ -605,6 +643,14 @@ U_BOOT_CMD(
>  	"    - delete environment variable 'name'\n"
>  );
>  
> +U_BOOT_CMD(
> +	ramenv, 4, 0, do_ramenv,
> +	"ramenv  - get environment variable from ram\n",

The "ramenv  - " and "\n" are no longer used in the above line.

> +	"name addr maxlen\n"
> +	"    - set environment variable 'name' from addr 'addr'\n"
> +	"    - delete environment variable if maxlen is 0\n"
> +);
> +
>  #if defined(CONFIG_CMD_ASKENV)
>  
>  U_BOOT_CMD(

In the email thread you mentioned above, Detlev mentions 2 alternatives
to the "ramenv" command - loading a uImage script and running it via
autoscr, or modifying autoscr to be able to raw files (non-uImages).
Both of these methods seem cleaner and more flexible at a glance.  Is
there a specific reason using autoscr wouldn't work for your setup?

For example, what is the process to load multiple environment variables
with the ramenv command?  If I understand correctly, in order to load 10
environment variables you'd have to repeat the process of "load a file
to RAM, run ramenv" 10 times?  That seems much more difficult than
loading 1 file with 10 environment variables and running autoscr once.

Best,
Peter

[U-Boot] [PATCH] Add support for setting environment variable from RAM.

[Eric Nelson]

Hello Peter,

Peter Tyser wrote:
> Hi Eric,
> 
> On Tue, 2009-02-03 at 08:28 -0700, Eric Nelson (Boundary Devices) wrote:
>>  /************************************************************************
>> + * Set a new environment variable from RAM.
>> + * Requires three arguments: the variable name, a memory address and a length.
>> + *
>> + * Deletes the environment variable if the length is zero.
>> + */
>> +int do_ramenv(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>> +{
>> +	unsigned long len, i;
>> +	char *addr;
>> +
>> +	if (argc != 4) {
>> +		cmd_usage(cmdtp);
>> +		return 1;
>> +	}
>> +	addr = (char *)simple_strtol(argv[2], NULL, 16);
>> +	len = simple_strtol(argv[3], NULL, 16);
>> +	if (!addr || !len) {
>> +		cmd_usage(cmdtp);
>> +		return 1;
>> +	}
>> +	addr[len] = '\0';
>> +	for (i = 0; i < len; i++) {
>> +		/* turn newlines into semicolon */
>> +		if (addr[i] == '\n')
>> +			addr[i] = ';'; /* ignore dos-style newlines */
>> +		if (addr[i] == '\r')
>> +			addr[i] = ' '; /* accept sh-comments and discard them */
>> +		if (addr[i] == '#') {
>> +			while (addr[i] && addr[i] != '\n')
>> +				addr[i++] = ' ';
>> +			i--;
>> +		}
>> +	}
>> +	setenv(argv[1], addr);
>> +	return 0;
>> +}
>> +
>> +/************************************************************************
>>   * Look up variable from environment,
>>   * return address of storage for that variable,
>>   * or NULL if not found
>> @@ -605,6 +643,14 @@ U_BOOT_CMD(
>>  	"    - delete environment variable 'name'\n"
>>  );
>>  
>> +U_BOOT_CMD(
>> +	ramenv, 4, 0, do_ramenv,
>> +	"ramenv  - get environment variable from ram\n",
> 
> The "ramenv  - " and "\n" are no longer used in the above line.
> 

Oops. Can you tell I started by implementing this on an older source tree?

Re-reading it, the comment should probably also say "set environment variable
from ram" instead of "get...".

If there's interest, I'll happily re-submit the patch.

>> +	"name addr maxlen\n"
>> +	"    - set environment variable 'name' from addr 'addr'\n"
>> +	"    - delete environment variable if maxlen is 0\n"
>> +);
>> +
>>  #if defined(CONFIG_CMD_ASKENV)
>>  
>>  U_BOOT_CMD(
> 
> In the email thread you mentioned above, Detlev mentions 2 alternatives
> to the "ramenv" command - loading a uImage script and running it via
> autoscr, or modifying autoscr to be able to raw files (non-uImages).
> Both of these methods seem cleaner and more flexible at a glance.  Is
> there a specific reason using autoscr wouldn't work for your setup?
> 
The customer requesting this feature operates in a regulated environment with
pretty strict rules about separation of code and data. Autoscr is kind of
a big stick for what we're trying to achieve: (configuring an LCD with
settings from a file on SD card).

> For example, what is the process to load multiple environment variables
> with the ramenv command?  If I understand correctly, in order to load 10
> environment variables you'd have to repeat the process of "load a file
> to RAM, run ramenv" 10 times?  That seems much more difficult than
> loading 1 file with 10 environment variables and running autoscr once.
> 
That's certainly true, although once you have an environment variable you
could use it for iteration...

Our particular need is just for a single environment variable, so the
update works pretty well. I started by updating our 'lcdpanel' U-Boot
command to read from file, but this is much more useful.

> Best,
> Peter
>

Regards,


Eric

[U-Boot] [PATCH] Add support for setting environment variable from RAM.

[Wolfgang Denk]

Dear Eric,

In message <4988936E.3070504@boundarydevices.com> you wrote:
> 
> > In the email thread you mentioned above, Detlev mentions 2 alternatives
> > to the "ramenv" command - loading a uImage script and running it via
> > autoscr, or modifying autoscr to be able to raw files (non-uImages).
> > Both of these methods seem cleaner and more flexible at a glance.  Is
> > there a specific reason using autoscr wouldn't work for your setup?
> > 
> The customer requesting this feature operates in a regulated environment with
> pretty strict rules about separation of code and data. Autoscr is kind of
> a big stick for what we're trying to achieve: (configuring an LCD with
> settings from a file on SD card).

Why is it a big stick? It has a couple of significant advantages over
your implementation:

* It already exissts in mainline.
* It can set several environment variables at once (and also perform
  any other commands you might need on your system).
* It is based on text files which are easy to edit without need for
  new tools.
* It will verify that your file has been transmitted correctly.

> Our particular need is just for a single environment variable, so the
> update works pretty well. I started by updating our 'lcdpanel' U-Boot
> command to read from file, but this is much more useful.

I suggest you use autoscr instead.

I think I will reject your patch.

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 most exciting phrase to hear in science, the one that heralds new
discoveries, is not "Eureka!" (I found it!) but "That's funny ..."
                                                      -- Isaac Asimov

[U-Boot] [PATCH] Add support for setting environment variable from RAM.

[Eric Nelson]

Wolfgang Denk wrote:
> Dear Eric,
> 
> In message <4988936E.3070504@boundarydevices.com> you wrote:
>>> In the email thread you mentioned above, Detlev mentions 2 alternatives
>>> to the "ramenv" command - loading a uImage script and running it via
>>> autoscr, or modifying autoscr to be able to raw files (non-uImages).
>>> Both of these methods seem cleaner and more flexible at a glance.  Is
>>> there a specific reason using autoscr wouldn't work for your setup?
>>>
>> The customer requesting this feature operates in a regulated environment with
>> pretty strict rules about separation of code and data. Autoscr is kind of
>> a big stick for what we're trying to achieve: (configuring an LCD with
>> settings from a file on SD card).
> 
> Why is it a big stick? It has a couple of significant advantages over
> your implementation:
> 
No offense intended ;) Perhaps I should have said it was a much more powerful feature.

> * It already exissts in mainline.
> * It can set several environment variables at once (and also perform
>   any other commands you might need on your system).
> * It is based on text files which are easy to edit without need for
>   new tools.
> * It will verify that your file has been transmitted correctly.
> 
Don't get me wrong, I'm a big fan of autoscr.

As I mentioned, the customer requesting the feature has a regulatory
requirement any time __code__ changes. They have interpreted boot loader
scripts as being executable, so they have to write checks if the script
changes, but not if "data" changes. They interpret a file containing a
variable as data.

Of course they'll need a code release to get there, ...

>> Our particular need is just for a single environment variable, so the
>> update works pretty well. I started by updating our 'lcdpanel' U-Boot
>> command to read from file, but this is much more useful.
> 
> I suggest you use autoscr instead.
> 
> I think I will reject your patch.
> 
That's reasonable. I don't mind carrying the patch around for this customer,
and you're the one preventing U-Boot from becoming bloated over time.

I appreciate all of your efforts.

Regards,


Eric

[U-Boot] [PATCH] Add support for setting environment variable from RAM.

[Wolfgang Denk]

Dear Eric Nelson,

In message <4988BBC0.4070004@boundarydevices.com> you wrote:
>
> As I mentioned, the customer requesting the feature has a regulatory
> requirement any time __code__ changes. They have interpreted boot loader
> scripts as being executable, so they have to write checks if the script
> changes, but not if "data" changes. They interpret a file containing a
> variable as data.

Hm... If we think this to a logical end that would mean that you have
to disable the setenv command completely, as any variable can in fact
contain a sequence of commands that might get run,  thus  environment
data should be treated as code ;-)

> > I think I will reject your patch.
> > 
> That's reasonable. I don't mind carrying the patch around for this customer,
> and you're the one preventing U-Boot from becoming bloated over time.
> 
> I appreciate all of your efforts.

Thanks.

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
A person who is more than casually interested in computers should  be
well  schooled in machine language, since it is a fundamental part of
a computer.                                           -- Donald Knuth

[U-Boot] [PATCH] Add support for setting environment variable from RAM.

[Wolfgang Denk]

Dear "Eric Nelson (Boundary Devices)",

In message <1233631739-17568-1-git-send-email-eric.nelson@boundarydevices.com> you wrote:
> This is useful for allowing scripts to read environment variables from
> file, among other things.
> 
> This is a slightly modified version of what Alessandro submitted to the
> mailing list last July:
> 	http://www.mail-archive.com/u-boot-users at lists.sourceforge.net/msg07932.html
> 
> I changed the name from 'setenvram' to 'ramenv' to prevent breakage of scripts
> that use the abbreviation 'set' (which my handss have the habit of doing).
> 
> Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>

We discussed thsi before, and I already NAKed it.

I hereby NAK it again.

This command makes no sense (especially since it will process only a
single variable).

Please use a script image with "askenv" instead.


Note: if the code would process a list of variables, say, in internal
U-Boot environment format (separated by  single  NUL,  terminated  by
double  NUL  characters),  that  could  be a useful building block to
implement  the  "reset  to  default  environment"  /  "reset  factory
defaults"  command  we discussed earlier (see mailing list archive) -
and as such, it had much better chances to be considered.

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
Question: How does one get fresh air into a Russian church?
Answer:   One clicks on an icon, and a window opens!

[U-Boot] [PATCH] Add support for setting environment variable from RAM.

[Eric Nelson]

Wolfgang Denk wrote:
> Dear "Eric Nelson (Boundary Devices)",
> 
> In message <1233631739-17568-1-git-send-email-eric.nelson@boundarydevices.com> you wrote:
>> This is useful for allowing scripts to read environment variables from
>> file, among other things.
>>
>> This is a slightly modified version of what Alessandro submitted to the
>> mailing list last July:
>> 	http://www.mail-archive.com/u-boot-users at lists.sourceforge.net/msg07932.html
>>
>> I changed the name from 'setenvram' to 'ramenv' to prevent breakage of scripts
>> that use the abbreviation 'set' (which my handss have the habit of doing).
>>
>> Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
> 
> We discussed thsi before, and I already NAKed it.
> 
> I hereby NAK it again.
> 
I'm not sure how this made it back onto the list. Sorry about that.

> This command makes no sense (especially since it will process only a
> single variable).
> 
> Please use a script image with "askenv" instead.
> 
Thanks for the tip. I hadn't noticed askenv previously.

> Note: if the code would process a list of variables, say, in internal
> U-Boot environment format (separated by  single  NUL,  terminated  by
> double  NUL  characters),  that  could  be a useful building block to
> implement  the  "reset  to  default  environment"  /  "reset  factory
> defaults"  command  we discussed earlier (see mailing list archive) -
> and as such, it had much better chances to be considered.
>
> Best regards,
> 
> Wolfgang Denk
> 

[U-Boot] [PATCH] Add support for setting environment variable from RAM.

[Wolfgang Denk]

Dear Eric Nelson,

In message <498B56FE.1080507@boundarydevices.com> you wrote:
>
> > Please use a script image with "askenv" instead.
> > 
> Thanks for the tip. I hadn't noticed askenv previously.

Sorry, typo. I meant "autoscr".

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
Killing is stupid; useless!
	-- McCoy, "A Private Little War", stardate 4211.8

[U-Boot] [PATCH] Add support for setting environment variable from RAM.

[Eric Nelson]

Wolfgang Denk wrote:
> Dear Eric Nelson,
> 
> In message <498B56FE.1080507@boundarydevices.com> you wrote:
>>> Please use a script image with "askenv" instead.
>>>
>> Thanks for the tip. I hadn't noticed askenv previously.
> 
> Sorry, typo. I meant "autoscr".
> 
Muscle memory. Can't live with it, can't turn it off...

Still and all, askenv's kinda cool.

Thanks,


Eric