[U-Boot] [PATCH 0/2] env export fixes
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(-)
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);
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
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
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
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
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
[ 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.
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
-----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
participants (4)
-
Pierre AUBERT -
Pierre Aubert
-
Tom Rini -
Wolfgang Denk