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