diff mbox

[U-Boot] Ignore all Carriage Returns when importing an environment.

Message ID 1336720486-7424-1-git-send-email-holler@ahsoftware.de
State Superseded
Headers show

Commit Message

Alexander Holler May 11, 2012, 7:14 a.m. UTC
This is used for compatibility with text files which are
using CRLF instead of LF as the end of a line.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 lib/hashtable.c |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

Comments

Wolfgang Denk May 11, 2012, 7:09 p.m. UTC | #1
Dear Alexander Holler,

In message <1336720486-7424-1-git-send-email-holler@ahsoftware.de> you wrote:
> This is used for compatibility with text files which are
> using CRLF instead of LF as the end of a line.

I don't think we should do this.  If you have text files with CR-LF
line endings, then please use external tools (like dos2unix) to filter
these and bring them into the appropriate format.

I would like to put as little restrictions on the content of an
environment variable as possible.  I can see valid use for strings
that contain a CR character.

Best regards,

Wolfgang Denk
Marek Vasut May 12, 2012, 6:17 a.m. UTC | #2
Dear Alexander Holler,

> This is used for compatibility with text files which are
> using CRLF instead of LF as the end of a line.
> 
> Signed-off-by: Alexander Holler <holler@ahsoftware.de>

Why don't you run the file trough dos2unix or tr -d '\r' ?

> ---
>  lib/hashtable.c |   18 +++++++++++++++---
>  1 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/hashtable.c b/lib/hashtable.c
> index abd61c8..6e146ce 100644
> --- a/lib/hashtable.c
> +++ b/lib/hashtable.c
> @@ -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.]
> @@ -642,6 +642,7 @@ int himport_r(struct hsearch_data *htab,
>  	      const char *env, size_t size, const char sep, int flag)
>  {
>  	char *data, *sp, *dp, *name, *value;
> +	unsigned ignored_crs;
> 
>  	/* Test for correct arguments.  */
>  	if (htab == NULL) {
> @@ -698,6 +699,17 @@ int himport_r(struct hsearch_data *htab,
>  		}
>  	}
> 
> +	/* Remove all Carriage Returns */
> +	ignored_crs = 0;
> +	for(;dp < data + size && *dp; ++dp) {
> +		if( *dp == '\r' )
> +			++ignored_crs;
> +		else if(ignored_crs)
> +			*(dp-ignored_crs) = *dp;
> +	}
> +	size -= ignored_crs;
> +	dp = data;
> +
>  	/* Parse environment; allow for '\0' and 'sep' as separators */
>  	do {
>  		ENTRY e, *rv;

Best regards,
Marek Vasut
Alexander Holler May 12, 2012, 2:22 p.m. UTC | #3
Am 11.05.2012 21:09, schrieb Wolfgang Denk:
> Dear Alexander Holler,
>
> In message<1336720486-7424-1-git-send-email-holler@ahsoftware.de>  you wrote:
>> This is used for compatibility with text files which are
>> using CRLF instead of LF as the end of a line.
>
> I don't think we should do this.  If you have text files with CR-LF
> line endings, then please use external tools (like dos2unix) to filter
> these and bring them into the appropriate format.
>
> I would like to put as little restrictions on the content of an
> environment variable as possible.  I can see valid use for strings
> that contain a CR character.

I don't see any reasonable usage for carriage returns in imported 
environment variables, but I've seen many people from the windows camp 
struggling in writing small text files to set some environment variables 
(which mostly end up in the kernel cmdline). Especially because those 
CR's often will lead to obscure errors because almost nothing (in u-boot 
or linux) is able to handle them.

Anyway I don't really care, I just found it very user friendly to strip 
the carriage returns, especially for those embedded newbies which are in 
need to use some unnamed windows IDE.

So I've decided (after having that patch lying around for about a year) 
to finally post it.

Regards,

Alexander
Alexander Holler May 12, 2012, 2:25 p.m. UTC | #4
Am 12.05.2012 08:17, schrieb Marek Vasut:
> Dear Alexander Holler,
>
>> This is used for compatibility with text files which are
>> using CRLF instead of LF as the end of a line.
>>
>> Signed-off-by: Alexander Holler<holler@ahsoftware.de>
>
> Why don't you run the file trough dos2unix or tr -d '\r' ?

Because my files don't contain CRs. ;)

Regards,

Alexander

>
>> ---
>>   lib/hashtable.c |   18 +++++++++++++++---
>>   1 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/hashtable.c b/lib/hashtable.c
>> index abd61c8..6e146ce 100644
>> --- a/lib/hashtable.c
>> +++ b/lib/hashtable.c
>> @@ -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.]
>> @@ -642,6 +642,7 @@ int himport_r(struct hsearch_data *htab,
>>   	      const char *env, size_t size, const char sep, int flag)
>>   {
>>   	char *data, *sp, *dp, *name, *value;
>> +	unsigned ignored_crs;
>>
>>   	/* Test for correct arguments.  */
>>   	if (htab == NULL) {
>> @@ -698,6 +699,17 @@ int himport_r(struct hsearch_data *htab,
>>   		}
>>   	}
>>
>> +	/* Remove all Carriage Returns */
>> +	ignored_crs = 0;
>> +	for(;dp<  data + size&&  *dp; ++dp) {
>> +		if( *dp == '\r' )
>> +			++ignored_crs;
>> +		else if(ignored_crs)
>> +			*(dp-ignored_crs) = *dp;
>> +	}
>> +	size -= ignored_crs;
>> +	dp = data;
>> +
>>   	/* Parse environment; allow for '\0' and 'sep' as separators */
>>   	do {
>>   		ENTRY e, *rv;
>
> Best regards,
> Marek Vasut
Marek Vasut May 12, 2012, 2:46 p.m. UTC | #5
Dear Alexander Holler,

> Am 12.05.2012 08:17, schrieb Marek Vasut:
> > Dear Alexander Holler,
> > 
> >> This is used for compatibility with text files which are
> >> using CRLF instead of LF as the end of a line.
> >> 
> >> Signed-off-by: Alexander Holler<holler@ahsoftware.de>
> > 
> > Why don't you run the file trough dos2unix or tr -d '\r' ?
> 
> Because my files don't contain CRs. ;)

s/you/someone else/ :)

> 
> Regards,
> 
> Alexander
> 
> >> ---
> >> 
> >>   lib/hashtable.c |   18 +++++++++++++++---
> >>   1 files changed, 15 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/lib/hashtable.c b/lib/hashtable.c
> >> index abd61c8..6e146ce 100644
> >> --- a/lib/hashtable.c
> >> +++ b/lib/hashtable.c
> >> @@ -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.]
> >> 
> >> @@ -642,6 +642,7 @@ int himport_r(struct hsearch_data *htab,
> >> 
> >>   	      const char *env, size_t size, const char sep, int flag)
> >>   
> >>   {
> >>   
> >>   	char *data, *sp, *dp, *name, *value;
> >> 
> >> +	unsigned ignored_crs;
> >> 
> >>   	/* Test for correct arguments.  */
> >>   	if (htab == NULL) {
> >> 
> >> @@ -698,6 +699,17 @@ int himport_r(struct hsearch_data *htab,
> >> 
> >>   		}
> >>   	
> >>   	}
> >> 
> >> +	/* Remove all Carriage Returns */
> >> +	ignored_crs = 0;
> >> +	for(;dp<  data + size&&  *dp; ++dp) {
> >> +		if( *dp == '\r' )
> >> +			++ignored_crs;
> >> +		else if(ignored_crs)
> >> +			*(dp-ignored_crs) = *dp;
> >> +	}
> >> +	size -= ignored_crs;
> >> +	dp = data;
> >> +
> >> 
> >>   	/* Parse environment; allow for '\0' and 'sep' as separators */
> >>   	do {
> >>   	
> >>   		ENTRY e, *rv;
> > 
> > Best regards,
> > Marek Vasut
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Alexander Holler May 12, 2012, 2:50 p.m. UTC | #6
Am 12.05.2012 16:46, schrieb Marek Vasut:
> Dear Alexander Holler,
>
>> Am 12.05.2012 08:17, schrieb Marek Vasut:
>>> Dear Alexander Holler,
>>>
>>>> This is used for compatibility with text files which are
>>>> using CRLF instead of LF as the end of a line.
>>>>
>>>> Signed-off-by: Alexander Holler<holler@ahsoftware.de>
>>>
>>> Why don't you run the file trough dos2unix or tr -d '\r' ?
>>
>> Because my files don't contain CRs. ;)
>
> s/you/someone else/ :)

Try to explain a windows user that a text file should not contain 
carriage returns and how he can achieve that.

You'll do it once, maybe twice, than you will surrender. ;)

Regards,

Alexander
Marek Vasut May 12, 2012, 4:08 p.m. UTC | #7
Dear Alexander Holler,

> Am 12.05.2012 16:46, schrieb Marek Vasut:
> > Dear Alexander Holler,
> > 
> >> Am 12.05.2012 08:17, schrieb Marek Vasut:
> >>> Dear Alexander Holler,
> >>> 
> >>>> This is used for compatibility with text files which are
> >>>> using CRLF instead of LF as the end of a line.
> >>>> 
> >>>> Signed-off-by: Alexander Holler<holler@ahsoftware.de>
> >>> 
> >>> Why don't you run the file trough dos2unix or tr -d '\r' ?
> >> 
> >> Because my files don't contain CRs. ;)
> > 
> > s/you/someone else/ :)
> 
> Try to explain a windows user that a text file should not contain
> carriage returns and how he can achieve that.
> 
> You'll do it once, maybe twice, than you will surrender. ;)

But you're fixing the problem at a wrong place, aren't you? Besides, removing \r 
might harm some environments, don't you think?

> Regards,
> 
> Alexander

Best regards,
Marek Vasut
Alexander Holler May 12, 2012, 6:33 p.m. UTC | #8
Am 12.05.2012 18:08, schrieb Marek Vasut:
> Dear Alexander Holler,
> 
>> Am 12.05.2012 16:46, schrieb Marek Vasut:
>>> Dear Alexander Holler,
>>>
>>>> Am 12.05.2012 08:17, schrieb Marek Vasut:
>>>>> Dear Alexander Holler,
>>>>>
>>>>>> This is used for compatibility with text files which are
>>>>>> using CRLF instead of LF as the end of a line.
>>>>>>
>>>>>> Signed-off-by: Alexander Holler<holler@ahsoftware.de>
>>>>>
>>>>> Why don't you run the file trough dos2unix or tr -d '\r' ?
>>>>
>>>> Because my files don't contain CRs. ;)
>>>
>>> s/you/someone else/ :)
>>
>> Try to explain a windows user that a text file should not contain
>> carriage returns and how he can achieve that.
>>
>> You'll do it once, maybe twice, than you will surrender. ;)
> 
> But you're fixing the problem at a wrong place, aren't you? Besides, removing \r 
> might harm some environments, don't you think?

No, there is no other layer between the user and that function.

Anyway, as already said, I don't care, just had this patch lying around
a year and I finally surrendered to post it here (in favor of those
users). But it seems I was right in not posting it. ;)

Regards,

Alexander
Marek Vasut May 12, 2012, 6:37 p.m. UTC | #9
Dear Alexander Holler,

> Am 12.05.2012 18:08, schrieb Marek Vasut:
> > Dear Alexander Holler,
> > 
> >> Am 12.05.2012 16:46, schrieb Marek Vasut:
> >>> Dear Alexander Holler,
> >>> 
> >>>> Am 12.05.2012 08:17, schrieb Marek Vasut:
> >>>>> Dear Alexander Holler,
> >>>>> 
> >>>>>> This is used for compatibility with text files which are
> >>>>>> using CRLF instead of LF as the end of a line.
> >>>>>> 
> >>>>>> Signed-off-by: Alexander Holler<holler@ahsoftware.de>
> >>>>> 
> >>>>> Why don't you run the file trough dos2unix or tr -d '\r' ?
> >>>> 
> >>>> Because my files don't contain CRs. ;)
> >>> 
> >>> s/you/someone else/ :)
> >> 
> >> Try to explain a windows user that a text file should not contain
> >> carriage returns and how he can achieve that.
> >> 
> >> You'll do it once, maybe twice, than you will surrender. ;)
> > 
> > But you're fixing the problem at a wrong place, aren't you? Besides,
> > removing \r might harm some environments, don't you think?
> 
> No, there is no other layer between the user and that function.
> 
> Anyway, as already said, I don't care, just had this patch lying around
> a year and I finally surrendered to post it here (in favor of those
> users). But it seems I was right in not posting it. ;)

Don't get me wrong, I'm not opposed to this patch, nor I want to demotivate you 
in further submissions. I'm just trying to figure out if actually surrending to 
crappy software on one side is a good move on our side. I'd prefer to get more 
oppinions from other people actually, I don't want the guilt to fall on me :-)

> 
> Regards,
> 
> Alexander

Best regards,
Marek Vasut
Alexander Holler May 12, 2012, 7:10 p.m. UTC | #10
Am 12.05.2012 20:37, schrieb Marek Vasut:
> Dear Alexander Holler,
>
>> Am 12.05.2012 18:08, schrieb Marek Vasut:
>>> Dear Alexander Holler,
>>>
>>>> Am 12.05.2012 16:46, schrieb Marek Vasut:
>>>>> Dear Alexander Holler,
>>>>>
>>>>>> Am 12.05.2012 08:17, schrieb Marek Vasut:
>>>>>>> Dear Alexander Holler,
>>>>>>>
>>>>>>>> This is used for compatibility with text files which are
>>>>>>>> using CRLF instead of LF as the end of a line.
>>>>>>>>
>>>>>>>> Signed-off-by: Alexander Holler<holler@ahsoftware.de>
>>>>>>>
>>>>>>> Why don't you run the file trough dos2unix or tr -d '\r' ?
>>>>>>
>>>>>> Because my files don't contain CRs. ;)
>>>>>
>>>>> s/you/someone else/ :)
>>>>
>>>> Try to explain a windows user that a text file should not contain
>>>> carriage returns and how he can achieve that.
>>>>
>>>> You'll do it once, maybe twice, than you will surrender. ;)
>>>
>>> But you're fixing the problem at a wrong place, aren't you? Besides,
>>> removing \r might harm some environments, don't you think?
>>
>> No, there is no other layer between the user and that function.
>>
>> Anyway, as already said, I don't care, just had this patch lying around
>> a year and I finally surrendered to post it here (in favor of those
>> users). But it seems I was right in not posting it. ;)
>
> Don't get me wrong, I'm not opposed to this patch, nor I want to demotivate you
> in further submissions. I'm just trying to figure out if actually surrending to
> crappy software on one side is a good move on our side. I'd prefer to get more
> oppinions from other people actually, I don't want the guilt to fall on me :-)

In the good old days (tm) with line printers and almost endless paper 
from dead trees, CRLF instead only LF as line endings in text files did 
made sense.

Regards,

Alexander
Wolfgang Denk May 12, 2012, 9:18 p.m. UTC | #11
Dear Alexander Holler,

In message <4FAE7232.1050407@ahsoftware.de> you wrote:
>
> > I would like to put as little restrictions on the content of an
> > environment variable as possible.  I can see valid use for strings
> > that contain a CR character.
> 
> I don't see any reasonable usage for carriage returns in imported 
> environment variables, but I've seen many people from the windows camp 

A CR causes the output to re-start from start of line. I can
construct all kind of fancy disply by using "echo $var" - especially
so if "var" can contain control characters including CR.  It makes no
sense striiping these out.

> struggling in writing small text files to set some environment variables 
> (which mostly end up in the kernel cmdline). Especially because those 
> CR's often will lead to obscure errors because almost nothing (in u-boot 
> or linux) is able to handle them.

This is a problem that is as old as DOS, and solutions for this have
been known since.  I already mentioned dos2unix.

> Anyway I don't really care, I just found it very user friendly to strip 
> the carriage returns, especially for those embedded newbies which are in 
> need to use some unnamed windows IDE.
> 
> So I've decided (after having that patch lying around for about a year) 
> to finally post it.

Thanks - but it adds restrictions to doing perfectly valid things.  I
see the disadvantages significantly bigger than what we can win - keep
in mind, that dealing wqith DOS line endings is a topic that is
decades old.

Best regards,

Wolfgang Denk
Wolfgang Denk May 12, 2012, 9:19 p.m. UTC | #12
Dear Alexander Holler,

In message <4FAE78BE.3090302@ahsoftware.de> you wrote:
>
> Try to explain a windows user that a text file should not contain 
> carriage returns and how he can achieve that.
> 
> You'll do it once, maybe twice, than you will surrender. ;)

I disagree here.  But of course YMMV.


But we should not remove functionality to make life easier for
ignorant people who don't listen to your advice.

Best regards,

Wolfgang Denk
Wolfgang Denk May 12, 2012, 9:21 p.m. UTC | #13
Dear Alexander Holler,

In message <4FAEB5C2.40004@ahsoftware.de> you wrote:
>
> In the good old days (tm) with line printers and almost endless paper 
> from dead trees, CRLF instead only LF as line endings in text files did 
> made sense.

It still makes a lot of sense in tty output.  Guess how all the
one-line status status lines are being printed?

Best regards,

Wolfgang Denk
Alexander Holler May 13, 2012, 7:43 a.m. UTC | #14
Am 12.05.2012 23:21, schrieb Wolfgang Denk:
> Dear Alexander Holler,
>
> In message<4FAEB5C2.40004@ahsoftware.de>  you wrote:
>>
>> In the good old days (tm) with line printers and almost endless paper
>> from dead trees, CRLF instead only LF as line endings in text files did
>> made sense.
>
> It still makes a lot of sense in tty output.  Guess how all the
> one-line status status lines are being printed?

That's why I hinted the line printers in response to "crappy software".

Regards,

Alexander
Alexander Holler May 13, 2012, 8:07 a.m. UTC | #15
Hello,

Am 12.05.2012 23:18, schrieb Wolfgang Denk:

> Thanks - but it adds restrictions to doing perfectly valid things.  I
> see the disadvantages significantly bigger than what we can win - keep
> in mind, that dealing wqith DOS line endings is a topic that is
> decades old.

Not dealing with them too. ;)

Two other solutions come to mind: Expanding the if( '\r' ) so that it 
only removes CRs if they are before LF or make \r possible in import and 
use \r in export.

Regards,

Alexander
Wolfgang Denk May 13, 2012, 9:27 a.m. UTC | #16
Dear Alexander Holler,

In message <4FAF6BAB.5040900@ahsoftware.de> you wrote:
> 
> Not dealing with them too. ;)
> 
> Two other solutions come to mind: Expanding the if( '\r' ) so that it 
> only removes CRs if they are before LF or make \r possible in import and 
> use \r in export.

I think I would accept an option (configurable) to accept \r\n line
endings when importing text input.  For output, there shall always be
\n only.

Best regards,

Wolfgang Denk
diff mbox

Patch

diff --git a/lib/hashtable.c b/lib/hashtable.c
index abd61c8..6e146ce 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -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.]
@@ -642,6 +642,7 @@  int himport_r(struct hsearch_data *htab,
 	      const char *env, size_t size, const char sep, int flag)
 {
 	char *data, *sp, *dp, *name, *value;
+	unsigned ignored_crs;
 
 	/* Test for correct arguments.  */
 	if (htab == NULL) {
@@ -698,6 +699,17 @@  int himport_r(struct hsearch_data *htab,
 		}
 	}
 
+	/* Remove all Carriage Returns */
+	ignored_crs = 0;
+	for(;dp < data + size && *dp; ++dp) {
+		if( *dp == '\r' )
+			++ignored_crs;
+		else if(ignored_crs)
+			*(dp-ignored_crs) = *dp;
+	}
+	size -= ignored_crs;
+	dp = data;
+
 	/* Parse environment; allow for '\0' and 'sep' as separators */
 	do {
 		ENTRY e, *rv;