diskfs / go-diskfs

MIT License
494 stars 112 forks source link

Wrap errors, not Unwrap #240

Closed quite closed 1 month ago

quite commented 1 month ago

Fixes https://github.com/diskfs/go-diskfs/issues/237

deitch commented 1 month ago

There are 2 tests failing, both related to converting superblock to bytes. Yet you didn't change anything there.

Can you run the tests on the master branch and on your own locally? You just need to check go test ./filesystem/ext4

quite commented 1 month ago

go test ./filesystem/ext4

After running the ./buildimg.sh they pass on both

deitch commented 1 month ago

Then why doesn't it pass in CI? 🤷‍♂️

quite commented 1 month ago

Something is up with that runner/image? Same here now https://github.com/diskfs/go-diskfs/pull/241

deitch commented 1 month ago

After running the ./buildimg.sh they pass on both

Same here. You don't need to run it manually, anyways. Just remove the contents of (or the directory itself) filesystem/ext4/testdata/dist/ and it will autogenerate it.

deitch commented 1 month ago

I just ran a test on macOS and on Ubuntu 22.04. It shouldn't matter, since the actual images generating from within buildimg.sh are run inside docker, but I did it anyways. Everything passes.

deitch commented 1 month ago

There are 2 sections of the superblock that are mismatching. The second is just the checksum, which is different because of the first. Resolve the first, the checksum issue should go away.

The first is total bytes written. The actual is showing 0x9400 = 37888, while the expected is 0x95da = 38362, a difference of 474 (the total KiB written is at position 0x178 in the superblock, little-endian).

Ah, I see it, the issue is here. After buildimg.sh builds the filesystem, we need some way to know the actual value there, so we can test that diskfs is reading it correctly. It uses the output of dumpe2fs ext4.img > stats.txt, which also is in testdata/dist/. The test utility testGetValidSuperblockAndGDTs() reads the output of that. For example, the line from the stats.txt file that gives total written:

Lifetime writes:          37 MB

Unfortunately, that is in MB and not KB, and is not precise. So a small variant can throw it off. We try to catch it here:

    sbKBWritten := float64(sb.totalKBWritten)
    parsedKBWritten := float64(parsed.totalKBWritten)
    KBdiff := math.Abs(parsedKBWritten - sbKBWritten)
    if KBdiff/sbKBWritten < 0.01 {
        sb.totalKBWritten = parsed.totalKBWritten
    }

But it is a bit of a guessing game. It would be good if dumpe2fs (or some other reliable tool) gave precise stats in KB written. debugfs -R gives the same output.

deitch commented 1 month ago

242 fixes this, it has been merged in. Rebase on master and try again.