Re: iso9660 endianness cleanup patch

Re: iso9660 endianness cleanup patch

[Andrzej Krzysztofowicz]

Are you sure that the arguments of the following casting

> +	return le16_to_cpu(*(u16 *)p);

> +	return be16_to_cpu(*(u16 *)p);

> +	return le32_to_cpu(*(u32 *)p);

> +	return be32_to_cpu(*(u32 *)p);

are properly aligned ?
I did not revise the code to check it, but AFAIK improperly aligned
char* pointers cause problem with casting to pointers to 16/32-bit data
on some architectures (I heard of sucj a problem with alpha).

Maybe there was a reason that the original code did operate on bytes here...

Andrzej

-- 
=======================================================================
  Andrzej M. Krzysztofowicz               ankry@mif.pg.gda.pl
  phone (48)(58) 347 14 61
Faculty of Applied Phys. & Math.,   Technical University of Gdansk

Re: iso9660 endianness cleanup patch

[H. Peter Anvin]

Andrzej Krzysztofowicz wrote:
> 
> Are you sure that the arguments of the following casting
> 
> > +     return le16_to_cpu(*(u16 *)p);
> 
> > +     return be16_to_cpu(*(u16 *)p);
> 
> > +     return le32_to_cpu(*(u32 *)p);
> 
> > +     return be32_to_cpu(*(u32 *)p);
> 
> are properly aligned ?
> I did not revise the code to check it, but AFAIK improperly aligned
> char* pointers cause problem with casting to pointers to 16/32-bit data
> on some architectures (I heard of sucj a problem with alpha).
> 
> Maybe there was a reason that the original code did operate on bytes here...
> 

Oh bother, you're right of course.  We need some kind of standardized
macro for indirecting through a potentially unaligned pointer.  It can
just do the dereference (e.g. x86), use left/right accesses (e.g. MIPS),
or do it by byte (others).

Ports people, what do you think?

	-hpa

-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt

Re: iso9660 endianness cleanup patch

[H. Peter Anvin]

hpa@transmeta.com wrote:
> 
> Oh bother, you're right of course.  We need some kind of standardized
> macro for indirecting through a potentially unaligned pointer.  It can
> just do the dereference (e.g. x86), use left/right accesses (e.g. MIPS),
> or do it by byte (others).
> 
> Ports people, what do you think?
> 

My, my, my... I'm really 100% alert this morning.  Before anyone clues me
in, I did find <asm/unaligned.h> when I actually bothered looking.  New
patch in the making.

	-hpa

-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt

Re: FIXED iso9660 endianness cleanup patch

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 521 bytes --]

Okay, this one should actually work as advertised.  Pardon the mental
meltdown earlier.

This also changes "extern inline" to "static inline" (per Linus' request)
and always ignores the bigendian part of a bi-endian datum (per Tim
Riker's observation that lots of [Windoze?] programs get that wrong.)

It's still a bit of a loss to have to do unaligned load and endianness
conversion as two operations; on some machines it's doubtlessly faster to
do both at the same time.  However, *those* macros I didn't find...

	-hpa

[-- Attachment #2: isofs-2.4.4-2.diff --]
[-- Type: text/plain, Size: 3408 bytes --]

diff -ur stock3/linux-2.4.4/fs/isofs/util.c linux-2.4.4/fs/isofs/util.c
--- stock3/linux-2.4.4/fs/isofs/util.c	Wed Nov 29 10:11:38 2000
+++ linux-2.4.4/fs/isofs/util.c	Mon Apr 30 20:49:33 2001
@@ -1,90 +1,9 @@
 /*
  *  linux/fs/isofs/util.c
- *
- *  The special functions in the file are numbered according to the section
- *  of the iso 9660 standard in which they are described.  isonum_733 will
- *  convert numbers according to section 7.3.3, etc.
- *
- *  isofs special functions.  This file was lifted in its entirety from
- *  the 386BSD iso9660 filesystem, by Pace Willisson <pace@blitz.com>.
  */
 
 #include <linux/time.h>
-
-int
-isonum_711 (char * p)
-{
-	return (*p & 0xff);
-}
-
-int
-isonum_712 (char * p)
-{
-	int val;
-	
-	val = *p;
-	if (val & 0x80)
-		val |= 0xffffff00;
-	return (val);
-}
-
-int
-isonum_721 (char * p)
-{
-	return ((p[0] & 0xff) | ((p[1] & 0xff) << 8));
-}
-
-int
-isonum_722 (char * p)
-{
-	return (((p[0] & 0xff) << 8) | (p[1] & 0xff));
-}
-
-int
-isonum_723 (char * p)
-{
-#if 0
-	if (p[0] != p[3] || p[1] != p[2]) {
-		fprintf (stderr, "invalid format 7.2.3 number\n");
-		exit (1);
-	}
-#endif
-	return (isonum_721 (p));
-}
-
-int
-isonum_731 (char * p)
-{
-	return ((p[0] & 0xff)
-		| ((p[1] & 0xff) << 8)
-		| ((p[2] & 0xff) << 16)
-		| ((p[3] & 0xff) << 24));
-}
-
-int
-isonum_732 (char * p)
-{
-	return (((p[0] & 0xff) << 24)
-		| ((p[1] & 0xff) << 16)
-		| ((p[2] & 0xff) << 8)
-		| (p[3] & 0xff));
-}
-
-int
-isonum_733 (char * p)
-{
-#if 0
-	int i;
-
-	for (i = 0; i < 4; i++) {
-		if (p[i] != p[7-i]) {
-			fprintf (stderr, "bad format 7.3.3 number\n");
-			exit (1);
-		}
-	}
-#endif
-	return (isonum_731 (p));
-}
+#include <linux/iso_fs.h>
 
 /* 
  * We have to convert from a MM/DD/YY format to the Unix ctime format.
diff -ur stock3/linux-2.4.4/include/linux/iso_fs.h linux-2.4.4/include/linux/iso_fs.h
--- stock3/linux-2.4.4/include/linux/iso_fs.h	Fri Apr 27 15:48:20 2001
+++ linux-2.4.4/include/linux/iso_fs.h	Tue May  1 11:45:21 2001
@@ -165,14 +165,46 @@
 #define ISOFS_SUPER_MAGIC 0x9660
 
 #ifdef __KERNEL__
-extern int isonum_711(char *);
-extern int isonum_712(char *);
-extern int isonum_721(char *);
-extern int isonum_722(char *);
-extern int isonum_723(char *);
-extern int isonum_731(char *);
-extern int isonum_732(char *);
-extern int isonum_733(char *);
+/* Number conversion inlines, named after the section in ISO 9660
+   they correspond to. */
+
+#include <asm/byteorder.h>
+#include <asm/unaligned.h>
+
+static inline int isonum_711(char *p)
+{
+	return *(u8 *)p;
+}
+static inline int isonum_712(char *p)
+{
+	return *(s8 *)p;
+}
+static inline int isonum_721(char *p)
+{
+	return le16_to_cpu(get_unaligned((u16 *)p));
+}
+static inline int isonum_722(char *p)
+{
+	return be16_to_cpu(get_unaligned((u16 *)p));
+}
+static inline int isonum_723(char *p)
+{
+	/* Ignore bigendian datum due to broken mastering programs */
+	return le16_to_cpu(get_unaligned((u16 *)p));
+}
+static inline int isonum_731(char *p)
+{
+	return le32_to_cpu(get_unaligned((u32 *)p));
+}
+static inline int isonum_732(char *p)
+{
+	return be32_to_cpu(get_unaligned((u32 *)p));
+}
+static inline int isonum_733(char *p)
+{
+	/* Ignore bigendian datum due to broken mastering programs */
+	return le32_to_cpu(get_unaligned((u32 *)p));
+}
 extern int iso_date(char *, int);
 
 extern int parse_rock_ridge_inode(struct iso_directory_record *, struct inode *);

Re: iso9660 endianness cleanup patch

[Alan Cox]

> Oh bother, you're right of course.  We need some kind of standardized
> macro for indirecting through a potentially unaligned pointer.  It can

get_unaligned()

Re: iso9660 endianness cleanup patch

[Linus Torvalds]

On Tue, 1 May 2001, H. Peter Anvin wrote:
>
> Oh bother, you're right of course.  We need some kind of standardized
> macro for indirecting through a potentially unaligned pointer.

No we don't - because it already exists.

It's called "get_unaligned()".

		Linus

Re: iso9660 endianness cleanup patch

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> On Tue, 1 May 2001, H. Peter Anvin wrote:
> >
> > Oh bother, you're right of course.  We need some kind of standardized
> > macro for indirecting through a potentially unaligned pointer.
> 
> No we don't - because it already exists.
> 
> It's called "get_unaligned()".
> 

Well, we presumably do need it since it's there.  I *did* correct this
brain fault of mine a few hours ago.

Note that it might still be an idea to have get_unaligned_le32() & co...
on some machines le32_to_cpu(get_unaligned()) could potentially be a lot
more painful than it needs to be.

	-hpa


-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt

iso9660 endianness cleanup patch

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 391 bytes --]

Hi guys,

I was looking over the iso9660 code, and noticed that it was doing
endianness conversion via ad hoc *functions*, not even inlines; nor did
it take any advantage of the fact that iso9660 is bi-endian (has "all"
data in both bigendian and littleendian format.)

The attached patch fixes both.  It is against 2.4.4, but from the looks
of it it should patch against -ac as well.

	-hpa

[-- Attachment #2: isofs-2.4.4-1.diff --]
[-- Type: text/plain, Size: 3152 bytes --]

--- linux-2.4.4/include/linux/iso_fs.h	Fri Apr 27 15:48:20 2001
+++ linux-2.4.4-ciso/include/linux/iso_fs.h	Mon Apr 30 20:09:31 2001
@@ -165,14 +165,51 @@
 #define ISOFS_SUPER_MAGIC 0x9660
 
 #ifdef __KERNEL__
-extern int isonum_711(char *);
-extern int isonum_712(char *);
-extern int isonum_721(char *);
-extern int isonum_722(char *);
-extern int isonum_723(char *);
-extern int isonum_731(char *);
-extern int isonum_732(char *);
-extern int isonum_733(char *);
+/* Number conversion inlines, named after the section in ISO 9660
+   they correspond to. */
+
+#include <asm/byteorder.h>
+
+extern inline int isonum_711(char *p)
+{
+	return *(u8 *)p;
+}
+extern inline int isonum_712(char *p)
+{
+	return *(s8 *)p;
+}
+extern inline int isonum_721(char *p)
+{
+	return le16_to_cpu(*(u16 *)p);
+}
+extern inline int isonum_722(char *p)
+{
+	return be16_to_cpu(*(u16 *)p);
+}
+extern inline int isonum_723(char *p)
+{
+#ifdef __BIG_ENDIAN
+	return be16_to_cpu(*(u16 *)(p+2));
+#else
+	return le16_to_cpu(*(u16 *)p);
+#endif
+}
+extern inline int isonum_731(char *p)
+{
+	return le32_to_cpu(*(u32 *)p);
+}
+extern inline int isonum_732(char *p)
+{
+	return be32_to_cpu(*(u32 *)p);
+}
+extern inline int isonum_733(char *p)
+{
+#ifdef __BIG_ENDIAN
+	return be32_to_cpu(*(u32 *)(p+4));
+#else
+	return le32_to_cpu(*(u32 *)p);
+#endif
+}
 extern int iso_date(char *, int);
 
 extern int parse_rock_ridge_inode(struct iso_directory_record *, struct inode *);
--- linux-2.4.4/fs/isofs/util.c	Wed Nov 29 10:11:38 2000
+++ linux-2.4.4-ciso/fs/isofs/util.c	Mon Apr 30 20:04:24 2001
@@ -1,90 +1,9 @@
 /*
  *  linux/fs/isofs/util.c
- *
- *  The special functions in the file are numbered according to the section
- *  of the iso 9660 standard in which they are described.  isonum_733 will
- *  convert numbers according to section 7.3.3, etc.
- *
- *  isofs special functions.  This file was lifted in its entirety from
- *  the 386BSD iso9660 filesystem, by Pace Willisson <pace@blitz.com>.
  */
 
 #include <linux/time.h>
-
-int
-isonum_711 (char * p)
-{
-	return (*p & 0xff);
-}
-
-int
-isonum_712 (char * p)
-{
-	int val;
-	
-	val = *p;
-	if (val & 0x80)
-		val |= 0xffffff00;
-	return (val);
-}
-
-int
-isonum_721 (char * p)
-{
-	return ((p[0] & 0xff) | ((p[1] & 0xff) << 8));
-}
-
-int
-isonum_722 (char * p)
-{
-	return (((p[0] & 0xff) << 8) | (p[1] & 0xff));
-}
-
-int
-isonum_723 (char * p)
-{
-#if 0
-	if (p[0] != p[3] || p[1] != p[2]) {
-		fprintf (stderr, "invalid format 7.2.3 number\n");
-		exit (1);
-	}
-#endif
-	return (isonum_721 (p));
-}
-
-int
-isonum_731 (char * p)
-{
-	return ((p[0] & 0xff)
-		| ((p[1] & 0xff) << 8)
-		| ((p[2] & 0xff) << 16)
-		| ((p[3] & 0xff) << 24));
-}
-
-int
-isonum_732 (char * p)
-{
-	return (((p[0] & 0xff) << 24)
-		| ((p[1] & 0xff) << 16)
-		| ((p[2] & 0xff) << 8)
-		| (p[3] & 0xff));
-}
-
-int
-isonum_733 (char * p)
-{
-#if 0
-	int i;
-
-	for (i = 0; i < 4; i++) {
-		if (p[i] != p[7-i]) {
-			fprintf (stderr, "bad format 7.3.3 number\n");
-			exit (1);
-		}
-	}
-#endif
-	return (isonum_731 (p));
-}
+#include <linux/iso_fs.h>
 
 /* 
  * We have to convert from a MM/DD/YY format to the Unix ctime format.

Re: iso9660 endianness cleanup patch

[Linus Torvalds]

On Mon, 30 Apr 2001, H. Peter Anvin wrote:
> 
> The attached patch fixes both.  It is against 2.4.4, but from the looks
> of it it should patch against -ac as well.

Btw, please use "static inline" instead of "extern inline", as gcc may
decide not to inline the latter at all, leading to confusing link-time
errors. (Gcc may also decide not to inline "static inline", but then gcc
will output the actual body of the function out-of-line if it gets used,
so you don't get the link-time failure).

Right now only certain broken versions of gcc will actually show this
behaviour, I think, but it's at least in theory going to be an issue.

		Linus

Re: iso9660 endianness cleanup patch

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> On Mon, 30 Apr 2001, H. Peter Anvin wrote:
> >
> > The attached patch fixes both.  It is against 2.4.4, but from the looks
> > of it it should patch against -ac as well.
> 
> Btw, please use "static inline" instead of "extern inline", as gcc may
> decide not to inline the latter at all, leading to confusing link-time
> errors. (Gcc may also decide not to inline "static inline", but then gcc
> will output the actual body of the function out-of-line if it gets used,
> so you don't get the link-time failure).
> 
> Right now only certain broken versions of gcc will actually show this
> behaviour, I think, but it's at least in theory going to be an issue.
> 

I guess I personally prefer an error over completely broken behaviour,
but feel free to change it.

	-hpa

-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt

Re: iso9660 endianness cleanup patch

[Pavel Machek]

Hi!

> > > The attached patch fixes both.  It is against 2.4.4, but from the looks
> > > of it it should patch against -ac as well.
> > 
> > Btw, please use "static inline" instead of "extern inline", as gcc may
> > decide not to inline the latter at all, leading to confusing link-time
> > errors. (Gcc may also decide not to inline "static inline", but then gcc
> > will output the actual body of the function out-of-line if it gets used,
> > so you don't get the link-time failure).
> > 
> > Right now only certain broken versions of gcc will actually show this
> > behaviour, I think, but it's at least in theory going to be an issue.
> > 
> 
> I guess I personally prefer an error over completely broken behaviour,
> but feel free to change it.

It  should ot break anything. gcc decides its bad to inline it, so it
does not inline it. Small code growth at worst. Compiler has right to
make your code bigger or slower, if it decides to do so.
-- 
Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

Re: iso9660 endianness cleanup patch

[Albert D. Cahalan]

Pavel Machek writes:

> It  should ot break anything. gcc decides its bad to inline it, so it
> does not inline it. Small code growth at worst. Compiler has right to
> make your code bigger or slower, if it decides to do so.

Oh come on. The logical way:

inline          Compiler must inline (only!) or report an error.
extern inline   This is a contradiction. Report an error.
static inline   This is a contradiction. Report an error.

Anything else is obvious crap. It isn't OK for the compiler
to ever ignore me; inline recursive functions are just wrong.
Taking the address of an inline function is just wrong too.

Of course the above is not what we are given. We get crap.
The old gcc behavior was crap, and I guess the C99 behavior
is too. So the only sane thing is a #define that is set to
whatever makes the compiler behave as nicely as possible.
Then we use _INLINE everywhere, and get decent behavior out
of both old and new compilers.

Re: iso9660 endianness cleanup patch

[Mike Galbraith]

On Thu, 3 May 2001, Albert D. Cahalan wrote:

> Pavel Machek writes:
>
> > It  should ot break anything. gcc decides its bad to inline it, so it
> > does not inline it. Small code growth at worst. Compiler has right to
> > make your code bigger or slower, if it decides to do so.
>
> Oh come on. The logical way:
>
> inline          Compiler must inline (only!) or report an error.

That's doable now.. if the code is otherwise warning free.

	-Mike

Re: iso9660 endianness cleanup patch

[Albert D. Cahalan]

Linus Torvalds writes:

> Btw, please use "static inline" instead of "extern inline", as gcc may
> decide not to inline the latter at all, leading to confusing link-time
> errors. (Gcc may also decide not to inline "static inline", but then gcc
> will output the actual body of the function out-of-line if it gets used,
> so you don't get the link-time failure).
> 
> Right now only certain broken versions of gcc will actually show this
> behaviour, I think, but it's at least in theory going to be an issue.

Since the best choice depends on compiler version:

#if(GCC_VERSION_FOO)
#define __inline extern inline
#else
#define __inline static inline
#endif

(that, or _INLINE if you prefer)

Re: iso9660 endianness cleanup patch

[Tim Riker]

hpa,

I'd remove the __BIG_ENDIAN test cases if I were you. Having written
many a iso9660 decoder I will tell you that the "required" bi-endian
implementation is sometimes broken as many vendors only test CDs with a
certain single endian OS. I've seen these set to 0s, or -1 or just a
copy of the le data byte for byte. I opened many a bug report with the
different CD formatting software vendors, some fixed them, many didn't
care. The extra savings does not justify the hassles IMHO.

The other changes look great!

Tim

"H. Peter Anvin" wrote:
> 
> Hi guys,
> 
> I was looking over the iso9660 code, and noticed that it was doing
> endianness conversion via ad hoc *functions*, not even inlines; nor did
> it take any advantage of the fact that iso9660 is bi-endian (has "all"
> data in both bigendian and littleendian format.)
> 
> The attached patch fixes both.  It is against 2.4.4, but from the looks
> of it it should patch against -ac as well.
> 
>         -hpa
-- 
Tim Riker - http://rikers.org/ - short SIGs! <g>
All I need to know I could have learned in Kindergarten
... if I'd just been paying attention.

Re: iso9660 endianness cleanup patch

[Martin Dalecki]

"H. Peter Anvin" wrote:
> 
> Hi guys,
> 
> I was looking over the iso9660 code, and noticed that it was doing
> endianness conversion via ad hoc *functions*, not even inlines; nor did
> it take any advantage of the fact that iso9660 is bi-endian (has "all"
> data in both bigendian and littleendian format.)
> 
> The attached patch fixes both.  It is against 2.4.4, but from the looks
> of it it should patch against -ac as well.

Please beware: There is a can of worms you are openning up here, 
since there are many broken CD producer programms out there, which
only provide the little endian data and incorrect big endian
entries. I had some CD's of this form myself. So the endian neutrality
of the iso9660 is only in the theory present...

Re: iso9660 endianness cleanup patch

[H. Peter Anvin]

Martin Dalecki wrote:
> 
> "H. Peter Anvin" wrote:
> >
> > Hi guys,
> >
> > I was looking over the iso9660 code, and noticed that it was doing
> > endianness conversion via ad hoc *functions*, not even inlines; nor did
> > it take any advantage of the fact that iso9660 is bi-endian (has "all"
> > data in both bigendian and littleendian format.)
> >
> > The attached patch fixes both.  It is against 2.4.4, but from the looks
> > of it it should patch against -ac as well.
> 
> Please beware: There is a can of worms you are openning up here,
> since there are many broken CD producer programms out there, which
> only provide the little endian data and incorrect big endian
> entries. I had some CD's of this form myself. So the endian neutrality
> of the iso9660 is only in the theory present...
> 

So it has been discussed, and been updated.

	-hpa

-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt

Re: iso9660 endianness cleanup patch

[Pavel Machek]

Hi!

> > I was looking over the iso9660 code, and noticed that it was doing
> > endianness conversion via ad hoc *functions*, not even inlines; nor did
> > it take any advantage of the fact that iso9660 is bi-endian (has "all"
> > data in both bigendian and littleendian format.)
> > 
> > The attached patch fixes both.  It is against 2.4.4, but from the looks
> > of it it should patch against -ac as well.
> 
> Please beware: There is a can of worms you are openning up here, 
> since there are many broken CD producer programms out there, which
> only provide the little endian data and incorrect big endian
> entries. I had some CD's of this form myself. So the endian neutrality
> of the iso9660 is only in the theory present...

Hmm, perhaps there's time to fsck.iso9660?
								Pavel
PS: It might be funny to *deliberately* create different filesystems;
one on little endian side and one on big endian side. That way windows
users would see "macs suck" and mac users "PCs suck", and that with
just one cd ;-).
-- 
I'm pavel@ucw.cz. "In my country we have almost anarchy and I don't care."
Panos Katsaloulis describing me w.r.t. patents at discuss@linmodems.org