Support DOS line endings

PATCH In review Matthew Wilcox 5 participants 11 messages Jul 7, 2006 – Jul 19, 2006 Kconfig

Share & tools

Matthew Wilcox

author

2006-07-07 17:35 UTC #0 lore raw

PATCH 1/1 Support DOS line endings

Kconfig doesn't currently handle config files with DOS line endings. While these are, of course, an abomination, etc, etc, it can be handy to not have to convert them first. It's also a tiny patch and even adds support for lines ending in just \r or even \n\r.

Signed-off-by Matthew Wilcox <[email protected]>

Index: ./scripts/kconfig/confdata.c
===================================================================
RCS file: /var/cvs/linux-2.6/scripts/kconfig/confdata.c,v retrieving revision 1.11

scripts/kconfig/confdata.c +4 -0

diff -u -p -r1.11 confdata.c
@@ -177,6 +177,9 @@ int conf_read_simple(const char *name)
 			p2 = strchr(p, '\n');
 			if (p2)
 				*p2 = 0;
+			p2 = strchr(p, '\r');
+			if (p2)
+				*p2 = 0;
 			sym = sym_find(line + 7);
 			if (!sym) {
 				conf_warning("trying to assign nonexistent symbol %s", line + 7);

@@ -234,6 +237,7 @@ int conf_read_simple(const char *name)
 			}
 			break;
 		case '\n':
+		case '\r':
 			break;
 		default:
 			conf_warning("unexpected data");

Sam Ravnborg

2006-07-07 22:36 UTC #1 lore raw

Quoted 4 lines · Matthew Wilcox

Kconfig doesn't currently handle config files with DOS line endings. While these are, of course, an abomination, etc, etc, it can be handy to not have to convert them first. It's also a tiny patch and even adds support for lines ending in just \r or even \n\r.

Applied after updating path to -rc1.

Sam

Roman Zippel

2006-07-08 03:23 UTC #2 lore raw

In reply to Matthew Wilcox

Hi,

Quoted 4 lines · Matthew Wilcox

Kconfig doesn't currently handle config files with DOS line endings. While these are, of course, an abomination, etc, etc, it can be handy to not have to convert them first. It's also a tiny patch and even adds support for lines ending in just \r or even \n\r.

Did you try the latter? Unless you told fgets() about it I don't see how it should work.

scripts/kconfig/confdata.c int conf_read_simple(const char *name) · line 182

Quoted 5 lines

scripts/kconfig/confdata.c +3 -0

 			if (p2)
 				*p2 = 0;
+			p2 = strchr(p, '\r');
+			if (p2)
+				*p2 = 0;

I think something like this would be simpler:

	if (p2[-1] == '\r')
		p2[-1] = 0;

bye, Roman

Sam Ravnborg

2006-07-13 18:18 UTC #3 lore raw

Quoted 20 lines · Roman Zippel

Hi,
Quoted 4 lines · Matthew Wilcox
Kconfig doesn't currently handle config files with DOS line endings. While these are, of course, an abomination, etc, etc, it can be handy to not have to convert them first. It's also a tiny patch and even adds support for lines ending in just \r or even \n\r.
Did you try the latter? Unless you told fgets() about it I don't see how it should work.
Review
Quoted 5 lines
(quoted diff) +3 -0
 			if (p2)
 				*p2 = 0;
+			p2 = strchr(p, '\r');
+			if (p2)
+				*p2 = 0;
I think something like this would be simpler:

Quoted 2 lines

	if (p2[-1] == '\r')
		p2[-1] = 0;

Negative index'es always make me supsicious.
I've applied followign patch.
The fgets thing I have not looked at.

Sam

scripts/kconfig/confdata.c +8 -3

index 2ee48c3..a30b1bb 100644
@@ -192,9 +192,14 @@ load:
 			if (!p)
 				continue;
 			*p++ = 0;
-			p2 = strchr(p, '\n');
-			if (p2)
-				*p2 = 0;
+			p2 = p;
+		        while (*p2) {
+				if (*p2 == '\r' || *p2 == '\n') {
+					*p2 = 0;
+					break;
+				}
+				p2++;
+			}
 			if (def == S_DEF_USER) {
 				sym = sym_find(line + 7);
 				if (!sym) {

Matthew Wilcox

author

2006-07-13 18:29 UTC #4 lore raw

Quoted 3 lines · Sam Ravnborg

Negative index'es always make me supsicious.
I've applied followign patch.
The fgets thing I have not looked at.

Did you apply the "case '\r'" hunk (around line 269) too? If not, it'll spew "unexpected data" error messages for every blank line.

Roman Zippel

2006-07-13 18:47 UTC #5 lore raw

In reply to Sam Ravnborg

Hi,

Quoted 3 lines · Sam Ravnborg

Quoted 2 lines
      if (p2[-1] == '\r')
              p2[-1] = 0;
Negative index'es always make me supsicious.

Opencoding of the strchr is not much better, you can change the above also to (*--p2 == '\r').

Quoted 9 lines

scripts/kconfig/confdata.c +0 -1

index 2ee48c3..a30b1bb 100644
@@ -192,9 +192,14 @@ load:
 			if (!p)
 				continue;
 			*p++ = 0;
-			p2 = strchr(p, '\n');

scripts/kconfig/confdata.c load: · line 202

Quoted 10 lines

scripts/kconfig/confdata.c +8 -2

-			if (p2)
-				*p2 = 0;
+			p2 = p;
+		        while (*p2) {
+				if (*p2 == '\r' || *p2 == '\n') {
+					*p2 = 0;
+					break;
+				}
+				p2++;
+			}

IMO that's still too complicated, a '\n' is always the last character (unless the line was too long), with an optional '\r' in front of it.

bye, Roman

Sam Ravnborg

2006-07-13 19:35 UTC #6 lore raw

Quoted 10 lines · Roman Zippel

Hi,
Quoted 3 lines · Sam Ravnborg
Quoted 2 lines
      if (p2[-1] == '\r')
              p2[-1] = 0;
Negative index'es always make me supsicious.
Opencoding of the strchr is not much better, you can change the above also to (*--p2 == '\r').

There is no semantic difference between decrementing a variable and a negative index.
But I used this one anyway.
So now it look like this.

Sam

scripts/kconfig/confdata.c +3 -0

index a69d8ac..b7e7281 100644
@@ -195,6 +195,8 @@ load:
 			p2 = strchr(p, '\n');
 			if (p2)
 				*p2 = 0;
+			if (*--p2 == '\r')
+				*p2 = 0;
 			if (def == S_DEF_USER) {
 				sym = sym_find(line + 7);
 				if (!sym) {

@@ -266,6 +268,7 @@ load:
 				;
 			}
 			break;
+		case '\r':
 		case '\n':
 			break;
 		default:

Matthew Wilcox

author

2006-07-13 20:02 UTC #7 lore raw

scripts/kconfig/confdata.c load: · line 199

Quoted 8 lines · Sam Ravnborg

scripts/kconfig/confdata.c +2 -0

@@ -195,6 +195,8 @@ load:
 			p2 = strchr(p, '\n');
 			if (p2)
 				*p2 = 0;
+			if (*--p2 == '\r')
+				*p2 = 0;

... but if p2 is NULL ...

so:

			if (p2) {
				*p2 = 0;
				if (*--p2 == '\r')
					*p2 = 0;
			}

but maybe it'd be better to do ...

			if (p2) {
				*p2-- = 0;
				if (*p2 == '\r')
					*p2 = 0;
			}

Sam Ravnborg

2006-07-13 21:07 UTC #8 lore raw

Quoted 19 lines · Matthew Wilcox

scripts/kconfig/confdata.c load: · line 199
Quoted 8 lines · Sam Ravnborg
scripts/kconfig/confdata.c +2 -0
@@ -195,6 +195,8 @@ load:
 			p2 = strchr(p, '\n');
 			if (p2)
 				*p2 = 0;
+			if (*--p2 == '\r')
+				*p2 = 0;
... but if p2 is NULL ...
so:
			if (p2) {
				*p2 = 0;
				if (*--p2 == '\r')
					*p2 = 0;
			}

Quoted 7 lines

but maybe it'd be better to do ...
			if (p2) {
				*p2-- = 0;
				if (*p2 == '\r')
					*p2 = 0;
			}

Thanks. Maybe I should just stop trying to code anything today ;-)

Sam

Bernd Petrovitsch

2006-07-13 21:41 UTC #9 lore raw

On Thu, 2006-07-13 at 23:07 +0200, Sam Ravnborg wrote:
[...]

Quoted 1 line

Thanks. Maybe I should just stop trying to code anything today ;-)

Or take it as a sign to not support DOS line endings. SCNR,
Bernd

Signature · 3 lines

Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services

Alistair John Strachan

2006-07-19 12:03 UTC #10 lore raw

Quoted 6 lines · Bernd Petrovitsch

On Thu, 2006-07-13 at 23:07 +0200, Sam Ravnborg wrote:
[...]
Quoted 1 line
Thanks. Maybe I should just stop trying to code anything today ;-)
Or take it as a sign to not support DOS line endings.

It certainly would be better to warn if a file with DOS line endings is being used, even if supported, so that such files never enter the kernel undetected.

Signature · 5 lines

Cheers,
Alistair.

Third year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.