Skip to contentSkip to conversation

e2fsprogs- fix ext2fs_swap_inode_full attr test on BE boxes

PATCH In review Eric Sandeen 3 participants 7 messages Feb 29, 2008 – Mar 14, 2008

lore
Share & tools
lore

Eric Sandeen

author
#0 lore raw

PATCH 1/1 e2fsprogs- fix ext2fs_swap_inode_full attr test on BE boxes

After the fix for resize2fs's inode mover losing in-inode extended attributes, the regression test I wrote caught that the attrs were still getting lost on powerpc.

Looks like the problem is that ext2fs_swap_inode_full() isn't paying attention to whether or not the EA magic is in hostorder, so it's not recognized (and not swapped) on BE machines. Patch below seems to fix it.

Yay for regression tests. ;)

Signed-off-by Eric Sandeen <[email protected]>

---

lib/ext2fs/swapfs.c +1 -4
Index: e2fsprogs-1.40.7/lib/ext2fs/swapfs.c
===================================================================
@@ -133,7 +133,7 @@ void ext2fs_swap_inode_full(ext2_filsys 
 			    struct ext2_inode_large *f, int hostorder,
 			    int bufsize)
 {
-	unsigned i, has_data_blocks, extra_isize;
+	unsigned i, has_data_blocks, extra_isize, attr_magic;
 	int islnk = 0;
 	__u32 *eaf, *eat;
 

@@ -231,13 +231,17 @@ void ext2fs_swap_inode_full(ext2_filsys 
 
 	eaf = (__u32 *) (((char *) f) + sizeof(struct ext2_inode) +
 					extra_isize);
-
-	if (ext2fs_swab32(*eaf) != EXT2_EXT_ATTR_MAGIC)
-		return; /* it seems no magic here */
#1 lore raw
Quoted 40 lines · Eric Sandeen

After the fix for resize2fs's inode mover losing in-inode extended attributes, the regression test I wrote caught that the attrs were still getting lost on powerpc.

Looks like the problem is that ext2fs_swap_inode_full() isn't paying attention to whether or not the EA magic is in hostorder, so it's not recognized (and not swapped) on BE machines. Patch below seems to fix it.

Yay for regression tests. ;)

Signed-off-by Eric Sandeen <[email protected]>

---

Index: e2fsprogs-1.40.7/lib/ext2fs/swapfs.c
===================================================================

lib/ext2fs/swapfs.c +3 -5
@@ -133,7 +133,7 @@ void ext2fs_swap_inode_full(ext2_filsys 
 			    struct ext2_inode_large *f, int hostorder,
 			    int bufsize)
 {
-	unsigned i, has_data_blocks, extra_isize;
+	unsigned i, has_data_blocks, extra_isize, attr_magic;
 	int islnk = 0;
 	__u32 *eaf, *eat;
 

@@ -231,13 +231,17 @@ void ext2fs_swap_inode_full(ext2_filsys 
 
 	eaf = (__u32 *) (((char *) f) + sizeof(struct ext2_inode) +
 					extra_isize);
-
-	if (ext2fs_swab32(*eaf) != EXT2_EXT_ATTR_MAGIC)
-		return; /* it seems no magic here */
-
 	eat = (__u32 *) (((char *) t) + sizeof(struct ext2_inode) +
 					extra_isize);
+
+	if (hostorder)

lib/ext2fs/swapfs.c void ext2fs_swap_inode_full(ext2_filsys · line 247

Quoted 10 lines
lib/ext2fs/swapfs.c +6 -0
+		attr_magic = *eaf;
 	*eat = ext2fs_swab32(*eaf);
+	if (!hostorder)
+		attr_magic = *eat;
+
+	if (attr_magic != EXT2_EXT_ATTR_MAGIC)
+		return; /* it seems no magic here */
 
 	/* convert EA(s) */
 	ext2fs_swap_ext_attr((char *) (eat + 1), (char *) (eaf + 1),

This patch seems to be missing in the current e2fsprogs 1.40.8 release (though announced in the release notes that this issue should be fixed on BE machines).
Check r_inline_xattr still fails on ppc and s390 architectures without this patch. It builds fine with this patch applied.
Could you please check (also the git tag for the 1.40.8 release seems to be missing).

Thanks,
Matthias

#2 lore raw
Quoted 1 line · Matthias Koenig

(also the git tag for the 1.40.8 release seems to be missing).

Oops, forgot to do a "git push --tags" again. Fixed.

Quoted 11 lines
Quoted 8 lines · Eric Sandeen

After the fix for resize2fs's inode mover losing in-inode extended attributes, the regression test I wrote caught that the attrs were still getting lost on powerpc.

Looks like the problem is that ext2fs_swap_inode_full() isn't paying attention to whether or not the EA magic is in hostorder, so it's not recognized (and not swapped) on BE machines. Patch below seems to fix it.

Quoted 5 lines

This patch seems to be missing in the current e2fsprogs 1.40.8 release (though announced in the release notes that this issue should be fixed on BE machines).
Check r_inline_xattr still fails on ppc and s390 architectures without this patch. It builds fine with this patch applied.

This patch is in the repo, which I thought superceded the one which Eric sent. (The authorship is wrong; and that was my fault for not including '--author="Eric Sandeen <[email protected]>"' on the commit command-line. I've already apologized to Eric privately; now I'm apologizing publically. :-)

But looking at it again, I believe I misunderstood, and the two patches are attacking quite separate problems. OK, i'll get this one in as well in the next go-around.

- Ted

commit edfd9b0a9fe7b90f56da981ca26d5233cc3749d6

Author: Theodore Ts'o <[email protected]>
Date:   Sat Mar 8 20:20:40 2008 -0500

resize2fs: Fix resizing filesystems with large inodes

Use ext2fs_get_next_inode_full() in resize2fs and clean up large inode handling; previous attempt was not properly handling all cases, and was incorrectly setting i_extra_isize. This caused some extended attributes to get removed or randomly assigned to other inodes as a result of the resize, which can be unfortunate on systems using SELinux.

The previous commit didn't fix things completely on big-endian systems like PowerPC.

Addresses-Red-Hat-Bugzilla: #434893

Signed-off-by Eric Sandeen <[email protected]>
Signed-off-by "Theodore Ts'o" <[email protected]>
resize/resize2fs.c +19 -24
index dd26089..6dec3eb 100644
@@ -1109,8 +1109,7 @@ static errcode_t inode_scan_and_fix(ext2_resize_t rfs)
 {
 	struct process_block_struct	pb;
 	ext2_ino_t		ino, new_inode;
-	struct ext2_inode 	inode, *buf = NULL;
-	struct ext2_inode_large	*large_inode;
+	struct ext2_inode 	*inode = NULL;
 	ext2_inode_scan 	scan = NULL;
 	errcode_t		retval;
 	int			group;

@@ -1154,12 +1153,12 @@ static errcode_t inode_scan_and_fix(ext2_resize_t rfs)
 	}
 	ext2fs_set_inode_callback(scan, progress_callback, (void *) rfs);
 	pb.rfs = rfs;
-	pb.inode = &inode;
+	pb.inode = inode;
 	pb.error = 0;
 	new_inode = EXT2_FIRST_INODE(rfs->new_fs->super);
 	inode_size = EXT2_INODE_SIZE(rfs->new_fs->super);
-	buf = malloc(inode_size);
-	if (!buf) {
+	inode = malloc(inode_size);
+	if (!inode) {
 		retval = ENOMEM;
 		goto errout;
 	}

@@ -1168,29 +1167,29 @@ static errcode_t inode_scan_and_fix(ext2_resize_t rfs)
 	 * elsewhere in the inode table
 	 */
 	while (1) {
-		retval = ext2fs_get_next_inode(scan, &ino, &inode);
+		retval = ext2fs_get_next_inode_full(scan, &ino, inode, inode_size);
 		if (retval) goto errout;
 		if (!ino)
 			break;
 
-		if (inode.i_links_count == 0 && ino != EXT2_RESIZE_INO)
+		if (inode->i_links_count == 0 && ino != EXT2_RESIZE_INO)
 			continue; /* inode not in use */
 
-		pb.is_dir = LINUX_S_ISDIR(inode.i_mode);
+		pb.is_dir = LINUX_S_ISDIR(inode->i_mode);
 		pb.changed = 0;
 
-		if (inode.i_file_acl && rfs->bmap) {
+		if (inode->i_file_acl && rfs->bmap) {
 			new_block = ext2fs_extent_translate(rfs->bmap, 
-							    inode.i_file_acl);
+							    inode->i_file_acl);
 			if (new_block) {
-				inode.i_file_acl = new_block;
-				retval = ext2fs_write_inode(rfs->old_fs, 
-							    ino, &inode);
+				inode->i_file_acl = new_block;
+				retval = ext2fs_write_inode_full(rfs->old_fs, 
+							    ino, inode, inode_size);
 				if (retval) goto errout;
 			}
 		}
 		
-		if (ext2fs_inode_has_valid_blocks(&inode) &&
+		if (ext2fs_inode_has_valid_blocks(inode) &&
 		    (rfs->bmap || pb.is_dir)) {
 			pb.ino = ino;
 			retval = ext2fs_block_iterate2(rfs->old_fs,

@@ -1221,23 +1220,19 @@ static errcode_t inode_scan_and_fix(ext2_resize_t rfs)
 			}
 		}
 		ext2fs_mark_inode_bitmap(rfs->new_fs->inode_map, new_inode);
-		memcpy(buf, &inode, sizeof(struct ext2_inode));
-		large_inode = (struct ext2_inode_large *)buf;
-		large_inode->i_extra_isize = sizeof(struct ext2_inode_large) -
-			EXT2_GOOD_OLD_INODE_SIZE;
 		if (pb.changed) {
 			/* Get the new version of the inode */
 			retval = ext2fs_read_inode_full(rfs->old_fs, ino,
-						buf, inode_size);
+						inode, inode_size);
 			if (retval) goto errout;
 		}
-		inode.i_ctime = time(0);
+		inode->i_ctime = time(0);
 		retval = ext2fs_write_inode_full(rfs->old_fs, new_inode,
-						buf, inode_size);
+						inode, inode_size);
 		if (retval) goto errout;
 
 		group = (new_inode-1) / EXT2_INODES_PER_GROUP(rfs->new_fs->super);
-		if (LINUX_S_ISDIR(inode.i_mode))
+		if (LINUX_S_ISDIR(inode->i_mode))
 			rfs->new_fs->group_desc[group].bg_used_dirs_count++;
 		
 #ifdef RESIZE2FS_DEBUG

@@ -1263,8 +1258,8 @@ errout:
 		ext2fs_close_inode_scan(scan);
 	if (block_buf)
 		ext2fs_free_mem(&block_buf);
-	if (buf)
-		free(buf);
+	if (inode)
+		free(inode);
 	return retval;
 }
author
#3 lore raw
Quoted 5 lines · Theodore Tso

This patch is in the repo, which I thought superceded the one which Eric sent. (The authorship is wrong; and that was my fault for not including '--author="Eric Sandeen <[email protected]>"' on the commit command-line. I've already apologized to Eric privately; now I'm apologizing publically. :-)

And I've realized that I really should start using git to make it easier on Ted. :)

Quoted 3 lines

But looking at it again, I believe I misunderstood, and the two patches are attacking quite separate problems. OK, i'll get this one in as well in the next go-around.

Yep, there were 2 problems.

One was that inode_scan_and_fix() wasn't moving all of large inodes.

The other was that ext2fs_swap_inode_full was doing bad things for in-inode attrs on big endian boxes, that was the patch in this thread which isn't in the repo yet, but is in fact needed.

I suppose that the iterative nature of my fix for this problem didn't help Ted keep things straight, sorry.

-Eric

#4 lore raw
Quoted 10 lines · Eric Sandeen

Yep, there were 2 problems.

One was that inode_scan_and_fix() wasn't moving all of large inodes.

The other was that ext2fs_swap_inode_full was doing bad things for in-inode attrs on big endian boxes, that was the patch in this thread which isn't in the repo yet, but is in fact needed.

I suppose that the iterative nature of my fix for this problem didn't help Ted keep things straight, sorry.

I actually *prefer* multiple patches, actually, but what works best when there is a series of patches is to resend the whole patch stack using git-format-patch or git-send-email --- or set up your own git repository on the network (or use repo.or.cz) and send me a pull request. But that requires much more use of git. :-)

In any case, I'll pull in the last change, and this time I promise I'll be careful about remembering to use --author. That *is* something I should be doing, but I certainly appreciate people using git's facilities to make my life easier, thanks.

- Ted

#5 lore raw
In reply to Eric Sandeen

Review

Quoted 9 lines · Eric Sandeen
(quoted diff) +8 -0
+
+	if (hostorder)
+		attr_magic = *eaf;
 	*eat = ext2fs_swab32(*eaf);
+	if (!hostorder)
+		attr_magic = *eat;
+
+	if (attr_magic != EXT2_EXT_ATTR_MAGIC)
+		return; /* it seems no magic here */

The problem with this is that if the magic isn't there, we still end up writing 4 bytes into the destination. So I think this is a better patch....

- Ted

Quoted 1 line

From 91a2fbe36ff5578c46f637687a56d7a0d169a807 Mon Sep 17 00:00:00 2001

From: Eric Sandeen <[email protected]>
Date: Fri, 29 Feb 2008 16:10:21 -0600
Subject: [PATCH] e2fsprogs- fix ext2fs_swap_inode_full attr test on BE boxes

After the fix for resize2fs's inode mover losing in-inode extended attributes, the regression test I wrote caught that the attrs were still getting lost on powerpc.

Looks like the problem is that ext2fs_swap_inode_full() isn't paying attention to whether or not the EA magic is in hostorder, so it's not recognized (and not swapped) on BE machines. Patch below seems to fix it.

Yay for regression tests. ;)

Signed-off-by Eric Sandeen <[email protected]>
Signed-off-by Theodore Ts'o <[email protected]>
---
 lib/ext2fs/swapfs.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)
lib/ext2fs/swapfs.c +6 -2
index 6576c59..e07e87c 100644
@@ -133,7 +133,7 @@ void ext2fs_swap_inode_full(ext2_filsys fs, struct ext2_inode_large *t,
 			    struct ext2_inode_large *f, int hostorder,
 			    int bufsize)
 {
-	unsigned i, has_data_blocks, extra_isize;
+	unsigned i, has_data_blocks, extra_isize, attr_magic;
 	int islnk = 0;
 	__u32 *eaf, *eat;
 

@@ -232,7 +232,11 @@ void ext2fs_swap_inode_full(ext2_filsys fs, struct ext2_inode_large *t,
 	eaf = (__u32 *) (((char *) f) + sizeof(struct ext2_inode) +
 					extra_isize);
 
-	if (ext2fs_swab32(*eaf) != EXT2_EXT_ATTR_MAGIC)
+	attr_magic = *eaf;
+	if (!hostorder)
+		attr_magic = ext2fs_swab32(attr_magic);
+
+	if (attr_magic != EXT2_EXT_ATTR_MAGIC)
 		return; /* it seems no magic here */
 
 	eat = (__u32 *) (((char *) t) + sizeof(struct ext2_inode) +
Signature · 1 line
1.5.4.1.144.gdfee-dirty
author
#6 lore raw
Quoted 15 lines · Theodore Tso

Review

Quoted 9 lines · Eric Sandeen
(quoted diff) +8 -0
+
+	if (hostorder)
+		attr_magic = *eaf;
 	*eat = ext2fs_swab32(*eaf);
+	if (!hostorder)
+		attr_magic = *eat;
+
+	if (attr_magic != EXT2_EXT_ATTR_MAGIC)
+		return; /* it seems no magic here */

The problem with this is that if the magic isn't there, we still end up writing 4 bytes into the destination. So I think this is a better patch....

Seems good to me.

Thanks,
-Eric