[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[stefan.althoefer at web.de]

[PATCH] common: nvedit to protect additional ethernet addresses

This patch adds "ethaddr1" and "ethaddr2" to the protected
environment variables that can only be written once.

----
The patch is against "latest" u-boot git-repository

Please be patient if style of submission or patches are
offending.

Signed-off-by: stefan.althoefer at web.de  (as at janz.de)
----

diff -uprN u-boot-orig//common/cmd_nvedit.c u-boot/common/cmd_nvedit.c
--- u-boot-orig//common/cmd_nvedit.c	2008-12-02 17:25:31.000000000 +0100
+++ u-boot/common/cmd_nvedit.c	2008-12-02 22:59:36.000000000 +0100
@@ -188,6 +188,8 @@ int _do_setenv (int flag, int argc, char
 #else
 		    (strcmp (name, "serial#") == 0) ||
 #endif
+		    (strcmp (name, "ethaddr1") == 0) ||
+		    (strcmp (name, "ethaddr2") == 0) ||
 		    ((strcmp (name, "ethaddr") == 0)
 #if defined(CONFIG_OVERWRITE_ETHADDR_ONCE) && defined(CONFIG_ETHADDR)
 		     && (strcmp ((char *)env_get_addr(oldval),MK_STR(CONFIG_ETHADDR)) != 0)

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Jerry Van Baren]

stefan.althoefer at web.de wrote:
> [PATCH] common: nvedit to protect additional ethernet addresses
> 
> This patch adds "ethaddr1" and "ethaddr2" to the protected
> environment variables that can only be written once.
> 
> ----
> The patch is against "latest" u-boot git-repository
> 
> Please be patient if style of submission or patches are
> offending.
> 
> Signed-off-by: stefan.althoefer at web.de  (as at janz.de)
> ----
> 
> diff -uprN u-boot-orig//common/cmd_nvedit.c u-boot/common/cmd_nvedit.c
> --- u-boot-orig//common/cmd_nvedit.c	2008-12-02 17:25:31.000000000 +0100
> +++ u-boot/common/cmd_nvedit.c	2008-12-02 22:59:36.000000000 +0100
> @@ -188,6 +188,8 @@ int _do_setenv (int flag, int argc, char
>  #else
>  		    (strcmp (name, "serial#") == 0) ||
>  #endif
> +		    (strcmp (name, "ethaddr1") == 0) ||
> +		    (strcmp (name, "ethaddr2") == 0) ||
>  		    ((strcmp (name, "ethaddr") == 0)
>  #if defined(CONFIG_OVERWRITE_ETHADDR_ONCE) && defined(CONFIG_ETHADDR)
>  		     && (strcmp ((char *)env_get_addr(oldval),MK_STR(CONFIG_ETHADDR)) != 0)

Would it be better to use "strncmp (name, "ethaddr", 7)" instead of 
adding all possible enumerations of ethaddr[0-9]*?  Using strncmp() will 
match any ethaddr.* variable (a wider net than ethaddr[0-9]*, but given 
our limited environment it shouldn't matter).

gvb

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Ben Warren]

stefan.althoefer at web.de wrote:
> [PATCH] common: nvedit to protect additional ethernet addresses
>
> This patch adds "ethaddr1" and "ethaddr2" to the protected
> environment variables that can only be written once.
>
> ----
> The patch is against "latest" u-boot git-repository
>
> Please be patient if style of submission or patches are
> offending.
>
> Signed-off-by: stefan.althoefer at web.de  (as at janz.de)
>   
This needs to be above a '---' line.  Also, this format is wrong.  It 
should have a real name and one e-mail address.  I recommend using 'git 
format-patch' for generating patches.
> ----
>
> diff -uprN u-boot-orig//common/cmd_nvedit.c u-boot/common/cmd_nvedit.c
> --- u-boot-orig//common/cmd_nvedit.c	2008-12-02 17:25:31.000000000 +0100
> +++ u-boot/common/cmd_nvedit.c	2008-12-02 22:59:36.000000000 +0100
> @@ -188,6 +188,8 @@ int _do_setenv (int flag, int argc, char
>  #else
>  		    (strcmp (name, "serial#") == 0) ||
>  #endif
> +		    (strcmp (name, "ethaddr1") == 0) ||
> +		    (strcmp (name, "ethaddr2") == 0) ||
>  		    ((strcmp (name, "ethaddr") == 0)
>   
How about this as a more scalable solution: (note that some boards have 
up to "ethaddr5")

       (strstr(name, "ethaddr") == name)
>  #if defined(CONFIG_OVERWRITE_ETHADDR_ONCE) && defined(CONFIG_ETHADDR)
>  		     && (strcmp ((char *)env_get_addr(oldval),MK_STR(CONFIG_ETHADDR)) != 0)
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>   
regards,
Ben

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Stefan Althöfer]

Then maybe we should have

#define CONFIG_ENV_PROTECTED_ITEM "ethaddr1,ethaddr2,...."

in board configuration to have the greatest flexibility?

> stefan.althoefer at web.de wrote:
> > [PATCH] common: nvedit to protect additional ethernet addresses
> >
> > This patch adds "ethaddr1" and "ethaddr2" to the protected
> > environment variables that can only be written once.
> >
> > ----
> > The patch is against "latest" u-boot git-repository
> >
> > Please be patient if style of submission or patches are
> > offending.
> >
> > Signed-off-by: stefan.althoefer at web.de  (as at janz.de)
> >   
> This needs to be above a '---' line.  Also, this format is wrong.  It 
> should have a real name and one e-mail address.  I recommend using 'git 
> format-patch' for generating patches.
> > ----
> >
> > diff -uprN u-boot-orig//common/cmd_nvedit.c u-boot/common/cmd_nvedit.c
> > --- u-boot-orig//common/cmd_nvedit.c	2008-12-02 17:25:31.000000000 +0100
> > +++ u-boot/common/cmd_nvedit.c	2008-12-02 22:59:36.000000000 +0100
> > @@ -188,6 +188,8 @@ int _do_setenv (int flag, int argc, char
> >  #else
> >  		    (strcmp (name, "serial#") == 0) ||
> >  #endif
> > +		    (strcmp (name, "ethaddr1") == 0) ||
> > +		    (strcmp (name, "ethaddr2") == 0) ||
> >  		    ((strcmp (name, "ethaddr") == 0)
> >   
> How about this as a more scalable solution: (note that some boards have 
> up to "ethaddr5")
> 
>        (strstr(name, "ethaddr") == name)
> >  #if defined(CONFIG_OVERWRITE_ETHADDR_ONCE) && defined(CONFIG_ETHADDR)
> >  		     && (strcmp ((char *)env_get_addr(oldval),MK_STR(CONFIG_ETHADDR)) != 0)
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> >   
> regards,
> Ben
> 


_______________________________________________________________________
Sensationsangebot verl?ngert: WEB.DE FreeDSL - Telefonanschluss + DSL
f?r nur 16,37 Euro/mtl.!* http://dsl.web.de/?ac=OM.AD.AD008K13805B7069a

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Jerry Van Baren]

(Resent in response to complex, non scalable suggestions: IMHO
   strncmp (name, "ethaddr", 7)
is a simple and good solution that covers all known and several unknown 
cases.)

stefan.althoefer at web.de wrote:
> [PATCH] common: nvedit to protect additional ethernet addresses
> 
> This patch adds "ethaddr1" and "ethaddr2" to the protected
> environment variables that can only be written once.
> 
> ----
> The patch is against "latest" u-boot git-repository
> 
> Please be patient if style of submission or patches are
> offending.
> 
> Signed-off-by: stefan.althoefer at web.de  (as at janz.de)
> ----
> 
> diff -uprN u-boot-orig//common/cmd_nvedit.c u-boot/common/cmd_nvedit.c
> --- u-boot-orig//common/cmd_nvedit.c	2008-12-02 17:25:31.000000000 +0100
> +++ u-boot/common/cmd_nvedit.c	2008-12-02 22:59:36.000000000 +0100
> @@ -188,6 +188,8 @@ int _do_setenv (int flag, int argc, char
>  #else
>  		    (strcmp (name, "serial#") == 0) ||
>  #endif
> +		    (strcmp (name, "ethaddr1") == 0) ||
> +		    (strcmp (name, "ethaddr2") == 0) ||
>  		    ((strcmp (name, "ethaddr") == 0)
>  #if defined(CONFIG_OVERWRITE_ETHADDR_ONCE) && defined(CONFIG_ETHADDR)
>  		     && (strcmp ((char *)env_get_addr(oldval),MK_STR(CONFIG_ETHADDR)) != 0)

Would it be better to use "strncmp (name, "ethaddr", 7)" instead of
adding all possible enumerations of ethaddr[0-9]*?  Using strncmp() will
match any ethaddr.* variable (a wider net than ethaddr[0-9]*, but given
our limited environment it shouldn't matter).

gvb

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Ben Warren]

Dear Stefan,

Stefan Alth?fer wrote:
> Then maybe we should have
>
> #define CONFIG_ENV_PROTECTED_ITEM "ethaddr1,ethaddr2,...."
>
> in board configuration to have the greatest flexibility?
>
>   
It might be useful to have a general protection mechanism, but probably 
not right now.  Please implement a general string comparison, either by 
strncmp() or strstr().  Also, please don't top post.

regards,
Ben

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Stefan Althoefer]

This patches cmd_nvedit to reject changes for "ethaddr." in addition to "ethaddr"
and "serial#". This is intendend to protect changes to additional ethernet
addresses (e.g. "ethernet1").

The code was rewritten to be more clear.


Signed-off-by: Stefan Althoefer <stefan.althoefer@web.de>
---
diff -uprN u-boot-orig/common/cmd_nvedit.c u-boot/common/cmd_nvedit.c
--- u-boot-orig/common/cmd_nvedit.c	2008-12-02 17:25:31.000000000 +0100
+++ u-boot/common/cmd_nvedit.c	2008-12-05 16:46:25.000000000 +0100
@@ -143,6 +143,9 @@ int do_printenv (cmd_tbl_t *cmdtp, int f
 int _do_setenv (int flag, int argc, char *argv[])
 {
 	int   i, len, oldval;
+#ifndef CONFIG_ENV_OVERWRITE
+	int   protected;
+#endif
 	int   console = -1;
 	uchar *env, *nxt = NULL;
 	char *name;
@@ -176,23 +179,29 @@ int _do_setenv (int flag, int argc, char
 	 */
 	if (oldval >= 0) {
 #ifndef CONFIG_ENV_OVERWRITE
+		protected = 0;

-		/*
-		 * Ethernet Address and serial# can be set only once,
-		 * ver is readonly.
-		 */
-		if (
+		/* "serial#" is protect unless allowed by 0xdeaf4add flag */
 #ifdef CONFIG_HAS_UID
-		/* Allow serial# forced overwrite with 0xdeaf4add flag */
-		    ((strcmp (name, "serial#") == 0) && (flag != 0xdeaf4add)) ||
+		if ((strcmp (name, "serial#") == 0) && (flag != 0xdeaf4add))
 #else
-		    (strcmp (name, "serial#") == 0) ||
+		if (strcmp (name, "serial#") == 0)
 #endif
-		    ((strcmp (name, "ethaddr") == 0)
+			protected = 1;
+
+		/* "ethaddr" is protected unless allowed by OVERWRITE_ONCE */
+		if (strcmp (name, "ethaddr") == 0)
 #if defined(CONFIG_OVERWRITE_ETHADDR_ONCE) && defined(CONFIG_ETHADDR)
-		     && (strcmp ((char *)env_get_addr(oldval),MK_STR(CONFIG_ETHADDR)) != 0)
+			if (strcmp ((char *)env_get_addr(oldval),MK_STR(CONFIG_ETHADDR)) != 0)
 #endif	/* CONFIG_OVERWRITE_ETHADDR_ONCE && CONFIG_ETHADDR */
-		    ) ) {
+				protected = 1;
+
+		/* "ethaddr." is always protected */
+		if (strncmp (name, "ethaddr", 7) == 0)
+			if (strlen (name) == 8)
+				protected = 1;
+
+		if (protected) {
 			printf ("Can't overwrite \"%s\"\n", name);
 			return 1;
 		}

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Wolfgang Denk]

Dear Stefan Althoefer,

In message <ghc625$k37$1@ger.gmane.org> you wrote:
> This patches cmd_nvedit to reject changes for "ethaddr." in addition to "ethaddr"
> and "serial#". This is intendend to protect changes to additional ethernet
> addresses (e.g. "ethernet1").

The patch is bogus, as additional ethernet addrssses are eth1addr,
eth2addr, etc. and not ethaddr1, etc.


Also, please don't remove perfectly good comments.

NAK.

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 more data I punch in this card,  the lighter it becomes, and the
lower the mailing cost."
                     - Stan Kelly-Bootle, "The Devil's DP Dictionary"

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Jerry Van Baren]

Wolfgang Denk wrote:
> Dear Stefan Althoefer,
> 
> In message <ghc625$k37$1@ger.gmane.org> you wrote:
>> This patches cmd_nvedit to reject changes for "ethaddr." in addition to "ethaddr"
>> and "serial#". This is intendend to protect changes to additional ethernet
>> addresses (e.g. "ethernet1").
> 
> The patch is bogus, as additional ethernet addrssses are eth1addr,
> eth2addr, etc. and not ethaddr1, etc.
> 
> 
> Also, please don't remove perfectly god comments.
> 
> NAK.
> 
> Best regards,
> 
> Wolfgang Denk

Arrgh, I was thinking I was so clever with strncmp() and it turns out I 
was being clever based on a totally bogus assumption (wrong format). 
:-(  I *hate* it when that happens.

The following should work for eth[0-9]+addr (untested):

	int ethnum;
	:
	:
		/* "eth[0-9]+addr" is always protected */
		if ((sscanf(name, "eth%daddr", &ethnum) == 1) &&
		    (ethnum < MAX_ETH_ADDRS))
			protected = 1;

Notes:
* The "ethaddr" case is handled prior to the above snippet of code.
* I took out the added check "if (strlen (name) == 8)", I'm not sure why 
that was in there, it would limit us to 10 ethernets.  If extra 
validation is desired, ethnum could be checked to be less than 
MAX_ETH_ADDRS.  On reflection, it seems like a good idea so I added it 
above.
* This is somewhat better than the strncmp() trick because the sscanf() 
will only convert digits.

gvb

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Ben Warren]

Jerry Van Baren wrote:
> Wolfgang Denk wrote:
>   
>> Dear Stefan Althoefer,
>>
>> In message <ghc625$k37$1@ger.gmane.org> you wrote:
>>     
>>> This patches cmd_nvedit to reject changes for "ethaddr." in addition to "ethaddr"
>>> and "serial#". This is intendend to protect changes to additional ethernet
>>> addresses (e.g. "ethernet1").
>>>       
>> The patch is bogus, as additional ethernet addrssses are eth1addr,
>> eth2addr, etc. and not ethaddr1, etc.
>>
>>
>> Also, please don't remove perfectly god comments.
>>
>> NAK.
>>
>> Best regards,
>>
>> Wolfgang Denk
>>     
>
> Arrgh, I was thinking I was so clever with strncmp() and it turns out I 
> was being clever based on a totally bogus assumption (wrong format). 
> :-(  I *hate* it when that happens.
>
>   
Uh yeah, I wasn't as clever as you by half, but definitely as bogus :)  
It's good to know Wolfgang has such keen eyes.
> The following should work for eth[0-9]+addr (untested):
>
> 	int ethnum;
> 	:
> 	:
> 		/* "eth[0-9]+addr" is always protected */
> 		if ((sscanf(name, "eth%daddr", &ethnum) == 1) &&
> 		    (ethnum < MAX_ETH_ADDRS))
> 			protected = 1;
>
> Notes:
> * The "ethaddr" case is handled prior to the above snippet of code.
> * I took out the added check "if (strlen (name) == 8)", I'm not sure why 
> that was in there, it would limit us to 10 ethernets.  If extra 
> validation is desired, ethnum could be checked to be less than 
> MAX_ETH_ADDRS.  On reflection, it seems like a good idea so I added it 
> above.
> * This is somewhat better than the strncmp() trick because the sscanf() 
> will only convert digits.
>
>   
Neat, although it's very tempting to pull a Python 3 and say "screw 
reverse compatibility" and change this naming convention.
> gvb
> _________________________

regards,
Ben

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Wolfgang Denk]

Dear Ben Warren,

In message <493B6373.3010808@gmail.com> you wrote:
>
> Neat, although it's very tempting to pull a Python 3 and say "screw 
> reverse compatibility" and change this naming convention.

If we'd change it, then my vote wouldbe to change ethaddr into
eth0addr :-)

The reasoning for the name (as I see it  today  [which  is  different
from  the  time  when "ethaddr" was used for the first time]) is "MAC
address for the Ethernet interface as it's being called in Linux", i.
e. "ethNaddr" means "MAC address of the `ethN' network interface  (in
Linux)".

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
It may be bad manners to talk with your mouth full, but it isn't  too
good either if you speak when your head is empty.

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Stefan Althoefer]

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Wolfgang Denk]

Dear Stefan Althoefer,

In message <ghgj6n$sen$1@ger.gmane.org> you wrote:
> From fdeee62f0902b25be1a2a6bf52fb714b0f4f9e59 Mon Sep 17 00:00:00 2001
> From: Stefan Althoefer <stefan.althoefer@web.de>
> Date: Sun, 7 Dec 2008 14:17:08 +0100
> Subject: [PATCH] common: nvedit to protect additional ethernet addresses
> 
> This adds "eth[0-9]+addr" to the protected
> environment variables that can only be written once.
> 
> Code for detecting protected variables was restructured.
> 
> Signed-off-by: Stefan Althoefer <stefan.althoefer@web.de>
...
> @@ -181,18 +186,31 @@ int _do_setenv (int flag, int argc, char *argv[])
>  		 * Ethernet Address and serial# can be set only once,
>  		 * ver is readonly.
>  		 */
> -		if (
> +		protected = 0;
>  #ifdef CONFIG_HAS_UID
>  		/* Allow serial# forced overwrite with 0xdeaf4add flag */
> -		    ((strcmp (name, "serial#") == 0) && (flag != 0xdeaf4add)) ||
> +		if ((strcmp (name, "serial#") == 0) && (flag != 0xdeaf4add))
>  #else
> -		    (strcmp (name, "serial#") == 0) ||
> +		if (strcmp (name, "serial#") == 0)
>  #endif
> -		    ((strcmp (name, "ethaddr") == 0)
> +			protected = 1;

Here we already know that the variable is "serial#", so it cannot be
any of the "eth*addr" variables.

> +		if (strcmp (name, "ethaddr") == 0)
>  #if defined(CONFIG_OVERWRITE_ETHADDR_ONCE) && defined(CONFIG_ETHADDR)
> -		     && (strcmp ((char *)env_get_addr(oldval),MK_STR(CONFIG_ETHADDR)) != 0)
> +			/* Allow "ethaddr" overwrite to change pre-configured address */
> +			if (strcmp ((char *)env_get_addr(oldval),MK_STR(CONFIG_ETHADDR)) != 0)
>  #endif	/* CONFIG_OVERWRITE_ETHADDR_ONCE && CONFIG_ETHADDR */
> -		    ) ) {
> +				protected = 1;
> +
> +		/* "eth[0-9]+addr" is always protected */
> +		if (strncmp (name, "eth", 3) == 0) {
> +			ethnum = simple_strtoul (name+3, &s, 10);
> +			if (s != name + 3)
> +				if (strcmp (s, "addr") == 0)
> +					protected = 1;
> +		}

Then why do we continue to test for these impossible cases? It's just
wasting CPU cycles.

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
How many seconds are there in a year? If I tell you there are 3.155 x
10^7, you won't even try to remember it. On the other hand, who could
forget that, to within half a percent, pi seconds is a nanocentury.
                                                - Tom Duff, Bell Labs

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Stefan Althoefer]

Wolfgang Denk schrieb:
> Dear Stefan Althoefer,
> 
> In message <ghgj6n$sen$1@ger.gmane.org> you wrote:
>> From fdeee62f0902b25be1a2a6bf52fb714b0f4f9e59 Mon Sep 17 00:00:00 2001
>> From: Stefan Althoefer <stefan.althoefer@web.de>
>> Date: Sun, 7 Dec 2008 14:17:08 +0100
>> Subject: [PATCH] common: nvedit to protect additional ethernet addresses
>>
>> This adds "eth[0-9]+addr" to the protected
>> environment variables that can only be written once.
>>
>> Code for detecting protected variables was restructured.
>>
>> Signed-off-by: Stefan Althoefer <stefan.althoefer@web.de>
> ...
>> @@ -181,18 +186,31 @@ int _do_setenv (int flag, int argc, char *argv[])
>>  		 * Ethernet Address and serial# can be set only once,
>>  		 * ver is readonly.
>>  		 */
>> -		if (
>> +		protected = 0;
>>  #ifdef CONFIG_HAS_UID
>>  		/* Allow serial# forced overwrite with 0xdeaf4add flag */
>> -		    ((strcmp (name, "serial#") == 0) && (flag != 0xdeaf4add)) ||
>> +		if ((strcmp (name, "serial#") == 0) && (flag != 0xdeaf4add))
>>  #else
>> -		    (strcmp (name, "serial#") == 0) ||
>> +		if (strcmp (name, "serial#") == 0)
>>  #endif
>> -		    ((strcmp (name, "ethaddr") == 0)
>> +			protected = 1;
> 
> Here we already know that the variable is "serial#", so it cannot be
> any of the "eth*addr" variables.
> 
>> +		if (strcmp (name, "ethaddr") == 0)
>>  #if defined(CONFIG_OVERWRITE_ETHADDR_ONCE) && defined(CONFIG_ETHADDR)
>> -		     && (strcmp ((char *)env_get_addr(oldval),MK_STR(CONFIG_ETHADDR)) != 0)
>> +			/* Allow "ethaddr" overwrite to change pre-configured address */
>> +			if (strcmp ((char *)env_get_addr(oldval),MK_STR(CONFIG_ETHADDR)) != 0)
>>  #endif	/* CONFIG_OVERWRITE_ETHADDR_ONCE && CONFIG_ETHADDR */
>> -		    ) ) {
>> +				protected = 1;
>> +
>> +		/* "eth[0-9]+addr" is always protected */
>> +		if (strncmp (name, "eth", 3) == 0) {
>> +			ethnum = simple_strtoul (name+3, &s, 10);
>> +			if (s != name + 3)
>> +				if (strcmp (s, "addr") == 0)
>> +					protected = 1;
>> +		}
> 
> Then why do we continue to test for these impossible cases? It's just
> wasting CPU cycles.
> 
> Best regards,
> 
> Wolfgang Denk
> 

You argue that the code should have a couple of hard to read else cases?

--- Stefan

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Wolfgang Denk]

Dear Stefan Althoefer,

In message <ghh632$ims$1@ger.gmane.org> you wrote:
>
> You argue that the code should have a couple of hard to read else cases?

That would be one way to avoid unnecessary tests.

Probably not the most elegant approach, agreed.

There are other options, though.

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
Anyone who isn't confused here doesn't really know what's going on.

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Stefan Althoefer]

Hi Wolfgang Denk

> Dear Stefan Althoefer,
> 
> In message <ghh632$ims$1@ger.gmane.org> you wrote:
>> You argue that the code should have a couple of hard to read else cases?
> 
> That would be one way to avoid unnecessary tests.
> 
> Probably not the most elegant approach, agreed.
> 
> There are other options, though.

But your suggested optimizations will only be effective if someone tries to
write to "serial#". This is not normally done (attempt can be considered
an error).

If access to any nonprotected environment variables is requested (and speed
does matter here) then any of the protected cases must be tested.

Even you code:

--------
if( "serial#" )
   ...
else if( "ethaddr" )
   ...
else if( "eth[0-9]+addr" )
   ...
---------

then all the ifs are triggered if you write to "videomode".

--- Stefan

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Jerry Van Baren]

Stefan Althoefer wrote:
> From fdeee62f0902b25be1a2a6bf52fb714b0f4f9e59 Mon Sep 17 00:00:00 2001
> From: Stefan Althoefer <stefan.althoefer@web.de>
> Date: Sun, 7 Dec 2008 14:17:08 +0100
> Subject: [PATCH] common: nvedit to protect additional ethernet addresses
> 
> This adds "eth[0-9]+addr" to the protected
> environment variables that can only be written once.
> 
> Code for detecting protected variables was restructured.
> 
> Signed-off-by: Stefan Althoefer <stefan.althoefer@web.de>
> ---
> This time I left god comments ;-)

I always leave god comments too.  People look at my comments and say "oh 
god!"

:-D

gvb

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Wolfgang Denk]

Dear Stefan Althoefer,

In message <ghhni0$6dv$1@ger.gmane.org> you wrote:
>
> > That would be one way to avoid unnecessary tests.
> > 
> > Probably not the most elegant approach, agreed.
> > 
> > There are other options, though.
> 
> But your suggested optimizations will only be effective if someone tries to
> write to "serial#". This is not normally done (attempt can be considered
> an error).

Can it? What make you think so?

There are lots of boards that come fresh out  of  production  with  a
virgin  environment,  where  setting  "serial#" is a perfectly normal
thing, and not an error at all.

> If access to any nonprotected environment variables is requested (and speed
> does matter here) then any of the protected cases must be tested.

Yes, but you can do this in many different ways - more and less
efficient ones.

> Even you code:
> 
> --------
> if( "serial#" )
>    ...
> else if( "ethaddr" )
>    ...
> else if( "eth[0-9]+addr" )
>    ...
> ---------
> 
> then all the ifs are triggered if you write to "videomode".

Yes, we probably can agree that this is one  of  the  less  efficient
implementations  [I  take  it  as  pseudo code, because otherwise all
cases would execute the "if" branch.]


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
If all you have is a hammer, everything looks like a nail.

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Wolfgang Denk]

Dear =?iso-8859-15?Q?Stefan_Alth=F6fer?=,

In message <531234379@web.de> you wrote:
> Then maybe we should have
> 
> #define CONFIG_ENV_PROTECTED_ITEM "ethaddr1,ethaddr2,...."
> 
> in board configuration to have the greatest flexibility?

Jerry Van Baren already showed you an elegant way to solve this using
scanf().

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
Administration: An ingenious abstraction  in  politics,  designed  to
receive the kicks and cuffs due to the premier or president.
- Ambrose Bierce

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Stefan Althoefer]

Dear Wolfgang Denk

> Dear =?iso-8859-15?Q?Stefan_Alth=F6fer?=,
> 
> In message <531234379@web.de> you wrote:
>> Then maybe we should have
>>
>> #define CONFIG_ENV_PROTECTED_ITEM "ethaddr1,ethaddr2,...."
>>
>> in board configuration to have the greatest flexibility?
> 
> Jerry Van Baren already showed you an elegant way to solve this using
> scanf().

Is scanf() available in u-boot? I couldn't find it.

-- Stefan

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Ben Warren]

Stefan Althoefer wrote:
> Dear Wolfgang Denk
>
>   
>> Dear =?iso-8859-15?Q?Stefan_Alth=F6fer?=,
>>
>> In message <531234379@web.de> you wrote:
>>     
>>> Then maybe we should have
>>>
>>> #define CONFIG_ENV_PROTECTED_ITEM "ethaddr1,ethaddr2,...."
>>>
>>> in board configuration to have the greatest flexibility?
>>>       
>> Jerry Van Baren already showed you an elegant way to solve this using
>> scanf().
>>     
>
> Is scanf() available in u-boot? I couldn't find it.
>
> -- Stefan
>   
I think he meant sscanf(), which should be there.

regards,
Ben

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Wolfgang Denk]

Dear Ben,

In message <4947FFDB.9040005@gmail.com> you wrote:
>
> >> Jerry Van Baren already showed you an elegant way to solve this using
> >> scanf().
> >
> > Is scanf() available in u-boot? I couldn't find it.
...
> I think he meant sscanf(), which should be there.

No, we don't have this yet in U-Boot.

But that's a small problem - as long as we just need to parse for  an
expression  like "eth\d*addr" we can easily do this with ten lines of
C or so.

If we find we might need sscanf() more often, we might een go as  far
and   borrow   some   200+  lines  of  code  for  Linux  (i.e.,  from
"lib/vsprintf.c") :-)

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
"Don't think; let the machine do it for you!"        - E. C. Berkeley

[U-Boot] [PATCH] common: nvedit to protect additional ethernet addresses Part 1/1

[Stefan Althoefer]

Dear Wolfgang Denx,
>> But your suggested optimizations will only be effective if someone tries to
>> write to "serial#". This is not normally done (attempt can be considered
>> an error).
> 
> Can it? What make you think so?
> 
> There are lots of boards that come fresh out  of  production  with  a
> virgin  environment,  where  setting  "serial#" is a perfectly normal
> thing, and not an error at all.

Indeed, in the factory it is normal to set the serial number. Any later
attempt to "overwrite" it is an error (why the code rejects doing it).

-- Stefan