Add option -r to env import to allow import of text files with CRLF as line endings

PATCH Superseded Alexander Holler 6 participants 23 messages May 13, 2012 – May 14, 2012 Library code

Share & tools

Alexander Holler

author

2012-05-13 12:50 UTC #0 lore raw

PATCH 1/2 Add option -r to env import to allow import of text files with CRLF as line endings

When this option is enabled, CRLF is treated like LF when importing environments from text files, which means CRs ('\r') in front of LFs ('\n') are just ignored.

Drawback of enabling this option is that (maybe exported) variables which have a trailing CR in their content will get imported without that CR.

Signed-off-by Alexander Holler <[email protected]>

---
 common/cmd_nvedit.c |   17 ++++++++++++++---
 common/env_common.c |    4 ++--
 include/search.h    |    2 +-
 lib/hashtable.c     |   23 +++++++++++++++++++----
 4 files changed, 36 insertions(+), 10 deletions(-)

common/cmd_nvedit.c +14 -3

index e1ccdd8..f136953 100644
@@ -793,11 +793,15 @@ sep_err:
 
 #ifdef CONFIG_CMD_IMPORTENV
 /*
- * env import [-d] [-t | -b | -c] addr [size]
+ * env import [-d] [-t [-r] | -b | -c] addr [size]
  *	-d:	delete existing environment before importing;
  *		otherwise overwrite / append to existion definitions
  *	-t:	assume text format; either "size" must be given or the
  *		text data must be '\0' terminated
+ *	-r:	handle CRLF like LF, that means exported variables with
+ *		a content which ends with \r won't get imported. Used
+ *		to import text files created with editors which are using CRLF
+ *		for line endings. Only effective in addition to -t.
  *	-b:	assume binary format ('\0' separated, "\0\0" terminated)
  *	-c:	assume checksum protected environment format
  *	addr:	memory address to read from

@@ -812,6 +816,7 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 	int	chk = 0;
 	int	fmt = 0;
 	int	del = 0;
+	int	crlf_is_lf = 0;
 	size_t	size;
 
 	cmd = *argv;

@@ -836,6 +841,9 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 					goto sep_err;
 				sep = '\n';
 				break;
+			case 'r':		/* handle CRLF like LF */
+				crlf_is_lf = 1;
+				break;
 			case 'd':
 				del = 1;
 				break;

@@ -851,6 +859,9 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 	if (!fmt)
 		printf("## Warning: defaulting to text format\n");
 
+	if (sep != '\n' && crlf_is_lf )
+		crlf_is_lf = 0;
+
 	addr = (char *)simple_strtoul(argv[0], NULL, 16);
 
 	if (argc == 2) {

@@ -888,7 +899,7 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 		addr = (char *)ep->data;
 	}
 
-	if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR) == 0) {
+	if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR, crlf_is_lf) == 0) {
 		error("Environment import failed: errno = %d\n", errno);
 		return 1;
 	}

@@ -974,7 +985,7 @@ U_BOOT_CMD(
 #if defined(CONFIG_CMD_GREPENV)
 	"env grep string [...] - search environment\n"
 #endif
-	"env import [-d] [-t | -b | -c] addr [size] - import environment\n"
+	"env import [-d] [-t [-r] | | -b | -c] addr [size] - import environment\n"
 	"env print [name ...] - print environment\n"
 #if defined(CONFIG_CMD_RUN)
 	"env run var [...] - run commands in an environment variable\n"

common/env_common.c +2 -2

index c33d22d..9b50ffe 100644
@@ -182,7 +182,7 @@ void set_default_env(const char *s)
 	}
 
 	if (himport_r(&env_htab, (char *)default_environment,
-			sizeof(default_environment), '\0', 0) == 0)
+			sizeof(default_environment), '\0', 0, 0) == 0)
 		error("Environment import failed: errno = %d\n", errno);
 
 	gd->flags |= GD_FLG_ENV_READY;

@@ -207,7 +207,7 @@ int env_import(const char *buf, int check)
 		}
 	}
 
-	if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0)) {
+	if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, 0)) {
 		gd->flags |= GD_FLG_ENV_READY;
 		return 1;
 	}

include/search.h +1 -1

index ef53edb..053cf07 100644
@@ -96,7 +96,7 @@ extern ssize_t hexport_r(struct hsearch_data *__htab,
 
 extern int himport_r(struct hsearch_data *__htab,
 		     const char *__env, size_t __size, const char __sep,
-		     int __flag);
+		     int __flag, int __crlf_is_lf);
 
 /* Flags for himport_r() */
 #define	H_NOCLEAR	1	/* do not clear hash table before importing */

lib/hashtable.c +19 -4

index abd61c8..e5fa3c3 100644
@@ -623,9 +623,9 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep,
  * (entries separated by newline characters).
  *
  * To allow for nicely formatted text input, leading white space
- * (sequences of SPACE and TAB chars) is ignored, and entries starting
- * (after removal of any leading white space) with a '#' character are
- * considered comments and ignored.
+ * (sequences of SPACE and TAB chars) is ignored, all Carriage Returns
+ * are ignored and entries starting (after removal of any leading white
+ * space) with a '#' character are considered comments and ignored.
  *
  * [NOTE: this means that a variable name cannot start with a '#'
  * character.]

@@ -639,7 +639,7 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep,
  */
 
 int himport_r(struct hsearch_data *htab,
-	      const char *env, size_t size, const char sep, int flag)
+	      const char *env, size_t size, const char sep, int flag, int crlf_is_lf)
 {
 	char *data, *sp, *dp, *name, *value;

@@ -698,6 +698,21 @@ int himport_r(struct hsearch_data *htab,
 		}
 	}
 
+	/* Remove all Carriage Returns */
+        if(!size)
+		return 1;		/* everything OK */
+	if(crlf_is_lf) {
+		unsigned ignored_crs = 0;
+		for(;dp < data + size && *dp; ++dp) {
+			if(*dp == '\r' &&
+			   dp < data + size - 1 && *(dp+1) == '\n')
+				++ignored_crs;
+			else
+				*(dp-ignored_crs) = *dp;
+		}
+		size -= ignored_crs;
+		dp = data;
+	}
 	/* Parse environment; allow for '\0' and 'sep' as separators */
 	do {
 		ENTRY e, *rv;

Signature · 1 line

1.7.3.4

Alexander Holler

author

2012-05-13 12:50 UTC #1 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

Use the new option -r for env import.

Signed-off-by Alexander Holler <[email protected]>

---
 include/configs/omap3_beagle.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

include/configs/omap3_beagle.h +1 -1

index ddeb414..d8b10c2 100644
@@ -258,7 +258,7 @@
 	"bootenv=uEnv.txt\0" \
 	"loadbootenv=fatload mmc ${mmcdev} ${loadaddr} ${bootenv}\0" \
 	"importbootenv=echo Importing environment from mmc ...; " \
-		"env import -t $loadaddr $filesize\0" \
+		"env import -t -r $loadaddr $filesize\0" \
 	"ramargs=setenv bootargs console=${console} " \
 		"${optargs} " \
 		"mpurate=${mpurate} " \

Signature · 1 line

1.7.3.4

Alexander Holler

author

2012-05-13 17:56 UTC #3 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

Quoted 9 lines · Marek Vasut

Dear Alexander Holler,
Quoted 7 lines
Use the new option -r for env import.
Signed-off-by Alexander Holler<[email protected]>
---
  include/configs/omap3_beagle.h |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

Quoted 12 lines

include/configs/omap3_beagle.h line 261
Quoted 10 lines
(quoted diff) +0 -0
b/include/configs/omap3_beagle.h index ddeb414..d8b10c2 100644
include/configs/omap3_beagle.h +1 -1
@@ -258,7 +258,7 @@
  	"bootenv=uEnv.txt\0" \
  	"loadbootenv=fatload mmc ${mmcdev} ${loadaddr} ${bootenv}\0" \
  	"importbootenv=echo Importing environment from mmc ...; " \
-		"env import -t $loadaddr $filesize\0" \
+		"env import -t -r $loadaddr $filesize\0" \
Not everyone importing env on beagle use broken tools ;-)

Who said that?

Regards,

Alexander

Marek Vasut

2012-05-13 18:52 UTC #4 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

Dear Alexander Holler,

Quoted 21 lines · Marek Vasut

Quoted 20 lines
Dear Alexander Holler,
Quoted 18 lines
Use the new option -r for env import.
Signed-off-by Alexander Holler<[email protected]>
---
  include/configs/omap3_beagle.h |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
(quoted diff) +0 -0
b/include/configs/omap3_beagle.h index ddeb414..d8b10c2 100644
include/configs/omap3_beagle.h +0 -0
@@ -258,7 +258,7 @@
 
  	"bootenv=uEnv.txt\0" \
  	"loadbootenv=fatload mmc ${mmcdev} ${loadaddr} ${bootenv}\0" \
  	"importbootenv=echo Importing environment from mmc ...; " \
 

Quoted 6 lines

Quoted 4 lines
include/configs/omap3_beagle.h line 261
Quoted 2 lines
include/configs/omap3_beagle.h +1 -1
-		"env import -t $loadaddr $filesize\0" \
+		"env import -t -r $loadaddr $filesize\0" \
Not everyone importing env on beagle use broken tools ;-)
Who said that?

You impose your -r option on all beagle users. That means everyone (even those with not-broken) tools will now use it (unless they replaced their env).

Quoted 4 lines

Regards,
Alexander

Best regards,
Marek Vasut

Alexander Holler

author

2012-05-13 18:58 UTC #5 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

Quoted 28 lines · Marek Vasut

Dear Alexander Holler,
Quoted 26 lines · Marek Vasut
Quoted 24 lines
Dear Alexander Holler,
Quoted 13 lines
Use the new option -r for env import.
Signed-off-by Alexander Holler<[email protected]>
---
   include/configs/omap3_beagle.h |    2 +-
   1 files changed, 1 insertions(+), 1 deletions(-)
(quoted diff) +0 -0
b/include/configs/omap3_beagle.h index ddeb414..d8b10c2 100644
include/configs/omap3_beagle.h line 263
Quoted 7 lines
include/configs/omap3_beagle.h +1 -1
@@ -258,7 +258,7 @@
 
   	"bootenv=uEnv.txt\0" \
   	"loadbootenv=fatload mmc ${mmcdev} ${loadaddr} ${bootenv}\0" \
   	"importbootenv=echo Importing environment from mmc ...; " \
 
-		"env import -t $loadaddr $filesize\0" \
+		"env import -t -r $loadaddr $filesize\0" \
Not everyone importing env on beagle use broken tools ;-)

Quoted 4 lines

Quoted 1 line
Who said that?
You impose your -r option on all beagle users. That means everyone (even those with not-broken) tools will now use it (unless they replaced their env).

Yes, and now they all will get doomed by ignored CRs before LFs.

Anyway, just ignore those patches, it was lost time from the beginning and I should have known it better.

Alexander

Wolfgang Denk

2012-05-13 18:49 UTC #6 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

In reply to Marek Vasut

Dear Marek Vasut,

Quoted 5 lines · In message <[email protected]> you wrote:

include/configs/omap3_beagle.h line 261
Quoted 3 lines
include/configs/omap3_beagle.h +1 -1
-		"env import -t $loadaddr $filesize\0" \
+		"env import -t -r $loadaddr $filesize\0" \
Not everyone importing env on beagle use broken tools ;-)

It's not a problem of using broken tools - the problem is of ignorant people _not_ using decade old, working tools like dos2unix if they need them.

As I mentionewd a coule of times before, I am seriously tempted to ignore this "problem".

On the other hand, we usually say we accept code if it helps some, and doesn't hurt others. As is, the increase of code size is hurting.

Best regards,

Wolfgang Denk

Signature · 5 lines

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
Never ascribe to malice that which can  adequately  be  explained  by
stupidity.

Marek Vasut

2012-05-13 18:57 UTC #7 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

Dear Wolfgang Denk,

Quoted 11 lines

Dear Marek Vasut,
Quoted 4 lines · In message <[email protected]> you wrote:
include/configs/omap3_beagle.h line 261
Quoted 2 lines
include/configs/omap3_beagle.h +1 -1
-		"env import -t $loadaddr $filesize\0" \
+		"env import -t -r $loadaddr $filesize\0" \
Not everyone importing env on beagle use broken tools ;-)
It's not a problem of using broken tools - the problem is of ignorant people _not_ using decade old, working tools like dos2unix if they need them.

tr -d '\r' won't work? :-)

Quoted 2 lines

As I mentionewd a coule of times before, I am seriously tempted to ignore this "problem".

Can't the wincrap people be taught to use cygwin? Or possibly some Windows (R) (C)(TM)(???) rebuilt version of tr -d '\r' ?

Quoted 6 lines

On the other hand, we usually say we accept code if it helps some, and doesn't hurt others. As is, the increase of code size is hurting.
Best regards,
Wolfgang Denk

Best regards,
Marek Vasut

Wolfgang Denk

2012-05-13 19:26 UTC #8 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

Dear Marek Vasut,

Quoted 6 lines · In message <[email protected]> you wrote:

Quoted 4 lines
It's not a problem of using broken tools - the problem is of ignorant people _not_ using decade old, working tools like dos2unix if they need them.
tr -d '\r' won't work? :-)

Not as well - it will delete _all_ ocurrences of \r, which is more than just changing "\r\n" into "\n" (which still may be wrong in some cases).

But yes, TIMTOWTDI.

Quoted 2 lines

Can't the wincrap people be taught to use cygwin? Or possibly some Windows (R) (C)(TM)(???) rebuilt version of tr -d '\r' ?

Do you care?

Best regards,

Wolfgang Denk

Signature · 5 lines

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 conservative is a man with two perfectly good legs  who  has  never
learned to walk.                              - Franklin D. Roosevelt

Marek Vasut

2012-05-13 19:45 UTC #9 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

Dear Wolfgang Denk,

Quoted 7 lines

Dear Marek Vasut,
Quoted 4 lines · In message <[email protected]> you wrote:
Quoted 3 lines
It's not a problem of using broken tools - the problem is of ignorant people _not_ using decade old, working tools like dos2unix if they need them.

Quoted 7 lines

Quoted 1 line
tr -d '\r' won't work? :-)
Not as well - it will delete _all_ ocurrences of \r, which is more than just changing "\r\n" into "\n" (which still may be wrong in some cases).
But yes, TIMTOWTDI.

What's this NLA (or 9LA) ? :)

Quoted 5 lines

Quoted 3 lines
Can't the wincrap people be taught to use cygwin? Or possibly some Windows (R) (C)(TM)(???) rebuilt version of tr -d '\r' ?
Do you care?

Well Alexander does, so I'm trying to find a fitting solution for both parties (uboot and him).

Quoted 4 lines

Best regards,
Wolfgang Denk

Best regards,
Marek Vasut

Wolfgang Denk

2012-05-13 19:53 UTC #10 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

Dear Marek Vasut,

Quoted 4 lines · In message <[email protected]> you wrote:

Quoted 2 lines
But yes, TIMTOWTDI.
What's this NLA (or 9LA) ? :)

http://en.wikipedia.org/wiki/TIMTOWTDI

Quoted 4 lines

Quoted 1 line
Do you care?
Well Alexander does, so I'm trying to find a fitting solution for both parties (uboot and him).

well, I suggested one: the code should be conditional, so it does not bloat code size for "all other" users. Except from that, I'd be wiling to accept it (ok, I haven't looked at the implementation yet).

Best regards,

Wolfgang Denk

Signature · 4 lines

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
Whom the gods would destroy, they first teach BASIC.

Alexander Holler

author

2012-05-13 20:18 UTC #11 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

In reply to Marek Vasut

Quoted 14 lines · Marek Vasut

Dear Wolfgang Denk,
Quoted 11 lines
Dear Marek Vasut,
Quoted 4 lines · In message<[email protected]> you wrote:
include/configs/omap3_beagle.h line 261
Quoted 2 lines
include/configs/omap3_beagle.h +1 -1
-		"env import -t $loadaddr $filesize\0" \
+		"env import -t -r $loadaddr $filesize\0" \
Not everyone importing env on beagle use broken tools ;-)
It's not a problem of using broken tools - the problem is of ignorant people _not_ using decade old, working tools like dos2unix if they need them.

Quoted 7 lines

tr -d '\r' won't work? :-)
Quoted 2 lines
As I mentionewd a coule of times before, I am seriously tempted to ignore this "problem".
Can't the wincrap people be taught to use cygwin? Or possibly some Windows (R) (C)(TM)(???) rebuilt version of tr -d '\r' ?

Sorry, it seems you just are unable or yiu don't want to understand the problem. So here is my last message on this topic, trying to explain it for you.

env import -t is a handy solution to give users (not only devs) a possibility to modify variables (options) used to boot some OS. They don't have to deal with the u-boot command-line and, depending how it's used, env import -t is an alternative to persistent modify the env without the need for NAND.

So one just tells the user(s) that if they want to change some option they just have to create a text file on a specific place which contains a line like e.g.

foo=bar

The only problem is that such a description currently only works for Linux users. I don't know with what devices and users you are dealing, but there are some people out in the wild which don't know (much) about Linux, or even about the difference in line endings between text files creates using Windows or Linux. Some of them even just want to use their device.

And in addition, the resulting problems hare very hard to diagnose, because there will be no obvious error message when something contains a trailing CR.

It would be interesting how long you would need to diagnose such a problem without knowing about that problem, but we now will never know.

Now I needed about 10 mails to describe a problem from which I thought it's totally obvious. Sorry, but I accept that I'm totally unable to deal with this list in any, for me reasonable, time frame.

Regards,

Alexander

Wolfgang Denk

2012-05-13 21:38 UTC #12 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

Dear Alexander,

Quoted 4 lines · In message <[email protected]> you wrote:

Sorry, it seems you just are unable or yiu don't want to understand the problem. So here is my last message on this topic, trying to explain it for you.

I'm sorry you are giving up so early, just one resubmit before getting this into an acceptable state.

Quoted 3 lines

So one just tells the user(s) that if they want to change some option they just have to create a text file on a specific place which contains a line like e.g.

I guess this is the point where the problem starts. You should not write anywhere "they just have to create a text file", because "a text file" is not a precise enough definition of the required input format.

Quoted 2 lines

The only problem is that such a description currently only works for Linux users. ...

The problem is not Windows users versus Linux users here. The problem is unsufficient information.

Quoted 5 lines

... I don't know with what devices and users you are dealing,
but there are some people out in the wild which don't know (much) about Linux, or even about the difference in line endings between text files creates using Windows or Linux. Some of them even just want to use their device.

In either case, people are usually pretty good in following pre-canned recipies for doing things.

Instead of "create a text file" one could for example document that "a UNIX style text file, i. e. with only "\n" line endings (and not the DOS-derived "\r\n" line endings) is needed". One could add some description that "on Windows systems, the command ... can be used to convert a DOS file into this format" (and provide a URL where to get this tool), "while on UNIX systems like Linux the dos2unix tool can be used".

If you then add an example suitable for copy & paste you can solve a very large percentage of the propblems you see now.

Quoted 3 lines

And in addition, the resulting problems hare very hard to diagnose, because there will be no obvious error message when something contains a trailing CR.

What exactly are such error modes, by the way? I would expect that trailing white space is pretty much harmless for most variable settings?

Quoted 3 lines

Now I needed about 10 mails to describe a problem from which I thought it's totally obvious. Sorry, but I accept that I'm totally unable to deal with this list in any, for me reasonable, time frame.

What you provide is valuable input, and it really makes sense to discuss such issues. I would like to bring such discussions to a point where either we understand why your approach makes sense, or where you agree that there are alternatives that solve the problem as well (or better) as your original proposal. However, this is difficult to acchieve if you give up early. And believe me, this is taking my time (and Marek's) as much as yours. We do this not to annoy you, but because we are interested in the best possible solution.

Best regards,

Wolfgang Denk

Signature · 6 lines

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
There's an old story about the person who wished his computer were as
easy to use as his telephone. That wish has come  true,  since  I  no
longer know how to use my telephone.

Alexander Holler

author

2012-05-14 08:29 UTC #13 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

Am 13.05.2012 23:38, schrieb Wolfgang Denk:

Quoted 42 lines

Dear Alexander,
Quoted 4 lines · In message<[email protected]> you wrote:
Sorry, it seems you just are unable or yiu don't want to understand the problem. So here is my last message on this topic, trying to explain it for you.
I'm sorry you are giving up so early, just one resubmit before getting this into an acceptable state.
Quoted 3 lines
So one just tells the user(s) that if they want to change some option they just have to create a text file on a specific place which contains a line like e.g.
I guess this is the point where the problem starts. You should not write anywhere "they just have to create a text file", because "a text file" is not a precise enough definition of the required input format.
Quoted 2 lines
The only problem is that such a description currently only works for Linux users. ...
The problem is not Windows users versus Linux users here. The problem is unsufficient information.
Quoted 5 lines
... I don't know with what devices and users you are dealing,
but there are some people out in the wild which don't know (much) about Linux, or even about the difference in line endings between text files creates using Windows or Linux. Some of them even just want to use their device.
In either case, people are usually pretty good in following pre-canned recipies for doing things.
Instead of "create a text file" one could for example document that "a UNIX style text file, i. e. with only "\n" line endings (and not the DOS-derived "\r\n" line endings) is needed". One could add some description that "on Windows systems, the command ... can be used to convert a DOS file into this format" (and provide a URL where to get this tool), "while on UNIX systems like Linux the dos2unix tool can be used".

Quoted 2 lines

If you then add an example suitable for copy& paste you can solve a very large percentage of the propblems you see now.

Sorry, but you are just describing one of the oldest dreams (not only) in IT.

You can write whatever you wish, users are able to not read it, to misread it, to ignore it, not to understand it or it will get missing in an one-line copy of the original multine description.

Regards,

Alexander

Alexander Holler

author

2012-05-14 09:07 UTC #14 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

In reply to Wolfgang Denk

Am 13.05.2012 23:38, schrieb Wolfgang Denk:

Quoted 7 lines

Dear Alexander,
In message<[email protected]> you wrote:
What exactly are such error modes, by the way? I would expect that trailing white space is pretty much harmless for most variable settings?

The problem already starts in u-boot itself:

a="b\r"

I'm not sure if \r on the u-boot-cmdline does work, but you know what I mean.

$a=="b"

is false

fatload $a

fails (if the file is named 'b').

...

Regards,

Alexander

Alexander Holler

author

2012-05-14 09:21 UTC #15 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

Am 14.05.2012 11:07, schrieb Alexander Holler:

Quoted 18 lines

Am 13.05.2012 23:38, schrieb Wolfgang Denk:
Quoted 7 lines
Dear Alexander,
In message<[email protected]> you wrote:
What exactly are such error modes, by the way? I would expect that trailing white space is pretty much harmless for most variable settings?
The problem already starts in u-boot itself:
a="b\r"
I'm not sure if \r on the u-boot-cmdline does work, but you know what I mean.
$a=="b"

Quoted 7 lines

is false
fatload $a
fails (if the file is named 'b').
...

And

a="1\r"

isn't a number. So anything which expects a number will fail. E.g.

fatls usb 0:${a}

and every arithmetic which uses ${a}

Regards,

Alexander

Måns Rullgård

2012-05-13 21:47 UTC #16 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

In reply to Marek Vasut

Quoted 14 lines · Marek Vasut

Dear Wolfgang Denk,
Quoted 11 lines
Dear Marek Vasut,
Quoted 4 lines · In message <[email protected]> you wrote:
include/configs/omap3_beagle.h line 261
Quoted 2 lines
include/configs/omap3_beagle.h +1 -1
-		"env import -t $loadaddr $filesize\0" \
+		"env import -t -r $loadaddr $filesize\0" \
Not everyone importing env on beagle use broken tools ;-)
It's not a problem of using broken tools - the problem is of ignorant people _not_ using decade old, working tools like dos2unix if they need them.

Quoted 7 lines

tr -d '\r' won't work? :-)
Quoted 2 lines
As I mentionewd a coule of times before, I am seriously tempted to ignore this "problem".
Can't the wincrap people be taught to use cygwin? Or possibly some Windows (R) (C)(TM)(???) rebuilt version of tr -d '\r' ?

Most text editors on windows except notepad.exe have an option for Unix-style line breaks. People who insist on using notepad.exe deserve no sympathy.

Signature · 2 lines

M?ns Rullg?rd
mans at mansr.com

Tom Rini

2012-05-14 16:02 UTC #17 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

In reply to Alexander Holler

Quoted 3 lines · Alexander Holler

Use the new option -r for env import.
Signed-off-by Alexander Holler <[email protected]>

While I am sympathetic, the right answer is to tell users to use a text editor that can save with UNIX-style newlines. NAK.

Signature · 1 line

Tom

Alexander Holler

author

2012-05-14 17:04 UTC #18 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

Am 14.05.2012 18:02, schrieb Tom Rini:

Quoted 9 lines · Alexander Holler

Quoted 4 lines
Use the new option -r for env import.
Signed-off-by Alexander Holler<[email protected]>
While I am sympathetic, the right answer is to tell users to use a text editor that can save with UNIX-style newlines. NAK.

As long as you don't commit another wrong patch with me as author, I accept it and stop, as already promised, with patches.

Regards

Alexander

Marek Vasut

2012-05-14 18:45 UTC #19 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

Dear Alexander Holler,

Quoted 10 lines

Am 14.05.2012 18:02, schrieb Tom Rini:
Quoted 7 lines · Alexander Holler
Quoted 3 lines
Use the new option -r for env import.
Signed-off-by Alexander Holler<[email protected]>
While I am sympathetic, the right answer is to tell users to use a text editor that can save with UNIX-style newlines. NAK.
As long as you don't commit another wrong patch with me as author

What happened?

Quoted 10 lines

, I
accept it and stop, as already promised, with patches.
Regards
Alexander
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Best regards,
Marek Vasut

Alexander Holler

author

2012-05-14 22:23 UTC #20 lore raw

PATCH 2/2 beagle: handle import of environments in files with CRLF as line endings

Quoted 6 lines · Marek Vasut

Dear Alexander Holler,
Quoted 4 lines
Am 14.05.2012 18:02, schrieb Tom Rini:
Quoted 3 lines · Alexander Holler
Quoted 2 lines
Use the new option -r for env import.

Quoted 9 lines

Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Alexander Holler<[email protected]>
While I am sympathetic, the right answer is to tell users to use a text editor that can save with UNIX-style newlines. NAK.
As long as you don't commit another wrong patch with me as author
What happened?

Someone, which I never met in realitiy before, greeted me at the FOSDEM as the one who got famous (in some circles, don't know) for tty02 (0 as in zeroMAP, the well known processor from TI).

Regards,

Alexander

Alexander Holler

author

2012-05-13 12:58 UTC #21 lore raw

PATCH v2 1/2 Add option -r to env import to allow import of text files with CRLF as line endings

In reply to Alexander Holler

When this option is enabled, CRLF is treated like LF when importing environments from text files, which means CRs ('\r') in front of LFs ('\n') are just ignored.

Drawback of enabling this option is that (maybe exported) variables which have a trailing CR in their content will get imported without that CR.

Signed-off-by Alexander Holler <[email protected]>

---
 common/cmd_nvedit.c |   17 ++++++++++++++---
 common/env_common.c |    4 ++--
 include/search.h    |    2 +-
 lib/hashtable.c     |   17 ++++++++++++++++-
 4 files changed, 33 insertions(+), 7 deletions(-)

common/cmd_nvedit.c +14 -3

index e1ccdd8..f136953 100644
@@ -793,11 +793,15 @@ sep_err:
 
 #ifdef CONFIG_CMD_IMPORTENV
 /*
- * env import [-d] [-t | -b | -c] addr [size]
+ * env import [-d] [-t [-r] | -b | -c] addr [size]
  *	-d:	delete existing environment before importing;
  *		otherwise overwrite / append to existion definitions
  *	-t:	assume text format; either "size" must be given or the
  *		text data must be '\0' terminated
+ *	-r:	handle CRLF like LF, that means exported variables with
+ *		a content which ends with \r won't get imported. Used
+ *		to import text files created with editors which are using CRLF
+ *		for line endings. Only effective in addition to -t.
  *	-b:	assume binary format ('\0' separated, "\0\0" terminated)
  *	-c:	assume checksum protected environment format
  *	addr:	memory address to read from

@@ -812,6 +816,7 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 	int	chk = 0;
 	int	fmt = 0;
 	int	del = 0;
+	int	crlf_is_lf = 0;
 	size_t	size;
 
 	cmd = *argv;

@@ -836,6 +841,9 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 					goto sep_err;
 				sep = '\n';
 				break;
+			case 'r':		/* handle CRLF like LF */
+				crlf_is_lf = 1;
+				break;
 			case 'd':
 				del = 1;
 				break;

@@ -851,6 +859,9 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 	if (!fmt)
 		printf("## Warning: defaulting to text format\n");
 
+	if (sep != '\n' && crlf_is_lf )
+		crlf_is_lf = 0;
+
 	addr = (char *)simple_strtoul(argv[0], NULL, 16);
 
 	if (argc == 2) {

@@ -888,7 +899,7 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 		addr = (char *)ep->data;
 	}
 
-	if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR) == 0) {
+	if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR, crlf_is_lf) == 0) {
 		error("Environment import failed: errno = %d\n", errno);
 		return 1;
 	}

@@ -974,7 +985,7 @@ U_BOOT_CMD(
 #if defined(CONFIG_CMD_GREPENV)
 	"env grep string [...] - search environment\n"
 #endif
-	"env import [-d] [-t | -b | -c] addr [size] - import environment\n"
+	"env import [-d] [-t [-r] | | -b | -c] addr [size] - import environment\n"
 	"env print [name ...] - print environment\n"
 #if defined(CONFIG_CMD_RUN)
 	"env run var [...] - run commands in an environment variable\n"

common/env_common.c +2 -2

index c33d22d..9b50ffe 100644
@@ -182,7 +182,7 @@ void set_default_env(const char *s)
 	}
 
 	if (himport_r(&env_htab, (char *)default_environment,
-			sizeof(default_environment), '\0', 0) == 0)
+			sizeof(default_environment), '\0', 0, 0) == 0)
 		error("Environment import failed: errno = %d\n", errno);
 
 	gd->flags |= GD_FLG_ENV_READY;

@@ -207,7 +207,7 @@ int env_import(const char *buf, int check)
 		}
 	}
 
-	if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0)) {
+	if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, 0)) {
 		gd->flags |= GD_FLG_ENV_READY;
 		return 1;
 	}

include/search.h +1 -1

index ef53edb..053cf07 100644
@@ -96,7 +96,7 @@ extern ssize_t hexport_r(struct hsearch_data *__htab,
 
 extern int himport_r(struct hsearch_data *__htab,
 		     const char *__env, size_t __size, const char __sep,
-		     int __flag);
+		     int __flag, int __crlf_is_lf);
 
 /* Flags for himport_r() */
 #define	H_NOCLEAR	1	/* do not clear hash table before importing */

lib/hashtable.c +16 -1

index abd61c8..24e73a8 100644
@@ -639,7 +639,7 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep,
  */
 
 int himport_r(struct hsearch_data *htab,
-	      const char *env, size_t size, const char sep, int flag)
+	      const char *env, size_t size, const char sep, int flag, int crlf_is_lf)
 {
 	char *data, *sp, *dp, *name, *value;

@@ -698,6 +698,21 @@ int himport_r(struct hsearch_data *htab,
 		}
 	}
 
+	if(!size)
+		return 1;		/* everything OK */
+	if(crlf_is_lf) {
+		/* Remove Carriage Returns in front of Line Feeds */
+		unsigned ignored_crs = 0;
+		for(;dp < data + size && *dp; ++dp) {
+			if(*dp == '\r' &&
+			   dp < data + size - 1 && *(dp+1) == '\n')
+				++ignored_crs;
+			else
+				*(dp-ignored_crs) = *dp;
+		}
+		size -= ignored_crs;
+		dp = data;
+	}
 	/* Parse environment; allow for '\0' and 'sep' as separators */
 	do {
 		ENTRY e, *rv;

Signature · 1 line

1.7.3.4

Wolfgang Denk

2012-05-13 18:45 UTC #22 lore raw

PATCH 1/2 Add option -r to env import to allow import of text files with CRLF as line endings

In reply to Alexander Holler

Dear Alexander Holler,

Quoted 6 lines · In message <[email protected]> you wrote:

When this option is enabled, CRLF is treated like LF when importing environments from text files, which means CRs ('\r') in front of LFs ('\n') are just ignored.
Drawback of enabling this option is that (maybe exported) variables which have a trailing CR in their content will get imported without that CR.

Quoted 7 lines

Signed-off-by Alexander Holler <[email protected]>

---
 common/cmd_nvedit.c |   17 ++++++++++++++---
 common/env_common.c |    4 ++--
 include/search.h    |    2 +-
 lib/hashtable.c     |   23 +++++++++++++++++++----
 4 files changed, 36 insertions(+), 10 deletions(-)

Please make this optional. I don't want the increased code size for all boards when probablyonly one or two will ever activate this. And please add documentation, and include a note that the use of this option is discouraged, as the problem should be addressed outside U-Boot.

Best regards,

Wolfgang Denk

Signature · 6 lines

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
In the future, you're going to get computers as prizes  in  breakfast
cereals.  You'll  throw  them out because your house will be littered
with them.                                             - Robert Lucky