Fix ext4 offset

[StoneMonarch]

Fixes an offset issue that is possibly related to #310. Was failing to read from a ext4 partition.

Read works on:

• 2025-10-01-raspios-bookworm-arm64-lite.img
• 2025-10-01-raspios-trixie-arm64-lite.img
• ubuntu-24.04.3-preinstalled-server-arm64+raspi.img
• ubuntu-25.04-preinstalled-server-arm64+raspi.img

Used update repro for Raspberry Pi Images.
Output:

go run ./tools/ext4-rpi.go ./tools/2025-10-01-raspios-trixie-arm64-lite.img
Partition 1: success, fs.Type=0
Partition 1: ReadDir(/) entries: 46
  - overlays (dir, 0 bytes)
  - bcm2710-rpi-2-b.dtb (file, 32503 bytes)
  - LICENCE.broadcom (file, 1594 bytes)
  - issue.txt (file, 145 bytes)
  - bcm2710-rpi-3-b-plus.dtb (file, 35330 bytes)
  - bcm2710-rpi-3-b.dtb (file, 34695 bytes)
  - bcm2710-rpi-cm0.dtb (file, 33684 bytes)
  - bcm2710-rpi-cm3.dtb (file, 32266 bytes)
  - bcm2710-rpi-zero-2-w.dtb (file, 33672 bytes)
  - bcm2710-rpi-zero-2.dtb (file, 33672 bytes)
Partition 2: success, fs.Type=3
Partition 2: ReadDir(/) entries: 21
  - . (dir, 4096 bytes)
  - .. (dir, 4096 bytes)
  - lost+found (dir, 16384 bytes)
  - boot (dir, 4096 bytes)
  - bin (file, 7 bytes)
  - lib (file, 7 bytes)
  - sbin (file, 8 bytes)
  - dev (dir, 4096 bytes)
  - etc (dir, 4096 bytes)
  - home (dir, 4096 bytes)
Partition 3: error: unknown filesystem on partition 3
Partition 4: error: unknown filesystem on partition 4

Used filesystem/ext4/testdata/buildimg.sh to build test image and used new ext4-test.go on that image.
Output:

go run ./tools/ext4-test.go ./tools/dist/ext4.img
Opening ext4 image: ./tools/dist/ext4.img
Filesystem type: 3
Filesystem label: 

=== Test 1: Reading root directory ===
Root directory has 15 entries:
  - . (dir, 1024 bytes)
  - .. (dir, 1024 bytes)
  - lost+found (dir, 12288 bytes)
  - foo (dir, 245760 bytes)
  - shortfile.txt (file, 21 bytes)
  - two-k-file.dat (file, 2048 bytes)
  - six-k-file.dat (file, 6144 bytes)
  - seven-k-file.dat (file, 7168 bytes)
  - ten-meg-file.dat (file, 10485760 bytes)
  - random.dat (file, 20480 bytes)
  - symlink.dat (file, 10 bytes)
  - absolutesymlink (file, 11 bytes)
  - deadlink (file, 11 bytes)
  - deadlonglink (file, 70 bytes)
  - hardlink.dat (file, 20480 bytes)

=== Test 2: Reading shortfile.txt ===
Read 21 bytes: "This is a short file\n"

=== Test 3: Reading /foo directory ===
/foo directory has 10005 entries
First 20 entries:
  - . (dir)
  - .. (dir)
  - dir105 (dir)
  - dir356 (dir)
  - dir469 (dir)
  - dir888 (dir)
  - dir986 (dir)
  - dir1065 (dir)
  - dir1147 (dir)
  - dir1202 (dir)
  - dir1668 (dir)
  - dir1682 (dir)
  - dir2413 (dir)
  - dir2851 (dir)
  - dir3249 (dir)
  - dir3274 (dir)
  - dir3394 (dir)
  - dir3507 (dir)
  - dir3563 (dir)
  - dir3666 (dir)

=== Test 4: Reading /foo/subdirfile.txt ===
Read 22 bytes: "This is a subdir file\n"

=== Test 5: Reading first 32 bytes of random.dat ===
Read 32 bytes: 518312c5dc479e7eada17c3c8ada1740d7144a8d495923e55ceff355842c09fa

=== Test 6: Reading /foo/bar directory ===
/foo/bar directory has 2 entries
  - .
  - ..

=== All tests completed successfully! ===

[deitch]

I was wondering why our tests didn't get this, and I realize that our tests all use an image where the filesystem starts at byte 0. So nice catch.

I have a few comments on this.

Most importantly, can we add a test where we have an image that is offset by even something small, say 1MB? Or even update the current tests to use an image where the ext4 filesystem is offset by 1MB? It does not need to be partitioned properly (shouldn't, as we want to exercise just ext4 in the tests), as long as the tests tell it what byte offset to start it.

Second, I count 7 places in ext4 where we do fs.backend.ReadAt() and 8 places where we make it writable via writable, err := fs.backend.Writable() and then each of those then calls WriteAt(). The writable ones have 3 times where it includes fs.start and 6 where it does not. So we have 3 more ReadAt() that need to be updated, and 6 WriteAt() that need to be updated.

All of which makes me question if this is the right approach. Maybe have fs.backup be just the original storage file as passed is wrong. Maybe we should wrap it with something that includes the offset.

The signature for creating the ext4.FileSystem looks like this:

func Read(b backend.Storage, size, start, sectorsize int64) (*FileSystem, error) {
...
	n, err := b.ReadAt(bs, start)
...
	n, err = b.ReadAt(superblockBytes, start+int64(BootSectorSize))
...
	n, err = b.ReadAt(gdtBytes, start+int64(gdtBlock)*int64(sb.blockSize))
...
	return &FileSystem{
		bootSector:       bs,
		superblock:       sb,
		groupDescriptors: gdt,
		blockGroups:      int64(sb.blockGroupCount()),
		size:             size,
		start:            start,
		backend:          b,
	}, nil

Maybe we should do something like this:

func Read(b backend.Storage, size, start, sectorsize int64) (*FileSystem, error) {
        // creates a new backend that wraps the original, but only includes
        // from start to start+size, and so you can do all ReadAt() and WriteTo without considering original offset
        fsBackend := backend.Sub(b, start, size) 
...
	n, err := fsBackend.ReadAt(bs, 0)
...
	n, err = fsBackend.ReadAt(superblockBytes, int64(BootSectorSize))
...
	n, err = fsBackend.ReadAt(gdtBytes, int64(gdtBlock)*int64(sb.blockSize))
...
	return &FileSystem{
		bootSector:       bs,
		superblock:       sb,
		groupDescriptors: gdt,
		blockGroups:      int64(sb.blockGroupCount()),
		size:             size,
		start:            start,
		backend:          fsBackend,
	}, nil

Then we not only do not need the changes here, but we can remove any other place we added it (the 3 correct WriteTo() out of the 9 total, maybe some other places).

Thoughts?

[StoneMonarch]

It took a while for me to understand why your suggestion was easier, but it did seem to make sense in the end. I think I implemented what you were talking about. I modified the generation of the test img build script to create a second img with an offset, and also added the tests.

I'm unsure if you want to keep the tests separate, one for offset and one for not, or if it would be safe to combine into a single test. I feel like it would be fine to use just the offset test, as if it works at 1024 bytes, I think it should work just fine at 0 as well as any other number.

Thanks for the direction and suggestions, let me know if this is what you had in mind.

[deitch]

I like this!

[deitch]

I feel like it would be fine to use just the offset test, as if it works at 1024 bytes, I think it should work just fine at 0 as well as any other number.

I instinctively feel the same way, but having had enough experience where things that should collapse down to zero and behave the same way somehow just do not, that if you have the tests, I would leave them.

[deitch]

Lint has a few complaints about extra spaces, and about duplicate code twice; I think it wants you to put it in a shared function.

[cryi]

Oh you. I should check existing PRs first. My bad #314 😄

Do you have any ETA for this?

[StoneMonarch]

@deitch I think all the lint complaints are fixed as well as updating all ext4 tests to try both with and without offset.

[deitch]

All good. Thank you for this!

[cryi]

Thank you both.