On Sat, Nov 30, 2024 at 12:36:07PM -0800, Darrick J. Wong wrote: > > A known issue is that f_clear_orphan_file is failing on s390x, > > powerpc, and ppc64 which is why Debian packages haven't been built on > > those architectures: > > > > https://buildd.debian.org/status/package.php?p=e2fsprogs > > IOWs, the big endian architectures. > > Hmmm, why does ext2fs_do_orphan_file_block_csum claim to return a __u32 > yet the actual return statement returns an __le32: > > return ext2fs_cpu_to_le32(crc); Yeah, I noticed this when I was taking look a bit earlier. The fundamental problem is indeed that ext2fs_orphan_file_block_csum() is returning an on-disk checksum, and on little endian systems, this confusion is harmless, but it's a problem on big endian systems. It doesn't help that in the e2fsprogs's headers, we have: struct ext4_orphan_block_tail { __u32 ob_magic; __u32 ob_checksum; }; ... but in the kernel'sheader files we have: struct ext4_orphan_block_tail { __le32 ob_magic; __le32 ob_checksum; }; ... and although e2fsprogs had been set up to use sparse as a static code checker some number of years ago, no one has used it in a while, and right now compiling with make C=1 generates a huge amount of noise. (For example, sparse is now warning about unused functions which are delcared "static inline" in header files, and short of just squeching all unusedFunction warnings, there doesn't seem to be an obvious way to get it to shut up about inline functions. So to fix things on big endian systems, we'll need to clean up the ambiguities. The simplest way to fix things is just to do this: (sid_ppc64-dchroot)tytso@perotto:~/e2fsprogs/e2fsprogs$ git diff diff --git a/lib/ext2fs/orphan.c b/lib/ext2fs/orphan.c index 913eb9a0..14b83b74 100644 --- a/lib/ext2fs/orphan.c +++ b/lib/ext2fs/orphan.c @@ -271,5 +271,5 @@ int ext2fs_orphan_file_block_csum_verify(ext2_filsys fs, ext2_ino_t ino, if (retval) return 0; tail = ext2fs_orphan_block_tail(fs, buf); - return ext2fs_le32_to_cpu(tail->ob_checksum) == crc; + return tail->ob_checksum == crc; } But it's not clear this is the cleanest way to fix things. I tend to agree with your suggestion as probably the better one, and that we should be making things explicit with the use of __le32 annotations. I should also try to see what needs to be done to make using "make C=1" more useful.... - Ted
