[PATCH 0/2] Make cramfs little endian only

[PATCH 0/2] Make cramfs little endian only

[Andi Drebes]

The following patchset makes cramfs little endian only and updates
the documentation.

The changes were tested on the following types of machines:
An i386 compatible box (little endian)
UltraSparc IIi (big endian)

Signed-off-by: Andi Drebes <andi@programmierforen.de>

[PATCH 1/2] Make cramfs little endian only

[Andi Drebes]

The following patch makes cramfs little endian only. When trying to mount a big endian image,
an error message is produced.

The changes were tested on the following types of machines:
An i386 compatible box (little endian)
UltraSparc IIi (big endian)

Signed-off-by: Andi Drebes <andi@programmierforen.de>
---
 inode.c |  163 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 136 insertions(+), 27 deletions(-)

diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 350680f..3fbf567 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -4,6 +4,10 @@
  * Copyright (C) 1999 Linus Torvalds.
  *
  * This file is released under the GPL.
+ *
+ * Changelog:
+ *	11/07 - Andi Drebes <andi@programmierforen.de>
+ *	Made cramfs little endian only.
  */
 
 /*
@@ -40,6 +44,95 @@ static DEFINE_MUTEX(read_mutex);
 #define CRAMINO(x)	(((x)->offset && (x)->size)?(x)->offset<<2:1)
 #define OFFSET(x)	((x)->i_ino)
 
+#ifdef __BIG_ENDIAN
+/* Converts a cramfs_info from little endian to big endian. */
+static inline void cramfs_convert_info_letobe(struct cramfs_info* info)
+{
+	 info->crc = swab32(info->crc);
+	 info->edition = swab32(info->edition);
+	 info->blocks = swab32(info->blocks);
+	 info->files = swab32(info->files);
+}
+
+/* Converts a cramfs_info from little endian to big endian. */
+static inline void cramfs_convert_inode_letobe(struct cramfs_inode* inode)
+{
+	u8* inode_bytes = (u8*)inode;
+	u8 old_nloffs[4];
+
+	inode->mode = swab16(inode->mode);
+	inode->uid = swab16(inode->uid);
+	inode->size = (inode_bytes[6] << 16) | (inode_bytes[5] << 8) | (inode_bytes[4]);
+
+	/* Save the old values of the namelength and the offset */
+	memcpy(old_nloffs, inode_bytes+8, 4);
+
+	/* Convert the namelength and the offset */
+	inode_bytes[8] = ((old_nloffs[0] & 0x3f) << 2) | ((old_nloffs[3] & 0xc0) >> 6);
+	inode_bytes[9] = ((old_nloffs[3] & 0x3f) << 2) | ((old_nloffs[2] & 0xc0) >> 6);
+	inode_bytes[10] = ((old_nloffs[2] & 0x3f) << 2) | ((old_nloffs[1] & 0xc0) >> 6);
+	inode_bytes[11] = ((old_nloffs[1] & 0x3f) << 2) | ((old_nloffs[0] & 0xc0) >> 6);
+}
+
+/* Converts a cramfs superblock from little endian to big endian. */
+static inline void cramfs_convert_super_letobe(struct cramfs_super* super)
+{
+	super->magic = swab32(super->magic);
+	super->size = swab32(super->size);
+	super->flags = swab32(super->flags);
+	super->future = swab32(super->future);
+	cramfs_convert_info_letobe(&super->fsid);
+	cramfs_convert_inode_letobe(&super->root);
+}
+
+/* Converts a 32 bit integer from little endian to big endian */
+static inline u32 cramfs_convert_u32_letobe(u32 val)
+{
+	return swab32(val);
+}
+
+static inline void cramfs_info_to_host(struct cramfs_info *info)
+{
+	cramfs_convert_info_letobe(info);
+}
+
+static inline void cramfs_inode_to_host(struct cramfs_inode *inode)
+{
+	cramfs_convert_inode_letobe(inode);
+}
+
+static inline void cramfs_super_to_host(struct cramfs_super *super)
+{
+	cramfs_convert_super_letobe(super);
+}
+
+static inline u32 cramfs_u32_to_host(u32 val)
+{
+	return cramfs_convert_u32_letobe(val);
+}
+
+#elif defined(__LITTLE_ENDIAN)
+
+static inline void cramfs_info_to_host(struct cramfs_info *info)
+{
+}
+
+static inline void cramfs_inode_to_host(struct cramfs_inode *inode)
+{
+}
+
+static inline void cramfs_super_to_host(struct cramfs_super *super)
+{
+}
+
+static inline u32 cramfs_u32_to_host(u32 val)
+{
+	return val;
+}
+
+#else
+#error "Neither __BIG_ENDIAN nor __LITTLE_ENDIAN defined."
+#endif
 
 static int cramfs_iget5_test(struct inode *inode, void *opaque)
 {
@@ -253,29 +346,35 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 
 	/* Read the first block and get the superblock from it */
 	memcpy(&super, cramfs_read(sb, 0, sizeof(super)), sizeof(super));
+	cramfs_super_to_host(&super);
 	mutex_unlock(&read_mutex);
 
 	/* Do sanity checks on the superblock */
-	if (super.magic != CRAMFS_MAGIC) {
-		/* check for wrong endianess */
-		if (super.magic == CRAMFS_MAGIC_WEND) {
-			if (!silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			goto out;
-		}
-
+	if (super.magic != CRAMFS_MAGIC && super.magic != CRAMFS_MAGIC_WEND) {
 		/* check at 512 byte offset */
 		mutex_lock(&read_mutex);
 		memcpy(&super, cramfs_read(sb, 512, sizeof(super)), sizeof(super));
+		cramfs_super_to_host(&super);
 		mutex_unlock(&read_mutex);
-		if (super.magic != CRAMFS_MAGIC) {
-			if (super.magic == CRAMFS_MAGIC_WEND && !silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			else if (!silent)
+
+		if (super.magic == CRAMFS_MAGIC_WEND) {
+			goto other_endian;
+		}
+		else if (super.magic != CRAMFS_MAGIC) {
+			if (!silent)
 				printk(KERN_ERR "cramfs: wrong magic\n");
+
 			goto out;
 		}
 	}
+	/* check for wrong endianess */
+	else if (super.magic == CRAMFS_MAGIC_WEND) {
+other_endian:
+		if (!silent)
+			printk(KERN_ERR "cramfs: filesystems in big endian format are not supported any longer.\n");
+
+		goto out;
+	}
 
 	/* get feature flags first */
 	if (super.flags & ~CRAMFS_SUPPORTED_FLAGS) {
@@ -367,7 +466,8 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 
 	copied = 0;
 	while (offset < inode->i_size) {
-		struct cramfs_inode *de;
+		void *inode_data;
+		struct cramfs_inode de;
 		unsigned long nextoffset;
 		char *name;
 		ino_t ino;
@@ -375,20 +475,22 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		int namelen, error;
 
 		mutex_lock(&read_mutex);
-		de = cramfs_read(sb, OFFSET(inode) + offset, sizeof(*de)+CRAMFS_MAXPATHLEN);
-		name = (char *)(de+1);
+		inode_data = cramfs_read(sb, OFFSET(inode) + offset, sizeof(de)+CRAMFS_MAXPATHLEN);
+		memcpy(&de, inode_data, sizeof(de));
+		name = inode_data+sizeof(de);
+		cramfs_inode_to_host(&de);
 
 		/*
 		 * Namelengths on disk are shifted by two
 		 * and the name padded out to 4-byte boundaries
 		 * with zeroes.
 		 */
-		namelen = de->namelen << 2;
+		namelen = de.namelen << 2;
 		memcpy(buf, name, namelen);
-		ino = CRAMINO(de);
-		mode = de->mode;
+		ino = CRAMINO(&de);
+		mode = de.mode;
 		mutex_unlock(&read_mutex);
-		nextoffset = offset + sizeof(*de) + namelen;
+		nextoffset = offset + sizeof(de) + namelen;
 		for (;;) {
 			if (!namelen) {
 				kfree(buf);
@@ -421,19 +523,22 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 	mutex_lock(&read_mutex);
 	sorted = CRAMFS_SB(dir->i_sb)->flags & CRAMFS_FLAG_SORTED_DIRS;
 	while (offset < dir->i_size) {
-		struct cramfs_inode *de;
+		void* inode_data;
+		struct cramfs_inode de;
 		char *name;
 		int namelen, retval;
 
-		de = cramfs_read(dir->i_sb, OFFSET(dir) + offset, sizeof(*de)+CRAMFS_MAXPATHLEN);
-		name = (char *)(de+1);
+		inode_data = cramfs_read(dir->i_sb, OFFSET(dir) + offset, sizeof(de)+CRAMFS_MAXPATHLEN);
+		memcpy(&de, inode_data, sizeof(de));
+		name = (char *)(inode_data+sizeof(de));
+		cramfs_inode_to_host(&de);
 
 		/* Try to take advantage of sorted directories */
 		if (sorted && (dentry->d_name.name[0] < name[0]))
 			break;
 
-		namelen = de->namelen << 2;
-		offset += sizeof(*de) + namelen;
+		namelen = de.namelen << 2;
+		offset += sizeof(de) + namelen;
 
 		/* Quick check that the name is roughly the right length */
 		if (((dentry->d_name.len + 3) & ~3) != namelen)
@@ -454,7 +559,7 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		if (retval > 0)
 			continue;
 		if (!retval) {
-			struct cramfs_inode entry = *de;
+			struct cramfs_inode entry = de;
 			mutex_unlock(&read_mutex);
 			d_add(dentry, get_cramfs_inode(dir->i_sb, &entry));
 			return NULL;
@@ -483,9 +588,13 @@ static int cramfs_readpage(struct file *file, struct page * page)
 
 		start_offset = OFFSET(inode) + maxblock*4;
 		mutex_lock(&read_mutex);
-		if (page->index)
+		if (page->index) {
 			start_offset = *(u32 *) cramfs_read(sb, blkptr_offset-4, 4);
-		compr_len = (*(u32 *) cramfs_read(sb, blkptr_offset, 4) - start_offset);
+			start_offset = cramfs_u32_to_host(start_offset);
+		}
+
+		compr_len = cramfs_u32_to_host(*(u32 *) cramfs_read(sb, blkptr_offset, 4)) - start_offset;
+
 		mutex_unlock(&read_mutex);
 		pgdata = kmap(page);
 		if (compr_len == 0)

Re: [PATCH 1/2] Make cramfs little endian only

[Jörn Engel]

On Tue, 4 December 2007 13:01:26 +0100, Andi Drebes wrote:
>  
> +#ifdef __BIG_ENDIAN
> +/* Converts a cramfs_info from little endian to big endian. */
> +static inline void cramfs_convert_info_letobe(struct cramfs_info* info)
> +{
> +	 info->crc = swab32(info->crc);
> +	 info->edition = swab32(info->edition);
> +	 info->blocks = swab32(info->blocks);
> +	 info->files = swab32(info->files);
> +}

Can you remove the #ifdef and use le32_to_cpu() directly?

Jörn

-- 
Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] Make cramfs little endian only

[Andi Drebes]

> > +#ifdef __BIG_ENDIAN
> > +/* Converts a cramfs_info from little endian to big endian. */
> > +static inline void cramfs_convert_info_letobe(struct cramfs_info* info)
> > +{
> > +	 info->crc = swab32(info->crc);
> > +	 info->edition = swab32(info->edition);
> > +	 info->blocks = swab32(info->blocks);
> > +	 info->files = swab32(info->files);
> > +}
> 
> Can you remove the #ifdef and use le32_to_cpu() directly?
Sure. This saves some definitions (and lines of code)...
Here's the new patch (tested on the same machines mentioned in the first message).
I tried to move as many lines as possible out of the endian dependent section.

Signed-off-by: Andi Drebes <andi@programmierforen.de>
---
 fs/cramfs/inode.c |  135 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 108 insertions(+), 27 deletions(-)

diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 350680f..ab05fd5 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -4,6 +4,10 @@
  * Copyright (C) 1999 Linus Torvalds.
  *
  * This file is released under the GPL.
+ *
+ * Changelog:
+ *	11/07 - Andi Drebes <andi@programmierforen.de>
+ *	Made cramfs little endian only.
  */
 
 /*
@@ -40,6 +44,67 @@ static DEFINE_MUTEX(read_mutex);
 #define CRAMINO(x)	(((x)->offset && (x)->size)?(x)->offset<<2:1)
 #define OFFSET(x)	((x)->i_ino)
 
+/* Converts a cramfs_info from little endian to host endian. */
+static inline void cramfs_info_to_host(struct cramfs_info* info)
+{
+	 info->crc = le32_to_cpu(info->crc);
+	 info->edition = le32_to_cpu(info->edition);
+	 info->blocks = le32_to_cpu(info->blocks);
+	 info->files = le32_to_cpu(info->files);
+}
+
+/* Converts a 32 bit integer from little endian to host endian */
+static inline u32 cramfs_u32_to_host(u32 val)
+{
+	return le32_to_cpu(val);
+}
+
+#ifdef __BIG_ENDIAN
+/* Converts a cramfs_inode from little endian to big endian. */
+static inline void cramfs_convert_inode_letobe(struct cramfs_inode* inode)
+{
+	u8* inode_bytes = (u8*)inode;
+	u8 old_nloffs[4];
+
+	inode->mode = swab16(inode->mode);
+	inode->uid = swab16(inode->uid);
+	inode->size = (inode_bytes[6] << 16) | (inode_bytes[5] << 8) | (inode_bytes[4]);
+
+	/* Save the old values of the namelength and the offset */
+	memcpy(old_nloffs, inode_bytes+8, 4);
+
+	/* Convert the namelength and the offset */
+	inode_bytes[8] = ((old_nloffs[0] & 0x3f) << 2) | ((old_nloffs[3] & 0xc0) >> 6);
+	inode_bytes[9] = ((old_nloffs[3] & 0x3f) << 2) | ((old_nloffs[2] & 0xc0) >> 6);
+	inode_bytes[10] = ((old_nloffs[2] & 0x3f) << 2) | ((old_nloffs[1] & 0xc0) >> 6);
+	inode_bytes[11] = ((old_nloffs[1] & 0x3f) << 2) | ((old_nloffs[0] & 0xc0) >> 6);
+}
+
+static inline void cramfs_inode_to_host(struct cramfs_inode *inode)
+{
+	cramfs_convert_inode_letobe(inode);
+}
+
+#elif defined(__LITTLE_ENDIAN)
+
+static inline void cramfs_inode_to_host(struct cramfs_inode *inode)
+{
+}
+
+#else
+#error "Neither __BIG_ENDIAN nor __LITTLE_ENDIAN defined."
+#endif
+
+/* Converts a cramfs superblock from little endian to host endian. */
+static inline void cramfs_super_to_host(struct cramfs_super* super)
+{
+	super->magic = le32_to_cpu(super->magic);
+	super->size = le32_to_cpu(super->size);
+	super->flags = le32_to_cpu(super->flags);
+	super->future = le32_to_cpu(super->future);
+	cramfs_info_to_host(&super->fsid);
+	cramfs_inode_to_host(&super->root);
+}
 
 static int cramfs_iget5_test(struct inode *inode, void *opaque)
 {
@@ -253,29 +318,35 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 
 	/* Read the first block and get the superblock from it */
 	memcpy(&super, cramfs_read(sb, 0, sizeof(super)), sizeof(super));
+	cramfs_super_to_host(&super);
 	mutex_unlock(&read_mutex);
 
 	/* Do sanity checks on the superblock */
-	if (super.magic != CRAMFS_MAGIC) {
-		/* check for wrong endianess */
-		if (super.magic == CRAMFS_MAGIC_WEND) {
-			if (!silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			goto out;
-		}
-
+	if (super.magic != CRAMFS_MAGIC && super.magic != CRAMFS_MAGIC_WEND) {
 		/* check at 512 byte offset */
 		mutex_lock(&read_mutex);
 		memcpy(&super, cramfs_read(sb, 512, sizeof(super)), sizeof(super));
+		cramfs_super_to_host(&super);
 		mutex_unlock(&read_mutex);
-		if (super.magic != CRAMFS_MAGIC) {
-			if (super.magic == CRAMFS_MAGIC_WEND && !silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			else if (!silent)
+
+		if (super.magic == CRAMFS_MAGIC_WEND) {
+			goto other_endian;
+		}
+		else if (super.magic != CRAMFS_MAGIC) {
+			if (!silent)
 				printk(KERN_ERR "cramfs: wrong magic\n");
+
 			goto out;
 		}
 	}
+	/* check for wrong endianess */
+	else if (super.magic == CRAMFS_MAGIC_WEND) {
+other_endian:
+		if (!silent)
+			printk(KERN_ERR "cramfs: filesystems in big endian format are not supported any longer.\n");
+
+		goto out;
+	}
 
 	/* get feature flags first */
 	if (super.flags & ~CRAMFS_SUPPORTED_FLAGS) {
@@ -367,7 +438,8 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 
 	copied = 0;
 	while (offset < inode->i_size) {
-		struct cramfs_inode *de;
+		void *inode_data;
+		struct cramfs_inode de;
 		unsigned long nextoffset;
 		char *name;
 		ino_t ino;
@@ -375,20 +447,22 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		int namelen, error;
 
 		mutex_lock(&read_mutex);
-		de = cramfs_read(sb, OFFSET(inode) + offset, sizeof(*de)+CRAMFS_MAXPATHLEN);
-		name = (char *)(de+1);
+		inode_data = cramfs_read(sb, OFFSET(inode) + offset, sizeof(de)+CRAMFS_MAXPATHLEN);
+		memcpy(&de, inode_data, sizeof(de));
+		name = inode_data+sizeof(de);
+		cramfs_inode_to_host(&de);
 
 		/*
 		 * Namelengths on disk are shifted by two
 		 * and the name padded out to 4-byte boundaries
 		 * with zeroes.
 		 */
-		namelen = de->namelen << 2;
+		namelen = de.namelen << 2;
 		memcpy(buf, name, namelen);
-		ino = CRAMINO(de);
-		mode = de->mode;
+		ino = CRAMINO(&de);
+		mode = de.mode;
 		mutex_unlock(&read_mutex);
-		nextoffset = offset + sizeof(*de) + namelen;
+		nextoffset = offset + sizeof(de) + namelen;
 		for (;;) {
 			if (!namelen) {
 				kfree(buf);
@@ -421,19 +495,22 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 	mutex_lock(&read_mutex);
 	sorted = CRAMFS_SB(dir->i_sb)->flags & CRAMFS_FLAG_SORTED_DIRS;
 	while (offset < dir->i_size) {
-		struct cramfs_inode *de;
+		void* inode_data;
+		struct cramfs_inode de;
 		char *name;
 		int namelen, retval;
 
-		de = cramfs_read(dir->i_sb, OFFSET(dir) + offset, sizeof(*de)+CRAMFS_MAXPATHLEN);
-		name = (char *)(de+1);
+		inode_data = cramfs_read(dir->i_sb, OFFSET(dir) + offset, sizeof(de)+CRAMFS_MAXPATHLEN);
+		memcpy(&de, inode_data, sizeof(de));
+		name = (char *)(inode_data+sizeof(de));
+		cramfs_inode_to_host(&de);
 
 		/* Try to take advantage of sorted directories */
 		if (sorted && (dentry->d_name.name[0] < name[0]))
 			break;
 
-		namelen = de->namelen << 2;
-		offset += sizeof(*de) + namelen;
+		namelen = de.namelen << 2;
+		offset += sizeof(de) + namelen;
 
 		/* Quick check that the name is roughly the right length */
 		if (((dentry->d_name.len + 3) & ~3) != namelen)
@@ -454,7 +531,7 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		if (retval > 0)
 			continue;
 		if (!retval) {
-			struct cramfs_inode entry = *de;
+			struct cramfs_inode entry = de;
 			mutex_unlock(&read_mutex);
 			d_add(dentry, get_cramfs_inode(dir->i_sb, &entry));
 			return NULL;
@@ -483,9 +560,13 @@ static int cramfs_readpage(struct file *file, struct page * page)
 
 		start_offset = OFFSET(inode) + maxblock*4;
 		mutex_lock(&read_mutex);
-		if (page->index)
+		if (page->index) {
 			start_offset = *(u32 *) cramfs_read(sb, blkptr_offset-4, 4);
-		compr_len = (*(u32 *) cramfs_read(sb, blkptr_offset, 4) - start_offset);
+			start_offset = cramfs_u32_to_host(start_offset);
+		}
+
+		compr_len = cramfs_u32_to_host(*(u32 *) cramfs_read(sb, blkptr_offset, 4)) - start_offset;
+
 		mutex_unlock(&read_mutex);
 		pgdata = kmap(page);
 		if (compr_len == 0)

Re: [PATCH 1/2] Make cramfs little endian only

[Linus Torvalds]

On Tue, 4 Dec 2007, Andi Drebes wrote:
>
> Sure. This saves some definitions (and lines of code)...
> Here's the new patch (tested on the same machines mentioned in the first message).
> I tried to move as many lines as possible out of the endian dependent section.

This really is the totally wrong way to do this.

You should *not* convert inodes to CPU-endian mode at all. You should 
*keep* them in the native mode, and then just use "le32_to_cpu()" when 
actually using them.

Basically, if you ever have a

	#ifdef __BIG_ENDIAN

or similar in the source code, you're simply doing something wrong. 

Btw, sparse can be a big help for things like this, by just marking the 
actual disk data structures as being the right type (ie "__le32" and 
friends), and then you can verify that any users will use "le32_to_cpu()" 
as required, because sparse will warn about bad endianness.

So don't copy things around and change them from one mode to the other. 
Keep all the data structures in native LE ordering, and only when you 
really need to use them for some real data do you need to do the 
conversion.

Now, for example, your code has two different "struct cramfs_inode" that 
you use: the unconverted and the converted one. That's confusing, but it's 
also error-prone. If you just have one type - the unconverted 
little-endian one - you have none of those issues.

			Linus

Re: [PATCH 1/2] Make cramfs little endian only

[Andi Drebes]

> > Sure. This saves some definitions (and lines of code)...
> > Here's the new patch (tested on the same machines mentioned in the first message).
> > I tried to move as many lines as possible out of the endian dependent section.
> 
> This really is the totally wrong way to do this.
> 
> You should *not* convert inodes to CPU-endian mode at all. You should 
> *keep* them in the native mode, and then just use "le32_to_cpu()" when 
> actually using them.
OK. I'll prepare a new patch and send it to the list (not today,
it's already too late in the evening here). 

> Basically, if you ever have a
> 
> 	#ifdef __BIG_ENDIAN
> 
> or similar in the source code, you're simply doing something wrong. 
Perhaps I'm missing somehting, but I think for cramfs, unfortunately,
there has to be this statement. The bitfields in the cramfs_inode structure
cause some problems. You can't simply call le32_to_cpu() on them.
Especially the namelength and offset fields are weird. There has to be at least
one routine for each kind of machine that converts those values (or not -- depending
ont the machine's endianness).

> Btw, sparse can be a big help for things like this, by just marking the 
> actual disk data structures as being the right type (ie "__le32" and 
> friends), and then you can verify that any users will use "le32_to_cpu()" 
> as required, because sparse will warn about bad endianness.
Ok, thanks for your advice. But what about the problems mentioned above?

	Andi

Re: [PATCH 1/2] Make cramfs little endian only

[Linus Torvalds]

On Tue, 4 Dec 2007, Andi Drebes wrote:
>
> Perhaps I'm missing somehting, but I think for cramfs, unfortunately,
> there has to be this statement. The bitfields in the cramfs_inode structure
> cause some problems.

I agree that bitfields can be painful, but they should likely be just 
rewritten to be accesses using actual masks and shifts. The thing is, 
bitfields aren't actually endianness safe *anyway*, in that a compiler may 
end up using a *different* bit order than the byte order.  So you cannot 
really use bitfields reliably on things like that (although Linux has a 
notion of a "__[BIG|LITTLE]_ENDIAN_BITFIELD", if you really want to).

		Linus

Re: [PATCH 1/2] Make cramfs little endian only

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Tue, 4 Dec 2007, Andi Drebes wrote:
>> Perhaps I'm missing somehting, but I think for cramfs, unfortunately,
>> there has to be this statement. The bitfields in the cramfs_inode structure
>> cause some problems.
> 
> I agree that bitfields can be painful, but they should likely be just 
> rewritten to be accesses using actual masks and shifts. The thing is, 
> bitfields aren't actually endianness safe *anyway*, in that a compiler may 
> end up using a *different* bit order than the byte order.  So you cannot 
> really use bitfields reliably on things like that (although Linux has a 
> notion of a "__[BIG|LITTLE]_ENDIAN_BITFIELD", if you really want to).

Bitfields also generate lower-quality assembly than masks&shifts 
(typically more instructions using additional temporaries to accomplish 
the same thing), based on my own informal gcc testing.

You would think gcc would be advanced enough to turn bitfield use into 
masks and shifts under the hood, but for whatever reason that often is 
not the case in kernel code.

Due to the way they're used, bitfields make more difficult the common 
code pattern of setting several flags at once:

	(assuming 'foo', 'bar' and 'baz' are bitfields in a struct)
	pdev->foo = 1;
	pdev->bar = 0;
	pdev->baz = 1;

     versus

	flag_foo = (1 << 0);
	flag_bar = (1 << 1);
	flag_baz = (1 << 2);
	...
	pdev->flags = flag_foo | flag_bar;

</pet peeve>

And getting back on topic, I think "pdev->flags = 
cpu_to_le32(flag1|flag2)" is nicer than dealing with bitfields, when 
your data structures are fixed-endian.

	Jeff

Re: [PATCH 1/2] Make cramfs little endian only

[Andi Drebes]

Jeff Garzik wrote:
> Bitfields also generate lower-quality assembly than masks&shifts 
> (typically more instructions using additional temporaries to accomplish 
> the same thing), based on my own informal gcc testing.
> 
> You would think gcc would be advanced enough to turn bitfield use into 
> masks and shifts under the hood, but for whatever reason that often is 
> not the case in kernel code.
OK. I'll modify the cramfs structures and replace the bitfields so that the
data can be accessed using masks and shifts. This requires updating the userspace
tools aswell. There will be a new patch avaliable soon.

I worked on the copy-and-convert-problem today and created a new patch that
avoids the negative things pointed out by Linus earlier. I hope I got that
in the right way. However, there are still some functions whose implementations
depend on the machine's endianness. I hope that they will be obsolete as soon as
the new patch is available.

The changes were tested on the following types of machines:
An i386 compatible box (little endian)
UltraSparc IIi (big endian)

Signed-off-by: Andi Drebes <andi@programmierforen.de>
---
 fs/cramfs/inode.c |  131 +++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 96 insertions(+), 35 deletions(-)

diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 350680f..ceb2c6e 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -4,6 +4,10 @@
  * Copyright (C) 1999 Linus Torvalds.
  *
  * This file is released under the GPL.
+ *
+ * Changelog:
+ *	11/07 - Andi Drebes <andi@programmierforen.de>
+ *	Made cramfs little endian only.
  */
 
 /*
@@ -37,9 +41,62 @@ static DEFINE_MUTEX(read_mutex);
 
 /* These two macros may change in future, to provide better st_ino
    semantics. */
-#define CRAMINO(x)	(((x)->offset && (x)->size)?(x)->offset<<2:1)
+#define CRAMINO(x)	((cramfs_offset_to_cpu(x) && cramfs_size_to_cpu(x)) ? \
+			 cramfs_offset_to_cpu(x) << 2 : 1)
 #define OFFSET(x)	((x)->i_ino)
 
+#ifdef __BIG_ENDIAN
+
+/* Converts a cramfs_inode's offset field
+   from little endianto cpu endian */
+static u32 cramfs_offset_to_cpu(struct cramfs_inode *inode)
+{
+	u8 *inode_bytes = (u8 *)inode;
+	return ((inode_bytes[8] & 0xc0) >> 6) | (inode_bytes[9] << 2) |
+		(inode_bytes[10] << 10) | (inode_bytes[11] << 18);
+}
+
+/* Converts a cramfs_inode's namelength field
+   from little endian to cpu endian */
+static u32 cramfs_namelen_to_cpu(struct cramfs_inode *inode)
+{
+	u8 *inode_bytes = (u8 *)inode;
+	return inode_bytes[8] & 0x3f;
+}
+
+/* Converts a cramfs_inode's size field
+   from little endian to cpu endian */
+static u32 cramfs_size_to_cpu(struct cramfs_inode *inode)
+{
+	return ((inode->size & 0xff0000) >> 16) | (inode->size & 0x00ff00) |
+		((inode->size & 0x0000ff) << 16);
+}
+
+#elif defined(__LITTLE_ENDIAN)
+
+/* Converts a cramfs_inode's offset field
+   from little endian to cpu endian */
+static u32 cramfs_offset_to_cpu(struct cramfs_inode *inode)
+{
+	return inode->offset;
+}
+
+/* Converts a cramfs_inode's namelength field
+   from little endian to cpu endian */
+static u32 cramfs_namelen_to_cpu(struct cramfs_inode *inode)
+{
+	return inode->namelen;
+}
+
+/* Converts a cramfs_inode's size field
+   from little endian to cpu endian */
+static u32 cramfs_size_to_cpu(struct cramfs_inode *inode)
+{
+	return inode->size;
+}
+#else
+#error "Neither __BIG_ENDIAN nor __LITTLE_ENDIAN defined."
+#endif
 
 static int cramfs_iget5_test(struct inode *inode, void *opaque)
 {
@@ -53,13 +110,13 @@ static int cramfs_iget5_test(struct inode *inode, void *opaque)
 
 	/* all empty directories, char, block, pipe, and sock, share inode #1 */
 
-	if ((inode->i_mode != cramfs_inode->mode) ||
+	if ((inode->i_mode != le16_to_cpu(cramfs_inode->mode)) ||
 	    (inode->i_gid != cramfs_inode->gid) ||
-	    (inode->i_uid != cramfs_inode->uid))
+	    (inode->i_uid != le16_to_cpu(cramfs_inode->uid)))
 		return 0; /* does not match */
 
 	if ((S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) &&
-	    (inode->i_rdev != old_decode_dev(cramfs_inode->size)))
+	    (inode->i_rdev != old_decode_dev(cramfs_size_to_cpu(cramfs_inode))))
 		return 0; /* does not match */
 
 	return 1; /* matches */
@@ -69,10 +126,10 @@ static int cramfs_iget5_set(struct inode *inode, void *opaque)
 {
 	static struct timespec zerotime;
 	struct cramfs_inode *cramfs_inode = opaque;
-	inode->i_mode = cramfs_inode->mode;
-	inode->i_uid = cramfs_inode->uid;
-	inode->i_size = cramfs_inode->size;
-	inode->i_blocks = (cramfs_inode->size - 1) / 512 + 1;
+	inode->i_mode = le16_to_cpu(cramfs_inode->mode);
+	inode->i_uid = le16_to_cpu(cramfs_inode->uid);
+	inode->i_size = cramfs_size_to_cpu(cramfs_inode);
+	inode->i_blocks = (cramfs_size_to_cpu(cramfs_inode) - 1) / 512 + 1;
 	inode->i_gid = cramfs_inode->gid;
 	/* Struct copy intentional */
 	inode->i_mtime = inode->i_atime = inode->i_ctime = zerotime;
@@ -94,7 +151,7 @@ static int cramfs_iget5_set(struct inode *inode, void *opaque)
 		inode->i_size = 0;
 		inode->i_blocks = 0;
 		init_special_inode(inode, inode->i_mode,
-			old_decode_dev(cramfs_inode->size));
+			old_decode_dev(cramfs_size_to_cpu(cramfs_inode)));
 	}
 	return 0;
 }
@@ -256,53 +313,57 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 	mutex_unlock(&read_mutex);
 
 	/* Do sanity checks on the superblock */
-	if (super.magic != CRAMFS_MAGIC) {
-		/* check for wrong endianess */
-		if (super.magic == CRAMFS_MAGIC_WEND) {
-			if (!silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			goto out;
-		}
-
+	if (le32_to_cpu(super.magic) != CRAMFS_MAGIC &&
+	    le32_to_cpu(super.magic) != CRAMFS_MAGIC_WEND) {
 		/* check at 512 byte offset */
 		mutex_lock(&read_mutex);
 		memcpy(&super, cramfs_read(sb, 512, sizeof(super)), sizeof(super));
 		mutex_unlock(&read_mutex);
-		if (super.magic != CRAMFS_MAGIC) {
-			if (super.magic == CRAMFS_MAGIC_WEND && !silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			else if (!silent)
+
+		if (le32_to_cpu(super.magic) == CRAMFS_MAGIC_WEND)
+			goto other_endian;
+		else if (le32_to_cpu(super.magic) != CRAMFS_MAGIC) {
+			if (!silent)
 				printk(KERN_ERR "cramfs: wrong magic\n");
+
 			goto out;
 		}
 	}
+	/* check for wrong endianess */
+	else if (le32_to_cpu(super.magic) == CRAMFS_MAGIC_WEND) {
+other_endian:
+		if (!silent)
+			printk(KERN_ERR "cramfs: filesystems in big endian format are not supported any longer.\n");
+
+		goto out;
+	}
 
 	/* get feature flags first */
-	if (super.flags & ~CRAMFS_SUPPORTED_FLAGS) {
+	if (le32_to_cpu(super.flags) & ~CRAMFS_SUPPORTED_FLAGS) {
 		printk(KERN_ERR "cramfs: unsupported filesystem features\n");
 		goto out;
 	}
 
 	/* Check that the root inode is in a sane state */
-	if (!S_ISDIR(super.root.mode)) {
+	if (!S_ISDIR(le16_to_cpu(super.root.mode))) {
 		printk(KERN_ERR "cramfs: root is not a directory\n");
 		goto out;
 	}
-	root_offset = super.root.offset << 2;
+	root_offset = cramfs_offset_to_cpu(&super.root) << 2;
 	if (super.flags & CRAMFS_FLAG_FSID_VERSION_2) {
-		sbi->size=super.size;
-		sbi->blocks=super.fsid.blocks;
-		sbi->files=super.fsid.files;
+		sbi->size = le32_to_cpu(super.size);
+		sbi->blocks = le32_to_cpu(super.fsid.blocks);
+		sbi->files = le32_to_cpu(super.fsid.files);
 	} else {
 		sbi->size=1<<28;
 		sbi->blocks=0;
 		sbi->files=0;
 	}
-	sbi->magic=super.magic;
-	sbi->flags=super.flags;
+	sbi->magic = le32_to_cpu(super.magic);
+	sbi->flags = le32_to_cpu(super.flags);
 	if (root_offset == 0)
 		printk(KERN_INFO "cramfs: empty filesystem");
-	else if (!(super.flags & CRAMFS_FLAG_SHIFTED_ROOT_OFFSET) &&
+	else if (!(le32_to_cpu(super.flags) & CRAMFS_FLAG_SHIFTED_ROOT_OFFSET) &&
 		 ((root_offset != sizeof(struct cramfs_super)) &&
 		  (root_offset != 512 + sizeof(struct cramfs_super))))
 	{
@@ -383,10 +444,10 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		 * and the name padded out to 4-byte boundaries
 		 * with zeroes.
 		 */
-		namelen = de->namelen << 2;
+		namelen = cramfs_namelen_to_cpu(de) << 2;
 		memcpy(buf, name, namelen);
 		ino = CRAMINO(de);
-		mode = de->mode;
+		mode = le16_to_cpu(de->mode);
 		mutex_unlock(&read_mutex);
 		nextoffset = offset + sizeof(*de) + namelen;
 		for (;;) {
@@ -432,7 +493,7 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		if (sorted && (dentry->d_name.name[0] < name[0]))
 			break;
 
-		namelen = de->namelen << 2;
+		namelen = cramfs_namelen_to_cpu(de) << 2;
 		offset += sizeof(*de) + namelen;
 
 		/* Quick check that the name is roughly the right length */
@@ -484,8 +545,8 @@ static int cramfs_readpage(struct file *file, struct page * page)
 		start_offset = OFFSET(inode) + maxblock*4;
 		mutex_lock(&read_mutex);
 		if (page->index)
-			start_offset = *(u32 *) cramfs_read(sb, blkptr_offset-4, 4);
-		compr_len = (*(u32 *) cramfs_read(sb, blkptr_offset, 4) - start_offset);
+			start_offset = le32_to_cpu(*(u32 *) cramfs_read(sb, blkptr_offset-4, 4));
+		compr_len = le32_to_cpu(*(u32 *) cramfs_read(sb, blkptr_offset, 4)) - start_offset;
 		mutex_unlock(&read_mutex);
 		pgdata = kmap(page);
 		if (compr_len == 0)

Re: [PATCH 1/2] Make cramfs little endian only

[Linus Torvalds]

On Wed, 5 Dec 2007, Andi Drebes wrote:
>
> +#ifdef __BIG_ENDIAN
> +
> +/* Converts a cramfs_inode's offset field
> +   from little endianto cpu endian */
> +static u32 cramfs_offset_to_cpu(struct cramfs_inode *inode)
> +{
> +	u8 *inode_bytes = (u8 *)inode;
> +	return ((inode_bytes[8] & 0xc0) >> 6) | (inode_bytes[9] << 2) |
> +		(inode_bytes[10] << 10) | (inode_bytes[11] << 18);
> +}
...
> +#elif defined(__LITTLE_ENDIAN)
> +
> +/* Converts a cramfs_inode's offset field
> +   from little endian to cpu endian */
> +static u32 cramfs_offset_to_cpu(struct cramfs_inode *inode)
> +{
> +	return inode->offset;
> +}

No, no, what I meant about not having any #ifdef __LITTLE_ENDIAN was to do 
the code do the same thing *regardless* of endianness. In other words, a 
simple:

	struct cramfs_inode {
		__le32 mode_uid;	/* CRAMFS_MODE_WIDTH:CRAMFS_UID_WIDTH */
		__le32 size_gid;	/* CRAMFS_SIZE_WIDTH:CRAMFS_GID_WIDTH */
		__le32 namelen_offset;	/* CRAMFS_NAMELEN_WIDTH:CRAMFS_OFFSET_WIDTH */
	};

	#define CRAMFS_UID_MASK ((1ul << CRAMFS_UID_WIDTH)-1)
	#define CRAMFS_GID_MASK ((1ul << CRAMFS_GID_WIDTH)-1)
	#define CRAMFS_NAMELEN_MASK ((1ul << CRAMFS_NAMELEN_WIDTH)-1)

	static inline u32 cramfs_mode(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->mode_uid) >> CRAMFS_UID_WIDTH;
	}

	static inline u32 cramfs_uid(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->mode_uid) & CRAMFS_UID_MASK;
	}

	static inline u32 cramfs_size(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->size_gid) >> CRAMFS_GID_WIDTH;
	}

	static inline u32 cramfs_gid(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->size_gid) & CRAMFS_GID_MASK;
	}

	static inline u32 cramfs_offset(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->namelen_offset) >> CRAMFS_NAMELEN_WIDTH;
	}

	static inline u32 cramfs_namelen(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->namelen_offset) & CRAMFS_NAMELEN_MASK;
	}

See? No #ifdef's required, no different code-paths, and the code generated 
will be pretty much optimal too.

(No, the above is not tested in any way, shape, or form, and no, I didn't 
double-check that I actually extracted the right bits, but you get the 
idea).

			Linus

Re: [PATCH 1/2] Make cramfs little endian only

[Jörn Engel]

On Wed, 5 December 2007 14:21:02 -0800, Linus Torvalds wrote:
> 
> No, no, what I meant about not having any #ifdef __LITTLE_ENDIAN was to do 
> the code do the same thing *regardless* of endianness. In other words, a 
> simple:
> 
> 	struct cramfs_inode {
> 		__le32 mode_uid;	/* CRAMFS_MODE_WIDTH:CRAMFS_UID_WIDTH */
> 		__le32 size_gid;	/* CRAMFS_SIZE_WIDTH:CRAMFS_GID_WIDTH */
> 		__le32 namelen_offset;	/* CRAMFS_NAMELEN_WIDTH:CRAMFS_OFFSET_WIDTH */
> 	};

Mode and uid are both 16bit.  So maybe we can change the structure:

	struct cramfs_inode {
		__le16 uid;
		__le16 mode;
		__le32 size_gid;	/* CRAMFS_SIZE_WIDTH:CRAMFS_GID_WIDTH */
		__le32 namelen_offset;	/* CRAMFS_NAMELEN_WIDTH:CRAMFS_OFFSET_WIDTH */
	};

That way masks and shifts are only needed for the remaining two shared
variables.

> (No, the above is not tested in any way, shape, or form, and no, I didn't 
> double-check that I actually extracted the right bits, but you get the 
> idea).

Same warning applies to my code as well.

Jörn

-- 
Rules of Optimization:
Rule 1: Don't do it.
Rule 2 (for experts only): Don't do it yet.
-- M.A. Jackson
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] Make cramfs little endian only

[Andi Drebes]

> Mode and uid are both 16bit.  So maybe we can change the structure:
> 
> 	struct cramfs_inode {
> 		__le16 uid;
> 		__le16 mode;
> 		__le32 size_gid;	/* CRAMFS_SIZE_WIDTH:CRAMFS_GID_WIDTH */
> 		__le32 namelen_offset;	/* CRAMFS_NAMELEN_WIDTH:CRAMFS_OFFSET_WIDTH */
> 	};
Yep, looks better...

	Andi

Re: [PATCH 1/2] Make cramfs little endian only

[Andi Drebes]

<snip>
> No, no, what I meant about not having any #ifdef __LITTLE_ENDIAN was to do 
> the code do the same thing *regardless* of endianness. In other words, a 
> simple:
> 
> 	struct cramfs_inode {
> 		__le32 mode_uid;	/* CRAMFS_MODE_WIDTH:CRAMFS_UID_WIDTH */
> 		__le32 size_gid;	/* CRAMFS_SIZE_WIDTH:CRAMFS_GID_WIDTH */
> 		__le32 namelen_offset;	/* CRAMFS_NAMELEN_WIDTH:CRAMFS_OFFSET_WIDTH */
> 	};
> 
> 	#define CRAMFS_UID_MASK ((1ul << CRAMFS_UID_WIDTH)-1)
> 	#define CRAMFS_GID_MASK ((1ul << CRAMFS_GID_WIDTH)-1)
> 	#define CRAMFS_NAMELEN_MASK ((1ul << CRAMFS_NAMELEN_WIDTH)-1)
> 
[...]
> 	static inline u32 cramfs_offset(struct cramfs_inode *inode)
> 	{
> 		return le32_to_cpu(node->namelen_offset) >> CRAMFS_NAMELEN_WIDTH;
> 	}
This requires changing the on-disk-structure (even the current "little endian only" one).
The problem is caused by the way GCC (and perhaps other compilers aswell) arranges
the 6 bits bits for the namelength and the 26 bits for the offset within the 32 bits.
I spent quite some time on figuring out how this is actually done. For little endian
machines, the data arranged in the following way:

|o02.o01.n06.n05.n04.n03.n02.n01|o10.o09.o08.o07.o06.o05.o04.o03|
|o18.o17.o16.o15.o14.o13.o12.o11|o26.o25.o24.o23.o22.o21.o20.o19| 

(where oXX means offset bit XX, nXX means namelength bit XX, | marks the beginning
/ end of a byte and . is a bit separator)

On big endian machines, the data is arranged differently:

|n06.n05.n04.n03.n02.n01.o26.o25|o24.o23.o22.o21.o20.o19.o18.o17|
|o16.o15.o14.o13.o12.o11.o10.o09|o08.o07.o06.o05.o04.o03.o02.o01|

So masking and then shifting *the whole* masked data doesn't solve the problem.

> 	static inline u32 cramfs_namelen(struct cramfs_inode *inode)
> 	{
> 		return le32_to_cpu(node->namelen_offset) & CRAMFS_NAMELEN_MASK;
> 	}
> 
> See? No #ifdef's required, no different code-paths, and the code generated 
> will be pretty much optimal too.
See above.

> ([...]but you get the idea).
Yes. And I agree with you. But making this consistent for all fields of cramfs structures
requires changing the on-disk-structure. This would mean that we don't only break support
for big endian images, but also for current little endian images.
However, I'm not totally against this solution. But one should keep in mind what I pointed
out above.

	Andi

Re: [PATCH 1/2] Make cramfs little endian only

[Jörn Engel]

On Thu, 6 December 2007 17:27:26 +0100, Andi Drebes wrote:
> [...]
> > 	static inline u32 cramfs_offset(struct cramfs_inode *inode)
> > 	{
> > 		return le32_to_cpu(node->namelen_offset) >> CRAMFS_NAMELEN_WIDTH;
> > 	}
> This requires changing the on-disk-structure (even the current "little endian only" one).
> The problem is caused by the way GCC (and perhaps other compilers aswell) arranges
> the 6 bits bits for the namelength and the 26 bits for the offset within the 32 bits.
> I spent quite some time on figuring out how this is actually done. For little endian
> machines, the data arranged in the following way:
> 
> |o02.o01.n06.n05.n04.n03.n02.n01|o10.o09.o08.o07.o06.o05.o04.o03|
> |o18.o17.o16.o15.o14.o13.o12.o11|o26.o25.o24.o23.o22.o21.o20.o19| 

How about shifting and masking _before_ converting to host endianness?

	static inline u32 cramfs_offset(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->namelen_offset >> CRAMFS_NAMELEN_WIDTH);
	}

Jörn

-- 
With a PC, I always felt limited by the software available. On Unix,
I am limited only by my knowledge.
-- Peter J. Schoenster
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] Make cramfs little endian only

[Linus Torvalds]

On Thu, 6 Dec 2007, Jörn Engel wrote:
> 
> How about shifting and masking _before_ converting to host endianness?
> 
> 	static inline u32 cramfs_offset(struct cramfs_inode *inode)
> 	{
> 		return le32_to_cpu(node->namelen_offset >> CRAMFS_NAMELEN_WIDTH);
> 	}

Stop this idiocy. The code I sent out was correct. You're just making it 
worse.

"le32_to_cpu()" is a no-op on little-endian machines. So your change won't 
make any difference there. But the point is, that if we want the disk 
layout to be the *same* for both big-endian and little-endian, we need to 
switch the word as it is loaded from memory, and not do *any* operations 
on it before we've done that equalization.

This is not something unusual. It's bog-standard procedure for a lot of 
filesystems. And sparse will help you (and would have complained about 
your code that tries to shift a little-endian entity before changing it 
into a CPU-endian one).

			Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] Make cramfs little endian only

[Linus Torvalds]

On Thu, 6 Dec 2007, Andi Drebes wrote:
>
> This requires changing the on-disk-structure (even the current "little endian only" one).

That makes no sense.

Your whole patch is about making it little-endian only.

If you do that, then big-endian is screwed. I'm ok with that, and support 
it.

But then you just make sure that the little-endian bits are the same, and 
now you're *done*.

> For little endian machines, the data arranged in the following way:
> 
> |o02.o01.n06.n05.n04.n03.n02.n01|o10.o09.o08.o07.o06.o05.o04.o03|
> |o18.o17.o16.o15.o14.o13.o12.o11|o26.o25.o24.o23.o22.o21.o20.o19| 

That's a singularly confused way or looking at it. The point is, the 
first byte in a little-endian machine is the lowest bits, so the *correct* 
way of looking at it is to see the above as one 32-bit word, and then it 
looks like this:

   bit-in-word:		 31   .. 6  5 ..  0
   bit-in-bitfield	o25 ..  o0 n5 .. n0

and my code works *perfectly*. When you do

	static inline u32 cramfs_offset(struct cramfs_inode *inode)
	{
		return le32_to_cpu(node->namelen_offset) >> CRAMFS_NAMELEN_WIDTH;
	}

you are getting *exactly* the bits "o25..o0" that you want (ie the 
offset).

> So masking and then shifting *the whole* masked data doesn't solve the problem.

Yes it does. Try it.

		Linus

Re: [PATCH 1/2] Make cramfs little endian only

[Andi Drebes]

> But then you just make sure that the little-endian bits are the same, and 
> now you're *done*.
Yes, you're perfectly right. Sorry for not getting it. I hope the next
time I will understand faster.

> > For little endian machines, the data arranged in the following way:
> > 
> > |o02.o01.n06.n05.n04.n03.n02.n01|o10.o09.o08.o07.o06.o05.o04.o03|
> > |o18.o17.o16.o15.o14.o13.o12.o11|o26.o25.o24.o23.o22.o21.o20.o19| 
> 
> That's a singularly confused way or looking at it. 
Yep. I saw everything with big-endian eyes and confused myself.
Shouldn't have happened.

> Yes it does. Try it.
I tried it. It does it. Here's the new patch (hopefully the last one
for this issue)...

Checked with sparse using C=2 and CF="-D__CHECK_ENDIAN__".
Tested on an i386 box.
Tested on an Ultrasparc IIi box.

Signed-off-by: Andi Drebes <andi@programmierforen.de>
---
 fs/cramfs/inode.c         |  119 ++++++++++++++++++++++++++++++---------------
 include/linux/cramfs_fs.h |   22 ++++----
 2 files changed, 91 insertions(+), 50 deletions(-)

diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 350680f..30dc640 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -4,6 +4,10 @@
  * Copyright (C) 1999 Linus Torvalds.
  *
  * This file is released under the GPL.
+ *
+ * Changelog:
+ *	11/07 - Andi Drebes <andi@programmierforen.de>
+ *	Made cramfs little endian only.
  */
 
 /*
@@ -34,13 +38,46 @@ static const struct address_space_operations cramfs_aops;
 
 static DEFINE_MUTEX(read_mutex);
 
+#define CRAMFS_NAMELEN_MASK ((1ul << CRAMFS_NAMELEN_WIDTH)-1)
+#define CRAMFS_MODE_MASK ((1ul << CRAMFS_MODE_WIDTH)-1)
+#define CRAMFS_SIZE_MASK ((1ul << CRAMFS_SIZE_WIDTH)-1)
+
+static inline u32 cramfs_mode(struct cramfs_inode *inode)
+{
+	return le32_to_cpu(inode->mode_uid) & CRAMFS_MODE_MASK;
+}
+
+static inline u32 cramfs_uid(struct cramfs_inode *inode)
+{
+	return le32_to_cpu(inode->mode_uid) >> CRAMFS_MODE_WIDTH;
+}
+
+static inline u32 cramfs_size(struct cramfs_inode *inode)
+{
+	return le32_to_cpu(inode->size_gid) & CRAMFS_SIZE_MASK;
+}
+
+static inline u32 cramfs_gid(struct cramfs_inode *inode)
+{
+	return le32_to_cpu(inode->size_gid) >> CRAMFS_SIZE_WIDTH;
+}
+
+static inline u32 cramfs_offset(struct cramfs_inode *inode)
+{
+	return le32_to_cpu(inode->namelen_offset) >> CRAMFS_NAMELEN_WIDTH;
+}
+
+static inline u32 cramfs_namelen(struct cramfs_inode *inode)
+{
+	return le32_to_cpu(inode->namelen_offset) & CRAMFS_NAMELEN_MASK;
+}
 
 /* These two macros may change in future, to provide better st_ino
    semantics. */
-#define CRAMINO(x)	(((x)->offset && (x)->size)?(x)->offset<<2:1)
+#define CRAMINO(x)	((cramfs_offset(x) && cramfs_size(x)) ? \
+			 cramfs_offset(x) << 2 : 1)
 #define OFFSET(x)	((x)->i_ino)
 
-
 static int cramfs_iget5_test(struct inode *inode, void *opaque)
 {
 	struct cramfs_inode *cramfs_inode = opaque;
@@ -53,13 +90,13 @@ static int cramfs_iget5_test(struct inode *inode, void *opaque)
 
 	/* all empty directories, char, block, pipe, and sock, share inode #1 */
 
-	if ((inode->i_mode != cramfs_inode->mode) ||
-	    (inode->i_gid != cramfs_inode->gid) ||
-	    (inode->i_uid != cramfs_inode->uid))
+	if ((inode->i_mode != cramfs_mode(cramfs_inode)) ||
+	    (inode->i_gid != cramfs_gid(cramfs_inode)) ||
+	    (inode->i_uid != cramfs_uid(cramfs_inode)))
 		return 0; /* does not match */
 
 	if ((S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) &&
-	    (inode->i_rdev != old_decode_dev(cramfs_inode->size)))
+	    (inode->i_rdev != old_decode_dev(cramfs_size(cramfs_inode))))
 		return 0; /* does not match */
 
 	return 1; /* matches */
@@ -69,11 +106,11 @@ static int cramfs_iget5_set(struct inode *inode, void *opaque)
 {
 	static struct timespec zerotime;
 	struct cramfs_inode *cramfs_inode = opaque;
-	inode->i_mode = cramfs_inode->mode;
-	inode->i_uid = cramfs_inode->uid;
-	inode->i_size = cramfs_inode->size;
-	inode->i_blocks = (cramfs_inode->size - 1) / 512 + 1;
-	inode->i_gid = cramfs_inode->gid;
+	inode->i_mode = cramfs_mode(cramfs_inode);
+	inode->i_uid = cramfs_uid(cramfs_inode);
+	inode->i_size = cramfs_size(cramfs_inode);
+	inode->i_blocks = (cramfs_size(cramfs_inode) - 1) / 512 + 1;
+	inode->i_gid = cramfs_gid(cramfs_inode);
 	/* Struct copy intentional */
 	inode->i_mtime = inode->i_atime = inode->i_ctime = zerotime;
 	inode->i_ino = CRAMINO(cramfs_inode);
@@ -94,7 +131,7 @@ static int cramfs_iget5_set(struct inode *inode, void *opaque)
 		inode->i_size = 0;
 		inode->i_blocks = 0;
 		init_special_inode(inode, inode->i_mode,
-			old_decode_dev(cramfs_inode->size));
+			old_decode_dev(cramfs_size(cramfs_inode)));
 	}
 	return 0;
 }
@@ -256,53 +293,57 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 	mutex_unlock(&read_mutex);
 
 	/* Do sanity checks on the superblock */
-	if (super.magic != CRAMFS_MAGIC) {
-		/* check for wrong endianess */
-		if (super.magic == CRAMFS_MAGIC_WEND) {
-			if (!silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			goto out;
-		}
-
+	if (le32_to_cpu(super.magic) != CRAMFS_MAGIC &&
+	    le32_to_cpu(super.magic) != CRAMFS_MAGIC_WEND) {
 		/* check at 512 byte offset */
 		mutex_lock(&read_mutex);
 		memcpy(&super, cramfs_read(sb, 512, sizeof(super)), sizeof(super));
 		mutex_unlock(&read_mutex);
-		if (super.magic != CRAMFS_MAGIC) {
-			if (super.magic == CRAMFS_MAGIC_WEND && !silent)
-				printk(KERN_ERR "cramfs: wrong endianess\n");
-			else if (!silent)
+
+		if (le32_to_cpu(super.magic) == CRAMFS_MAGIC_WEND)
+			goto other_endian;
+		else if (le32_to_cpu(super.magic) != CRAMFS_MAGIC) {
+			if (!silent)
 				printk(KERN_ERR "cramfs: wrong magic\n");
+
 			goto out;
 		}
 	}
+	/* check for wrong endianess */
+	else if (le32_to_cpu(super.magic) == CRAMFS_MAGIC_WEND) {
+other_endian:
+		if (!silent)
+			printk(KERN_ERR "cramfs: filesystems in big endian format are not supported any longer.\n");
+
+		goto out;
+	}
 
 	/* get feature flags first */
-	if (super.flags & ~CRAMFS_SUPPORTED_FLAGS) {
+	if (le32_to_cpu(super.flags) & ~CRAMFS_SUPPORTED_FLAGS) {
 		printk(KERN_ERR "cramfs: unsupported filesystem features\n");
 		goto out;
 	}
 
 	/* Check that the root inode is in a sane state */
-	if (!S_ISDIR(super.root.mode)) {
+	if (!S_ISDIR(cramfs_mode(&super.root))) {
 		printk(KERN_ERR "cramfs: root is not a directory\n");
 		goto out;
 	}
-	root_offset = super.root.offset << 2;
-	if (super.flags & CRAMFS_FLAG_FSID_VERSION_2) {
-		sbi->size=super.size;
-		sbi->blocks=super.fsid.blocks;
-		sbi->files=super.fsid.files;
+	root_offset = cramfs_offset(&super.root) << 2;
+	if (__le32_to_cpu(super.flags) & CRAMFS_FLAG_FSID_VERSION_2) {
+		sbi->size = le32_to_cpu(super.size);
+		sbi->blocks = le32_to_cpu(super.fsid.blocks);
+		sbi->files = le32_to_cpu(super.fsid.files);
 	} else {
 		sbi->size=1<<28;
 		sbi->blocks=0;
 		sbi->files=0;
 	}
-	sbi->magic=super.magic;
-	sbi->flags=super.flags;
+	sbi->magic = le32_to_cpu(super.magic);
+	sbi->flags = le32_to_cpu(super.flags);
 	if (root_offset == 0)
 		printk(KERN_INFO "cramfs: empty filesystem");
-	else if (!(super.flags & CRAMFS_FLAG_SHIFTED_ROOT_OFFSET) &&
+	else if (!(le32_to_cpu(super.flags) & CRAMFS_FLAG_SHIFTED_ROOT_OFFSET) &&
 		 ((root_offset != sizeof(struct cramfs_super)) &&
 		  (root_offset != 512 + sizeof(struct cramfs_super))))
 	{
@@ -383,10 +424,10 @@ static int cramfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		 * and the name padded out to 4-byte boundaries
 		 * with zeroes.
 		 */
-		namelen = de->namelen << 2;
+		namelen = cramfs_namelen(de) << 2;
 		memcpy(buf, name, namelen);
 		ino = CRAMINO(de);
-		mode = de->mode;
+		mode = cramfs_mode(de);
 		mutex_unlock(&read_mutex);
 		nextoffset = offset + sizeof(*de) + namelen;
 		for (;;) {
@@ -432,7 +473,7 @@ static struct dentry * cramfs_lookup(struct inode *dir, struct dentry *dentry, s
 		if (sorted && (dentry->d_name.name[0] < name[0]))
 			break;
 
-		namelen = de->namelen << 2;
+		namelen = cramfs_namelen(de) << 2;
 		offset += sizeof(*de) + namelen;
 
 		/* Quick check that the name is roughly the right length */
@@ -484,8 +525,8 @@ static int cramfs_readpage(struct file *file, struct page * page)
 		start_offset = OFFSET(inode) + maxblock*4;
 		mutex_lock(&read_mutex);
 		if (page->index)
-			start_offset = *(u32 *) cramfs_read(sb, blkptr_offset-4, 4);
-		compr_len = (*(u32 *) cramfs_read(sb, blkptr_offset, 4) - start_offset);
+			start_offset = le32_to_cpu(*(__le32 *) cramfs_read(sb, blkptr_offset-4, 4));
+		compr_len = le32_to_cpu(*(__le32 *) cramfs_read(sb, blkptr_offset, 4)) - start_offset;
 		mutex_unlock(&read_mutex);
 		pgdata = kmap(page);
 		if (compr_len == 0)
diff --git a/include/linux/cramfs_fs.h b/include/linux/cramfs_fs.h
index 3be4e5a..66aba06 100644
--- a/include/linux/cramfs_fs.h
+++ b/include/linux/cramfs_fs.h
@@ -28,9 +28,9 @@
  * Reasonably terse representation of the inode data.
  */
 struct cramfs_inode {
-	__u32 mode:CRAMFS_MODE_WIDTH, uid:CRAMFS_UID_WIDTH;
+	__le32 mode_uid;
 	/* SIZE for device files is i_rdev */
-	__u32 size:CRAMFS_SIZE_WIDTH, gid:CRAMFS_GID_WIDTH;
+	__le32 size_gid;
 	/* NAMELEN is the length of the file name, divided by 4 and
            rounded up.  (cramfs doesn't support hard links.) */
 	/* OFFSET: For symlinks and non-empty regular files, this
@@ -39,24 +39,24 @@ struct cramfs_inode {
 	   see README).  For non-empty directories it is the offset
 	   (divided by 4) of the inode of the first file in that
 	   directory.  For anything else, offset is zero. */
-	__u32 namelen:CRAMFS_NAMELEN_WIDTH, offset:CRAMFS_OFFSET_WIDTH;
+	__le32 namelen_offset;
 };
 
 struct cramfs_info {
-	__u32 crc;
-	__u32 edition;
-	__u32 blocks;
-	__u32 files;
+	__le32 crc;
+	__le32 edition;
+	__le32 blocks;
+	__le32 files;
 };
 
 /*
  * Superblock information at the beginning of the FS.
  */
 struct cramfs_super {
-	__u32 magic;			/* 0x28cd3d45 - random number */
-	__u32 size;			/* length in bytes */
-	__u32 flags;			/* feature flags */
-	__u32 future;			/* reserved for future use */
+	__le32 magic;			/* 0x28cd3d45 - random number */
+	__le32 size;			/* length in bytes */
+	__le32 flags;			/* feature flags */
+	__le32 future;			/* reserved for future use */
 	__u8 signature[16];		/* "Compressed ROMFS" */
 	struct cramfs_info fsid;	/* unique filesystem info */
 	__u8 name[16];			/* user-defined name */

Re: [PATCH 1/2] Make cramfs little endian only

[Andrew Morton]

On Tue, 4 Dec 2007 13:01:26 +0100
Andi Drebes <lists-receive@programmierforen.de> wrote:

> The following patch makes cramfs little endian only. When trying to mount a big endian image,
> an error message is produced.
> 
> The changes were tested on the following types of machines:
> An i386 compatible box (little endian)
> UltraSparc IIi (big endian)
> 
> Signed-off-by: Andi Drebes <andi@programmierforen.de>
> ---
>  inode.c |  163 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 136 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 350680f..3fbf567 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c
> @@ -4,6 +4,10 @@
>   * Copyright (C) 1999 Linus Torvalds.
>   *
>   * This file is released under the GPL.
> + *
> + * Changelog:
> + *	11/07 - Andi Drebes <andi@programmierforen.de>
> + *	Made cramfs little endian only.
>   */
>  
>  /*
> @@ -40,6 +44,95 @@ static DEFINE_MUTEX(read_mutex);
>  #define CRAMINO(x)	(((x)->offset && (x)->size)?(x)->offset<<2:1)
>  #define OFFSET(x)	((x)->i_ino)
>  
> +#ifdef __BIG_ENDIAN
> +/* Converts a cramfs_info from little endian to big endian. */
> +static inline void cramfs_convert_info_letobe(struct cramfs_info* info)
> +{
> +	 info->crc = swab32(info->crc);
> +	 info->edition = swab32(info->edition);
> +	 info->blocks = swab32(info->blocks);
> +	 info->files = swab32(info->files);
> +}
> +
> +/* Converts a cramfs_info from little endian to big endian. */
> +static inline void cramfs_convert_inode_letobe(struct cramfs_inode* inode)
> +{
> +	u8* inode_bytes = (u8*)inode;
> +	u8 old_nloffs[4];
> +
> +	inode->mode = swab16(inode->mode);
> +	inode->uid = swab16(inode->uid);
> +	inode->size = (inode_bytes[6] << 16) | (inode_bytes[5] << 8) | (inode_bytes[4]);

eww.  Is there a nicer way of doing that?

Might be a bit tricky given the weird way in which struct cramfs_inode was
defined.

> +
> +	/* Save the old values of the namelength and the offset */
> +	memcpy(old_nloffs, inode_bytes+8, 4);
> +
> +	/* Convert the namelength and the offset */
> +	inode_bytes[8] = ((old_nloffs[0] & 0x3f) << 2) | ((old_nloffs[3] & 0xc0) >> 6);
> +	inode_bytes[9] = ((old_nloffs[3] & 0x3f) << 2) | ((old_nloffs[2] & 0xc0) >> 6);
> +	inode_bytes[10] = ((old_nloffs[2] & 0x3f) << 2) | ((old_nloffs[1] & 0xc0) >> 6);
> +	inode_bytes[11] = ((old_nloffs[1] & 0x3f) << 2) | ((old_nloffs[0] & 0xc0) >> 6);
> +}
> +
> +/* Converts a cramfs superblock from little endian to big endian. */
> +static inline void cramfs_convert_super_letobe(struct cramfs_super* super)
> +{
> +	super->magic = swab32(super->magic);
> +	super->size = swab32(super->size);
> +	super->flags = swab32(super->flags);
> +	super->future = swab32(super->future);
> +	cramfs_convert_info_letobe(&super->fsid);
> +	cramfs_convert_inode_letobe(&super->root);
> +}

These inlines are not sane.  Just removing those three takes the sparc64
fs/cramfs/inode.o from 6856 bytes of text down to 5668, which is rather a
large difference.

The patch has a number of trivial coding-style errors. 
scripts/checkpatch.pl finds them.

Re: [PATCH 1/2] Make cramfs little endian only

[Andi Drebes]

> > +/* Converts a cramfs_info from little endian to big endian. */
> > +static inline void cramfs_convert_inode_letobe(struct cramfs_inode* inode)
> > +{
> > +	u8* inode_bytes = (u8*)inode;
> > +	u8 old_nloffs[4];
> > +
> > +	inode->mode = swab16(inode->mode);
> > +	inode->uid = swab16(inode->uid);
> > +	inode->size = (inode_bytes[6] << 16) | (inode_bytes[5] << 8) | (inode_bytes[4]);
> 
> eww.  Is there a nicer way of doing that?
Yes, I know it's ugly. I searched for a swab24 function. The file
fs/jfs/endian24.h defines such a function. Including this file in
fs/cramfs/inode.c would be even more ugly than the code above. What
about moving this to a header file in include/linux/byteorder? (just a suggestion,
perhaps it isn't worth the effort...)

> Might be a bit tricky given the weird way in which struct cramfs_inode was
> defined.
> 
> > +
> > +	/* Save the old values of the namelength and the offset */
> > +	memcpy(old_nloffs, inode_bytes+8, 4);
> > +
> > +	/* Convert the namelength and the offset */
> > +	inode_bytes[8] = ((old_nloffs[0] & 0x3f) << 2) | ((old_nloffs[3] & 0xc0) >> 6);
> > +	inode_bytes[9] = ((old_nloffs[3] & 0x3f) << 2) | ((old_nloffs[2] & 0xc0) >> 6);
> > +	inode_bytes[10] = ((old_nloffs[2] & 0x3f) << 2) | ((old_nloffs[1] & 0xc0) >> 6);
> > +	inode_bytes[11] = ((old_nloffs[1] & 0x3f) << 2) | ((old_nloffs[0] & 0xc0) >> 6);
> > +}
> > +
> > +/* Converts a cramfs superblock from little endian to big endian. */
> > +static inline void cramfs_convert_super_letobe(struct cramfs_super* super)
> > +{
> > +	super->magic = swab32(super->magic);
> > +	super->size = swab32(super->size);
> > +	super->flags = swab32(super->flags);
> > +	super->future = swab32(super->future);
> > +	cramfs_convert_info_letobe(&super->fsid);
> > +	cramfs_convert_inode_letobe(&super->root);
> > +}
> 
> These inlines are not sane.  Just removing those three takes the sparc64
> fs/cramfs/inode.o from 6856 bytes of text down to 5668, which is rather a
> large difference.
OK. I'll remove the inline keywords in my copies aswell. Should I prepare a new patch
and send it to the mailinglist?

> The patch has a number of trivial coding-style errors. 
> scripts/checkpatch.pl finds them.
Oh, I'm sorry. I should have checked that.

	Andi

[PATCH 2/2] Update documentation

[Andi Drebes]

The following patch updates the cramfs documentation according to the changes in PATCH 1/2.

The changes were tested on the following types of machines:
An i386 compatible box (little endian)
UltraSparc IIi (big endian)

Signed-off-by: Andi Drebes <andi@programmierforen.de>
---
 README |   19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/fs/cramfs/README b/fs/cramfs/README
index 445d1c2..91efe77 100644
--- a/fs/cramfs/README
+++ b/fs/cramfs/README
@@ -6,8 +6,11 @@ a bit looser, e.g. it doesn't care if the <file_data> items are
 swapped around (though it does care that directory entries (inodes) in
 a given directory are contiguous, as this is used by readdir).
 
-All data is currently in host-endian format; neither mkcramfs nor the
-kernel ever do swabbing.  (See section `Block Size' below.)
+All data is now in little endian format. Before, it was in host endian
+format. In order to make filesystem images more shareable between machines
+with a different byte order, cramfs' specification ("this README file")
+was updated. There will be no support for big endian filesystems in the
+future.
 
 <filesystem>:
 	<superblock>
@@ -108,18 +111,6 @@ kernels, not even necessarily kernels of the same architecture if
 PAGE_CACHE_SIZE is subject to change between kernel versions
 (currently possible with arm and ia64).
 
-The remaining options try to make cramfs more sharable.
-
-One part of that is addressing endianness.  The two options here are
-`always use little-endian' (like ext2fs) or `writer chooses
-endianness; kernel adapts at runtime'.  Little-endian wins because of
-code simplicity and little CPU overhead even on big-endian machines.
-
-The cost of swabbing is changing the code to use the le32_to_cpu
-etc. macros as used by ext2fs.  We don't need to swab the compressed
-data, only the superblock, inodes and block pointers.
-
-
 The other part of making cramfs more sharable is choosing a block
 size.  The options are: