[U-Boot] [PATCH 0/2] env export fixes

[U-Boot] [PATCH 0/2] env export fixes

[Pierre Aubert]

These patches fix two issues in the export of environment 
variables with CRC calculation.

Pierre Aubert (2):
  hashtable: fix the export lenght computation.
  env export fix: compute the CRC on the real lenght of the exported
    variables.

 common/cmd_nvedit.c |    5 +++--
 lib/hashtable.c     |    2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

-- 
1.7.6.5

[U-Boot] [PATCH 1/2] hashtable: fix the export lenght computation.

[Pierre Aubert]

The room for the '=' and the sep char was reserved twice.

Signed-off-by: Pierre Aubert <p.aubert@staubli.com>
---
 lib/hashtable.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/hashtable.c b/lib/hashtable.c
index 4356b23..480d207 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -623,7 +623,7 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, int flag,
 
 			list[n++] = ep;
 
-			totlen += strlen(ep->key) + 2;
+			totlen += strlen(ep->key);
 
 			if (sep == '\0') {
 				totlen += strlen(ep->data);
-- 
1.7.6.5

[U-Boot] [PATCH 1/2] hashtable: fix the export lenght computation.

[Wolfgang Denk]

Dear Pierre Aubert,

In message <1384434720-11214-2-git-send-email-p.aubert@staubli.com> you wrote:
> The room for the '=' and the sep char was reserved twice.

Are you sure?  Keep in mind that the termination of the list is a
_double_ NUL byte.  IIRC this is what we do here.

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
When properly administered, vacations do not  diminish  productivity:
for every week you're away and get nothing done, there's another when
your boss is away and you get twice as much done.  -- Daniel B. Luten

[U-Boot] [PATCH 1/2] hashtable: fix the export lenght computation.

[Pierre AUBERT]

Dear Wolfgang Denk,

Le 14/11/2013 18:25, Wolfgang Denk a ?crit :
> Dear Pierre Aubert,
>
> In message <1384434720-11214-2-git-send-email-p.aubert@staubli.com> you wrote:
>> The room for the '=' and the sep char was reserved twice.
> Are you sure?  Keep in mind that the termination of the list is a
> _double_ NUL byte.  IIRC this is what we do here.
Yes, I'm sure. The termination of the list (last NULL after the last 
separator) is added at line 666
>  666                 size = totlen + 1;
In the actual code, the lenght is two large of 2 char for each exported 
variable.

Best regards

>
> Best regards,
>
> Wolfgang Denk
>

[U-Boot] [PATCH 2/2] env export fix: compute the CRC on the real lenght of the exported variables.

[Pierre Aubert]

Signed-off-by: Pierre Aubert <p.aubert@staubli.com>
---
 common/cmd_nvedit.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 5bcc324..c32a932 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -922,14 +922,15 @@ NXTARG:		;
 
 	len = hexport_r(&env_htab, '\0',
 			H_MATCH_KEY | H_MATCH_IDENT,
-			&res, ENV_SIZE, argc, argv);
+			&res, size, argc, argv);
+
 	if (len < 0) {
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;
 	}
 
 	if (chk) {
-		envp->crc = crc32(0, envp->data, ENV_SIZE);
+		envp->crc = crc32(0, envp->data, len);
 #ifdef CONFIG_ENV_ADDR_REDUND
 		envp->flags = ACTIVE_FLAG;
 #endif
-- 
1.7.6.5

[U-Boot] [PATCH 2/2] env export fix: compute the CRC on the real lenght of the exported variables.

[Wolfgang Denk]

Dear Pierre Aubert,

In message <1384434720-11214-3-git-send-email-p.aubert@staubli.com> you wrote:
> Signed-off-by: Pierre Aubert <p.aubert@staubli.com>
> ---
>  common/cmd_nvedit.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
> index 5bcc324..c32a932 100644
> --- a/common/cmd_nvedit.c
> +++ b/common/cmd_nvedit.c
> @@ -922,14 +922,15 @@ NXTARG:		;
>  
>  	len = hexport_r(&env_htab, '\0',
>  			H_MATCH_KEY | H_MATCH_IDENT,
> -			&res, ENV_SIZE, argc, argv);
> +			&res, size, argc, argv);
> +
>  	if (len < 0) {
>  		error("Cannot export environment: errno = %d\n", errno);
>  		return 1;
>  	}
>  
>  	if (chk) {
> -		envp->crc = crc32(0, envp->data, ENV_SIZE);
> +		envp->crc = crc32(0, envp->data, len);

This is not correct.  When exporting with CRC, then the CRC
computation should be the same as is done with the persistently
stored environment, i. e. it should be taken over ENV_SIZE bytes.

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
Hindsight is an exact science.

[U-Boot] [PATCH 2/2] env export fix: compute the CRC on the real lenght of the exported variables.

[Pierre AUBERT]

Dear Wolfgang Denk,

Le 14/11/2013 18:24, Wolfgang Denk a ?crit :
> Dear Pierre Aubert,
>
> In message <1384434720-11214-3-git-send-email-p.aubert@staubli.com> you wrote:
>> Signed-off-by: Pierre Aubert <p.aubert@staubli.com>
>> ---
>>   common/cmd_nvedit.c |    5 +++--
>>   1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
>> index 5bcc324..c32a932 100644
>> --- a/common/cmd_nvedit.c
>> +++ b/common/cmd_nvedit.c
>> @@ -922,14 +922,15 @@ NXTARG:		;
>>   
>>   	len = hexport_r(&env_htab, '\0',
>>   			H_MATCH_KEY | H_MATCH_IDENT,
>> -			&res, ENV_SIZE, argc, argv);
>> +			&res, size, argc, argv);
>> +
>>   	if (len < 0) {
>>   		error("Cannot export environment: errno = %d\n", errno);
>>   		return 1;
>>   	}
>>   
>>   	if (chk) {
>> -		envp->crc = crc32(0, envp->data, ENV_SIZE);
>> +		envp->crc = crc32(0, envp->data, len);
> This is not correct.  When exporting with CRC, then the CRC
> computation should be the same as is done with the persistently
> stored environment, i. e. it should be taken over ENV_SIZE bytes.
In this case, there's an inconstisency between the export and the 
import. It isn't possible to export some variables and the reimport them:
U-Boot > env export -c <addr>
U-Boot > env import -c <same addr> fails with a CRC error.
The import computes the CRC on the real lenght of the environment 
variables. IMO, as the import and the export are done to work together, 
it seems to me that the export should compute its CRC on the real  lenght.

Best regards
>
> Best regards,
>
> Wolfgang Denk
>

[U-Boot] [PATCH 2/2] env export fix: compute the CRC on the real lenght of the exported variables.

[Tom Rini]

[ Catching up on some old emails ]

On Fri, Nov 15, 2013 at 08:20:09AM +0100, Pierre AUBERT wrote:
> Dear Wolfgang Denk,
> 
> Le 14/11/2013 18:24, Wolfgang Denk a ?crit :
> >Dear Pierre Aubert,
> >
> >In message <1384434720-11214-3-git-send-email-p.aubert@staubli.com> you wrote:
> >>Signed-off-by: Pierre Aubert <p.aubert@staubli.com>
> >>---
> >>  common/cmd_nvedit.c |    5 +++--
> >>  1 files changed, 3 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
> >>index 5bcc324..c32a932 100644
> >>--- a/common/cmd_nvedit.c
> >>+++ b/common/cmd_nvedit.c
> >>@@ -922,14 +922,15 @@ NXTARG:		;
> >>  	len = hexport_r(&env_htab, '\0',
> >>  			H_MATCH_KEY | H_MATCH_IDENT,
> >>-			&res, ENV_SIZE, argc, argv);
> >>+			&res, size, argc, argv);
> >>+
> >>  	if (len < 0) {
> >>  		error("Cannot export environment: errno = %d\n", errno);
> >>  		return 1;
> >>  	}
> >>  	if (chk) {
> >>-		envp->crc = crc32(0, envp->data, ENV_SIZE);
> >>+		envp->crc = crc32(0, envp->data, len);
> >This is not correct.  When exporting with CRC, then the CRC
> >computation should be the same as is done with the persistently
> >stored environment, i. e. it should be taken over ENV_SIZE bytes.
> In this case, there's an inconstisency between the export and the
> import. It isn't possible to export some variables and the reimport
> them:
> U-Boot > env export -c <addr>
> U-Boot > env import -c <same addr> fails with a CRC error.
> The import computes the CRC on the real lenght of the environment
> variables. IMO, as the import and the export are done to work
> together, it seems to me that the export should compute its CRC on
> the real  lenght.

But env import -c <same addr> $filesize does work as the export sets
filesize.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140226/4f4673e6/attachment.pgp>

[U-Boot] [PATCH 2/2] env export fix: compute the CRC on the real lenght of the exported variables.

[Pierre AUBERT]

Dear Tom Rini

Le 26/02/2014 21:02, Tom Rini a ?crit :
> [ Catching up on some old emails ]
>
> On Fri, Nov 15, 2013 at 08:20:09AM +0100, Pierre AUBERT wrote:
>> Dear Wolfgang Denk,
>>
>> Le 14/11/2013 18:24, Wolfgang Denk a ?crit :
>>> Dear Pierre Aubert,
>>>
>>> In message <1384434720-11214-3-git-send-email-p.aubert@staubli.com> you wrote:
>>>> Signed-off-by: Pierre Aubert <p.aubert@staubli.com>
>>>> ---
>>>>   common/cmd_nvedit.c |    5 +++--
>>>>   1 files changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
>>>> index 5bcc324..c32a932 100644
>>>> --- a/common/cmd_nvedit.c
>>>> +++ b/common/cmd_nvedit.c
>>>> @@ -922,14 +922,15 @@ NXTARG:		;
>>>>   	len = hexport_r(&env_htab, '\0',
>>>>   			H_MATCH_KEY | H_MATCH_IDENT,
>>>> -			&res, ENV_SIZE, argc, argv);
>>>> +			&res, size, argc, argv);
>>>> +
>>>>   	if (len < 0) {
>>>>   		error("Cannot export environment: errno = %d\n", errno);
>>>>   		return 1;
>>>>   	}
>>>>   	if (chk) {
>>>> -		envp->crc = crc32(0, envp->data, ENV_SIZE);
>>>> +		envp->crc = crc32(0, envp->data, len);
>>> This is not correct.  When exporting with CRC, then the CRC
>>> computation should be the same as is done with the persistently
>>> stored environment, i. e. it should be taken over ENV_SIZE bytes.
>> In this case, there's an inconstisency between the export and the
>> import. It isn't possible to export some variables and the reimport
>> them:
>> U-Boot > env export -c <addr>
>> U-Boot > env import -c <same addr> fails with a CRC error.
>> The import computes the CRC on the real lenght of the environment
>> variables. IMO, as the import and the export are done to work
>> together, it seems to me that the export should compute its CRC on
>> the real  lenght.
> But env import -c <same addr> $filesize does work as the export sets
> filesize.
>

You're right, but my purpose wasn't  to export and reimport immediatly 
the same environment.
I thought it was more logical to have the same behaviour between 
"sister" functions.

Best regards

[U-Boot] [PATCH 2/2] env export fix: compute the CRC on the real lenght of the exported variables.

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/03/2014 12:14 PM, Pierre AUBERT wrote:
> Dear Tom Rini
> 
> Le 26/02/2014 21:02, Tom Rini a ?crit :
>> [ Catching up on some old emails ]
>>
>> On Fri, Nov 15, 2013 at 08:20:09AM +0100, Pierre AUBERT wrote:
>>> Dear Wolfgang Denk,
>>>
>>> Le 14/11/2013 18:24, Wolfgang Denk a ?crit :
>>>> Dear Pierre Aubert,
>>>>
>>>> In message <1384434720-11214-3-git-send-email-p.aubert@staubli.com>
>>>> you wrote:
>>>>> Signed-off-by: Pierre Aubert <p.aubert@staubli.com>
>>>>> ---
>>>>>   common/cmd_nvedit.c |    5 +++--
>>>>>   1 files changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
>>>>> index 5bcc324..c32a932 100644
>>>>> --- a/common/cmd_nvedit.c
>>>>> +++ b/common/cmd_nvedit.c
>>>>> @@ -922,14 +922,15 @@ NXTARG:        ;
>>>>>       len = hexport_r(&env_htab, '\0',
>>>>>               H_MATCH_KEY | H_MATCH_IDENT,
>>>>> -            &res, ENV_SIZE, argc, argv);
>>>>> +            &res, size, argc, argv);
>>>>> +
>>>>>       if (len < 0) {
>>>>>           error("Cannot export environment: errno = %d\n", errno);
>>>>>           return 1;
>>>>>       }
>>>>>       if (chk) {
>>>>> -        envp->crc = crc32(0, envp->data, ENV_SIZE);
>>>>> +        envp->crc = crc32(0, envp->data, len);
>>>> This is not correct.  When exporting with CRC, then the CRC
>>>> computation should be the same as is done with the persistently
>>>> stored environment, i. e. it should be taken over ENV_SIZE bytes.
>>> In this case, there's an inconstisency between the export and the
>>> import. It isn't possible to export some variables and the reimport
>>> them:
>>> U-Boot > env export -c <addr>
>>> U-Boot > env import -c <same addr> fails with a CRC error.
>>> The import computes the CRC on the real lenght of the environment
>>> variables. IMO, as the import and the export are done to work
>>> together, it seems to me that the export should compute its CRC on
>>> the real  lenght.
>> But env import -c <same addr> $filesize does work as the export sets
>> filesize.
>>
> 
> You're right, but my purpose wasn't  to export and reimport immediatly
> the same environment.
> I thought it was more logical to have the same behaviour between
> "sister" functions.

I see what you mean, yes.  I think that we need to make the size
parameter to import non-optional as assuming a whole ENV_SIZE chunk
would have been imported doesn't make sense.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTFLzwAAoJENk4IS6UOR1WM9sP/RriAefiFFrD5GcVURLfD6Ek
GDmPWuRg+NFEWHH/Z4eCyvxXaEurLnJcsVF8ZQblWT7zFYu+2weYpLPNYC/zKxzr
VDENmOHndPiDRzr5tfkXGiNXYWE46cSVysx5VYznRDH6sDsE+URJxXqZlefBZgw/
1l0ct5iQUWLmXkRJGQPsj7E4hpedUrBCNavoMh10adLZifWrtty03jN3sOerXhvE
qsAbm6ghrvtnC0b9Yj/FcyYFPgw0iG6VhLfCB10U7kwtVK4/Pl+QQNZaaDAPAgkN
OkdqiktIa5SyX4l94TBxb2Ixnxe7emZcub1WKrIa3zWNvy744e0bVPmtIVX1vYyT
GoaBTG+RcB0fnELt2Uj+odRqQS08KJrSJDOBzIiNzw0VlfSRwaXgK+wTl+3UY0Zt
JFiAPeOFGuYndWQJzRf/GKwhaElRV1NjLS1vFN1DHlNLjkv9xF5XJTiED+j7jGYQ
UCA26m75KtpAJXBjfE7Y7kyGENP6e9RxwSjwbnJcf0smbpIXi1KyU8bOXyriIK4e
oDtprRIxwfL0EdGAlbBNB+RfNxXjB+/lVsVyZyXjmDSeMogYqPIElhRsTMRJzsPx
upHUDvSeHqcTW/3nTtkBlnKTRRc+kze+fP3jwSsY06MWwQQxucvLdoqVzPKbQn/E
hBWjNm6FMSV0DYbaf1k8
=GS4O
-----END PGP SIGNATURE-----